LinuxLists.cc - [RFC 1/3] Unified NMI delayed call mechanism

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

* Huang Ying <[email protected]> wrote:

> NMI can be triggered even when IRQ is masked. So it is not safe for NMI
> handler to call some functions. One solution is to delay the call via self
> interrupt, so that the delayed call can be done once the interrupt is
> enabled again. This has been implemented in MCE and perf event. This patch
> provides a unified version and make it easier for other NMI semantic handler
> to take use of the delayed call.
>
> Signed-off-by: Huang Ying <[email protected]>
> ---
> arch/x86/include/asm/entry_arch.h | 1
> arch/x86/include/asm/hw_irq.h | 1
> arch/x86/include/asm/irq_vectors.h | 5 +
> arch/x86/include/asm/nmi.h | 7 ++
> arch/x86/kernel/entry_64.S | 3 +
> arch/x86/kernel/irqinit.c | 3 +
> arch/x86/kernel/traps.c | 104 +++++++++++++++++++++++++++++++++++++
> 7 files changed, 124 insertions(+)

Instead of introducing this extra intermediate facility please use the same
approach the unified NMI watchdog is using (see latest -tip): a perf event
callback gives all the extra functionality needed.

The MCE code needs to be updated to use that - and then it will be integrated
into the events framework.

Thanks,

Ingo

2010-06-13 01:54:12

by Huang, Ying

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

On Sat, 2010-06-12 at 18:25 +0800, Ingo Molnar wrote:
> * Huang Ying <[email protected]> wrote:
>
> > NMI can be triggered even when IRQ is masked. So it is not safe for NMI
> > handler to call some functions. One solution is to delay the call via self
> > interrupt, so that the delayed call can be done once the interrupt is
> > enabled again. This has been implemented in MCE and perf event. This patch
> > provides a unified version and make it easier for other NMI semantic handler
> > to take use of the delayed call.
> >
> > Signed-off-by: Huang Ying <[email protected]>
> > ---
> > arch/x86/include/asm/entry_arch.h | 1
> > arch/x86/include/asm/hw_irq.h | 1
> > arch/x86/include/asm/irq_vectors.h | 5 +
> > arch/x86/include/asm/nmi.h | 7 ++
> > arch/x86/kernel/entry_64.S | 3 +
> > arch/x86/kernel/irqinit.c | 3 +
> > arch/x86/kernel/traps.c | 104 +++++++++++++++++++++++++++++++++++++
> > 7 files changed, 124 insertions(+)
>
> Instead of introducing this extra intermediate facility please use the same
> approach the unified NMI watchdog is using (see latest -tip): a perf event
> callback gives all the extra functionality needed.

Sorry, if my understanding is correct, the perf event overflow callback
should be run in NMI context instead of a delayed context (such as IRQ,
soft_irq, process context). That is, the backtrace of
watchdog_overflow_callback should be something as follow:

x86_pmu_handle_irq
perf_event_overflow
__perf_event_overflow
watchdog_overflow_callback

I do not find the delayed mechanism here.

> The MCE code needs to be updated to use that - and then it will be integrated
> into the events framework.

MCE is NMI-like, and there are other NMI users too. I think some of them
will need some kind of delayed call mechanism. In fact, perf itself uses
self-made NMI delayed call mechanism too, I just want to generalize it
for other users too.

Best Regards,
Huang Ying

2010-06-14 03:47:07

by Hidetoshi Seto

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

(2010/06/12 19:25), Ingo Molnar wrote:
>
> * Huang Ying <[email protected]> wrote:
>
>> NMI can be triggered even when IRQ is masked. So it is not safe for NMI
>> handler to call some functions. One solution is to delay the call via self
>> interrupt, so that the delayed call can be done once the interrupt is
>> enabled again. This has been implemented in MCE and perf event. This patch
>> provides a unified version and make it easier for other NMI semantic handler
>> to take use of the delayed call.
>
> Instead of introducing this extra intermediate facility please use the same
> approach the unified NMI watchdog is using (see latest -tip): a perf event
> callback gives all the extra functionality needed.
>
> The MCE code needs to be updated to use that - and then it will be integrated
> into the events framework.

