Skip to contentSkip to conversation

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

PATCH Superseded Chen Gang 4 participants 8 messages Jul 8, 2013 – Jul 9, 2013

lore
Share & tools
lore

Chen Gang

author
#0 lore raw

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

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 <[email protected]>
---
 kernel/smp.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)
kernel/smp.c +9 -4
index 02a885d..c264623 100644
@@ -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;
Signature · 1 line
1.7.7.6
#1 lore raw
Quoted 4 lines · [[PATCH] kernel/smp.c: free related resources when failure occurs in…

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

Quoted 37 lines
Signed-off-by Chen Gang <[email protected]>
---
 kernel/smp.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)
kernel/smp.c +9 -4
index 02a885d..c264623 100644
@@ -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;
Signature · 1 line
1.7.7.6
#2 lore raw
Quoted 10 lines · Paul Gortmaker
Quoted 4 lines · [[PATCH] kernel/smp.c: free related resources when failure occurs in…
Quoted 4 lines

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.

Quoted 7 lines

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

Quoted 4 lines

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

Quoted 3 lines

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.

Signature · 1 line
Chen Gang
#3 lore raw
In reply to Chen Gang
Quoted 15 lines · 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 <[email protected]>
---
 kernel/smp.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)
kernel/smp.c +0 -0
index 02a885d..c264623 100644

kernel/smp.c hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu) · line 50

Quoted 8 lines
kernel/smp.c +2 -2
@@ -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.

Quoted 6 lines
kernel/smp.c +4 -2
-		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);

kernel/smp.c hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu) · line 64

Quoted 10 lines
kernel/smp.c +3 -0
 			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.

author
#4 lore raw
Quoted 11 lines · Wang YanQing
Quoted 10 lines · Chen Gang
Quoted 10 lines

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 <[email protected]>
---
 kernel/smp.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)
Quoted 16 lines
Quoted 5 lines
Quoted 5 lines
kernel/smp.c +0 -0
index 02a885d..c264623 100644
@@ -45,15 +45,20 @@ hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu)
Quoted 8 lines

kernel/smp.c hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu) · line 50

Quoted 8 lines
kernel/smp.c +2 -2
 	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.

Quoted 18 lines
Quoted 8 lines
Quoted 8 lines
kernel/smp.c +5 -2
-		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);
+		}
Quoted 8 lines

kernel/smp.c hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu) · line 64

Quoted 8 lines
kernel/smp.c +2 -0
+
 		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.

Quoted 1 line

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

Thanks.

Signature · 1 line
Chen Gang
author
#5 lore raw

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

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

Signed-off-by Chen Gang <[email protected]>
---
 kernel/smp.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
kernel/smp.c +4 -1
index 02a885d..2a3a017 100644
@@ -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);
 		}
Signature · 1 line
1.7.7.6
Acked-by on v2 1/1
#6 lore raw
Quoted 29 lines · Chen Gang

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

Signed-off-by Chen Gang <[email protected]>
---
 kernel/smp.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
kernel/smp.c +4 -1
index 02a885d..2a3a017 100644
@@ -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);
 		}
Signature · 1 line
1.7.7.6
Acked-by Wang YanQing <[email protected]>
author
#7 lore raw
Quoted 23 lines · Wang YanQing
Quoted 22 lines · Chen Gang
Quoted 22 lines

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

Signed-off-by Chen Gang <[email protected]>
---
 kernel/smp.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
kernel/smp.c +3 -1
index 02a885d..2a3a017 100644
@@ -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);
Quoted 10 lines
Quoted 7 lines
Quoted 7 lines
kernel/smp.c +1 -0
 		if (!cfd->csd) {
+			free_cpumask_var(cfd->cpumask_ipi);
 			free_cpumask_var(cfd->cpumask);
 			return notifier_from_errno(-ENOMEM);
 		}
Signature · 1 line
1.7.7.6
Acked-by Wang YanQing <[email protected]>

Thanks :-)

Signature · 1 line
Chen Gang