Prev: lib: vsprintf: optimised put_dec() for 32-bit machines
Next: [PATCH RFC] [X86] Fix potential issue on memmove
From: Robert Richter on 10 Aug 2010 03:50
On 09.08.10 16:02:45, Cyrill Gorcunov wrote:

> > @@ -1222,14 +1245,12 @@ perf_event_nmi_handler(struct notifier_block *self,
> > regs = args->regs;
> >
> > apic_write(APIC_LVTPC, APIC_DM_NMI);
>
> If only I'm not missing something this apic_write should go up to
> "case DIE_NMIUNKNOWN" site, no?

This seems to be code from the non-nmi implementation and can be
removed at all, which should be a separate patch. The vector is
already set up.

-Robert

--
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: Robert Richter on 10 Aug 2010 12:50
On 10.08.10 12:16:27, Cyrill Gorcunov wrote:
> On Tue, Aug 10, 2010 at 09:42:00AM +0200, Robert Richter wrote:
> > On 09.08.10 16:02:45, Cyrill Gorcunov wrote:
> >
> > > > @@ -1222,14 +1245,12 @@ perf_event_nmi_handler(struct notifier_block *self,
> > > > regs = args->regs;
> > > >
> > > > apic_write(APIC_LVTPC, APIC_DM_NMI);
> > >
> > > If only I'm not missing something this apic_write should go up to
> > > "case DIE_NMIUNKNOWN" site, no?
> >
> > This seems to be code from the non-nmi implementation and can be
> > removed at all, which should be a separate patch. The vector is
> > already set up.
> >
> > -Robert
> >
>
> No, this is just a short way to unmask LVTPC (which is required for
> cpus). Actually lookig into this snippet I found that in p4 pmu
> I've made one redundant unmaksing operation. will update as only
> this area settle down.

The vector is setup in hw_perf_enable() and then never masked. The
perfctrs nmi is alwayes enabled since then. I still see no reason for
unmasking it again with every nmi. Once you handle the nmi it is also
enabled.

-Robert

--
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: Robert Richter on 10 Aug 2010 15:10
On 10.08.10 13:24:51, Cyrill Gorcunov wrote:

> It gets masked on NMI arrival, at least for some models (Core Duo, P4,
> P6 M and I suspect more theh that, that was the reason oprofile has
> it, also there is a note in SDM V3a page 643).

Yes, that's right, I never noticed that. Maybe it is better to
implement the apic write it in model specific code then.

-Robert

--
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 10 Aug 2010 16:50
On Mon, Aug 09, 2010 at 09:48:29PM +0200, Robert Richter wrote:
> On 06.08.10 10:21:31, Don Zickus wrote:
> > On Fri, Aug 06, 2010 at 08:52:03AM +0200, Robert Richter wrote:
>
> > > I was playing around with it yesterday trying to fix this. My idea is
> > > to skip an unkown nmi if the privious nmi was a *handled* perfctr
> >
> > You might want to add a little more logic that says *handled* _and_ had
> > more than one perfctr trigger. Most of the time only one perfctr is
> > probably triggering, so you might be eating unknown_nmi's needlessly.
> >
> > Just a thought.
>
> Yes, that's true. It could be implemented on top of the patch below.

I did, but the changes basically revert the bulk of your patch.

>
> >
> > > nmi. I will probably post an rfc patch early next week.
>
> Here it comes:
>
> 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)

From 548cf5148f47618854a0eff22b1d55db71b6f8fc Mon Sep 17 00:00:00 2001
From: Don Zickus <dzickus(a)redhat.com>
Date: Tue, 10 Aug 2010 16:40:03 -0400
Subject: [PATCH] perf, x86: only skip NMIs when multiple perfctrs trigger

A small optimization on top of Robert's patch that limits the
skipping of NMI's to cases where we detect multiple perfctr events
have happened.

Signed-off-by: Don Zickus <dzickus(a)redhat.com>

---
arch/x86/kernel/cpu/perf_event.c | 34 ++++++++++++++++++++--------------
1 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index c3cd159..066046d 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1154,7 +1154,7 @@ static int x86_pmu_handle_irq(struct pt_regs *regs)
/*
* event overflow
*/
- handled = 1;
+ handled += 1;
data.period = event->hw.last_period;

if (!x86_perf_event_set_period(event))
@@ -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);

static int __kprobes
perf_event_nmi_handler(struct notifier_block *self,
@@ -1208,8 +1208,7 @@ perf_event_nmi_handler(struct notifier_block *self,
{
struct die_args *args = __args;
struct pt_regs *regs;
- unsigned int this_nmi;
- unsigned int prev_nmi;
+ int handled = 0;

if (!atomic_read(&active_events))
return NOTIFY_DONE;
@@ -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;

return NOTIFY_STOP;
}
--
1.7.2
--
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: Huang Ying on 10 Aug 2010 23:30
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:

int perf_event_nmi_handler()
{
...
switch (cmd) {
case DIE_NMIUNKNOWN:
if (per_cpu(perfctr_prev_handled) > 1
&& rdtsc() - per_cpu(perfctr_handled_timestamp) < 1000)
return NOTIFY_STOP;
else
return NOTIFY_DONE;
}
...
handled = x86_pmu.handle_irq(regs);
per_cpu(perfctr_prev_handled) = per_cpu(perfctr_handled);
per_cpu(perfctr_handled) = handled;
if (handled) {
per_cpu(perfctr_handled_timestamp) = rdtsc();
return NOTIFY_STOP;
} else
return NOTIFY_DONE;
}

Best Regards,
Huang Ying


--
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/
 |  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