Re: [PATCH v1 2/9]powerpc/powernv: nest pmu init function with cpumask attr

[Madhavan Srinivasan]

On Wednesday 03 June 2015 04:44 AM, Daniel Axtens wrote:

On Tue, 2015-06-02 at 21:29 +0530, Madhavan Srinivasan wrote:

Patch creates a file "nest-pmu-c" to contain nest pmu related functions.

"nest-pmu.c"

Patch adds nest pmu init function and cpumask function since Nest pmu units
are per-chip. First online cpu for a given node is picked as
designated thread to read the counter data.

Subsequent patch adds the hotplug support.

Cc: Michael Ellerman 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Sukadev Bhattiprolu 
Cc: Anshuman Khandual 
Cc: Stephane Eranian 
Cc: Preeti U Murthy 
Cc: Ingo Molnar 
Cc: Peter Zijlstra 
Signed-off-by: Madhavan Srinivasan 
---
  arch/powerpc/perf/nest-pmu.c | 70 
  1 file changed, 70 insertions(+)
  create mode 100644 arch/powerpc/perf/nest-pmu.c

diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c
new file mode 100644
index 000..d4413bb
--- /dev/null
+++ b/arch/powerpc/perf/nest-pmu.c
@@ -0,0 +1,70 @@
+/*
+ * Nest Performance Monitor counter support for POWER8 processors.
+ *
+ * Copyright 2015 Madhavan Srinivasan, IBM Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+

Again, I think this is supposed to be v2 only.


+#include "nest-pmu.h"
+
+static cpumask_t cpu_mask_nest_pmu;
+
+static ssize_t cpumask_nest_pmu_get_attr(struct device *dev,
+   struct device_attribute *attr, char *buf)
+{
+   return cpumap_print_to_pagebuf(true, buf, &cpu_mask_nest_pmu);
+}
+
+static DEVICE_ATTR(cpumask, S_IRUGO, cpumask_nest_pmu_get_attr, NULL);
+
+static struct attribute *cpumask_nest_pmu_attrs[] = {
+   &dev_attr_cpumask.attr,
+   NULL,
+};
+
+static struct attribute_group cpumask_nest_pmu_attr_group = {
+   .attrs = cpumask_nest_pmu_attrs,
+};
+
+void cpumask_chip(void)
+{
+   const struct cpumask *l_cpumask;
+   int cpu, nid;
+
+   if (!cpumask_empty(&cpu_mask_nest_pmu)) {
+   printk(KERN_INFO "cpumask not empty\n");
+   return;
+   }
+
+   cpu_notifier_register_begin();
+   for_each_online_node(nid) {
+   l_cpumask = cpumask_of_node(nid);
+   cpu = cpumask_first(l_cpumask);
+   cpumask_set_cpu(cpu, &cpu_mask_nest_pmu);
+   }
+
+   cpu_notifier_register_done();
+}

It's not clear from the name of this function what it does. I don't
think I actually understand what it does: it appears to register a
notifier on the first cpu of each node; maybe that should be reflected
in the name.


My bad. Hotplug notification registration happens in the next patch.
could merge both as single patch.




+static int __init nest_pmu_init(void)
+{
+   int ret = 0;
+
+   /*
+* Lets do this only if we are hypervisor
+*/
+   if (!cur_cpu_spec->oprofile_cpu_type ||
+  strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power8") ||
+  !cpu_has_feature(CPU_FTR_HVMODE))
+   return ret;
+
+   cpumask_chip();
+
+   return 0;
+}

  - Where is ret set? I can only see it set when it's defined: the if
statment doesn't change the value of ret as far as I can see...

Yes. It should have set to error value. Will fix it.


  - Would it be clearer if you said
 !(strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power8") == 0)
That would make it clearer that you're trying to get a list of
possible failure conditions.
  

Yes. Sure will change it.


  - Is there really no better way to check if a CPU is a power 8 than an
string comparison?
One other way I can think of is using PVR (Processor Version Register), 
but then will end up having multiple checks for Power8 itself, so this 
is lot simpler.



+device_initcall(nest_pmu_init);

Regards,
Daniel Axtens

Thanks for the review
Maddy

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v1 2/9]powerpc/powernv: nest pmu init function with cpumask attr

