[dpdk-dev] [PATCH] eal: fix unlock in rte_eal_memzone_init

[dpdk-dev] [PATCH] eal: fix unlock in rte_eal_memzone_init

[gfree.wind]

From: Gao Feng <davidfgao@tencent.com>

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

Signed-off-by: Gao Feng <davidfgao@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

Re: [dpdk-dev] [PATCH] eal: fix unlock in rte_eal_memzone_init

[Burakov, Anatoly]

On 06-Dec-18 12:47 AM, gfree.wind@vip.163.com wrote:
> From: Gao Feng <davidfgao@tencent.com>
> 
> The RTE_PROC_PRIMARY error handler lost the unlock statement in the
> current codes. Now fix it.
> 
> Signed-off-by: Gao Feng <davidfgao@tencent.com>

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

> ---
>   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 <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

Re: [dpdk-dev] [PATCH] eal: fix unlock in rte_eal_memzone_init

[Gao Feng]

At 2018-12-06 17:09:23, "Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:
>On 06-Dec-18 12:47 AM, gfree.wind@vip.163.com wrote:
>> From: Gao Feng <davidfgao@tencent.com>
>> 
>> The RTE_PROC_PRIMARY error handler lost the unlock statement in the
>> current codes. Now fix it.
>> 
>> Signed-off-by: Gao Feng <davidfgao@tencent.com>
>
>Fixes: 49df3db84883 ("memzone: replace memzone array with fbarray")
>Cc: stable@dpdk.org
>
>> ---
>>   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 <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. 


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 dev@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/dev@dpdk.org/msg119925.html.


Best Regards
Feng


>
>-- 
>Thanks,
>Anatoly

Re: [dpdk-dev] [PATCH] eal: fix unlock in rte_eal_memzone_init

[Burakov, Anatoly]

On 06-Dec-18 9:44 AM, Gao Feng wrote:
> At 2018-12-06 17:09:23, "Burakov, Anatoly" <anatoly.burakov@intel.com> 
> wrote:
> 
>>On 06-Dec-18 12:47 AM, gfree.wind@vip.163.com wrote:
>>> From: Gao Feng <davidfgao@tencent.com>
>>> 
>>> The RTE_PROC_PRIMARY error handler lost the unlock statement in the
>>> current codes. Now fix it.
>>> 
>>> Signed-off-by: Gao Feng <davidfgao@tencent.com>
>>
>>Fixes: 49df3db84883 ("memzone: replace memzone array with fbarray")
>>Cc: stable@dpdk.org
>>
>>> ---
>>>   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 <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.
> 
> 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.

> 
> And could I ask you one question, Anatoly?
> 
> I sent another dpdk patch with wrong git-send-email command, "git 
> send-email -1 --to dev@dpdk.org <mailto:dev@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/dev@dpdk.org/msg119925.html.

No need to do anything :)

> 
> Best Regards
> Feng
> 
>>
>>-- 
>>Thanks,
>>Anatoly
> 


-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v2] eal: fix unlock in rte_eal_memzone_init

[Gao Feng]

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: stable@dpdk.org

Signed-off-by: Gao Feng <davidfgao@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

Re: [dpdk-dev] [PATCH v2] eal: fix unlock in rte_eal_memzone_init

[Burakov, Anatoly]

On 07-Dec-18 1:20 AM, Gao Feng wrote:
> 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: stable@dpdk.org
> 
> Signed-off-by: Gao Feng <davidfgao@tencent.com>
> ---
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] eal: fix unlock in rte_eal_memzone_init

[Thomas Monjalon]

07/12/2018 09:57, Burakov, Anatoly:
> On 07-Dec-18 1:20 AM, Gao Feng wrote:
> > 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: stable@dpdk.org
> > 
> > Signed-off-by: Gao Feng <davidfgao@tencent.com>
> > ---
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks

I changed your author email (From:) to match the Signed-off-by one.
Please configure your git to use the Tencent email if possible.
Note you can have a "git From" address and use another one for sending.

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] eal: fix unlock in rte_eal_memzone_init

[Thomas Monjalon]

