Discussion:
[dpdk-dev] [PATCH] eal: fix unlock in rte_eal_memzone_init
(too old to reply)
g***@vip.163.com
2018-12-06 00:47:31 UTC
Permalink
From: Gao Feng <***@tencent.com>

The RTE_PROC_PRIMARY error handler lost the unlock statement in the
current codes. Now fix it.

Signed-off-by: Gao Feng <***@tencent.com>
---
lib/librte_eal/common/eal_common_memzone.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/common/eal_common_memzone.c b/lib/librte_eal/common/eal_common_memzone.c
index b7081af..649cad4 100644
--- a/lib/librte_eal/common/eal_common_memzone.c
+++ b/lib/librte_eal/common/eal_common_memzone.c
@@ -375,6 +375,7 @@
rte_fbarray_init(&mcfg->memzones, "memzone",
RTE_MAX_MEMZONE, sizeof(struct rte_memzone))) {
RTE_LOG(ERR, EAL, "Cannot allocate memzone list\n");
+ rte_rwlock_write_unlock(&mcfg->mlock);
return -1;
} else if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
rte_fbarray_attach(&mcfg->memzones)) {
--
1.8.3.1
Burakov, Anatoly
2018-12-06 09:09:23 UTC
Permalink
Post by g***@vip.163.com
The RTE_PROC_PRIMARY error handler lost the unlock statement in the
current codes. Now fix it.
Fixes: 49df3db84883 ("memzone: replace memzone array with fbarray")
Post by g***@vip.163.com
---
lib/librte_eal/common/eal_common_memzone.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/librte_eal/common/eal_common_memzone.c b/lib/librte_eal/common/eal_common_memzone.c
index b7081af..649cad4 100644
--- a/lib/librte_eal/common/eal_common_memzone.c
+++ b/lib/librte_eal/common/eal_common_memzone.c
@@ -375,6 +375,7 @@
rte_fbarray_init(&mcfg->memzones, "memzone",
RTE_MAX_MEMZONE, sizeof(struct rte_memzone))) {
RTE_LOG(ERR, EAL, "Cannot allocate memzone list\n");
+ rte_rwlock_write_unlock(&mcfg->mlock);
return -1;
} else if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
rte_fbarray_attach(&mcfg->memzones)) {
Acked-by: Anatoly Burakov <***@intel.com>

Although i would probably remove both unlocks and instead save and
return a value, so that unlock happens in one place. But this is OK too.
--
Thanks,
Anatoly
Gao Feng
2018-12-06 09:44:56 UTC
Permalink
Post by Burakov, Anatoly
Post by g***@vip.163.com
The RTE_PROC_PRIMARY error handler lost the unlock statement in the
current codes. Now fix it.
Fixes: 49df3db84883 ("memzone: replace memzone array with fbarray")
Post by g***@vip.163.com
---
lib/librte_eal/common/eal_common_memzone.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/librte_eal/common/eal_common_memzone.c b/lib/librte_eal/common/eal_common_memzone.c
index b7081af..649cad4 100644
--- a/lib/librte_eal/common/eal_common_memzone.c
+++ b/lib/librte_eal/common/eal_common_memzone.c
@@ -375,6 +375,7 @@
rte_fbarray_init(&mcfg->memzones, "memzone",
RTE_MAX_MEMZONE, sizeof(struct rte_memzone))) {
RTE_LOG(ERR, EAL, "Cannot allocate memzone list\n");
+ rte_rwlock_write_unlock(&mcfg->mlock);
return -1;
} else if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
rte_fbarray_attach(&mcfg->memzones)) {
Although i would probably remove both unlocks and instead save and
return a value, so that unlock happens in one place. But this is OK too.
Thanks Anatoly.
Thanks Anatoly's review.
I also prefer keep unlock in one place.
As a new guy, finally I choose just a fix with a minor change. I would do better next time.


And could I ask you one question, Anatoly?


I sent another dpdk patch with wrong git-send-email command, "git send-email -1 --to ***@dpdk.org patch_xxxx".
As a result, it generated another wrong reply and email thread.


I don't know if i need to send v2 patch to correct it then?
Its url is https://www.mail-archive.com/***@dpdk.org/msg119925.html.


B
Burakov, Anatoly
2018-12-06 11:18:17 UTC
Permalink
Post by Gao Feng
Post by Burakov, Anatoly
Post by g***@vip.163.com
The RTE_PROC_PRIMARY error handler lost the unlock statement in the
current codes. Now fix it.
Fixes: 49df3db84883 ("memzone: replace memzone array with fbarray")
Post by g***@vip.163.com
---
lib/librte_eal/common/eal_common_memzone.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/librte_eal/common/eal_common_memzone.c b/lib/librte_eal/common/eal_common_memzone.c
index b7081af..649cad4 100644
--- a/lib/librte_eal/common/eal_common_memzone.c
+++ b/lib/librte_eal/common/eal_common_memzone.c
@@ -375,6 +375,7 @@
rte_fbarray_init(&mcfg->memzones, "memzone",
RTE_MAX_MEMZONE, sizeof(struct rte_memzone))) {
RTE_LOG(ERR, EAL, "Cannot allocate memzone list\n");
+ rte_rwlock_write_unlock(&mcfg->mlock);
return -1;
} else if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
rte_fbarray_attach(&mcfg->memzones)) {
Although i would probably remove both unlocks and instead save and
return a value, so that unlock happens in one place. But this is OK too.
Thanks Anatoly.
Thanks Anatoly's review.
I also prefer keep unlock in one place.
As a new guy, finally I choose just a fix with a minor change. I would do better next time.
That's OK. Regardless, you should resubmit it with proper Fixes: tag and
a Cc: to stable, since this bug has been there since 18.05 and therefore
our stable branches will benefit from your contribution as well.
Post by Gao Feng
And could I ask you one question, Anatoly?
I sent another dpdk patch with wrong git-send-email command, "git
As a result, it generated another wrong reply and email thread.
I don't know if i need to send v2 patch to correct it then?
No need to do anything :)
Post by Gao Feng
Best Regards
Feng
Post by Burakov, Anatoly
--
Thanks,
Anatoly
--
Thanks,
Anatoly
Gao Feng
2018-12-07 01:20:08 UTC
Permalink
The RTE_PROC_PRIMARY error handler lost the unlock statement in the
current codes. Now unlock and return in one place to fix it.

Fixes: 49df3db84883 ("memzone: replace memzone array with fbarray")
Cc: ***@dpdk.org

Signed-off-by: Gao Feng <***@tencent.com>
---
v2: Unlock and return in one place, per Anatoly

lib/librte_eal/common/eal_common_memzone.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_memzone.c b/lib/librte_eal/common/eal_common_memzone.c
index b7081af..664df5b 100644
--- a/lib/librte_eal/common/eal_common_memzone.c
+++ b/lib/librte_eal/common/eal_common_memzone.c
@@ -365,6 +365,7 @@ int
rte_eal_memzone_init(void)
{
struct rte_mem_config *mcfg;
+ int ret = 0;

/* get pointer to global configuration */
mcfg = rte_eal_get_configuration()->mem_config;
@@ -375,17 +376,16 @@ rte_eal_memzone_init(void)
rte_fbarray_init(&mcfg->memzones, "memzone",
RTE_MAX_MEMZONE, sizeof(struct rte_memzone))) {
RTE_LOG(ERR, EAL, "Cannot allocate memzone list\n");
- return -1;
+ ret = -1;
} else if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
rte_fbarray_attach(&mcfg->memzones)) {
RTE_LOG(ERR, EAL, "Cannot attach to memzone list\n");
- rte_rwlock_write_unlock(&mcfg->mlock);
- return -1;
+ ret = -1;
}

rte_rwlock_write_unlock(&mcfg->mlock);

- return 0;
+ return ret;
}

/* Walk all reserved memory zones */
--
2.7.4
Burakov, Anatoly
2018-12-07 08:57:37 UTC
Permalink
Post by g***@vip.163.com
The RTE_PROC_PRIMARY error handler lost the unlock statement in the
current codes. Now unlock and return in one place to fix it.
Fixes: 49df3db84883 ("memzone: replace memzone array with fbarray")
---
Acked-by: Anatoly Burakov <***@intel.com>
--
Thanks,
Anatoly
Loading...