[Daniel Axtens]

On Tue, 2015-06-02 at 21:29 +0530, Madhavan Srinivasan wrote:
> Patch creates a file "nest-pmu-c" to contain nest pmu related functions.
"nest-pmu.c"
> Patch adds nest pmu init function and cpumask function since Nest pmu units
> are per-chip. First online cpu for a given node is picked as
> designated thread to read the counter data.
> 
> Subsequent patch adds the hotplug support.
> 
> Cc: Michael Ellerman 
> Cc: Benjamin Herrenschmidt 
> Cc: Paul Mackerras 
> Cc: Sukadev Bhattiprolu 
> Cc: Anshuman Khandual 
> Cc: Stephane Eranian 
> Cc: Preeti U Murthy 
> Cc: Ingo Molnar 
> Cc: Peter Zijlstra 
> Signed-off-by: Madhavan Srinivasan 
> ---
>  arch/powerpc/perf/nest-pmu.c | 70 
> 
>  1 file changed, 70 insertions(+)
>  create mode 100644 arch/powerpc/perf/nest-pmu.c
> 
> diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c
> new file mode 100644
> index 000..d4413bb
> --- /dev/null
> +++ b/arch/powerpc/perf/nest-pmu.c
> @@ -0,0 +1,70 @@
> +/*
> + * Nest Performance Monitor counter support for POWER8 processors.
> + *
> + * Copyright 2015 Madhavan Srinivasan, IBM Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
Again, I think this is supposed to be v2 only.

> +#include "nest-pmu.h"
> +
> +static cpumask_t cpu_mask_nest_pmu;
> +
> +static ssize_t cpumask_nest_pmu_get_attr(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return cpumap_print_to_pagebuf(true, buf, &cpu_mask_nest_pmu);
> +}
> +
> +static DEVICE_ATTR(cpumask, S_IRUGO, cpumask_nest_pmu_get_attr, NULL);
> +
> +static struct attribute *cpumask_nest_pmu_attrs[] = {
> + &dev_attr_cpumask.attr,
> + NULL,
> +};
> +
> +static struct attribute_group cpumask_nest_pmu_attr_group = {
> + .attrs = cpumask_nest_pmu_attrs,
> +};
> +
> +void cpumask_chip(void)
> +{
> + const struct cpumask *l_cpumask;
> + int cpu, nid;
> +
> + if (!cpumask_empty(&cpu_mask_nest_pmu)) {
> + printk(KERN_INFO "cpumask not empty\n");
> + return;
> + }
> +
> + cpu_notifier_register_begin();
> + for_each_online_node(nid) {
> + l_cpumask = cpumask_of_node(nid);
> + cpu = cpumask_first(l_cpumask);
> + cpumask_set_cpu(cpu, &cpu_mask_nest_pmu);
> + }
> +
> + cpu_notifier_register_done();
> +}
It's not clear from the name of this function what it does. I don't
think I actually understand what it does: it appears to register a
notifier on the first cpu of each node; maybe that should be reflected
in the name.

> +static int __init nest_pmu_init(void)
> +{
> + int ret = 0;
> +
> + /*
> +  * Lets do this only if we are hypervisor
> +  */
> + if (!cur_cpu_spec->oprofile_cpu_type ||
> +strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power8") ||
> +!cpu_has_feature(CPU_FTR_HVMODE))
> + return ret;
> +
> + cpumask_chip();
> +
> + return 0;
> +}
 - Where is ret set? I can only see it set when it's defined: the if
statment doesn't change the value of ret as far as I can see...

 - Would it be clearer if you said
!(strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power8") == 0)
   That would make it clearer that you're trying to get a list of
possible failure conditions.
 
 - Is there really no better way to check if a CPU is a power 8 than an
string comparison?

> +device_initcall(nest_pmu_init);

Regards,
Daniel Axtens


signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev