percpu irq APIs and perf

percpu irq APIs and perf

[Vineet Gupta]

Hi Marc / Daniel / Jason,

I had a couple of questions about percpu irq API, hopefully you can help answer.

On ARM, how do u handle requesting per cpu IRQs - specifically usage of request_percpu_irq() / enable_percpu_irq() API.
It seems, for using them, we obviously need to explicitly set irq as percpu and as a consequence explicitly enable autoen (since former disables that). See arch/arc/kernel/irq.c: arc_request_percpu_irq() called by ARC per cpu timer setup.

    if (!cpu)  {
           irq_set_percpu_devid()   <--- disables AUTOEN
           irq_modify_status(IRQ_NOAUTOEN)  <-- to undo side-effect of above
           request_percpu_irq
    }
    enable_percpu_irq

I don't see pattern in general for drivers/clocksource/ and/or arm_arch_timer.c for PPI case.

Further there is an ordering requirement as in request_percpu_irq() needs to be called only for the first calling core, and enable_percpu_irq() on each one. If enable is done ahead of request it obviously fails.

For ARC I've historically used a wrapper arc_request_percpu_irq() [pseudo code above] - which has an inherent assumption (now realize fragile) that it will be called on core0 first thus guaranteeing the ordering above. This is true for timer, IPI etc but not for other late probed peripherals - specially perf.

Infact ARC perf probe open codes on_each_cpu() to ensure irq request is done locally first.

But this all falls apart, when perf probe happens on coreX (not core0), causing enable to be called ahead of request anyways. This is what I'm running into now.

I think the solution is to call request_percpu_irq() on whatever core hits first and call enable_percpu_irq() from a cpu up notifier. But I think the notifier won't run on boot cpu ?  Or is there a better way to clean up all this mess.

