Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753184AbbFDIIz (ORCPT ); Thu, 4 Jun 2015 04:08:55 -0400 Received: from e23smtp06.au.ibm.com ([202.81.31.148]:46387 "EHLO e23smtp06.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753147AbbFDIIn (ORCPT ); Thu, 4 Jun 2015 04:08:43 -0400 Message-ID: <55700701.7050301@linux.vnet.ibm.com> Date: Thu, 04 Jun 2015 13:36:25 +0530 From: Madhavan Srinivasan User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Daniel Axtens CC: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, Peter Zijlstra , Stephane Eranian , Paul Mackerras , Preeti U Murthy , Sukadev Bhattiprolu , Ingo Molnar , Anshuman Khandual Subject: Re: [PATCH v1 2/9]powerpc/powernv: nest pmu init function with cpumask attr References: <1433260778-26497-1-git-send-email-maddy@linux.vnet.ibm.com> <1433260778-26497-3-git-send-email-maddy@linux.vnet.ibm.com> <1433286889.438.13.camel@axtens.net> In-Reply-To: <1433286889.438.13.camel@axtens.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15060408-0021-0000-0000-0000015ABC41 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4596 Lines: 139 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 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/