Corey, Let's do the 2nd option as I have applied each patch independently so we keep a trace.
I will drop patch 4. Please submit a replacement and update patch 5 accordingly I noticed it was modifying the same file. On Tue, Jul 15, 2008 at 8:44 PM, Corey J Ashford <[EMAIL PROTECTED]> wrote: > Hi Stephane, > > Yes, that code is in arch/powerpc/kernel/process.c > > From here, we can handle this two ways: > 1) I can re-submit 5 patches, getting rid of the get_cpu/put_cpu patch, and > adding the patch to fix the bug described below. > 2) I can just submit a new patch to fix this bug separately, and we can just > agree to reject [PATCH 4/5] which is ghe get_cpu/put_cpu > patch > > Which do you prefer? Or some other option? > > Thanks, > > - Corey > > Corey Ashford > Software Engineer > IBM Linux Technology Center, Linux Toolchain > Beaverton, OR > 503-578-3507 > [EMAIL PROTECTED] > > > "stephane eranian" <[EMAIL PROTECTED]> wrote on 07/15/2008 11:36:50 AM: > >> Corey, >> >> I assume what you are showing in Power specific. In that case, I agree >> in needs to be inside >> the critical section. >> >> >> On Tue, Jul 15, 2008 at 8:27 PM, Corey J Ashford <[EMAIL PROTECTED]> >> wrote: >> > Thanks for the reply >> > "stephane eranian" <[EMAIL PROTECTED]> wrote on 07/15/2008 09:52:28 >> > AM: >> > >> > Looking at the code, it appears you are right about the sread and swrite >> > functions, but I'm now looking at the function >> > pfm_power6_enable_counters. >> > In one case, the call stack looks like this: >> > >> > >> > arch/powerpc/perfmon/perfmon_power6.c: pfm_power6_enable_counters >> > arch/powerpc/perfmon/perfmon.c: pfm_arch_ctxswin_thread >> > perfmon/perfmon_ctxsw.c: __pfm_ctxswin_thread >> > perfmon/perfmon_ctxsw.c: pfm_ctxsw_in >> > arch/powerpc/kernel/process.c: __switch_to >> > >> > >> > The only code that I can find that disables interrupts in this call >> > chain is >> > in __switch_to, but the call to pfm_ctxsw_in is >> > not inside the interrupts-disabled section. The code is as follows: >> > >> > ... >> > if (test_tsk_thread_flag(prev, TIF_PERFMON_CTXSW)) >> > pfm_ctxsw_out(prev, new); >> > >> > if (test_tsk_thread_flag(new, TIF_PERFMON_CTXSW)) >> > pfm_ctxsw_in(prev, new); >> > >> > local_irq_save(flags); >> > >> > account_system_vtime(current); >> > account_process_vtime(current); >> > calculate_steal_time(); >> > >> > /* >> > * We can't take a PMU exception inside _switch() since there is >> > a >> > * window where the kernel stack SLB and the kernel stack are >> > out >> > * of sync. Hard disable here. >> > */ >> > hard_irq_disable(); >> > last = _switch(old_thread, new_thread); >> > >> > local_irq_restore(flags); >> > >> > return last; >> > } >> > >> > The comments in __pfm_ctxswin_thread indicate that it is expected to be >> > run >> > with interrupts off. Do you agree that this POWER-specific code looks >> > incorrect, that those if-statements should be inside the >> > local_irq_save/local_irq_restore block? >> > >> > Thanks for your consideration, >> > >> > - Corey >> > >> > Corey Ashford >> > Software Engineer >> > IBM Linux Technology Center, Linux Toolchain >> > Beaverton, OR >> > 503-578-3507 >> > [EMAIL PROTECTED] >> > >> > >> >> Corey, >> >> >> >> AFAIK, PMD registers can be read at 5 different times: >> >> >> >> - on pfm_read_pmds() >> >> - on context switch save >> >> - set switch save >> >> - on counter overflow to record a sample >> >> - on unload for flush PMU state to software >> >> >> >> In all situations, interrupts are indeed disabled. So you should not >> >> need >> >> getcpu/putcpu. >> >> >> >> >> >> On Tue, Jul 15, 2008 at 6:46 PM, Corey J Ashford <[EMAIL PROTECTED]> >> >> wrote: >> >> > Hi Stephane, >> >> > >> >> > I was not aware that interrupts are disabled during the call the >> >> > pmd_sread >> >> > call, thanks for catching that. Is that also the case for the >> >> > enable_counters and disable_counters calls as well? >> >> > >> >> > Regards, >> >> > >> >> > - Corey >> >> > >> >> > Corey Ashford >> >> > Software Engineer >> >> > IBM Linux Technology Center, Linux Toolchain >> >> > Beaverton, OR >> >> > 503-578-3507 >> >> > [EMAIL PROTECTED] >> >> > >> >> > >> >> > >> >> > >> >> > "stephane eranian" <[EMAIL PROTECTED]> >> >> > 07/15/2008 03:24 AM >> >> > Please respond to >> >> > [EMAIL PROTECTED] >> >> > >> >> > >> >> > To >> >> > Corey J Ashford/Beaverton/[EMAIL PROTECTED] >> >> > cc >> >> > perfmon2-devel@lists.sourceforge.net >> >> > Subject >> >> > Re: [PATCH 5/5] fixes for full perfmon2 on POWER >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > Corey, >> >> > >> >> > >> >> > I don't understand this patch. >> >> > You say: >> >> > "This patch fixes a problem with accessing the array of cpu-specific >> >> > PMU >> >> > counters, where there was a race condition that could occur where we >> >> > acquire the cpu number, and then have the cpu switch out from >> >> > underneath >> >> > us. Using get_cpu()/put_cpu() fixes this problem because get_cpu() >> >> > disables preemption until put_cpu() is called again." >> >> > >> >> > What do you mean by "switch from underneath". AFAIK, pmd_sread() is >> >> > called >> >> > with interrupts masked. I thought, this was enough to prevent any >> >> > preemption? >> >> > >> >> > >> >> > >> >> > On Mon, Jul 14, 2008 at 11:50 PM, Corey Ashford <[EMAIL PROTECTED]> >> >> > wrote: >> >> >> This fixes an issue on POWER4 and POWER6 where PMU exceptions need >> >> >> to >> >> >> be >> >> >> disabled when the context is in masked mode. >> >> >> >> >> >> -- >> >> >> 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 Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ _______________________________________________ perfmon2-devel mailing list perfmon2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