FWIW, I see this issue on 3.18 kernel but not latest 4.4-rcX because in 3.18 arc perf probe invariably happens on coreX (due to init task migration right after clocksource switch - something which doesn't happen on 4.4 likely due to recent timer core changes).

Thx,
-Vineet

percpu irq APIs and perf

[Vineet Gupta]

Hi Marc / Daniel / Jason,

I had a couple of questions about percpu irq API, hopefully you can help answer.

On ARM, how do u handle requesting per cpu IRQs - specifically usage of request_percpu_irq() / enable_percpu_irq() API.
It seems, for using them, we obviously need to explicitly set irq as percpu and as a consequence explicitly enable autoen (since former disables that). See arch/arc/kernel/irq.c: arc_request_percpu_irq() called by ARC per cpu timer setup.

    if (!cpu)  {
           irq_set_percpu_devid()   <--- disables AUTOEN
           irq_modify_status(IRQ_NOAUTOEN)  <-- to undo side-effect of above
           request_percpu_irq
    }
    enable_percpu_irq

I don't see pattern in general for drivers/clocksource/ and/or arm_arch_timer.c for PPI case.

Further there is an ordering requirement as in request_percpu_irq() needs to be called only for the first calling core, and enable_percpu_irq() on each one. If enable is done ahead of request it obviously fails.

For ARC I've historically used a wrapper arc_request_percpu_irq() [pseudo code above] - which has an inherent assumption (now realize fragile) that it will be called on core0 first thus guaranteeing the ordering above. This is true for timer, IPI etc but not for other late probed peripherals - specially perf.

Infact ARC perf probe open codes on_each_cpu() to ensure irq request is done locally first.

But this all falls apart, when perf probe happens on coreX (not core0), causing enable to be called ahead of request anyways. This is what I'm running into now.

I think the solution is to call request_percpu_irq() on whatever core hits first and call enable_percpu_irq() from a cpu up notifier. But I think the notifier won't run on boot cpu ?  Or is there a better way to clean up all this mess.

FWIW, I see this issue on 3.18 kernel but not latest 4.4-rcX because in 3.18 arc perf probe invariably happens on coreX (due to init task migration right after clocksource switch - something which doesn't happen on 4.4 likely due to recent timer core changes).

Thx,
-Vineet

Re: percpu irq APIs and perf

[Marc Zyngier]

Hi Vinnet,

On 10/12/15 09:25, Vineet Gupta wrote:
> Hi Marc / Daniel / Jason,
> 
> I had a couple of questions about percpu irq API, hopefully you can help answer.
> 
> On ARM, how do u handle requesting per cpu IRQs - specifically usage
> of request_percpu_irq() / enable_percpu_irq() API.
> It seems, for using them, we obviously need to explicitly set irq as
> percpu and as a consequence explicitly enable autoen (since former
> disables that). See arch/arc/kernel/irq.c: arc_request_percpu_irq()
> called by ARC per cpu timer setup.

Indeed. The interrupt controller code flags these interrupts as being
per-cpu, and we do rely on each CPU performing an enable_percpu_irq().

So the way the whole thing flows is as such:
- Interrupt controller (GIC) flags the PPIs (Private Peripheral
Interrupt) as per-CPU (hwirq 16 to 31 are replicated per CPU) very early
in the boot process
- request_percpu_irq() only occurs once, usually on the boot CPU (but
that's not a requirement)
- each CPU executes enable_percpu_irq(), which touches per-CPU
registers. This usually involves a CPU notifier to enable/disable the
interrupt when hotplug is on.


>     if (!cpu)  {
>            irq_set_percpu_devid()   <--- disables AUTOEN
>            irq_modify_status(IRQ_NOAUTOEN)  <-- to undo side-effect of above
>            request_percpu_irq
>     }
>     enable_percpu_irq
> 
> I don't see pattern in general for drivers/clocksource/ and/or
> arm_arch_timer.c for PPI case.

You can have a look at arch/arm/smp/smp_twd.c which is probably less
cryptic.

> Further there is an ordering requirement as in request_percpu_irq()
> needs to be called only for the first calling core, and
> enable_percpu_irq() on each one. If enable is done ahead of request
> it obviously fails.

Yup.

> For ARC I've historically used a wrapper arc_request_percpu_irq()
> [pseudo code above] - which has an inherent assumption (now realize
> fragile) that it will be called on core0 first thus guaranteeing the
> ordering above. This is true for timer, IPI etc but not for other
> late probed peripherals - specially perf.
> 
> Infact ARC perf probe open codes on_each_cpu() to ensure irq request
> is done locally first.
> 
> But this all falls apart, when perf probe happens on coreX (not
> core0), causing enable to be called ahead of request anyways. This is
> what I'm running into now.
> 
> I think the solution is to call request_percpu_irq() on whatever core
> hits first and call enable_percpu_irq() from a cpu up notifier. But I
> think the notifier won't run on boot cpu ?  Or is there a better way
> to clean up all this mess.

I think that's pretty much it.

See drivers/perf/arm_pmu.c::cpu_pmu_request_irq() for example.

> FWIW, I see this issue on 3.18 kernel but not latest 4.4-rcX because
> in 3.18 arc perf probe invariably happens on coreX (due to init task
> migration right after clocksource switch - something which doesn't
> happen on 4.4 likely due to recent timer core changes).

Hope this helps,

	M.
-- 
Jazz is not dead. It just smells funny...

percpu irq APIs and perf

[Marc Zyngier]

Hi Vinnet,

On 10/12/15 09:25, Vineet Gupta wrote:
> Hi Marc / Daniel / Jason,
> 
> I had a couple of questions about percpu irq API, hopefully you can help answer.
> 
> On ARM, how do u handle requesting per cpu IRQs - specifically usage
> of request_percpu_irq() / enable_percpu_irq() API.
> It seems, for using them, we obviously need to explicitly set irq as
> percpu and as a consequence explicitly enable autoen (since former
> disables that). See arch/arc/kernel/irq.c: arc_request_percpu_irq()
> called by ARC per cpu timer setup.

Indeed. The interrupt controller code flags these interrupts as being
per-cpu, and we do rely on each CPU performing an enable_percpu_irq().

So the way the whole thing flows is as such:
- Interrupt controller (GIC) flags the PPIs (Private Peripheral
Interrupt) as per-CPU (hwirq 16 to 31 are replicated per CPU) very early
in the boot process
- request_percpu_irq() only occurs once, usually on the boot CPU (but
that's not a requirement)
- each CPU executes enable_percpu_irq(), which touches per-CPU
registers. This usually involves a CPU notifier to enable/disable the
interrupt when hotplug is on.


>     if (!cpu)  {
>            irq_set_percpu_devid()   <--- disables AUTOEN
>            irq_modify_status(IRQ_NOAUTOEN)  <-- to undo side-effect of above
>            request_percpu_irq
>     }
>     enable_percpu_irq
> 
> I don't see pattern in general for drivers/clocksource/ and/or
> arm_arch_timer.c for PPI case.

You can have a look at arch/arm/smp/smp_twd.c which is probably less
cryptic.

> Further there is an ordering requirement as in request_percpu_irq()
> needs to be called only for the first calling core, and
> enable_percpu_irq() on each one. If enable is done ahead of request
> it obviously fails.

Yup.

> For ARC I've historically used a wrapper arc_request_percpu_irq()
> [pseudo code above] - which has an inherent assumption (now realize
> fragile) that it will be called on core0 first thus guaranteeing the
> ordering above. This is true for timer, IPI etc but not for other
> late probed peripherals - specially perf.
> 
> Infact ARC perf probe open codes on_each_cpu() to ensure irq request
> is done locally first.
> 
> But this all falls apart, when perf probe happens on coreX (not
> core0), causing enable to be called ahead of request anyways. This is
> what I'm running into now.
> 
> I think the solution is to call request_percpu_irq() on whatever core
> hits first and call enable_percpu_irq() from a cpu up notifier. But I
> think the notifier won't run on boot cpu ?  Or is there a better way
> to clean up all this mess.

I think that's pretty much it.

See drivers/perf/arm_pmu.c::cpu_pmu_request_irq() for example.

> FWIW, I see this issue on 3.18 kernel but not latest 4.4-rcX because
> in 3.18 arc perf probe invariably happens on coreX (due to init task
> migration right after clocksource switch - something which doesn't
> happen on 4.4 likely due to recent timer core changes).

Hope this helps,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: percpu irq APIs and perf

[Vineet Gupta]

Hi Marc,

On Thursday 10 December 2015 03:26 PM, Marc Zyngier wrote:
> Hi Vinnet,
>
> On 10/12/15 09:25, Vineet Gupta wrote:
>> Hi Marc / Daniel / Jason,
>>
>> I had a couple of questions about percpu irq API, hopefully you can help answer.
>>
>> On ARM, how do u handle requesting per cpu IRQs - specifically usage
>> of request_percpu_irq() / enable_percpu_irq() API.
>> It seems, for using them, we obviously need to explicitly set irq as
>> percpu and as a consequence explicitly enable autoen (since former
>> disables that). See arch/arc/kernel/irq.c: arc_request_percpu_irq()
>> called by ARC per cpu timer setup.
> Indeed. The interrupt controller code flags these interrupts as being
> per-cpu, and we do rely on each CPU performing an enable_percpu_irq().
>
> So the way the whole thing flows is as such:
> - Interrupt controller (GIC) flags the PPIs (Private Peripheral
> Interrupt) as per-CPU (hwirq 16 to 31 are replicated per CPU) very early
> in the boot process

Thx for your reply and the pointers

irq-gic.c seems to be doing

   irq_set_status_flags(irq, IRQ_NOAUTOEN);

So it is setting NOAUTOEN, which is already the case per side effect of
irq_set_percpu_devid(). No harm in making it explicit.
So this will make __setup_irq() skip irq_startup() and instead rely on
enable_precpu_irq() to be called even for the local core.

I think we can make percpu irq API a bit easier to use.

(1) First thing which request_percpu_irq() does is check for
irq_settings_is_per_cpu_devid(). Thus irq_set_percpu_devid() can be built into the
API itself eliding the need to set it apriori.

(2) It seems that disabling autoen by default for percpu irq makes sense as
evident from drivers/net/ethernet/marvell/mvneta.c where users want to control
this. However the comment there is misleading

    /* Even though the documentation says that request_percpu_irq
     * doesn't enable the interrupts automatically, it actually
     * does so on the local CPU.
     *
     * Make sure it's disabled.
     */

Either sme core code is clearing NOAUTOEN or calling enable_precpu_irq() making
request_percpu_irq() enable it.

IMHO it makes more sense to make autoen explicit in the API.
Perhaps introduce a API flavour, which takes the autoen as arg.
It could take flags to make it more extensible / future safe but that will be an
overkill I think.

Do let me know what you think and I can send RFC patches to same effect.

Thx,
-Vineet

percpu irq APIs and perf

[Vineet Gupta]

Hi Marc,

On Thursday 10 December 2015 03:26 PM, Marc Zyngier wrote:
> Hi Vinnet,
>
> On 10/12/15 09:25, Vineet Gupta wrote:
>> Hi Marc / Daniel / Jason,
>>
>> I had a couple of questions about percpu irq API, hopefully you can help answer.
>>
>> On ARM, how do u handle requesting per cpu IRQs - specifically usage
>> of request_percpu_irq() / enable_percpu_irq() API.
>> It seems, for using them, we obviously need to explicitly set irq as
>> percpu and as a consequence explicitly enable autoen (since former
>> disables that). See arch/arc/kernel/irq.c: arc_request_percpu_irq()
>> called by ARC per cpu timer setup.
> Indeed. The interrupt controller code flags these interrupts as being
> per-cpu, and we do rely on each CPU performing an enable_percpu_irq().
>
> So the way the whole thing flows is as such:
> - Interrupt controller (GIC) flags the PPIs (Private Peripheral
> Interrupt) as per-CPU (hwirq 16 to 31 are replicated per CPU) very early
> in the boot process

Thx for your reply and the pointers

irq-gic.c seems to be doing

   irq_set_status_flags(irq, IRQ_NOAUTOEN);

So it is setting NOAUTOEN, which is already the case per side effect of
irq_set_percpu_devid(). No harm in making it explicit.
So this will make __setup_irq() skip irq_startup() and instead rely on
enable_precpu_irq() to be called even for the local core.

I think we can make percpu irq API a bit easier to use.

(1) First thing which request_percpu_irq() does is check for
irq_settings_is_per_cpu_devid(). Thus irq_set_percpu_devid() can be built into the
API itself eliding the need to set it apriori.

(2) It seems that disabling autoen by default for percpu irq makes sense as
evident from drivers/net/ethernet/marvell/mvneta.c where users want to control
this. However the comment there is misleading

    /* Even though the documentation says that request_percpu_irq
     * doesn't enable the interrupts automatically, it actually
     * does so on the local CPU.
     *
     * Make sure it's disabled.
     */

Either sme core code is clearing NOAUTOEN or calling enable_precpu_irq() making
request_percpu_irq() enable it.

IMHO it makes more sense to make autoen explicit in the API.
Perhaps introduce a API flavour, which takes the autoen as arg.
It could take flags to make it more extensible / future safe but that will be an
overkill I think.

Do let me know what you think and I can send RFC patches to same effect.

Thx,
-Vineet

Re: percpu irq APIs and perf

[Marc Zyngier]

On Fri, 11 Dec 2015 05:26:02 +0000
Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:

Hi Vineet,

> Hi Marc,
> 
> On Thursday 10 December 2015 03:26 PM, Marc Zyngier wrote:
> > Hi Vinnet,
> >
> > On 10/12/15 09:25, Vineet Gupta wrote:
> >> Hi Marc / Daniel / Jason,
> >>
> >> I had a couple of questions about percpu irq API, hopefully you can help answer.
> >>
> >> On ARM, how do u handle requesting per cpu IRQs - specifically usage
> >> of request_percpu_irq() / enable_percpu_irq() API.
> >> It seems, for using them, we obviously need to explicitly set irq as
> >> percpu and as a consequence explicitly enable autoen (since former
> >> disables that). See arch/arc/kernel/irq.c: arc_request_percpu_irq()
> >> called by ARC per cpu timer setup.
> > Indeed. The interrupt controller code flags these interrupts as being
> > per-cpu, and we do rely on each CPU performing an enable_percpu_irq().
> >
> > So the way the whole thing flows is as such:
> > - Interrupt controller (GIC) flags the PPIs (Private Peripheral
> > Interrupt) as per-CPU (hwirq 16 to 31 are replicated per CPU) very early
> > in the boot process
> 
> Thx for your reply and the pointers
> 
> irq-gic.c seems to be doing
> 
>    irq_set_status_flags(irq, IRQ_NOAUTOEN);
> 
> So it is setting NOAUTOEN, which is already the case per side effect of
> irq_set_percpu_devid(). No harm in making it explicit.

Indeed, this looks completely superfluous. I'll fix that.

> So this will make __setup_irq() skip irq_startup() and instead rely on
> enable_precpu_irq() to be called even for the local core.
> 
> I think we can make percpu irq API a bit easier to use.
> 
> (1) First thing which request_percpu_irq() does is check for
> irq_settings_is_per_cpu_devid(). Thus irq_set_percpu_devid() can be built into the
> API itself eliding the need to set it apriori.

I don't think we can. At least in the case I'm concerned about (GIC's
PPIs), this is a hardware requirement. You cannot turn a global
interrupt into a per-CPU one, nor the other way around. We also have
drivers (at least our PMUs) that do test the state of that interrupt
(per-CPU or not) to find out how they should be requested.

I agree that the API is probably not the ideal one, but there is HW
constraints that we cannot just ignore.

> (2) It seems that disabling autoen by default for percpu irq makes sense as
> evident from drivers/net/ethernet/marvell/mvneta.c where users want to control
> this. However the comment there is misleading
> 
>     /* Even though the documentation says that request_percpu_irq
>      * doesn't enable the interrupts automatically, it actually
>      * does so on the local CPU.
>      *
>      * Make sure it's disabled.
>      */
> 
> Either sme core code is clearing NOAUTOEN or calling enable_precpu_irq() making
> request_percpu_irq() enable it.

If that's the case, this is a bug. Nobody should enable that interrupt
until the driver has chosen to do so.

> IMHO it makes more sense to make autoen explicit in the API.
> Perhaps introduce a API flavour, which takes the autoen as arg.
> It could take flags to make it more extensible / future safe but that will be an
> overkill I think.

But auto-enabling cannot be done from a single CPU. It can only be done
from the core that is going to be delivered that interrupt. This
requires access to registers that are simply not available to other CPUs.

> Do let me know what you think and I can send RFC patches to same effect.

If you can find an elegant way to do this and keep the existing
semantics, I'm all ear!

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

percpu irq APIs and perf

[Marc Zyngier]

On Fri, 11 Dec 2015 05:26:02 +0000
Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:

Hi Vineet,

> Hi Marc,
> 
> On Thursday 10 December 2015 03:26 PM, Marc Zyngier wrote:
> > Hi Vinnet,
> >
> > On 10/12/15 09:25, Vineet Gupta wrote:
> >> Hi Marc / Daniel / Jason,
> >>
> >> I had a couple of questions about percpu irq API, hopefully you can help answer.
> >>
> >> On ARM, how do u handle requesting per cpu IRQs - specifically usage
> >> of request_percpu_irq() / enable_percpu_irq() API.
> >> It seems, for using them, we obviously need to explicitly set irq as
> >> percpu and as a consequence explicitly enable autoen (since former
> >> disables that). See arch/arc/kernel/irq.c: arc_request_percpu_irq()
> >> called by ARC per cpu timer setup.
> > Indeed. The interrupt controller code flags these interrupts as being
> > per-cpu, and we do rely on each CPU performing an enable_percpu_irq().
> >
> > So the way the whole thing flows is as such:
> > - Interrupt controller (GIC) flags the PPIs (Private Peripheral
> > Interrupt) as per-CPU (hwirq 16 to 31 are replicated per CPU) very early
> > in the boot process
> 
> Thx for your reply and the pointers
> 
> irq-gic.c seems to be doing
> 
>    irq_set_status_flags(irq, IRQ_NOAUTOEN);
> 
> So it is setting NOAUTOEN, which is already the case per side effect of
> irq_set_percpu_devid(). No harm in making it explicit.

Indeed, this looks completely superfluous. I'll fix that.

> So this will make __setup_irq() skip irq_startup() and instead rely on
> enable_precpu_irq() to be called even for the local core.
> 
> I think we can make percpu irq API a bit easier to use.
> 
> (1) First thing which request_percpu_irq() does is check for
> irq_settings_is_per_cpu_devid(). Thus irq_set_percpu_devid() can be built into the
> API itself eliding the need to set it apriori.

I don't think we can. At least in the case I'm concerned about (GIC's
PPIs), this is a hardware requirement. You cannot turn a global
interrupt into a per-CPU one, nor the other way around. We also have
drivers (at least our PMUs) that do test the state of that interrupt
(per-CPU or not) to find out how they should be requested.

I agree that the API is probably not the ideal one, but there is HW
constraints that we cannot just ignore.

> (2) It seems that disabling autoen by default for percpu irq makes sense as
> evident from drivers/net/ethernet/marvell/mvneta.c where users want to control
> this. However the comment there is misleading
> 
>     /* Even though the documentation says that request_percpu_irq
>      * doesn't enable the interrupts automatically, it actually
>      * does so on the local CPU.
>      *
>      * Make sure it's disabled.
>      */
> 
> Either sme core code is clearing NOAUTOEN or calling enable_precpu_irq() making
> request_percpu_irq() enable it.

If that's the case, this is a bug. Nobody should enable that interrupt
until the driver has chosen to do so.

> IMHO it makes more sense to make autoen explicit in the API.
> Perhaps introduce a API flavour, which takes the autoen as arg.
> It could take flags to make it more extensible / future safe but that will be an
> overkill I think.

But auto-enabling cannot be done from a single CPU. It can only be done
from the core that is going to be delivered that interrupt. This
requires access to registers that are simply not available to other CPUs.

> Do let me know what you think and I can send RFC patches to same effect.

If you can find an elegant way to do this and keep the existing
semantics, I'm all ear!

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: percpu irq APIs and perf

[Vineet Gupta]

Hi Marc,

On Friday 11 December 2015 04:53 PM, Marc Zyngier wrote:
> On Fri, 11 Dec 2015 05:26:02 +0000
>> I think we can make percpu irq API a bit easier to use.
>>
>> (1) First thing which request_percpu_irq() does is check for
>> irq_settings_is_per_cpu_devid(). Thus irq_set_percpu_devid() can be built into the
>> API itself eliding the need to set it apriori.
> 
> I don't think we can. At least in the case I'm concerned about (GIC's
> PPIs), this is a hardware requirement. You cannot turn a global
> interrupt into a per-CPU one, nor the other way around.

Understood.

> We also have
> drivers (at least our PMUs) that do test the state of that interrupt
> (per-CPU or not) to find out how they should be requested.

But they call request_percpu_irq() only after determining that irq is percpu.
Otherwise they will call vanilla request_irq()
e.g. drivers/perf/arm/arc_pmu.c

Which means that request_percpu_irq() can safely assume that caller absolutely
wants percpu semantics and hence do equivalent of irq_set_percpu_devid()
internally - NO. I'm sure I'm missing something.


> I agree that the API is probably not the ideal one, but there is HW
> constraints that we cannot just ignore.

The API is pretty nice :-) there are these quirks which I want to avoid.
My naive'ity in this area of code fails me to see how the hardware constraint is
coming into play.


>> (2) It seems that disabling autoen by default for percpu irq makes sense as
>> evident from drivers/net/ethernet/marvell/mvneta.c where users want to control
>> this. However the comment there is misleading
>>
>>     /* Even though the documentation says that request_percpu_irq
>>      * doesn't enable the interrupts automatically, it actually
>>      * does so on the local CPU.
>>      *
>>      * Make sure it's disabled.
>>      */
>>
>> Either sme core code is clearing NOAUTOEN or calling enable_precpu_irq() making
>> request_percpu_irq() enable it.
> 
> If that's the case, this is a bug. Nobody should enable that interrupt
> until the driver has chosen to do so.

Perhaps Maxim can shed more light as this seems to be his comment.


>> IMHO it makes more sense to make autoen explicit in the API.
>> Perhaps introduce a API flavour, which takes the autoen as arg.
>> It could take flags to make it more extensible / future safe but that will be an
>> overkill I think.
> 
> But auto-enabling cannot be done from a single CPU. It can only be done
> from the core that is going to be delivered that interrupt. This
> requires access to registers that are simply not available to other CPUs.

I'm not talking about eliminating enable_percpu_irq() call from all cores and
still getting the auto-enable semantics. What I mean is doing the equivalent of

     irq_set_status_flags(irq, IRQ_NOAUTOEN);

from within request_percpu_irq_xxx() based on an additional arg (vs. doing it
aprioiri outside).

OTOH, thinking a bit more abt this, I think the current semantics of auto-disable
w/o any arg is just fine. Most percpu irqs in general purpose drivers would want
the auto-disable anyways. Only for core irws such as timer / IPI etc do we want
auto-enable.

Thx,
-Vineet

percpu irq APIs and perf

[Vineet Gupta]

Hi Marc,

On Friday 11 December 2015 04:53 PM, Marc Zyngier wrote:
> On Fri, 11 Dec 2015 05:26:02 +0000
>> I think we can make percpu irq API a bit easier to use.
>>
>> (1) First thing which request_percpu_irq() does is check for
>> irq_settings_is_per_cpu_devid(). Thus irq_set_percpu_devid() can be built into the
>> API itself eliding the need to set it apriori.
> 
> I don't think we can. At least in the case I'm concerned about (GIC's
> PPIs), this is a hardware requirement. You cannot turn a global
> interrupt into a per-CPU one, nor the other way around.

Understood.

> We also have
> drivers (at least our PMUs) that do test the state of that interrupt
> (per-CPU or not) to find out how they should be requested.

But they call request_percpu_irq() only after determining that irq is percpu.
Otherwise they will call vanilla request_irq()
e.g. drivers/perf/arm/arc_pmu.c

Which means that request_percpu_irq() can safely assume that caller absolutely
wants percpu semantics and hence do equivalent of irq_set_percpu_devid()
internally - NO. I'm sure I'm missing something.


> I agree that the API is probably not the ideal one, but there is HW
> constraints that we cannot just ignore.

The API is pretty nice :-) there are these quirks which I want to avoid.
My naive'ity in this area of code fails me to see how the hardware constraint is
coming into play.


>> (2) It seems that disabling autoen by default for percpu irq makes sense as
>> evident from drivers/net/ethernet/marvell/mvneta.c where users want to control
>> this. However the comment there is misleading
>>
>>     /* Even though the documentation says that request_percpu_irq
>>      * doesn't enable the interrupts automatically, it actually
>>      * does so on the local CPU.
>>      *
>>      * Make sure it's disabled.
>>      */
>>
>> Either sme core code is clearing NOAUTOEN or calling enable_precpu_irq() making
>> request_percpu_irq() enable it.
> 
> If that's the case, this is a bug. Nobody should enable that interrupt
> until the driver has chosen to do so.

Perhaps Maxim can shed more light as this seems to be his comment.


>> IMHO it makes more sense to make autoen explicit in the API.
>> Perhaps introduce a API flavour, which takes the autoen as arg.
>> It could take flags to make it more extensible / future safe but that will be an
>> overkill I think.
> 
> But auto-enabling cannot be done from a single CPU. It can only be done
> from the core that is going to be delivered that interrupt. This
> requires access to registers that are simply not available to other CPUs.

I'm not talking about eliminating enable_percpu_irq() call from all cores and
still getting the auto-enable semantics. What I mean is doing the equivalent of

     irq_set_status_flags(irq, IRQ_NOAUTOEN);

from within request_percpu_irq_xxx() based on an additional arg (vs. doing it
aprioiri outside).

OTOH, thinking a bit more abt this, I think the current semantics of auto-disable
w/o any arg is just fine. Most percpu irqs in general purpose drivers would want
the auto-disable anyways. Only for core irws such as timer / IPI etc do we want
auto-enable.

Thx,
-Vineet

Re: percpu irq APIs and perf

[Marc Zyngier]

Vineet,

On 11/12/15 12:20, Vineet Gupta wrote:
> Hi Marc,
> 
> On Friday 11 December 2015 04:53 PM, Marc Zyngier wrote:
>> On Fri, 11 Dec 2015 05:26:02 +0000
>>> I think we can make percpu irq API a bit easier to use.
>>>
>>> (1) First thing which request_percpu_irq() does is check for
>>> irq_settings_is_per_cpu_devid(). Thus irq_set_percpu_devid() can be built into the
>>> API itself eliding the need to set it apriori.
>>
>> I don't think we can. At least in the case I'm concerned about (GIC's
>> PPIs), this is a hardware requirement. You cannot turn a global
>> interrupt into a per-CPU one, nor the other way around.
> 
> Understood.
> 
>> We also have
>> drivers (at least our PMUs) that do test the state of that interrupt
>> (per-CPU or not) to find out how they should be requested.
> 
> But they call request_percpu_irq() only after determining that irq is percpu.
> Otherwise they will call vanilla request_irq()
> e.g. drivers/perf/arm/arc_pmu.c
> 
> Which means that request_percpu_irq() can safely assume that caller absolutely
> wants percpu semantics and hence do equivalent of irq_set_percpu_devid()
> internally - NO. I'm sure I'm missing something.

It actually works the other way around. The caller cannot find out about
the per-cpu property of the interrupt just by looking at the virtual IRQ
number. It needs to ask the core code about it, and that's why the GIC
tags these interrupts as per-cpu.

>> I agree that the API is probably not the ideal one, but there is HW
>> constraints that we cannot just ignore.
> 
> The API is pretty nice :-) there are these quirks which I want to avoid.
> My naive'ity in this area of code fails me to see how the hardware constraint is
> coming into play.
> 
> 
>>> (2) It seems that disabling autoen by default for percpu irq makes sense as
>>> evident from drivers/net/ethernet/marvell/mvneta.c where users want to control
>>> this. However the comment there is misleading
>>>
>>>     /* Even though the documentation says that request_percpu_irq
>>>      * doesn't enable the interrupts automatically, it actually
>>>      * does so on the local CPU.
>>>      *
>>>      * Make sure it's disabled.
>>>      */
>>>
>>> Either sme core code is clearing NOAUTOEN or calling enable_precpu_irq() making
>>> request_percpu_irq() enable it.
>>
>> If that's the case, this is a bug. Nobody should enable that interrupt
>> until the driver has chosen to do so.
> 
> Perhaps Maxim can shed more light as this seems to be his comment.
> 
> 
>>> IMHO it makes more sense to make autoen explicit in the API.
>>> Perhaps introduce a API flavour, which takes the autoen as arg.
>>> It could take flags to make it more extensible / future safe but that will be an
>>> overkill I think.
>>
>> But auto-enabling cannot be done from a single CPU. It can only be done
>> from the core that is going to be delivered that interrupt. This
>> requires access to registers that are simply not available to other CPUs.
> 
> I'm not talking about eliminating enable_percpu_irq() call from all cores and
> still getting the auto-enable semantics. What I mean is doing the equivalent of
> 
>      irq_set_status_flags(irq, IRQ_NOAUTOEN);
> 
> from within request_percpu_irq_xxx() based on an additional arg (vs. doing it
> aprioiri outside).
> 
> OTOH, thinking a bit more abt this, I think the current semantics of auto-disable
> w/o any arg is just fine. Most percpu irqs in general purpose drivers would want
> the auto-disable anyways. Only for core irws such as timer / IPI etc do we want
> auto-enable.

So assuming we can do this (forgetting about any form of HW limitation):
CPU0 request the per-CPU IRQ with an AUTOEN flag. What happens on CPU1?
Are you expecting it to immediately be able to take interrupts? What
handler data gets passed to it?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

percpu irq APIs and perf

[Marc Zyngier]

Vineet,

On 11/12/15 12:20, Vineet Gupta wrote:
> Hi Marc,
> 
> On Friday 11 December 2015 04:53 PM, Marc Zyngier wrote:
>> On Fri, 11 Dec 2015 05:26:02 +0000
>>> I think we can make percpu irq API a bit easier to use.
>>>
>>> (1) First thing which request_percpu_irq() does is check for
>>> irq_settings_is_per_cpu_devid(). Thus irq_set_percpu_devid() can be built into the
>>> API itself eliding the need to set it apriori.
>>
>> I don't think we can. At least in the case I'm concerned about (GIC's
>> PPIs), this is a hardware requirement. You cannot turn a global
>> interrupt into a per-CPU one, nor the other way around.
> 
> Understood.
> 
>> We also have
>> drivers (at least our PMUs) that do test the state of that interrupt
>> (per-CPU or not) to find out how they should be requested.
> 
> But they call request_percpu_irq() only after determining that irq is percpu.
> Otherwise they will call vanilla request_irq()
> e.g. drivers/perf/arm/arc_pmu.c
> 
> Which means that request_percpu_irq() can safely assume that caller absolutely
> wants percpu semantics and hence do equivalent of irq_set_percpu_devid()
> internally - NO. I'm sure I'm missing something.

It actually works the other way around. The caller cannot find out about
the per-cpu property of the interrupt just by looking at the virtual IRQ
number. It needs to ask the core code about it, and that's why the GIC
tags these interrupts as per-cpu.

>> I agree that the API is probably not the ideal one, but there is HW
>> constraints that we cannot just ignore.
> 
> The API is pretty nice :-) there are these quirks which I want to avoid.
> My naive'ity in this area of code fails me to see how the hardware constraint is
> coming into play.
> 
> 
>>> (2) It seems that disabling autoen by default for percpu irq makes sense as
>>> evident from drivers/net/ethernet/marvell/mvneta.c where users want to control
>>> this. However the comment there is misleading
>>>
>>>     /* Even though the documentation says that request_percpu_irq
>>>      * doesn't enable the interrupts automatically, it actually
>>>      * does so on the local CPU.
>>>      *
>>>      * Make sure it's disabled.
>>>      */
>>>
>>> Either sme core code is clearing NOAUTOEN or calling enable_precpu_irq() making
>>> request_percpu_irq() enable it.
>>
>> If that's the case, this is a bug. Nobody should enable that interrupt
>> until the driver has chosen to do so.
> 
> Perhaps Maxim can shed more light as this seems to be his comment.
> 
> 
>>> IMHO it makes more sense to make autoen explicit in the API.
>>> Perhaps introduce a API flavour, which takes the autoen as arg.
>>> It could take flags to make it more extensible / future safe but that will be an
>>> overkill I think.
>>
>> But auto-enabling cannot be done from a single CPU. It can only be done
>> from the core that is going to be delivered that interrupt. This
>> requires access to registers that are simply not available to other CPUs.
> 
> I'm not talking about eliminating enable_percpu_irq() call from all cores and
> still getting the auto-enable semantics. What I mean is doing the equivalent of
> 
>      irq_set_status_flags(irq, IRQ_NOAUTOEN);
> 
> from within request_percpu_irq_xxx() based on an additional arg (vs. doing it
> aprioiri outside).
> 
> OTOH, thinking a bit more abt this, I think the current semantics of auto-disable
> w/o any arg is just fine. Most percpu irqs in general purpose drivers would want
> the auto-disable anyways. Only for core irws such as timer / IPI etc do we want
> auto-enable.

