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
> >> > [email protected]
> >> > 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
[email protected]
https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
