Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1475975 > unrolled thread

[PATCH 0/4] perf/x86/cqm: Fine-tuning for two function implementations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 0/4] perf/x86/cqm: Fine-tuning for two function  implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-04 20:00 +0200
    [PATCH 1/4] perf/x86/cqm: Use kmalloc_array() in intel_mbm_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-04 20:00 +0200
      Re: [PATCH 1/4] perf/x86/cqm: Use kmalloc_array() in intel_mbm_init() Peter Zijlstra <peterz@infradead.org> - 2016-09-05 10:00 +0200
    [PATCH 2/4] perf/x86/cqm: Replace two kmalloc() calls by  kmalloc_array() in intel_mbm_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-04 20:00 +0200
      Re: [PATCH 2/4] perf/x86/cqm: Replace two kmalloc() calls by  kmalloc_array() in intel_mbm_init() Peter Zijlstra <peterz@infradead.org> - 2016-09-05 10:00 +0200
    [PATCH 4/4] perf/x86/cqm: Rename jump labels in intel_cqm_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-04 20:10 +0200
    [PATCH 3/4] perf/x86/cqm: One check and another variable less in  intel_mbm_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-04 20:10 +0200
      Re: [PATCH 3/4] perf/x86/cqm: One check and another variable less in  intel_mbm_init() Peter Zijlstra <peterz@infradead.org> - 2016-09-05 10:00 +0200
    Re: [PATCH 0/4] perf/x86/cqm: Fine-tuning for two function  implementations Peter Zijlstra <peterz@infradead.org> - 2016-09-05 10:00 +0200

#1475975 — [PATCH 0/4] perf/x86/cqm: Fine-tuning for two function implementations

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 4 Sep 2016 19:40:40 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Use kmalloc_array() in intel_mbm_init()
  Replace two kmalloc() calls by kmalloc_array() in intel_mbm_init()
  One check and another variable less in intel_mbm_init()
  Rename jump labels in intel_cqm_init()

 arch/x86/events/intel/cqm.c | 52 ++++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 27 deletions(-)

-- 
2.9.3

[toc] | [next] | [standalone]

#1475977 — [PATCH 1/4] perf/x86/cqm: Use kmalloc_array() in intel_mbm_init()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 4 Sep 2016 17:28:13 +0200

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data structure by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/x86/events/intel/cqm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index 783c49d..ec61522 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -1651,8 +1651,9 @@ static int intel_mbm_init(void)
 		goto out;
 	}
 
