Re: [PATCH v1 2/9]powerpc/powernv: nest pmu init function with cpumask attr
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
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
[PATCH v1 2/9]powerpc/powernv: nest pmu init function with cpumask attr
Patch creates a file "nest-pmu-c" to contain nest pmu related functions. 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. + */ + +#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(); +} + + +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; +} +device_initcall(nest_pmu_init); -- 1.9.1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev