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
