Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752158AbbFBXPq (ORCPT ); Tue, 2 Jun 2015 19:15:46 -0400 Received: from mail-pa0-f41.google.com ([209.85.220.41]:34594 "EHLO mail-pa0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751122AbbFBXPi (ORCPT ); Tue, 2 Jun 2015 19:15:38 -0400 Message-ID: <1433286889.438.13.camel@axtens.net> Subject: Re: [PATCH v1 2/9]powerpc/powernv: nest pmu init function with cpumask attr From: Daniel Axtens To: Madhavan Srinivasan 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 Date: Wed, 03 Jun 2015 09:14:49 +1000 In-Reply-To: <1433260778-26497-3-git-send-email-maddy@linux.vnet.ibm.com> References: <1433260778-26497-1-git-send-email-maddy@linux.vnet.ibm.com> <1433260778-26497-3-git-send-email-maddy@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-wUvBtxTCN0sKNtuDJftd" X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5234 Lines: 158 --=-wUvBtxTCN0sKNtuDJftd Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 uni= ts > are per-chip. First online cpu for a given node is picked as > designated thread to read the counter data. >=20 > Subsequent patch adds the hotplug support. >=20 > 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 >=20 > 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[] =3D { > + &dev_attr_cpumask.attr, > + NULL, > +}; > + > +static struct attribute_group cpumask_nest_pmu_attr_group =3D { > + .attrs =3D 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 =3D cpumask_of_node(nid); > + cpu =3D 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 =3D 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") =3D=3D 0) That would make it clearer that you're trying to get a list of possible failure conditions. =20 - 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 --=-wUvBtxTCN0sKNtuDJftd Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: GPGTools - https://gpgtools.org iQIcBAABCAAGBQJVbjjpAAoJEPC3R3P2I92FMY4P/RSySbQkZhVHnLbC6E3R0w6w 95suMaaK12Uy3GQIGauT6apbR83WxwdVMBW1MVUfhbuhk0yjIM0WFkkTh40V0k47 V4IO1M9hSTDbeGOcH380jcTdJYYQX34OXmKB24DTgev0KVjGA53WbfRLee6Hb40q 8a/BGgawL+nmxAaMiW+eFSBto3HPMDUJJkbQ7Qs8J5Bh15RaIwMHm9oRn5VihvoF JHP/oHAJWvXILgYLSWWpMNrcbLTN2Xvw6dC6Usw3IMD0LQhfNFMMwacBMde/1wu1 Qzl2VkxpN0CxzBXo+QLifAm9vdUTNuJK2MWNR3f3+1N5+wyErimU6XunniK90b0w CNbrtOTU7wrgSXB1h5Ju1TWQ+VNObdh9WiegNVimIbHpkqdXtCDZWlrgJopTFTBl 8wXamwiZbfeDQ0Lwit/qFcueE4TzLIF5dO8NjUuAfzBUTOgt624CKfqq7zJeC0EI m5csn2XcgF429QjmGZ2soUmcsZ4NrtXffOHBOpnmkNpshf2lEbp2iAh9NqE3CuPQ +ERp7m0gxLbtrSSgkjjr0FquGNwHAN7PbrRf5/JhxNocjW/Ie8D97z1mzq9xUu6g H46YgJlqInGRW9GBGvYnKxwjqRzDYlg9ix/iOHpWasfuZi8YnNnGpnp+Wu+pJfA8 nRnrqEITdgPzusX3sdKf =89RX -----END PGP SIGNATURE----- --=-wUvBtxTCN0sKNtuDJftd-- -- 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/