[PATCH] kernel/smp.c: free related resources when failure occurs in hotplug_cfd()

[PATCH] kernel/smp.c: free related resources when failure occurs in hotplug_cfd()

[Chen Gang]

When failure occurs in hotplug_cfd(), need release related resources,
or will cause memory leak.

Also beautify the related code.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/smp.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index 02a885d..c264623 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -45,15 +45,20 @@ hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu)
 	switch (action) {
 	case CPU_UP_PREPARE:
 	case CPU_UP_PREPARE_FROZEN:
-		if (!zalloc_cpumask_var_node(&cfd->cpumask, GFP_KERNEL,
-				cpu_to_node(cpu)))
+		if (!zalloc_cpumask_var_node(&cfd->cpumask,
+						GFP_KERNEL, cpu_to_node(cpu)))
 			return notifier_from_errno(-ENOMEM);
-		if (!zalloc_cpumask_var_node(&cfd->cpumask_ipi, GFP_KERNEL,
-				cpu_to_node(cpu)))
+
+		if (!zalloc_cpumask_var_node(&cfd->cpumask_ipi,
+						GFP_KERNEL, cpu_to_node(cpu))) {
+			free_cpumask_var(cfd->cpumask);
 			return notifier_from_errno(-ENOMEM);
+		}
+
 		cfd->csd = alloc_percpu(struct call_single_data);
 		if (!cfd->csd) {
 			free_cpumask_var(cfd->cpumask);
+			free_cpumask_var(cfd->cpumask_ipi);
 			return notifier_from_errno(-ENOMEM);
 		}
 		break;
-- 
1.7.7.6

Re: [PATCH] kernel/smp.c: free related resources when failure occurs in hotplug_cfd()

[Paul Gortmaker]

[[PATCH] kernel/smp.c: free related resources when failure occurs in hotplug_cfd()] On 08/07/2013 (Mon 16:50) Chen Gang wrote:

> When failure occurs in hotplug_cfd(), need release related resources,
> or will cause memory leak.
> 
> Also beautify the related code.

No.  Please do not mix real fixes with trivial whitespace changes.
It makes it harder for the reviewer to find the actual fix, and it
makes the fix less portable to other releases (i.e. stable trees.)

Also, you say "beautify", but that is a matter of opinion.  You
shuffle around the tabs in your whitespace change, and yet even
then you don't manage to adapt it to the general coding style of
having multi-line args align with where the 1st arg starts.  So
you have done nothing but pollute the "git blame" history of that
file for other users.

You might want to slow down on the quantity of patches you send,
and spend more time reading the comments from other people on
reviewed patches and learning some of the implicit requirements
from those.  I've noticed that you are already dangerously close
to annoying several key subsystem maintainers, and that is not
the right long term approach to working with the linux community.

Paul.
--

> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/smp.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 02a885d..c264623 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -45,15 +45,20 @@ hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu)
>  	switch (action) {
>  	case CPU_UP_PREPARE:
>  	case CPU_UP_PREPARE_FROZEN:
> -		if (!zalloc_cpumask_var_node(&cfd->cpumask, GFP_KERNEL,
> -				cpu_to_node(cpu)))
> +		if (!zalloc_cpumask_var_node(&cfd->cpumask,
> +						GFP_KERNEL, cpu_to_node(cpu)))
>  			return notifier_from_errno(-ENOMEM);
> -		if (!zalloc_cpumask_var_node(&cfd->cpumask_ipi, GFP_KERNEL,
> -				cpu_to_node(cpu)))
> +
> +		if (!zalloc_cpumask_var_node(&cfd->cpumask_ipi,
> +						GFP_KERNEL, cpu_to_node(cpu))) {
> +			free_cpumask_var(cfd->cpumask);
>  			return notifier_from_errno(-ENOMEM);
> +		}
> +
>  		cfd->csd = alloc_percpu(struct call_single_data);
>  		if (!cfd->csd) {
>  			free_cpumask_var(cfd->cpumask);
> +			free_cpumask_var(cfd->cpumask_ipi);
>  			return notifier_from_errno(-ENOMEM);
>  		}
>  		break;
> -- 
> 1.7.7.6

Re: [PATCH] kernel/smp.c: free related resources when failure occurs in hotplug_cfd()

[Chen Gang F T]

On 07/08/2013 10:26 PM, Paul Gortmaker wrote:
> [[PATCH] kernel/smp.c: free related resources when failure occurs in hotplug_cfd()] On 08/07/2013 (Mon 16:50) Chen Gang wrote:
> 
>> > When failure occurs in hotplug_cfd(), need release related resources,
>> > or will cause memory leak.
>> > 
>> > Also beautify the related code.
> No.  Please do not mix real fixes with trivial whitespace changes.
> It makes it harder for the reviewer to find the actual fix, and it
> makes the fix less portable to other releases (i.e. stable trees.)
> 

OH, at least, need delete white spaces.

> Also, you say "beautify", but that is a matter of opinion.  You
> shuffle around the tabs in your whitespace change, and yet even
> then you don't manage to adapt it to the general coding style of
> having multi-line args align with where the 1st arg starts.  So
> you have done nothing but pollute the "git blame" history of that
> file for other users.
> 

OK, I will send patch v2 for it (which will remove the waste
'beautifying' operation).

> You might want to slow down on the quantity of patches you send,
> and spend more time reading the comments from other people on
> reviewed patches and learning some of the implicit requirements
> from those.

No, that only means I still need improving my patch sending.

Hmm... for 'learning', I think: "learn with each other, never too old to
learn, never stop learning", so we can learn from most of patches or
replies which sent by many members.

e.g. I understand that for beautifying code, I need more consideration:
not only about coding style rules, but also the readers from 'git' and
the reviewers from 'diff'.

>             I've noticed that you are already dangerously close
> to annoying several key subsystem maintainers, and that is not
> the right long term approach to working with the linux community.

If no reply, I will(of cause) no reply either.

Especially, every members' time resource is always expensive.


Thanks.
-- 
Chen Gang

Re: [PATCH] kernel/smp.c: free related resources when failure occurs in hotplug_cfd()

[Wang YanQing]

On Mon, Jul 08, 2013 at 04:50:24PM +0800, Chen Gang wrote:
> When failure occurs in hotplug_cfd(), need release related resources,
> or will cause memory leak.
> 
> Also beautify the related code.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/smp.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 02a885d..c264623 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -45,15 +45,20 @@ hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu)
>  	switch (action) {
>  	case CPU_UP_PREPARE:
>  	case CPU_UP_PREPARE_FROZEN:
> -		if (!zalloc_cpumask_var_node(&cfd->cpumask, GFP_KERNEL,
> -				cpu_to_node(cpu)))
> +		if (!zalloc_cpumask_var_node(&cfd->cpumask,
> +						GFP_KERNEL, cpu_to_node(cpu)))
>  			return notifier_from_errno(-ENOMEM);

What did you fixed here? code style?
You can drop this part.

> -		if (!zalloc_cpumask_var_node(&cfd->cpumask_ipi, GFP_KERNEL,
> -				cpu_to_node(cpu)))
> +
> +		if (!zalloc_cpumask_var_node(&cfd->cpumask_ipi,
> +						GFP_KERNEL, cpu_to_node(cpu))) {
> +			free_cpumask_var(cfd->cpumask);
>  			return notifier_from_errno(-ENOMEM);
> +		}
> +
>  		cfd->csd = alloc_percpu(struct call_single_data);
>  		if (!cfd->csd) {
>  			free_cpumask_var(cfd->cpumask);
> +			free_cpumask_var(cfd->cpumask_ipi);
>  			return notifier_from_errno(-ENOMEM);
>  		}
>  		break;

Yes, we need this fix.

If you resend the v2, I will give acked-by :)