-	array_size = sizeof(struct hrtimer) * mbm_socket_max;
-	mbm_timers = kmalloc(array_size, GFP_KERNEL);
+	mbm_timers = kmalloc_array(mbm_socket_max,
+				   sizeof(*mbm_timers),
+				   GFP_KERNEL);
 	if (!mbm_timers) {
 		ret = -ENOMEM;
 		goto out;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1476169 — Re: [PATCH 1/4] perf/x86/cqm: Use kmalloc_array() in intel_mbm_init()

On Sun, Sep 04, 2016 at 07:54:47PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 4 Sep 2016 17:28:13 +0200
> 
> * A multiplication for the size determination of a memory allocation
>   indicated that an array data structure should be processed.
>   Thus use the corresponding function "kmalloc_array".
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Replace the specification of a data structure by a pointer dereference
>   to make the corresponding size determination a bit safer according to
>   the Linux coding style convention.

Again, lack of actual reason..

[toc] | [prev] | [next] | [standalone]

#1475982 — [PATCH 2/4] perf/x86/cqm: Replace two kmalloc() calls by kmalloc_array() in intel_mbm_init()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 4 Sep 2016 18:25:09 +0200

1. A multiplication for the size determination of memory allocations
   indicated that array data structures should be processed.
   Thus use the corresponding function "kmalloc_array".

2. Replace the specification of a data structure by pointer dereferences
   to make the corresponding size determination a bit safer according to
   the Linux coding style convention.

3. Delete the local variable "array_size" which became unnecessary
   with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/x86/events/intel/cqm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index ec61522..29f756e 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -1637,15 +1637,18 @@ static const struct x86_cpu_id intel_mbm_total_match[] = {
 
 static int intel_mbm_init(void)
 {
-	int ret = 0, array_size, maxid = cqm_max_rmid + 1;
+	int ret = 0, maxid = cqm_max_rmid + 1;
 
 	mbm_socket_max = topology_max_packages();
-	array_size = sizeof(struct sample) * maxid * mbm_socket_max;
-	mbm_local = kmalloc(array_size, GFP_KERNEL);
+	mbm_local = kmalloc_array(maxid * mbm_socket_max,
+				  sizeof(*mbm_local),
+				  GFP_KERNEL);
 	if (!mbm_local)
 		return -ENOMEM;
 
-	mbm_total = kmalloc(array_size, GFP_KERNEL);
+	mbm_total = kmalloc_array(maxid * mbm_socket_max,
+				  sizeof(*mbm_total),
+				  GFP_KERNEL);
 	if (!mbm_total) {
 		ret = -ENOMEM;
 		goto out;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1476173 — Re: [PATCH 2/4] perf/x86/cqm: Replace two kmalloc() calls by kmalloc_array() in intel_mbm_init()

On Sun, Sep 04, 2016 at 07:56:30PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 4 Sep 2016 18:25:09 +0200
> 
> 1. A multiplication for the size determination of memory allocations
>    indicated that array data structures should be processed.
>    Thus use the corresponding function "kmalloc_array".
> 
> 2. Replace the specification of a data structure by pointer dereferences
>    to make the corresponding size determination a bit safer according to
>    the Linux coding style convention.
> 
> 3. Delete the local variable "array_size" which became unnecessary
>    with this refactoring.

3 is an admission you made the code worse in absense of other
improvements.

[toc] | [prev] | [next] | [standalone]

#1475986 — [PATCH 4/4] perf/x86/cqm: Rename jump labels in intel_cqm_init()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 4 Sep 2016 19:23:57 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/x86/events/intel/cqm.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index 304caf53..6cf508a 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -1701,7 +1701,7 @@ static int __init intel_cqm_init(void)
 		if (c->x86_cache_occ_scale != cqm_l3_scale) {
 			pr_err("Multiple LLC scale values, disabling\n");
 			ret = -EINVAL;
-			goto out;
+			goto put_cpus;
 		}
 	}
 
@@ -1719,19 +1719,19 @@ static int __init intel_cqm_init(void)
 	str = kstrdup(scale, GFP_KERNEL);
 	if (!str) {
 		ret = -ENOMEM;
-		goto out;
+		goto put_cpus;
 	}
 
 	event_attr_intel_cqm_llc_scale.event_str = str;
 
 	ret = intel_cqm_setup_rmid_cache();
 	if (ret)
-		goto out;
+		goto put_cpus;
 
 	if (mbm_enabled)
 		ret = intel_mbm_init();
 	if (ret && !cqm_enabled)
-		goto out;
+		goto put_cpus;
 
 	if (cqm_enabled && mbm_enabled)
 		intel_cqm_events_group.attrs = intel_cmt_mbm_events_attr;
@@ -1743,7 +1743,7 @@ static int __init intel_cqm_init(void)
 	ret = perf_pmu_register(&intel_cqm_pmu, "intel_cqm", -1);
 	if (ret) {
 		pr_err("Intel CQM perf registration failed: %d\n", ret);
-		goto out;
+		goto put_cpus;
 	}
 
 	if (cqm_enabled)
@@ -1760,8 +1760,7 @@ static int __init intel_cqm_init(void)
 			  intel_cqm_cpu_starting, NULL);
 	cpuhp_setup_state(CPUHP_AP_PERF_X86_CQM_ONLINE, "AP_PERF_X86_CQM_ONLINE",
 			  NULL, intel_cqm_cpu_exit);
-
-out:
+ put_cpus:
 	put_online_cpus();
 
 	if (ret) {
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1475987 — [PATCH 3/4] perf/x86/cqm: One check and another variable less in intel_mbm_init()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 4 Sep 2016 19:06:33 +0200

1. Adjust a jump target to eliminate an extra check at the end.

2. Move a jump label according to the current Linux coding style convention.

3. Return only constant values for the success or failure indication.

4. Delete the local variable "ret" which became unnecessary with
   this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/x86/events/intel/cqm.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index 29f756e..304caf53 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -1637,7 +1637,7 @@ static const struct x86_cpu_id intel_mbm_total_match[] = {
 
 static int intel_mbm_init(void)
 {
-	int ret = 0, maxid = cqm_max_rmid + 1;
+	int const maxid = cqm_max_rmid + 1;
 
 	mbm_socket_max = topology_max_packages();
 	mbm_local = kmalloc_array(maxid * mbm_socket_max,
@@ -1649,25 +1649,20 @@ static int intel_mbm_init(void)
 	mbm_total = kmalloc_array(maxid * mbm_socket_max,
 				  sizeof(*mbm_total),
 				  GFP_KERNEL);
-	if (!mbm_total) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!mbm_total)
+		goto free_mbm;
 
 	mbm_timers = kmalloc_array(mbm_socket_max,
 				   sizeof(*mbm_timers),
 				   GFP_KERNEL);
-	if (!mbm_timers) {
-		ret = -ENOMEM;
-		goto out;
-	}
-	mbm_hrtimer_init();
+	if (!mbm_timers)
+		goto free_mbm;
 
-out:
-	if (ret)
-		mbm_cleanup();
-
-	return ret;
+	mbm_hrtimer_init();
+	return 0;
+ free_mbm:
+	mbm_cleanup();
+	return -ENOMEM;
 }
 
 static int __init intel_cqm_init(void)
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1476166 — Re: [PATCH 3/4] perf/x86/cqm: One check and another variable less in intel_mbm_init()

On Sun, Sep 04, 2016 at 07:58:32PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 4 Sep 2016 19:06:33 +0200
> 
> 1. Adjust a jump target to eliminate an extra check at the end.
> 
> 2. Move a jump label according to the current Linux coding style convention.

WTF does that even mean?

> 
> 3. Return only constant values for the success or failure indication.
> 
> 4. Delete the local variable "ret" which became unnecessary with
>    this refactoring.

Also, 4 things should be 4 patches if anything.

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/x86/events/intel/cqm.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
> index 29f756e..304caf53 100644
> --- a/arch/x86/events/intel/cqm.c
> +++ b/arch/x86/events/intel/cqm.c
> @@ -1637,7 +1637,7 @@ static const struct x86_cpu_id intel_mbm_total_match[] = {
>  
>  static int intel_mbm_init(void)
>  {
> -	int ret = 0, maxid = cqm_max_rmid + 1;
> +	int const maxid = cqm_max_rmid + 1;
>  
>  	mbm_socket_max = topology_max_packages();
>  	mbm_local = kmalloc_array(maxid * mbm_socket_max,
> @@ -1649,25 +1649,20 @@ static int intel_mbm_init(void)
>  	mbm_total = kmalloc_array(maxid * mbm_socket_max,
>  				  sizeof(*mbm_total),
>  				  GFP_KERNEL);
> -	if (!mbm_total) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> +	if (!mbm_total)
> +		goto free_mbm;
>  
>  	mbm_timers = kmalloc_array(mbm_socket_max,
>  				   sizeof(*mbm_timers),
>  				   GFP_KERNEL);
> -	if (!mbm_timers) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -	mbm_hrtimer_init();
> +	if (!mbm_timers)
> +		goto free_mbm;
>  
> -out:
> -	if (ret)
> -		mbm_cleanup();
> -
> -	return ret;
> +	mbm_hrtimer_init();
> +	return 0;
> + free_mbm:

No!! no stupid indented labels.

> +	mbm_cleanup();
> +	return -ENOMEM;
>  }

[toc] | [prev] | [next] | [standalone]

#1476167

Send me more of this mindless gunk and you're on a /dev/null filter.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web