Will do, thanks.
- Corey,
"stephane eranian" <[EMAIL PROTECTED]> wrote on 07/15/2008 12:38:45
PM:
> 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