20/12/2018 12:20, Thomas Monjalon:
> 07/12/2018 09:57, Burakov, Anatoly:
> > On 07-Dec-18 1:20 AM, Gao Feng wrote:
> > > 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: stable@dpdk.org
> > > 
> > > Signed-off-by: Gao Feng <davidfgao@tencent.com>
> > > ---
> > Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Applied, thanks
> 
> I changed your author email (From:) to match the Signed-off-by one.
> Please configure your git to use the Tencent email if possible.
> Note you can have a "git From" address and use another one for sending.

One more tip about patch formatting:

I reworded the title to:
	memzone: fix unlock on initialization failure

The prefix "memzone" gives the scope of the change.
We decided that memzone is important enough to have its own scope prefix.
You can find other prefixes in the git history.

Then the rest of the title is written in plain english,
detailing more the scope with "on initialization failure".

Hope it helps, and welcome :)

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] eal: fix unlock in rte_eal_memzone_init

[Gao Feng]

At 2018-12-20 19:36:14, "Thomas Monjalon" <thomas@monjalon.net> wrote:
>20/12/2018 12:20, Thomas Monjalon:
>> 07/12/2018 09:57, Burakov, Anatoly:
>> > On 07-Dec-18 1:20 AM, Gao Feng wrote:
>> > > 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: stable@dpdk.org
>> > > 
>> > > Signed-off-by: Gao Feng <davidfgao@tencent.com>
>> > > ---
>> > Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> 
>> Applied, thanks
>> 
>> I changed your author email (From:) to match the Signed-off-by one.
>> Please configure your git to use the Tencent email if possible.
>> Note you can have a "git From" address and use another one for sending.
>
>One more tip about patch formatting:
>
>I reworded the title to:
>	memzone: fix unlock on initialization failure
>
>The prefix "memzone" gives the scope of the change.
>We decided that memzone is important enough to have its own scope prefix.
>You can find other prefixes in the git history.
>
>Then the rest of the title is written in plain english,
>detailing more the scope with "on initialization failure".
>
>Hope it helps, and welcome :)

Thanks your enhancement, I think it's better.

Best Regards
Feng

>
>

Re: [dpdk-dev] [PATCH v2] eal: fix unlock in rte_eal_memzone_init

[Gao Feng]

At 2018-12-20 19:20:53, "Thomas Monjalon" <thomas@monjalon.net> wrote:
>07/12/2018 09:57, Burakov, Anatoly:
>> On 07-Dec-18 1:20 AM, Gao Feng wrote:
>> > 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: stable@dpdk.org
>> > 
>> > Signed-off-by: Gao Feng <davidfgao@tencent.com>
>> > ---
>> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
>
>Applied, thanks
>
>I changed your author email (From:) to match the Signed-off-by one.
>Please configure your git to use the Tencent email if possible.
>Note you can have a "git From" address and use another one for sending.

Thanks，there is one reason.
Because the tencent mailbox is exchange, so I couldn't use git sendmail with tencent email.

Maybe I could add two signed-off to avoid it.

Best Regards
Feng

>
>

Re: [dpdk-dev] [PATCH v2] eal: fix unlock in rte_eal_memzone_init

[Thomas Monjalon]

20/12/2018 12:44, Gao Feng:
> At 2018-12-20 19:20:53, "Thomas Monjalon" <thomas@monjalon.net> wrote:
> >07/12/2018 09:57, Burakov, Anatoly:
> >> On 07-Dec-18 1:20 AM, Gao Feng wrote:
> >> > 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: stable@dpdk.org
> >> > 
> >> > Signed-off-by: Gao Feng <davidfgao@tencent.com>
> >> > ---
> >> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >
> >Applied, thanks
> >
> >I changed your author email (From:) to match the Signed-off-by one.
> >Please configure your git to use the Tencent email if possible.
> >Note you can have a "git From" address and use another one for sending.
> 
> Thanks，there is one reason.
> Because the tencent mailbox is exchange, so I couldn't use git sendmail with tencent email.
> 
> Maybe I could add two signed-off to avoid it.

No, you can use your Tencent email in git,
and send with another email.
In such case, git will add "From: Gao Feng <davidfgao@tencent.com>"
at the head of the patch when sending, so it will be fine when applying.