Hi Ingo,

I think this "NMI delayed call mechanism" could be a part of "the events
framework" that we are planning to get in kernel soon. At least APEI will
use NMI to report some hardware events (likely error) to kernel. So I
suppose we will go to have a delayed call as an event handler for APEI.

Generally speaking "event" can occur independently of the situation.
NMI can tell us some of external events, expecting urgent reaction for
the event, but we cannot do everything in NMI context. Or we might have
a sudden urge to generate an internal event while interrupts are disabled.

I agree that generating a self interrupt is reasonable solution.
Note that it could be said that both of "MCE handled (=event log should
be delivered to userland asap)" and "perf events pending (=pending events
should be handled asap)" are kind of internal event that requires urgent
handling in non-NMI kernel context. One question here is why we should
have different vectors for these events that uses same mechanism.

How about calling the vector LOCAL_EVENT_VECTOR or so?
I guess there should be better name if it is possible to inject an event
to other cpus via IPI with this vector...

Thanks,
H.Seto

2010-06-14 13:54:38

by Don Zickus

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

On Mon, Jun 14, 2010 at 12:45:21PM +0900, Hidetoshi Seto wrote:
> (2010/06/12 19:25), Ingo Molnar wrote:
> >
> > * Huang Ying <[email protected]> wrote:
> >
> >> NMI can be triggered even when IRQ is masked. So it is not safe for NMI
> >> handler to call some functions. One solution is to delay the call via self
> >> interrupt, so that the delayed call can be done once the interrupt is
> >> enabled again. This has been implemented in MCE and perf event. This patch
> >> provides a unified version and make it easier for other NMI semantic handler
> >> to take use of the delayed call.
> >
> > Instead of introducing this extra intermediate facility please use the same
> > approach the unified NMI watchdog is using (see latest -tip): a perf event
> > callback gives all the extra functionality needed.
> >
> > The MCE code needs to be updated to use that - and then it will be integrated
> > into the events framework.
>
> Hi Ingo,
>
> I think this "NMI delayed call mechanism" could be a part of "the events
> framework" that we are planning to get in kernel soon. At least APEI will
> use NMI to report some hardware events (likely error) to kernel. So I
> suppose we will go to have a delayed call as an event handler for APEI.
>
> Generally speaking "event" can occur independently of the situation.
> NMI can tell us some of external events, expecting urgent reaction for
> the event, but we cannot do everything in NMI context. Or we might have
> a sudden urge to generate an internal event while interrupts are disabled.
>
> I agree that generating a self interrupt is reasonable solution.
> Note that it could be said that both of "MCE handled (=event log should
> be delivered to userland asap)" and "perf events pending (=pending events
> should be handled asap)" are kind of internal event that requires urgent
> handling in non-NMI kernel context. One question here is why we should
> have different vectors for these events that uses same mechanism.

I think the perf event subsytem can log events in NMI context already and
deliver them to userspace when the NMI is done. This is why I think Ingo
wants MCE to be updated to sit on top of the perf event subsytem to avoid
re-invent everything again.

Then again I do not know enough about the MCE stuff to understand what you
mean when an event comes in but you can't handle it in an NMI-safe
context. An example would be helpful.

Cheers,
Don

2010-06-14 14:44:10

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

> I think the perf event subsytem can log events in NMI context already and
> deliver them to userspace when the NMI is done. This is why I think Ingo
> wants MCE to be updated to sit on top of the perf event subsytem to avoid
> re-invent everything again.

perf is not solving the problem this is trying to solve.

> Then again I do not know enough about the MCE stuff to understand what you
> mean when an event comes in but you can't handle it in an NMI-safe
> context. An example would be helpful.

