Re: [perfmon2] [PATCH 5/5] fixes for full perfmon2 on POWER

[Corey J A. <cja...@us...>]

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
cja...@us...


"stephane eranian" <er...@go...> 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 <cja...@us...>
wrote:
> > Thanks for the reply
> > "stephane eranian" <er...@go...> 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
> > cja...@us...
> >
> >
> >> 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 <cja...@us...>
> >> 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
> >> > cja...@us...
> >> >
> >> >
> >> >
> >> >
> >> > "stephane eranian" <er...@go...>
> >> > 07/15/2008 03:24 AM
> >> > Please respond to
> >> > er...@gm...
> >> >
> >> >
> >> > To
> >> > Corey J Ashford/Beaverton/IBM@IBMUS
> >> > cc
> >> > per...@li...
> >> > 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
<cja...@us...>
> >> > 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
> >> >> cja...@us...
> >> >>
> >> >
> >> >
> >> >
> >