| Message ID | 1473892358-22574-8-git-send-email-jeremy.linton@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 14, 2016 at 05:32:35PM -0500, Jeremy Linton wrote: > On systems with multiple PMU types the PMU to CPU affinity > needs to be detected and set. The CPU to interrupt affinity > should also be set. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/perf/arm_pmu.c | 63 ++++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 53 insertions(+), 10 deletions(-) > > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c > index 58117d7..63f16a5 100644 > --- a/drivers/perf/arm_pmu.c > +++ b/drivers/perf/arm_pmu.c > @@ -11,6 +11,7 @@ > */ > #define pr_fmt(fmt) "hw perfevents: " fmt > > +#include <linux/acpi.h> > #include <linux/bitmap.h> > #include <linux/cpumask.h> > #include <linux/cpu_pm.h> > @@ -24,6 +25,7 @@ > #include <linux/irq.h> > #include <linux/irqdesc.h> > > +#include <asm/cpu.h> > #include <asm/cputype.h> > #include <asm/irq_regs.h> > > @@ -876,25 +878,67 @@ static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu) > } > > /* > - * CPU PMU identification and probing. > + * CPU PMU identification and probing. Its possible to have > + * multiple CPU types in an ARM machine. Assure that we are > + * picking the right PMU types based on the CPU in question > */ > -static int probe_current_pmu(struct arm_pmu *pmu, > - const struct pmu_probe_info *info) > +static int probe_plat_pmu(struct arm_pmu *pmu, > + const struct pmu_probe_info *info, > + unsigned int pmuid) > { > - int cpu = get_cpu(); > - unsigned int cpuid = read_cpuid_id(); > int ret = -ENODEV; > + int cpu; > + int aff_ctr = 0; > + static int duplicate_pmus; > + struct platform_device *pdev = pmu->plat_device; > + int irq = platform_get_irq(pdev, 0); > > - pr_info("probing PMU on CPU %d\n", cpu); > + if (irq >= 0 && !irq_is_percpu(irq)) { > + pmu->irq_affinity = kcalloc(pdev->num_resources, sizeof(int), > + GFP_KERNEL); > + if (!pmu->irq_affinity) > + return -ENOMEM; > + } > > + for_each_possible_cpu(cpu) { > + unsigned int cpuid = read_specific_cpuid(cpu); > + > + if (cpuid == pmuid) { > + cpumask_set_cpu(cpu, &pmu->supported_cpus); > + if (pmu->irq_affinity) { > + pmu->irq_affinity[aff_ctr] = cpu; > + aff_ctr++; > + } > + } > + } > + > + /* find the type of PMU given the CPU */ > for (; info->init != NULL; info++) { > - if ((cpuid & info->mask) != info->cpuid) > + if ((pmuid & info->mask) != info->cpuid) > continue; > ret = info->init(pmu); > + /* > + * if this pmu declaration is unspecified and we have > + * previously found a PMU on this platform then append > + * a PMU number to the pmu name. This avoids changing > + * the names of PMUs that are specific to a class of CPUs. > + * The assumption is that if we match a specific PMU in the > + * provided pmu_probe_info then it's unique, and another PMU > + * in the system will match a different entry rather than > + * needing the _number to assure its unique. > + */ > + if ((!info->cpuid) && (duplicate_pmus)) { Hmm, the duplicate_pmus check looks a little odd here. Doesn't it mean that you'd end up with things like: "arm,armv8-pmuv3" "arm,armv8-pmuv3_1" which looks needlessly fiddly to parse. Is this intentional? Will
Hi Thanks for taking a look at this.. On 09/16/2016 08:29 AM, Will Deacon wrote: > On Wed, Sep 14, 2016 at 05:32:35PM -0500, Jeremy Linton wrote: >> On systems with multiple PMU types the PMU to CPU affinity >> needs to be detected and set. The CPU to interrupt affinity >> should also be set. >> (trimming) >> continue; >> ret = info->init(pmu); >> + /* >> + * if this pmu declaration is unspecified and we have >> + * previously found a PMU on this platform then append >> + * a PMU number to the pmu name. This avoids changing >> + * the names of PMUs that are specific to a class of CPUs. >> + * The assumption is that if we match a specific PMU in the >> + * provided pmu_probe_info then it's unique, and another PMU >> + * in the system will match a different entry rather than >> + * needing the _number to assure its unique. >> + */ >> + if ((!info->cpuid) && (duplicate_pmus)) { > > Hmm, the duplicate_pmus check looks a little odd here. Doesn't it mean > that you'd end up with things like: > > "arm,armv8-pmuv3" > "arm,armv8-pmuv3_1" > > which looks needlessly fiddly to parse. Is this intentional? Well, IIRC, you recommend that format, or maybe I misunderstood. Anyway, per the comment I'm trying to assure that legacy platform devices don't accidentally get a "_X" appended to their name and break something. Further, by itself the name itself doesn't have any meaning/ordering and could just as well be a random string. So, I don't think anyone is going to try and parse it except to compare it as a whole something like "armv8_cortex_a53". Which is why the cpu affinity is required. Additionally, doing it this way allows a tiny tweak to the pmu table in the future to re-enable meaningful PMU names. Lastly, its big.little exclusive, so I would rather apply pain to non-existant big.little server machines, than uglify the common case.
On Fri, Sep 16, 2016 at 10:35:32AM -0500, Jeremy Linton wrote: > On 09/16/2016 08:29 AM, Will Deacon wrote: > >On Wed, Sep 14, 2016 at 05:32:35PM -0500, Jeremy Linton wrote: > >>On systems with multiple PMU types the PMU to CPU affinity > >>needs to be detected and set. The CPU to interrupt affinity > >>should also be set. > >> > (trimming) > >> continue; > >> ret = info->init(pmu); > >>+ /* > >>+ * if this pmu declaration is unspecified and we have > >>+ * previously found a PMU on this platform then append > >>+ * a PMU number to the pmu name. This avoids changing > >>+ * the names of PMUs that are specific to a class of CPUs. > >>+ * The assumption is that if we match a specific PMU in the > >>+ * provided pmu_probe_info then it's unique, and another PMU > >>+ * in the system will match a different entry rather than > >>+ * needing the _number to assure its unique. > >>+ */ > >>+ if ((!info->cpuid) && (duplicate_pmus)) { > > > >Hmm, the duplicate_pmus check looks a little odd here. Doesn't it mean > >that you'd end up with things like: > > > >"arm,armv8-pmuv3" > >"arm,armv8-pmuv3_1" > > > >which looks needlessly fiddly to parse. Is this intentional? > > Well, IIRC, you recommend that format, or maybe I misunderstood. Anyway, per > the comment I'm trying to assure that legacy platform devices don't > accidentally get a "_X" appended to their name and break something. Ah, right, for the old 32-bit platforms that rely on probing, Gotcha. Will
Hi Jeremy, One comment below. Jeremy Linton <jeremy.linton@arm.com> writes: > On systems with multiple PMU types the PMU to CPU affinity > needs to be detected and set. The CPU to interrupt affinity > should also be set. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/perf/arm_pmu.c | 63 ++++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 53 insertions(+), 10 deletions(-) > > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c > index 58117d7..63f16a5 100644 > --- a/drivers/perf/arm_pmu.c > +++ b/drivers/perf/arm_pmu.c > @@ -11,6 +11,7 @@ > */ > #define pr_fmt(fmt) "hw perfevents: " fmt > > +#include <linux/acpi.h> > #include <linux/bitmap.h> > #include <linux/cpumask.h> > #include <linux/cpu_pm.h> > @@ -24,6 +25,7 @@ > #include <linux/irq.h> > #include <linux/irqdesc.h> > > +#include <asm/cpu.h> > #include <asm/cputype.h> > #include <asm/irq_regs.h> > > @@ -876,25 +878,67 @@ static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu) > } > > /* > - * CPU PMU identification and probing. > + * CPU PMU identification and probing. Its possible to have > + * multiple CPU types in an ARM machine. Assure that we are > + * picking the right PMU types based on the CPU in question > */ > -static int probe_current_pmu(struct arm_pmu *pmu, > - const struct pmu_probe_info *info) > +static int probe_plat_pmu(struct arm_pmu *pmu, > + const struct pmu_probe_info *info, > + unsigned int pmuid) > { > - int cpu = get_cpu(); > - unsigned int cpuid = read_cpuid_id(); > int ret = -ENODEV; > + int cpu; > + int aff_ctr = 0; > + static int duplicate_pmus; > + struct platform_device *pdev = pmu->plat_device; > + int irq = platform_get_irq(pdev, 0); > > - pr_info("probing PMU on CPU %d\n", cpu); > + if (irq >= 0 && !irq_is_percpu(irq)) { Marc's got a patch[0] changing all instances of "irq >= 0" in this file to "irq > 0" on the basis that irq 0 is an error. If this version doesn't get merged, please drop the "=" for the next version. [0] http://marc.info/?l=linux-arm-kernel&m=147318284923863 [...]
diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c index 58117d7..63f16a5 100644 --- a/drivers/perf/arm_pmu.c +++ b/drivers/perf/arm_pmu.c @@ -11,6 +11,7 @@ */ #define pr_fmt(fmt) "hw perfevents: " fmt +#include <linux/acpi.h> #include <linux/bitmap.h> #include <linux/cpumask.h> #include <linux/cpu_pm.h> @@ -24,6 +25,7 @@ #include <linux/irq.h> #include <linux/irqdesc.h> +#include <asm/cpu.h> #include <asm/cputype.h> #include <asm/irq_regs.h> @@ -876,25 +878,67 @@ static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu) } /* - * CPU PMU identification and probing. + * CPU PMU identification and probing. Its possible to have + * multiple CPU types in an ARM machine. Assure that we are + * picking the right PMU types based on the CPU in question */ -static int probe_current_pmu(struct arm_pmu *pmu, - const struct pmu_probe_info *info) +static int probe_plat_pmu(struct arm_pmu *pmu, + const struct pmu_probe_info *info, + unsigned int pmuid) { - int cpu = get_cpu(); - unsigned int cpuid = read_cpuid_id(); int ret = -ENODEV; + int cpu; + int aff_ctr = 0; + static int duplicate_pmus; + struct platform_device *pdev = pmu->plat_device; + int irq = platform_get_irq(pdev, 0); - pr_info("probing PMU on CPU %d\n", cpu); + if (irq >= 0 && !irq_is_percpu(irq)) { + pmu->irq_affinity = kcalloc(pdev->num_resources, sizeof(int), + GFP_KERNEL); + if (!pmu->irq_affinity) + return -ENOMEM; + } + for_each_possible_cpu(cpu) { + unsigned int cpuid = read_specific_cpuid(cpu); + + if (cpuid == pmuid) { + cpumask_set_cpu(cpu, &pmu->supported_cpus); + if (pmu->irq_affinity) { + pmu->irq_affinity[aff_ctr] = cpu; + aff_ctr++; + } + } + } + + /* find the type of PMU given the CPU */ for (; info->init != NULL; info++) { - if ((cpuid & info->mask) != info->cpuid) + if ((pmuid & info->mask) != info->cpuid) continue; ret = info->init(pmu); + /* + * if this pmu declaration is unspecified and we have + * previously found a PMU on this platform then append + * a PMU number to the pmu name. This avoids changing + * the names of PMUs that are specific to a class of CPUs. + * The assumption is that if we match a specific PMU in the + * provided pmu_probe_info then it's unique, and another PMU + * in the system will match a different entry rather than + * needing the _number to assure its unique. + */ + if ((!info->cpuid) && (duplicate_pmus)) { + pmu->name = kasprintf(GFP_KERNEL, "%s_%d", + pmu->name, duplicate_pmus); + if (!pmu->name) { + kfree(pmu->irq_affinity); + ret = -ENOMEM; + } + } + duplicate_pmus++; break; } - put_cpu(); return ret; } @@ -1030,8 +1074,7 @@ int arm_pmu_device_probe(struct platform_device *pdev, if (!ret) ret = init_fn(pmu); } else if (probe_table) { - cpumask_setall(&pmu->supported_cpus); - ret = probe_current_pmu(pmu, probe_table); + ret = probe_plat_pmu(pmu, probe_table, read_cpuid_id()); } if (ret) {
On systems with multiple PMU types the PMU to CPU affinity needs to be detected and set. The CPU to interrupt affinity should also be set. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- drivers/perf/arm_pmu.c | 63 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 53 insertions(+), 10 deletions(-)