Corey, Thanks for the update. Please send the 2.6.24 patch. It should not be too hard to port to 2.6.25.
On Tue, Apr 15, 2008 at 2:53 AM, Corey Ashford <[EMAIL PROTECTED]> wrote: > Hi Stephane, > > Sorry for the delay. I back-ported your suggested fix to 2.6.24 and it > worked fine. Of course, I did need to do the override of the > pfm_spin_lock and pfm_spin_unlock calls in include/asm-powerpc/perfmon.h > (now perfmon_kern.h in latest git). > > One thing that caught my eye, though, was that there is a compilation > warning generated in perfmon_intr.c:__pfm_interrupt_handler() that says > the flags variable may not be initialized. So I added a "= 0" to the > declaration. I believe this warning is generated because of the > if-statement before the pfm_spin_lock call that has a conditional "goto > spurious" line which can cause the pfm_spin_lock call to be skipped. > > Would you like me to submit a patch against the 2.6.24 perfmon2 source? > > - Corey > > > > > Corey, > > > > Ok, that should not be too bad. Just grab the latest commit from the GIT > tree. > > > > On Thu, Apr 10, 2008 at 7:50 PM, Corey J Ashford <[EMAIL PROTECTED]> wrote: > >> > >> > >> The perfmon kernel we are using for our app is based on 2.6.24, not the > >> latest git repo. I will backport the changes > >> and let you know how it goes. > >> > >> Thanks, > >> > >> - Corey > >> > >> "stephane eranian" <[EMAIL PROTECTED]> wrote on 04/10/2008 06:21:45 > >> AM: > >> > >> > >> > >> > Corey, > >> > > >> > I ran a few tests. It appears that on both X87 (Core 2) and Itanium 2 > >> > (Montvale) there is a 4% penalty on average PMU ctxsw cost by using > >> > interrupt masking. That is a bit high. So instead, I have provided > >> > macros for you to override just like what you did for > >> > spin_lock_irqsave and > >> > pfm_spin_unlock_irqrestore(). Try overriding them on PPC and let me > >> > know if this works now. Just pull from GIT to get the patch. > >> > > >> > > >> > On Thu, Apr 10, 2008 at 2:20 AM, Corey J Ashford <[EMAIL PROTECTED]> > >> wrote: > >> > > > >> > > > >> > > 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 > >> > > [EMAIL PROTECTED] > >> > > > >> > > > >> > > "stephane eranian" <[EMAIL PROTECTED]> wrote on 04/09/2008 > >> 03:46:56 > >> > > PM: > >> > > > >> > > > >> > > > >> > > > Corey, > >> > > > > >> > > > > >> > > > On Thu, Apr 10, 2008 at 12:26 AM, Corey Ashford > >> <[EMAIL PROTECTED]> > >> > > 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 ifyou > >> 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 > >> > > > > [EMAIL PROTECTED] > >> > > > > > >> > > > > > >> > > > >> > > > >> > > > >> > > -- > > Corey Ashford > Software Engineer > IBM Linux Technology Center, Linux Toolchain > Beaverton, OR > 503-578-3507 > [EMAIL PROTECTED] > > > > > ------------------------------------------------------------------------- > This SF.net email is sponsored by the 2008 JavaOne(SM) Conference > Don't miss this year's exciting event. There's still time to save $100. > Use priority code J8TL2D2. > > http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone > _______________________________________________ > perfmon2-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/perfmon2-devel > ------------------------------------------------------------------------- This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2. http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone _______________________________________________ perfmon2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
