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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55700701.7050301@linux.vnet.ibm.com>
Date:	Thu, 04 Jun 2015 13:36:25 +0530
From:	Madhavan Srinivasan <maddy@...ux.vnet.ibm.com>
To:	Daniel Axtens <dja@...ens.net>
CC:	linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
	Peter Zijlstra <peterz@...radead.org>,
	Stephane Eranian <eranian@...gle.com>,
	Paul Mackerras <paulus@...ba.org>,
	Preeti U Murthy <preeti@...ux.vnet.ibm.com>,
	Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>,
	Ingo Molnar <mingo@...nel.org>,
	Anshuman Khandual <khandual@...ux.vnet.ibm.com>
Subject: 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 <mpe@...erman.id.au>
>> Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>
>> Cc: Paul Mackerras <paulus@...ba.org>
>> Cc: Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>
>> Cc: Anshuman Khandual <khandual@...ux.vnet.ibm.com>
>> Cc: Stephane Eranian <eranian@...gle.com>
>> Cc: Preeti U Murthy <preeti@...ux.vnet.ibm.com>
>> Cc: Ingo Molnar <mingo@...nel.org>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Signed-off-by: Madhavan Srinivasan <maddy@...ux.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.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/