So assuming we can do this (forgetting about any form of HW limitation):
CPU0 request the per-CPU IRQ with an AUTOEN flag. What happens on CPU1?
Are you expecting it to immediately be able to take interrupts? What
handler data gets passed to it?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: percpu irq APIs and perf

[Vineet Gupta]

On Friday 11 December 2015 11:28 PM, Marc Zyngier wrote:
>>> But auto-enabling cannot be done from a single CPU. It can only be done
>>> >> from the core that is going to be delivered that interrupt. This
>>> >> requires access to registers that are simply not available to other CPUs.
>> > 
>> > I'm not talking about eliminating enable_percpu_irq() call from all cores and
>> > still getting the auto-enable semantics. What I mean is doing the equivalent of
>> > 
>> >      irq_set_status_flags(irq, IRQ_NOAUTOEN);
>> > 
>> > from within request_percpu_irq_xxx() based on an additional arg (vs. doing it
>> > aprioiri outside).
>> > 
>> > OTOH, thinking a bit more abt this, I think the current semantics of auto-disable
>> > w/o any arg is just fine. Most percpu irqs in general purpose drivers would want
>> > the auto-disable anyways. Only for core irws such as timer / IPI etc do we want
>> > auto-enable.
> So assuming we can do this (forgetting about any form of HW limitation):
> CPU0 request the per-CPU IRQ with an AUTOEN flag. What happens on CPU1?
> Are you expecting it to immediately be able to take interrupts? What
> handler data gets passed to it?