At least for MCE hwpoison recovery needs to sleep and you obviously cannot sleep in
NMI like context. The way it's done is to first do a self interrupt, then do a work queue
wakeup and finally the sleeping operations.

perf does not fit into this because it has no way to process such an event
inside the kernel.

Anyways this just cleans up the existing mechanism to share some code.

-Andi

--
[email protected] -- Speaking for myself only.

2010-06-14 15:12:57

by Don Zickus

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

On Mon, Jun 14, 2010 at 04:44:03PM +0200, Andi Kleen wrote:
> > I think the perf event subsytem can log events in NMI context already and
> > deliver them to userspace when the NMI is done. This is why I think Ingo
> > wants MCE to be updated to sit on top of the perf event subsytem to avoid
> > re-invent everything again.
>
> perf is not solving the problem this is trying to solve.
>
> > Then again I do not know enough about the MCE stuff to understand what you
> > mean when an event comes in but you can't handle it in an NMI-safe
> > context. An example would be helpful.
>
> At least for MCE hwpoison recovery needs to sleep and you obviously cannot sleep in
> NMI like context. The way it's done is to first do a self interrupt, then do a work queue
> wakeup and finally the sleeping operations.
>
> perf does not fit into this because it has no way to process such an event
> inside the kernel.

Ah, makes sense. Thanks for the example.

Cheers,
Don

2010-06-18 09:48:54

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

* Hidetoshi Seto <[email protected]> wrote:

> (2010/06/12 19:25), Ingo Molnar wrote:
> >
> > * Huang Ying <[email protected]> wrote:
> >
> >> NMI can be triggered even when IRQ is masked. So it is not safe for NMI
> >> handler to call some functions. One solution is to delay the call via self
> >> interrupt, so that the delayed call can be done once the interrupt is
> >> enabled again. This has been implemented in MCE and perf event. This patch
> >> provides a unified version and make it easier for other NMI semantic handler
> >> to take use of the delayed call.
> >
> > Instead of introducing this extra intermediate facility please use the same
> > approach the unified NMI watchdog is using (see latest -tip): a perf event
> > callback gives all the extra functionality needed.
> >
> > The MCE code needs to be updated to use that - and then it will be integrated
> > into the events framework.
>
> Hi Ingo,
>
> I think this "NMI delayed call mechanism" could be a part of "the events
> framework" that we are planning to get in kernel soon. [...]

My request was to make it part of perf events - which is a generic event
logging framework. We dont really need/want a second 'events framework'
as we have one already ;-)

> [...] At least APEI will use NMI to report some hardware events (likely
> error) to kernel. So I suppose we will go to have a delayed call as an
> event handler for APEI.

Yep, that makes sense. I wasnt arguing against the functionality itself, i was
arguing against the illogical layering that limits its utility. By making it
part of perf events it becomes a generic part of that framework and can be
used by anything that deals with events and uses that framework.

Thanks,

Ingo

2010-06-18 10:31:17

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

* Andi Kleen <[email protected]> wrote:

> > I think the perf event subsytem can log events in NMI context already and
> > deliver them to userspace when the NMI is done. This is why I think Ingo
> > wants MCE to be updated to sit on top of the perf event subsytem to avoid
> > re-invent everything again.
>
> perf is not solving the problem this is trying to solve.

That is why i requested to extend the events backend. That will unify more of
the code than the first few steps achieved by these three patches - and offers
the functionality to all code that uses the events framework.

> [...]
>
> perf does not fit into this because it has no way to process such an event
> inside the kernel.

It 'does not fit' into the events backend only if you pretend that it is
impossible or undesirable to have a delayed, in-context callback mechanism
implemented there.

If you look at it more closely you'll notice that in reality it's not only
possible but that it is also a pretty natural fit.

Thanks,

Ingo

2010-06-18 11:34:57

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

Hi, Ingo,

On Fri, Jun 18, 2010 at 5:48 PM, Ingo Molnar <[email protected]> wrote:
>
> * Hidetoshi Seto <[email protected]> wrote:
>
>> (2010/06/12 19:25), Ingo Molnar wrote:
>> >
>> > * Huang Ying <[email protected]> wrote:
>> >
>> >> NMI can be triggered even when IRQ is masked. So it is not safe for NMI
>> >> handler to call some functions. One solution is to delay the call via self
>> >> interrupt, so that the delayed call can be done once the interrupt is
>> >> enabled again. This has been implemented in MCE and perf event. This patch
>> >> provides a unified version and make it easier for other NMI semantic handler
>> >> to take use of the delayed call.
>> >
>> > Instead of introducing this extra intermediate facility please use the same
>> > approach the unified NMI watchdog is using (see latest -tip): a perf event
>> > callback gives all the extra functionality needed.
>> >
>> > The MCE code needs to be updated to use that - and then it will be integrated
>> > into the events framework.
>>
>> Hi Ingo,
>>
>> I think this "NMI delayed call mechanism" could be a part of "the events
>> framework" that we are planning to get in kernel soon. [...]
>
> My request was to make it part of perf events - which is a generic event
> logging framework. We dont really need/want a second 'events framework'
> as we have one already ;-)

This patchset is simple and straightforward, it is just a delayed
execution mechanism, not another 'events framework'. There are several
other NMI users other than perf, should we integrate all NMI users
into perf framework?

>> [...] At least APEI will use NMI to report some hardware events (likely
>> error) to kernel. So I suppose we will go to have a delayed call as an
>> event handler for APEI.
>
> Yep, that makes sense. I wasnt arguing against the functionality itself, i was
> arguing against the illogical layering that limits its utility. By making it
> part of perf events it becomes a generic part of that framework and can be
> used by anything that deals with events and uses that framework.

I think the the 'layering' in the patchset helps instead of 'limits'
its utility. It is designed to be as general as possible, so that it
can be used by both perf and other NMI users. Do you think so?

Best Regards,
Huang Ying

2010-06-18 11:56:09

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

On Sat, 2010-06-12 at 17:28 +0800, Huang Ying wrote:
> +#define NMI_DELAYED_CALL_ID_MAX 32
> +#define NMI_DELAYED_CALL_RESTART_MAX 5
> +
> +static nmi_delayed_call_func_t nmi_delayed_call_funcs[NMI_DELAYED_CALL_ID_MAX];
> +static DEFINE_SPINLOCK(nmi_delayed_call_lock);
> +
> +static DEFINE_PER_CPU(unsigned long, nmi_delayed_call_pending);
> +
> +static void nmi_delayed_call_run(void)
> +{
> + int cpu, restart = NMI_DELAYED_CALL_RESTART_MAX;
> + unsigned long pending, *ppending;
> + nmi_delayed_call_func_t *pfunc, func;
> +
> + cpu = smp_processor_id();
> + ppending = per_cpu_ptr(&nmi_delayed_call_pending, cpu);
> + while (*ppending && restart--) {
> + pending = xchg(ppending, 0);
> + pfunc = nmi_delayed_call_funcs;
> + do {
> + if (pending & 1) {
> + func = *pfunc;
> + if (func)
> + func();
> + }
> + pfunc++;
> + pending >>= 1;
> + } while (pending);
> + }
> +}

So aside from the should this be perf or not, the above is utter
gibberish. Whoever came up with this nonsense?

Why not make a work_struct like thing and enqueue it using cmpxchg on a
percpu list, then have the interrupt process them. Read
perf_pending_queue() and __perf_pending_run().

That way you don't need this whole register/id/limit crap.

> +#ifdef CONFIG_X86_LOCAL_APIC

What's the point of the rest of this code if we don't have a lapic?

> +asmlinkage void smp_nmi_delayed_call_interrupt(struct pt_regs *regs)
> +{
> + ack_APIC_irq();
> + irq_enter();

You're missing inc_irq_stat() there.

> + nmi_delayed_call_run();
> + irq_exit();
> +}
> +#endif

2010-06-18 12:25:43

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

> So aside from the should this be perf or not, the above is utter
> gibberish. Whoever came up with this nonsense?

This is pretty much how softirqs (and before them bottom halves) work.
I believe Linus invented that scheme originally back in the early
days of Linux.

It's actually quite simple and works well

-Andi

2010-06-18 12:46:10

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

* huang ying <[email protected]> wrote:

> Hi, Ingo,
>
> On Fri, Jun 18, 2010 at 5:48 PM, Ingo Molnar <[email protected]> wrote:
> >
> > * Hidetoshi Seto <[email protected]> wrote:
> >
> >> (2010/06/12 19:25), Ingo Molnar wrote:
> >> >
> >> > * Huang Ying <[email protected]> wrote:
> >> >
> >> >> NMI can be triggered even when IRQ is masked. So it is not safe for NMI
> >> >> handler to call some functions. One solution is to delay the call via self
> >> >> interrupt, so that the delayed call can be done once the interrupt is
> >> >> enabled again. This has been implemented in MCE and perf event. This patch
> >> >> provides a unified version and make it easier for other NMI semantic handler
> >> >> to take use of the delayed call.
> >> >
> >> > Instead of introducing this extra intermediate facility please use the same
> >> > approach the unified NMI watchdog is using (see latest -tip): a perf event
> >> > callback gives all the extra functionality needed.
> >> >
> >> > The MCE code needs to be updated to use that - and then it will be integrated
> >> > into the events framework.
> >>
> >> Hi Ingo,
> >>
> >> I think this "NMI delayed call mechanism" could be a part of "the events
> >> framework" that we are planning to get in kernel soon. [...]
> >
> > My request was to make it part of perf events - which is a generic event
> > logging framework. We dont really need/want a second 'events framework' as
> > we have one already ;-)
>
> This patchset is simple and straightforward, [...]

We wouldnt want to add another workqueue or memory allocation mechanism
either, even if it was 'simple and straightforward'. We try to make things
more generally useful.

> [...] it is just a delayed execution mechanism, not another 'events
> framework'. There are several other NMI users other than perf, should we
> integrate all NMI users into perf framework?

We already did so with the NMI watchdog. What other significant NMI event
users do you have in mind?

> >> [...] ??At least APEI will use NMI to report some hardware events (likely
> >> error) to kernel. ??So I suppose we will go to have a delayed call as an
> >> event handler for APEI.
> >
> > Yep, that makes sense. I wasnt arguing against the functionality itself, i
> > was arguing against the illogical layering that limits its utility. By
> > making it part of perf events it becomes a generic part of that framework
> > and can be used by anything that deals with events and uses that
> > framework.
>
> I think the the 'layering' in the patchset helps instead of 'limits' its
> utility. It is designed to be as general as possible, so that it can be used
> by both perf and other NMI users. Do you think so?

What other NMI users do you mean? EDAC/MCE is going to go utilize events as
well (away from the horrible /dev/mcelog interface), the NMI watchdog already
did it and the perf tool obviously does as well. There's a few leftovers like
kcrash which isnt really event centric and i dont think it needs to be
converted.

Thanks,

Ingo

2010-06-18 12:48:42

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

On Fri, 2010-06-18 at 14:25 +0200, Andi Kleen wrote:
> > So aside from the should this be perf or not, the above is utter
> > gibberish. Whoever came up with this nonsense?
>
> This is pretty much how softirqs (and before them bottom halves) work.
> I believe Linus invented that scheme originally back in the early
> days of Linux.

Doesn't mean its the right abstraction for this.

> It's actually quite simple and works well

And adds more code than it removes whilst providing a very limited
service.

You generally want to pass more information along anyway, now your
callback function needs to go look for it. Much better to pass a
work_struct like thing around that is contained in the state it needs.

2010-06-18 13:09:29

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

> You generally want to pass more information along anyway, now your
> callback function needs to go look for it. Much better to pass a
> work_struct like thing around that is contained in the state it needs.

But how would you allocate the work queue in an NMI?

If it's only a single instance (like this bit) it can be always put
into a per cpu variable.

-Andi

2010-06-18 13:13:10

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

On Fri, 2010-06-18 at 15:09 +0200, Andi Kleen wrote:
> > You generally want to pass more information along anyway, now your
> > callback function needs to go look for it. Much better to pass a
> > work_struct like thing around that is contained in the state it needs.
>
> But how would you allocate the work queue in an NMI?
>
> If it's only a single instance (like this bit) it can be always put
> into a per cpu variable.

Pre-allocate. For the perf-event stuff we use the perf_event allocated
at creation time. But yeah, per-cpu storage also works.

2010-06-18 13:23:35

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

On Fri, Jun 18, 2010 at 03:12:49PM +0200, Peter Zijlstra wrote:
> On Fri, 2010-06-18 at 15:09 +0200, Andi Kleen wrote:
> > > You generally want to pass more information along anyway, now your
> > > callback function needs to go look for it. Much better to pass a
> > > work_struct like thing around that is contained in the state it needs.
> >
> > But how would you allocate the work queue in an NMI?
> >
> > If it's only a single instance (like this bit) it can be always put
> > into a per cpu variable.
>
> Pre-allocate. For the perf-event stuff we use the perf_event allocated
> at creation time. But yeah, per-cpu storage also works.

So you could just preallocate the bits instead ?

-Andi

--
[email protected] -- Speaking for myself only.

2010-06-18 13:25:26

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

On Fri, 2010-06-18 at 15:23 +0200, Andi Kleen wrote:
> On Fri, Jun 18, 2010 at 03:12:49PM +0200, Peter Zijlstra wrote:
> > On Fri, 2010-06-18 at 15:09 +0200, Andi Kleen wrote:
> > > > You generally want to pass more information along anyway, now your
> > > > callback function needs to go look for it. Much better to pass a
> > > > work_struct like thing around that is contained in the state it needs.
> > >
> > > But how would you allocate the work queue in an NMI?
> > >
> > > If it's only a single instance (like this bit) it can be always put
> > > into a per cpu variable.
> >
> > Pre-allocate. For the perf-event stuff we use the perf_event allocated
> > at creation time. But yeah, per-cpu storage also works.
>
> So you could just preallocate the bits instead ?

You mean the bits in your function array? Those are limited to 32 and
you'd need a secondary lookup to match them to your data object, not
very useful.

2010-06-18 13:40:45

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

On Fri, Jun 18, 2010 at 8:45 PM, Ingo Molnar <[email protected]> wrote:
>> >> [...] ??At least APEI will use NMI to report some hardware events (likely
>> >> error) to kernel. ??So I suppose we will go to have a delayed call as an
>> >> event handler for APEI.
>> >
>> > Yep, that makes sense. I wasnt arguing against the functionality itself, i
>> > was arguing against the illogical layering that limits its utility. By
>> > making it part of perf events it becomes a generic part of that framework
>> > and can be used by anything that deals with events and uses that
>> > framework.
>>
>> I think the the 'layering' in the patchset helps instead of 'limits' its
>> utility. It is designed to be as general as possible, so that it can be used
>> by both perf and other NMI users. Do you think so?
>
> What other NMI users do you mean? EDAC/MCE is going to go utilize events as
> well (away from the horrible /dev/mcelog interface), the NMI watchdog already
> did it and the perf tool obviously does as well. There's a few leftovers like
> kcrash which isnt really event centric and i dont think it needs to be
> converted.

But why not just make it more general? It does not hurt anyone including perf.

Best Regards,
Huang Ying

2010-06-18 14:35:52

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

* huang ying <[email protected]> wrote:

> On Fri, Jun 18, 2010 at 8:45 PM, Ingo Molnar <[email protected]> wrote:
> >> >> [...] ??At least APEI will use NMI to report some hardware events (likely
> >> >> error) to kernel. ??So I suppose we will go to have a delayed call as an
> >> >> event handler for APEI.
> >> >
> >> > Yep, that makes sense. I wasnt arguing against the functionality itself, i
> >> > was arguing against the illogical layering that limits its utility. By
> >> > making it part of perf events it becomes a generic part of that framework
> >> > and can be used by anything that deals with events and uses that
> >> > framework.
> >>
> >> I think the the 'layering' in the patchset helps instead of 'limits' its
> >> utility. It is designed to be as general as possible, so that it can be used
> >> by both perf and other NMI users. Do you think so?
> >
> > What other NMI users do you mean? EDAC/MCE is going to go utilize events
> > as well (away from the horrible /dev/mcelog interface), the NMI watchdog
> > already did it and the perf tool obviously does as well. There's a few
> > leftovers like kcrash which isnt really event centric and i dont think it
> > needs to be converted.
>
> But why not just make it more general? It does not hurt anyone including
> perf.

Because it's not actually more generic that way - just look at the code. It's
x86 specific, plus it ties it to NMI delivery while the concept of delayed
execution has nothing to do with NMIs.

Ingo

2010-06-18 14:38:09

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

* Andi Kleen <[email protected]> wrote:

> > So aside from the should this be perf or not, the above is utter
> > gibberish. Whoever came up with this nonsense?
>
> This is pretty much how softirqs (and before them bottom halves) work.
> I believe Linus invented that scheme originally back in the early
> days of Linux.

Nope - the softirq code was written by Alexey Kuznetsov and David S. Miller.
But it's irrelevant, because:

> It's actually quite simple and works well

... as Peter said the last thing we want is yet another softirq vector.

Thanks,

Ingo

2010-06-18 15:16:26

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

On Fri, Jun 18, 2010 at 10:35 PM, Ingo Molnar <[email protected]> wrote:
>
> * huang ying <[email protected]> wrote:
>
>> On Fri, Jun 18, 2010 at 8:45 PM, Ingo Molnar <[email protected]> wrote:
>> >> >> [...] ??At least APEI will use NMI to report some hardware events (likely
>> >> >> error) to kernel. ??So I suppose we will go to have a delayed call as an
>> >> >> event handler for APEI.
>> >> >
>> >> > Yep, that makes sense. I wasnt arguing against the functionality itself, i
>> >> > was arguing against the illogical layering that limits its utility. By
>> >> > making it part of perf events it becomes a generic part of that framework
>> >> > and can be used by anything that deals with events and uses that
>> >> > framework.
>> >>
>> >> I think the the 'layering' in the patchset helps instead of 'limits' its
>> >> utility. It is designed to be as general as possible, so that it can be used
>> >> by both perf and other NMI users. Do you think so?
>> >
>> > What other NMI users do you mean? EDAC/MCE is going to go utilize events
>> > as well (away from the horrible /dev/mcelog interface), the NMI watchdog
>> > already did it and the perf tool obviously does as well. There's a few
>> > leftovers like kcrash which isnt really event centric and i dont think it
>> > needs to be converted.
>>
>> But why not just make it more general? It does not hurt anyone including
>> perf.
>
> Because it's not actually more generic that way - just look at the code. It's
> x86 specific, plus it ties it to NMI delivery while the concept of delayed
> execution has nothing to do with NMIs.

soft_irq is a delayed mechanism for IRQ, a self interrupt can be a
delayed mechanism for NMI. If we can make soft_irq NMI-safe, we can
use soft_irq as a backup of self interrupt (for systems without APIC
and maybe for other architectures).

Best Regards,
Huang Ying

2010-06-18 15:31:25

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