Thanks.

Re: [PATCH] kernel/smp.c: free related resources when failure occurs in hotplug_cfd()

[Chen Gang]

On 07/09/2013 08:28 AM, Wang YanQing wrote:
> On Mon, Jul 08, 2013 at 04:50:24PM +0800, Chen Gang wrote:
>> > When failure occurs in hotplug_cfd(), need release related resources,
>> > or will cause memory leak.
>> > 
>> > Also beautify the related code.
>> > 
>> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> > ---
>> >  kernel/smp.c |   13 +++++++++----
>> >  1 files changed, 9 insertions(+), 4 deletions(-)
>> > 
>> > diff --git a/kernel/smp.c b/kernel/smp.c
>> > index 02a885d..c264623 100644
>> > --- a/kernel/smp.c
>> > +++ b/kernel/smp.c
>> > @@ -45,15 +45,20 @@ hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu)
>> >  	switch (action) {
>> >  	case CPU_UP_PREPARE:
>> >  	case CPU_UP_PREPARE_FROZEN:
>> > -		if (!zalloc_cpumask_var_node(&cfd->cpumask, GFP_KERNEL,
>> > -				cpu_to_node(cpu)))
>> > +		if (!zalloc_cpumask_var_node(&cfd->cpumask,
>> > +						GFP_KERNEL, cpu_to_node(cpu)))
>> >  			return notifier_from_errno(-ENOMEM);
> What did you fixed here? code style?
> You can drop this part.
> 

Yes, I should drop.

>> > -		if (!zalloc_cpumask_var_node(&cfd->cpumask_ipi, GFP_KERNEL,
>> > -				cpu_to_node(cpu)))
>> > +
>> > +		if (!zalloc_cpumask_var_node(&cfd->cpumask_ipi,
>> > +						GFP_KERNEL, cpu_to_node(cpu))) {
>> > +			free_cpumask_var(cfd->cpumask);
>> >  			return notifier_from_errno(-ENOMEM);
>> > +		}
>> > +
>> >  		cfd->csd = alloc_percpu(struct call_single_data);
>> >  		if (!cfd->csd) {
>> >  			free_cpumask_var(cfd->cpumask);
>> > +			free_cpumask_var(cfd->cpumask_ipi);
>> >  			return notifier_from_errno(-ENOMEM);
>> >  		}
>> >  		break;
> Yes, we need this fix.
> 
> If you resend the v2, I will give acked-by :)

