Re: [PATCH 2/7] ARM: perf_event: Support percpu irqs for the CPU PMU

[Stephen Boyd <sboyd@codeaurora.org>]

On 01/13/14 03:52, Will Deacon wrote:
> On Fri, Jan 10, 2014 at 07:36:57PM +0000, Stephen Boyd wrote:
>>
>> Passing the hw_events as the pcpu token here is kind of hacky.
>> The reason is because the token is dereferenced into cpu_pmu in
>> armv7pmu_handle_irq() like so:
>>
>> 	struct arm_pmu *cpu_pmu = (struct arm_pmu *)dev;
>>
>> It would be great if we could pass cpu_pmu directly to the
>> request call like so:
>>
>> 	request_percpu_irq(irq, cpu_pmu->handle_irq, "arm-pmu", &cpu_pmu);
>>
>> but no. request_percpu_irq() wants a percpu pointer so this won't
>> work. If cpu_pmu was declared as DEFINE_PER_CPU, this would work
>> out just fine.
> That feels really broken though, since we rely on the cpu_pmu being a
> container for the struct pmu that was registered with perf core.
>
>> Should the cpu_pmu become a per-cpu variable? That sounds rather
>> invasive.
> I also don't think that's the right solution, based on the above. It's
> actually pretty hard to work out what's the right thing to do here...

Yes it doesn't seem like the right solution.

>
> We *could* have a per-cpu pointer to the cpu_pmu_pointer, but then we'd
> need to update the IRQ handlers, including things like the CCI PMU which
> really doesn't care about per-cpu stuff. So after all this, the shim we have
> around the IRQ handler for the U8500 SPI workarounds might be the right
> thing after all -- it allows us to consolidate the conversion of a pcpu
> pointer into the relevant instance (actually any instance, since they'd all
> point at the same thing) for the current CPU.
>
> What do you think to having that shim throw away the second level pcpu
> pointer in the case of a PPI? (probably means we need to revisit that
> renaming again).

Ok I think I understand what you're getting at. We pass a per-cpu
pointer to the cpu_pmu pointer as the dev_id argument to the PPI irq
handler, and then we check to see if the irq is per-cpu inside the
armpmu_dispatch_irq() function and throw away the second level of
pointer, i.e.

static irqreturn_t armpmu_dispatch_irq(int irq, void *dev)
{
        struct arm_pmu *armpmu;
        struct platform_device *plat_device;
        struct arm_pmu_platdata *plat;

        if (irq_is_percpu(irq))
                dev = *(struct arm_pmu_cpu **)dev;
        armpmu = dev;
        plat_device = armpmu->plat_device;
        plat = dev_get_platdata(&plat_device->dev);

        if (plat && plat->handle_irq)
                return plat->handle_irq(irq, dev, armpmu->handle_irq);
        else
                return armpmu->handle_irq(irq, dev);
}


We still need to make a per-cpu variable to hold the pointer, and assign
it during cpu_pmu_init like this patch does. Hopefully that is ok.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation