Messages in this thread | | | Subject | Re: [PATCH v1 2/9]powerpc/powernv: nest pmu init function with cpumask attr | From | Daniel Axtens <> | Date | Wed, 03 Jun 2015 09:14:49 +1000 |
| |
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 <mpe@ellerman.id.au> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> > Cc: Anshuman Khandual <khandual@linux.vnet.ibm.com> > Cc: Stephane Eranian <eranian@google.com> > Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> > --- > 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 0000000..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 [unhandled content-type:application/pgp-signature] | |