Thanks.
-- 
Chen Gang

[PATCH v2] kernel/smp.c: free related resources when failure occurs in hotplug_cfd()

[Chen Gang]

When failure occurs in hotplug_cfd(), need release related resources,
or will cause memory leak.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/smp.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index 02a885d..2a3a017 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -49,10 +49,13 @@ hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu)
 				cpu_to_node(cpu)))
 			return notifier_from_errno(-ENOMEM);
 		if (!zalloc_cpumask_var_node(&cfd->cpumask_ipi, GFP_KERNEL,
-				cpu_to_node(cpu)))
+				cpu_to_node(cpu))) {
+			free_cpumask_var(cfd->cpumask);
 			return notifier_from_errno(-ENOMEM);
+		}
 		cfd->csd = alloc_percpu(struct call_single_data);
 		if (!cfd->csd) {
+			free_cpumask_var(cfd->cpumask_ipi);
 			free_cpumask_var(cfd->cpumask);
 			return notifier_from_errno(-ENOMEM);
 		}
-- 
1.7.7.6

Re: [PATCH v2] kernel/smp.c: free related resources when failure occurs in hotplug_cfd()

[Wang YanQing]

On Tue, Jul 09, 2013 at 08:43:05AM +0800, Chen Gang wrote:
> When failure occurs in hotplug_cfd(), need release related resources,
> or will cause memory leak.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/smp.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 02a885d..2a3a017 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -49,10 +49,13 @@ hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu)
>  				cpu_to_node(cpu)))
>  			return notifier_from_errno(-ENOMEM);
>  		if (!zalloc_cpumask_var_node(&cfd->cpumask_ipi, GFP_KERNEL,
> -				cpu_to_node(cpu)))
> +				cpu_to_node(cpu))) {
> +			free_cpumask_var(cfd->cpumask);
>  			return notifier_from_errno(-ENOMEM);
> +		}
>  		cfd->csd = alloc_percpu(struct call_single_data);
>  		if (!cfd->csd) {
> +			free_cpumask_var(cfd->cpumask_ipi);
>  			free_cpumask_var(cfd->cpumask);
>  			return notifier_from_errno(-ENOMEM);
>  		}
> -- 
> 1.7.7.6
Acked-by: Wang YanQing <udknight@gmail.com>

Re: [PATCH v2] kernel/smp.c: free related resources when failure occurs in hotplug_cfd()

[Chen Gang]

On 07/09/2013 08:46 AM, Wang YanQing wrote:
> On Tue, Jul 09, 2013 at 08:43:05AM +0800, Chen Gang wrote:
>> > When failure occurs in hotplug_cfd(), need release related resources,
>> > or will cause memory leak.
>> > 
>> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> > ---
>> >  kernel/smp.c |    5 ++++-
>> >  1 files changed, 4 insertions(+), 1 deletions(-)
>> > 
>> > diff --git a/kernel/smp.c b/kernel/smp.c
>> > index 02a885d..2a3a017 100644
>> > --- a/kernel/smp.c
>> > +++ b/kernel/smp.c
>> > @@ -49,10 +49,13 @@ hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu)
>> >  				cpu_to_node(cpu)))
>> >  			return notifier_from_errno(-ENOMEM);
>> >  		if (!zalloc_cpumask_var_node(&cfd->cpumask_ipi, GFP_KERNEL,
>> > -				cpu_to_node(cpu)))
>> > +				cpu_to_node(cpu))) {
>> > +			free_cpumask_var(cfd->cpumask);
>> >  			return notifier_from_errno(-ENOMEM);
>> > +		}
>> >  		cfd->csd = alloc_percpu(struct call_single_data);
>> >  		if (!cfd->csd) {
>> > +			free_cpumask_var(cfd->cpumask_ipi);
>> >  			free_cpumask_var(cfd->cpumask);
>> >  			return notifier_from_errno(-ENOMEM);
>> >  		}
>> > -- 
>> > 1.7.7.6
> Acked-by: Wang YanQing <udknight@gmail.com>
> 
> 

Thanks  :-)

-- 
Chen Gang