[PATCH] Fix performance monitor exception in 2.6.20-series

[PATCH] Fix performance monitor exception in 2.6.20-series

[Livio Soares]

  Hi all,

[  I hope this is the correct mailing list for this sort of patch. Also, I am not
   subscribed; please Cc: with responses.  ]

  To the issue: some point  during 2.6.20 development, Paul Mackerras introduced
the  "lazy IRQ  disabling" patch  (very  cool work,  BTW).  In  that patch,  the
performance monitor unit exception was marked as "maskable", in the sense the if
interrupts were  soft-disabled, that exception  could be ignored. This  broke my
PowerPC  profiling code. The  sympton that  I see  is that  a varying  number of
interrupts  (from 0  to $n$,  typically  closer to  0) get  delivered, when,  in
reality, it should always be very close to $n$.

  The issue stems  from the way masking is being done.   Masking in this fashion
seems to  work well with the  decrementer and external  interrupts, because they
are raised  again until "really"  handled. For the  PMU, however, this  does not
apply  (at least on  my Xserver  machine with  a 970FX  processor).  If  the PMU
exception  is  not  handled,  it  will  _not_  be  re-raised  (at  least  on  my
machine). The documentation states  that the PMXE bit in MMCR0 is  set to 0 when
the PMU exception is raised. However,  software must re-set the bit to re-enable
PMU exceptions.   If the exception  is ignored (as  currently) not only  is that
interrupt lost, but because software does  not reset PMXE, the PMU registers are
"frozen" forever.

  Although I  do not use Oprofile  for performance monitoring, I  suspect it, as
well, will be affected. There are 2 options, as far as I can see, for fixing the
problem:

  1) Just let the PMU exception through, even with interrupts disabled. 

  2) When  hard-disabling   interrupts  (masked_interrupt:  in   head_64.S,  for
     example), if the  exception is a PMU exception, remember  to set PMXE back,
     so that the interrupt will be raised in the future.

     I don't like this option; specifically, I am pretty sure the actual bit for
     enabling PMU interrupts can vary from  one PowerPC chip to the next. Custom
     CPU code will be needed to make this work.

     However, I tested this option #2 on  my 970, and it made my profiling works
     again. 


  IMHO, option  #1 is very  nice, as long  as the PMU interrupt  handler behaves
itself.  One reason option #1 is desirable is, with PC-sampling, we are now able
to  sample  regions _inside_  interrupt-disabled  sections  (assuming an  actual
external interrupt  hasn't really occured yet). Before,  with hardware disabling
of  interrupts,  the  PMU  exceptions  were  necessarily  delivered  outside  of
interrupt disabled sections. 

  Anyways, does anyone see a problem with the following patch? 

  regards,


				Livio


--- linux-2.6.20-rc4/arch/powerpc/kernel/head_64.S	2007-01-07 00:45:51.000000000 -0500
+++ linux-2.6.20-rc4.pmu/arch/powerpc/kernel/head_64.S	2007-01-13 10:28:49.894734542 -0500
@@ -613,7 +613,7 @@ system_call_pSeries:
 /*** pSeries interrupt support ***/
 
 	/* moved from 0xf00 */
-	MASKABLE_EXCEPTION_PSERIES(., performance_monitor)
+	STD_EXCEPTION_PSERIES(., performance_monitor)
 
 /*
  * An interrupt came in while soft-disabled; clear EE in SRR1,

Re: [PATCH] Fix performance monitor exception in 2.6.20-series

[Benjamin Herrenschmidt]

>   IMHO, option  #1 is very  nice, as long  as the PMU interrupt  handler behaves
> itself.  One reason option #1 is desirable is, with PC-sampling, we are now able
> to  sample  regions _inside_  interrupt-disabled  sections  (assuming an  actual
> external interrupt  hasn't really occured yet). Before,  with hardware disabling
> of  interrupts,  the  PMU  exceptions  were  necessarily  delivered  outside  of
> interrupt disabled sections. 
> 
>   Anyways, does anyone see a problem with the following patch? 

Well, are you absolutely sure that nothing will break as a result of
having a PMU interrupt happening right when it's not expected to ?

You are basically turning the PMU interrupt into an NMI... I'm not sure
how safe that is.

Ben.

Re: [PATCH] Fix performance monitor exception in 2.6.20-series

[Segher Boessenkool]

> Well, are you absolutely sure that nothing will break as a result of
> having a PMU interrupt happening right when it's not expected to ?
>
> You are basically turning the PMU interrupt into an NMI... I'm not sure
> how safe that is.

That just depends what code is run from the PMU exception.
It very much makes sense *conceptually* to have it behave
like an NMI I think -- but yeah the code run from the
exception had better behave ;-)


Segher

Re: [PATCH] Fix performance monitor exception in 2.6.20-series

[Livio Soares]

  Hi Ben,

  First,  I'd  like to  state  that  I have,  since  writting  my first  e-mail,
experimented with Oprofile on 2.6.20-rc4, and it _is_ affected as I theorized. I
get  something around  5  to  7 PMU  exceptions,  and no  more.  With my  patch,
exceptions keep coming as before the lazy IRQ patch.

Benjamin Herrenschmidt writes:
> 
> >   IMHO, option  #1 is very  nice, as long  as the PMU interrupt  handler behaves
> > itself.  One reason option #1 is desirable is, with PC-sampling, we are now able
> > to  sample  regions _inside_  interrupt-disabled  sections  (assuming an  actual
> > external interrupt  hasn't really occured yet). Before,  with hardware disabling
> > of  interrupts,  the  PMU  exceptions  were  necessarily  delivered  outside  of
> > interrupt disabled sections. 
> > 
> >   Anyways, does anyone see a problem with the following patch? 
> 
> Well, are you absolutely sure that nothing will break as a result of
> having a PMU interrupt happening right when it's not expected to ?
> 
> You are basically turning the PMU interrupt into an NMI... I'm not sure
> how safe that is.

  Yes, it is turning the PMU exception into an NMI. And, you are correct, it has
potential  for  problems. However,  if  you  look  closely through  the  current
Oprofile code it doesn't seem to execute anything dangerous. We have:

a) Looking at local CPU registers

b) Looking at current stack (when logging backtrace is enabled)

c) Writting information to a  per-CPU pre-allocated buffer. This is done without
   any form of locking. 

d) PMU  exception nesting cannot  occur (at least  on the PowerPC  machines I've
   looked  at).  Handling  must  'rfid'  before  the  PMU  can  deliver  another
   exception. 


  So, unless I missed something, the current code seems to be safe. 

  Another thing I tried was stress testing 2.6.20-rc4 with my patch and Oprofile
turned on. I  used an Apache2 benchmark for about  30 minutes. Everything worked
as  usual. I realize  this test  does not  guarantee the  safeness of  the code,
however, it served as a sanity check for obvious, easy to trigger bugs. 

  Thanks,

			Livio

Re: [PATCH] Fix performance monitor exception in 2.6.20-series

[David Woodhouse]

On Sun, 2007-01-14 at 11:29 +1100, Benjamin Herrenschmidt wrote:
> 
> Well, are you absolutely sure that nothing will break as a result of
> having a PMU interrupt happening right when it's not expected to ?
> 
> You are basically turning the PMU interrupt into an NMI... I'm not sure
> how safe that is.

If we can _make_ it safe, and in doing so avoid the failure mode where
the machine locks up with interrupts disabled and then the PMU resets
itself entirely (including the date), that might be a good thing...

-- 
dwmw2

Re: [PATCH] Fix performance monitor exception in 2.6.20-series

[Paul Mackerras]

David Woodhouse writes:

> If we can _make_ it safe, and in doing so avoid the failure mode where
> the machine locks up with interrupts disabled and then the PMU resets
> itself entirely (including the date), that might be a good thing...

Different PMU, I think; they are talking about the Performance Monitor
Unit in POWERx cpus.

Paul.

Re: [PATCH] Fix performance monitor exception in 2.6.20-series

[David Woodhouse]

On Mon, 2007-01-15 at 13:53 +1100, Paul Mackerras wrote:
> Different PMU, I think; they are talking about the Performance Monitor
> Unit in POWERx cpus. 

Ah, OK. That makes the "doesn't do anything you can't do in an NMI"
assessment make a little more sense to me.

Still, might be worth thinking about whether we can make the Apple PMU
into an NMI to help prevent the error case I mentioned.

-- 
dwmw2