Prev: lib: vsprintf: optimised put_dec() for 32-bit machines
Next: [PATCH RFC] [X86] Fix potential issue on memmove
From: Robert Richter on 11 Aug 2010 07:20
On 10.08.10 22:44:55, Frederic Weisbecker wrote:
> On Tue, Aug 10, 2010 at 04:48:56PM -0400, Don Zickus wrote:
> > @@ -1200,7 +1200,7 @@ void perf_events_lapic_init(void)
> > apic_write(APIC_LVTPC, APIC_DM_NMI);
> > }
> >
> > -static DEFINE_PER_CPU(unsigned int, perfctr_handled);
> > +static DEFINE_PER_CPU(unsigned int, perfctr_skip);

Yes, using perfctr_skip is better to understand ...

> > @@ -1229,14 +1228,11 @@ perf_event_nmi_handler(struct notifier_block *self,
> > * was handling a perfctr. Otherwise we pass it and
> > * let the kernel handle the unknown nmi.
> > *
> > - * Note: this could be improved if we drop unknown
> > - * NMIs only if we handled more than one perfctr in
> > - * the previous NMI.
> > */
> > - this_nmi = percpu_read(irq_stat.__nmi_count);
> > - prev_nmi = __get_cpu_var(perfctr_handled);
> > - if (this_nmi == prev_nmi + 1)
> > + if (__get_cpu_var(perfctr_skip)){
> > + __get_cpu_var(perfctr_skip) -=1;
> > return NOTIFY_STOP;
> > + }
> > return NOTIFY_DONE;
> > default:
> > return NOTIFY_DONE;
> > @@ -1246,11 +1242,21 @@ perf_event_nmi_handler(struct notifier_block *self,
> >
> > apic_write(APIC_LVTPC, APIC_DM_NMI);
> >
> > - if (!x86_pmu.handle_irq(regs))
> > + handled = x86_pmu.handle_irq(regs);
> > + if (!handled)
> > + /* not our NMI */
> > return NOTIFY_DONE;
> > -
> > - /* handled */
> > - __get_cpu_var(perfctr_handled) = percpu_read(irq_stat.__nmi_count);
> > + else if (handled > 1)
> > + /*
> > + * More than one perfctr triggered. This could have
> > + * caused a second NMI that we must now skip because
> > + * we have already handled it. Remember it.
> > + *
> > + * NOTE: We have no way of knowing if a second NMI was
> > + * actually triggered, so we may accidentally skip a valid
> > + * unknown nmi later.
> > + */
> > + __get_cpu_var(perfctr_skip) +=1;

.... but this will not work. You have to mark the *absolute* nmi number
here. If you only raise a flag, the next unknown nmi will be dropped,
every. Because, in between there could have been other nmis that
stopped the chain and thus the 'unknown' path is not executed. The
trick in my patch is that you *know*, which nmi you want to skip.

I will send an updated version of my patch.

-Robert

>
>
>
> May be make it just a pending bit. I mean not something that can
> go further 1, because you can't have more than 1 pending anyway. I don't
> know how that could happen you get accidental perctr_skip > 1, may be
> expected pending NMIs that don't happen somehow, but better be paranoid with
> that, as it's about trying not to miss hardware errors.
>
> Thanks.
>
>

--
Advanced Micro Devices, Inc.
Operating System Research Center

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Don Zickus on 11 Aug 2010 08:40
On Wed, Aug 11, 2010 at 11:19:39AM +0800, Huang Ying wrote:
> On Wed, 2010-08-11 at 04:48 +0800, Don Zickus wrote:
> > > From d2739578199d881ae6a9537c1b96a0efd1cdea43 Mon Sep 17 00:00:00 2001
> > > From: Robert Richter <robert.richter(a)amd.com>
> > > Date: Thu, 5 Aug 2010 16:19:59 +0200
> > > Subject: [PATCH] perf, x86: try to handle unknown nmis with running perfctrs
> >
> > On top of Robert's patch:
> > (compiled tested only because I don't have a fancy button to trigger
> > unknown nmis)
>
> You can trigger unknown NMIs via apic->send_IPI_mask(cpu_mask,
> NMI_VECTOR).
>
> How about the algorithm as follow:

Heh, I thought about the following too, just couldn't figure out an easy
way to timestamp. I forgot about the rdtsc(). :-)

The only thing that might screw this up would be an SMI which takes longer
than 1000 but that should be rare and would probably be related to the
unknown NMI anyway. Also under virt that would probably break (due to
time jumping) but until they emulate the perfctr, it won't matter. :-)

Cheers,
Don
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Don Zickus on 11 Aug 2010 08:50
On Wed, Aug 11, 2010 at 01:10:46PM +0200, Robert Richter wrote:
> > > + *
> > > + * NOTE: We have no way of knowing if a second NMI was
> > > + * actually triggered, so we may accidentally skip a valid
> > > + * unknown nmi later.
> > > + */
> > > + __get_cpu_var(perfctr_skip) +=1;
>
> ... but this will not work. You have to mark the *absolute* nmi number
> here. If you only raise a flag, the next unknown nmi will be dropped,
> every. Because, in between there could have been other nmis that
> stopped the chain and thus the 'unknown' path is not executed. The
> trick in my patch is that you *know*, which nmi you want to skip.

I guess I am confused. The way I read your patch was that you assumed the
next NMI would be the one you skip and if there was another NMI in between
the handled one and the one to skip, you would not skip it (nmi count !=
prev + 1) and it would produce an accidental unknown nmi.

I tried to change that with my patch by setting the skip flag which would
be drained on the next unknown nmi, independent of where it is in the NMI
backlog of NMIs.

Did I misread something?

Cheers,
Don
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Don Zickus on 11 Aug 2010 08:50
On Wed, Aug 11, 2010 at 04:44:55AM +0200, Frederic Weisbecker wrote:
> May be make it just a pending bit. I mean not something that can
> go further 1, because you can't have more than 1 pending anyway. I don't
> know how that could happen you get accidental perctr_skip > 1, may be
> expected pending NMIs that don't happen somehow, but better be paranoid with
> that, as it's about trying not to miss hardware errors.

I guess I was thinking about the SMI case where it drains the perfctr(s)
and then retriggers them but I guess even in that case the most you can
have is one extra NMI. So yeah, you are probably right, I should have
used a flag instead of incrementing.

Cheers,
Don
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Robert Richter on 11 Aug 2010 10:10
On 11.08.10 08:44:43, Don Zickus wrote:
> On Wed, Aug 11, 2010 at 01:10:46PM +0200, Robert Richter wrote:
> > > > + *
> > > > + * NOTE: We have no way of knowing if a second NMI was
> > > > + * actually triggered, so we may accidentally skip a valid
> > > > + * unknown nmi later.
> > > > + */
> > > > + __get_cpu_var(perfctr_skip) +=1;
> >
> > ... but this will not work. You have to mark the *absolute* nmi number
> > here. If you only raise a flag, the next unknown nmi will be dropped,
> > every. Because, in between there could have been other nmis that
> > stopped the chain and thus the 'unknown' path is not executed. The
> > trick in my patch is that you *know*, which nmi you want to skip.
>
> I guess I am confused. The way I read your patch was that you assumed the
> next NMI would be the one you skip and if there was another NMI in between
> the handled one and the one to skip, you would not skip it (nmi count !=
> prev + 1) and it would produce an accidental unknown nmi.

That's how it works.

> I tried to change that with my patch by setting the skip flag which would
> be drained on the next unknown nmi, independent of where it is in the NMI
> backlog of NMIs.

"setting the skip flag which would be drained on the next unknown nmi"

That's what is wrong, it drops every unknown nmi, no matter when it is
detected. In between there could be 1000's of valid other nmis
handled. You even could have been returned from nmi mode. But still,
the next unknown nmi will be dropped. Your patch could accumulate also
the number of unknown nmis to skip, and then, if 'real' unknown nmis
happen, all of them will be dropped.

-Robert

>
> Did I misread something?
>
> Cheers,
> Don
>

--
Advanced Micro Devices, Inc.
Operating System Research Center

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: lib: vsprintf: optimised put_dec() for 32-bit machines
Next: [PATCH RFC] [X86] Fix potential issue on memmove