On Fri, 2010-06-18 at 23:16 +0800, huang ying wrote:
>
> soft_irq is a delayed mechanism for IRQ,

No its not.

> a self interrupt can be a
> delayed mechanism for NMI. If we can make soft_irq NMI-safe,

No you can't.

> we can
> use soft_irq as a backup of self interrupt (for systems without APIC
> and maybe for other architectures).

Whatever would you want to do that for.

2010-06-19 01:51:05

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

On Fri, Jun 18, 2010 at 11:31 PM, Peter Zijlstra <[email protected]> wrote:
> On Fri, 2010-06-18 at 23:16 +0800, huang ying wrote:
>>
>> soft_irq is a delayed mechanism for IRQ,
>
> No its not.

Why? What do you think soft_irq is for?

>> a self interrupt can be a
>> delayed mechanism for NMI. If we can make soft_irq NMI-safe,
>
> No you can't.
>
>> we can
>> use soft_irq as a backup of self interrupt (for systems without APIC
>> and maybe for other architectures).
>
> Whatever would you want to do that for.

Best Regards,
Huang Ying

2010-06-19 08:02:15

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

> > we can
> > use soft_irq as a backup of self interrupt (for systems without APIC
> > and maybe for other architectures).
>
> Whatever would you want to do that for.

The idea is that the work would be done latest on the next
timer interrupt as a fallback if APIC is not available.

That's what mce does already and it's probably approbiate
for most other users too.

-Andi

--
[email protected] -- Speaking for myself only.

2010-06-19 10:54:10

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

( Ugh: what's this new fad of you not quoting the name of the person who
wrote a mail? It makes multi-level quotes utterly unreadable as it's not
clear who wrote what. You should also respect others by quoting their names. )

* Andi Kleen <[email protected]> wrote:

> > > we can use soft_irq as a backup of self interrupt (for systems without
> > > APIC and maybe for other architectures).
> >
> > Whatever would you want to do that for.
>
> The idea is that the work would be done latest on the next timer interrupt
> as a fallback if APIC is not available.

Abusing the timer irq for that is an exceedingly ugly and unacceptable design,
as machine check events have nothing to do with timers. (That approach is also
buggy because it inserts an arbitrary delay - which could be rather long on
nohz.)

This kind of messy, ad-hoc piggybacking only creates unmaintainable code in
the long run.

> That's what mce does already and it's probably approbiate for most other
> users too.

Hell no, the unfortunate and unclean practices of the MCE code must not be
propagated elsewhere.

The proper, generic approach would be to enable softirq notifications (on x86)
from NMI contexts as well (it's actually possible without overhead), and to
extend user return notifiers with the logical next step: nmi return notifiers.
If presented in such a form then those could use softirqs for atomic callbacks
and per cpu kthreads for sleepable callbacks, etc.

Ingo

2010-06-19 14:07:44

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

On Sat, Jun 19, 2010 at 6:53 PM, Ingo Molnar <[email protected]> wrote:
> The proper, generic approach would be to enable softirq notifications (on x86)
> from NMI contexts as well (it's actually possible without overhead),

Yes. I will do that. And I think self interrupt can be used as the
short-cut for soft_irq if available. The next soft_irq may be too late
if there is too few interrupts.

> and to
> extend user return notifiers with the logical next step: nmi return notifiers.
> If presented in such a form then those could use softirqs for atomic callbacks
> and per cpu kthreads for sleepable callbacks, etc.

NMI return notifiers fired in soft_irq?

Best Regards,
Huang Ying

2010-06-19 14:17:27

[permalink] [raw]

Subject: Re: [RFC 1/3] Unified NMI delayed call mechanism

> Nope - the softirq code was written by Alexey Kuznetsov and David S. Miller.

It was based on the design of the previous bottom halves in case you
missed it. I believe BHs were there from nearly the beginning.

(softirqs were basically just per cpu bottom halves, otherwise
being very similar)

-Andi

2010-06-19 14:24:11