Right - autoen=true will only help on CPU0. Others will still need to call enable
- for register tinkling etc. A true autoen API should have done that across the
board which it clearly can't and hence is pointless.

Thx,
-Vineet

percpu irq APIs and perf

[Vineet Gupta]

On Friday 11 December 2015 11:28 PM, Marc Zyngier wrote:
>>> But auto-enabling cannot be done from a single CPU. It can only be done
>>> >> from the core that is going to be delivered that interrupt. This
>>> >> requires access to registers that are simply not available to other CPUs.
>> > 
>> > I'm not talking about eliminating enable_percpu_irq() call from all cores and
>> > still getting the auto-enable semantics. What I mean is doing the equivalent of
>> > 
>> >      irq_set_status_flags(irq, IRQ_NOAUTOEN);
>> > 
>> > from within request_percpu_irq_xxx() based on an additional arg (vs. doing it
>> > aprioiri outside).
>> > 
>> > OTOH, thinking a bit more abt this, I think the current semantics of auto-disable
>> > w/o any arg is just fine. Most percpu irqs in general purpose drivers would want
>> > the auto-disable anyways. Only for core irws such as timer / IPI etc do we want
>> > auto-enable.
> So assuming we can do this (forgetting about any form of HW limitation):
> CPU0 request the per-CPU IRQ with an AUTOEN flag. What happens on CPU1?
> Are you expecting it to immediately be able to take interrupts? What
> handler data gets passed to it?

