Re: [perfmon2] [patch 1/1] spin lock fix for ctx->lock
Status: Beta
Brought to you by:
seranian
From: Corey J A. <cja...@us...> - 2008-04-10 00:42:14
|
Hi Stephane, Thanks for the replies. I didn't check with Paul, but I looked into the code in schedule() and it does call local_irq_disable(), which does use the soft interrupt disabling mechanism. - Corey Corey Ashford Software Engineer IBM Linux Technology Center, Linux Toolchain Beaverton, OR 503-578-3507 cja...@us... "stephane eranian" <er...@go...> wrote on 04/09/2008 03:46:56 PM: > Corey, > > > On Thu, Apr 10, 2008 at 12:26 AM, Corey Ashford <cja...@us...> wrote: > > If schedule() uses lazy interrupt disabling, the problem we are seeing could > > result, since the PMU interrupt does not use the lazy interrupt wrapper on > > POWER. > > > Could you confirm this with Paul? > > > Do you think that using the pfm irq versions of this code is too much of a > > performance hit? > > You would be masking interrupts which are already masked. I don't know > the cost for each arch. > > > > > > I don't see any way for this to work right on POWER without this change, > > short of changing the wrapper used for the PMU interrupt (which we've > > already decided was a bad idea). Of course we could ifdef this code for > > POWER, I suppose. Ick. > > No, I'd rather pay the (hopefully) small penalty but have more readable code. > > > > > In any case, the fix in perfmon_smpl.c needs to be fixed, right? The other > > unlock in that function is a pfm_spin_unlock_irqrestore(...) > > > > I don't think the change in perfmon_smpl.c is needed. There is a reason why > the unlock is broken down in multiple steps. We need to have > interrupts enabled > for vfree() in pfm_context_free(). pfm_spin_unlock_irqrestore() does > not guarantee > that interrupts will be enabled, it simply restores them to the state > they were before > spin_lock_irqsave(). I have seen situations (I think on IA-64) where > pfm_handle_work > is not invoked with interrupts enabled. This part of the code is > handling a very special > case where the context is zombie, i.e., no more controlling > (monitoring) process. > > As for perfmon_intr.c, the reason this is regular spin_lock is because > interrupts are already > masked up to the same priority level. The PMU interrupts should be set > very high in the > priority levels. On X86, all interrupts are automatically masked. I > think that we could > enforce all interrupts masked with irqsave() to guarantee across the > board that we have > the same level of intr. masking. > > > > > - Corey > > > > > > > > > > > > The reason regular spin_lock/spin_unlock are used is because the > > > __pfm_ctxsw() routine is expected to > > > be run with interrupts ALREADY masked by upper level code (likely > > > schedule()). It may be that on PPC, > > > schedule does things differently at its tail (where switch_to()) is > > invoked. > > > > > > You are trying adding a BUG_ON(!irqs_disabled()) and see if you catch > > > something. If not then I wonder > > > how this could be related to the lazy masking for which you had to add > > > the special pfm_spin_lock_irq..... > > > > > > > > > > -- > > > > Corey Ashford > > Software Engineer > > IBM Linux Technology Center, Linux Toolchain > > Beaverton, OR > > 503-578-3507 > > cja...@us... > > > > |