Right - autoen=true will only help on CPU0. Others will still need to call enable
- for register tinkling etc. A true autoen API should have done that across the
board which it clearly can't and hence is pointless.

Thx,
-Vineet

Re: percpu irq APIs and perf

[maxime.ripard]

[-- Attachment #1: Type: text/plain, Size: 1587 bytes --]

Hi,

On Fri, Dec 11, 2015 at 05:50:22PM +0530, Vineet Gupta wrote:
> >> (2) It seems that disabling autoen by default for percpu irq makes sense as
> >> evident from drivers/net/ethernet/marvell/mvneta.c where users want to control
> >> this. However the comment there is misleading
> >>
> >>     /* Even though the documentation says that request_percpu_irq
> >>      * doesn't enable the interrupts automatically, it actually
> >>      * does so on the local CPU.
> >>      *
> >>      * Make sure it's disabled.
> >>      */
> >>
> >> Either sme core code is clearing NOAUTOEN or calling enable_precpu_irq() making
> >> request_percpu_irq() enable it.
> > 
> > If that's the case, this is a bug. Nobody should enable that interrupt
> > until the driver has chosen to do so.
> 
> Perhaps Maxim can shed more light as this seems to be his comment.

I don't really have the full context here, but we did this in order to
allow RX queue interrupts to be enabled on a particular CPU.

The IP was supposed to do that by itself, but we hadn't figure that
out at the time. So what we ended up doing is disabling the interrupts
by default and then enable it only on the CPU meant to receive the
queue interrupts.

What we found out doing so was that the per-cpu interrupts were
enabled on the cpu calling request_percpu_irq, and so that's why it
ended up with that comment.

I also fixed the documentation accordingly in a1b7febd725a.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

percpu irq APIs and perf

[maxime.ripard]

Hi,

On Fri, Dec 11, 2015@05:50:22PM +0530, Vineet Gupta wrote:
> >> (2) It seems that disabling autoen by default for percpu irq makes sense as
> >> evident from drivers/net/ethernet/marvell/mvneta.c where users want to control
> >> this. However the comment there is misleading
> >>
> >>     /* Even though the documentation says that request_percpu_irq
> >>      * doesn't enable the interrupts automatically, it actually
> >>      * does so on the local CPU.
> >>      *
> >>      * Make sure it's disabled.
> >>      */
> >>
> >> Either sme core code is clearing NOAUTOEN or calling enable_precpu_irq() making
> >> request_percpu_irq() enable it.
> > 
> > If that's the case, this is a bug. Nobody should enable that interrupt
> > until the driver has chosen to do so.
> 
> Perhaps Maxim can shed more light as this seems to be his comment.

I don't really have the full context here, but we did this in order to
allow RX queue interrupts to be enabled on a particular CPU.

The IP was supposed to do that by itself, but we hadn't figure that
out at the time. So what we ended up doing is disabling the interrupts
by default and then enable it only on the CPU meant to receive the
queue interrupts.

What we found out doing so was that the per-cpu interrupts were
enabled on the cpu calling request_percpu_irq, and so that's why it
ended up with that comment.

I also fixed the documentation accordingly in a1b7febd725a.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-snps-arc/attachments/20151214/540099ea/attachment.sig>