Hi Stephane,
"stephane eranian" <[EMAIL PROTECTED]> wrote on 07/23/2008 08:25:28
AM:
> Corey,
> On Wed, Jul 23, 2008 at 1:37 AM, Corey J Ashford <[EMAIL PROTECTED]>
wrote:
> Thanks for your reply, Stephane. Some comments below.
>
> "stephane eranian" <[EMAIL PROTECTED]> wrote on 07/22/2008 04:11:18
PM:
>
> > Corey,
> >
>
> > On Wed, Jul 23, 2008 at 1:03 AM, Corey Ashford <[EMAIL PROTECTED]>
wrote:
> >
> >> In this patch, I've also fixed the pfm_arch_restore_pmds to
> >> correctly set the upper bit of the pmd register using the
> >> corresponding povfl_mask bit. This wasn't being done before, and I
> >> believe it was an error.
> >
> > You don't need to do this because that's the whole point of the
> > interrupt replay mechanism. It does check povfl_pmds and it will call
the
> > interrupt handler and trigger the right sequence of actions, e.g.,
> > add a new sample to the sampling buffer.
> There are a couple of things going on in that code. As it was, we
> were taking the 32 least significant bits of the count, and just
> writing them into the pmd register, regardless of whether or not the
> most significant bit was set. In the case where bit 31 = 1 and the
> corresponding povfl bit for that pmd = 0, this will generate a
> spurious interrupt and make the interrupt handler think that another
> 2^32 counts have occurred. So, at a minimum, we need to select only
> the lower 31 bits to write into the counter.
>
> As for bit 31, I think we need to set it according to the povfl mask
> because otherwise, when we arrive at the interrupt handler (via a
> replay), we won't know that the counter has overflowed. It won't
> appear to have because its upper bit won't be set.
>
> Am I missing something there?
>
> Yes, there is a small details which makes this work without
> correcting for overflow in pfm_arch_restore_pmds(). Suppose you
> don't do what you are suggesting.
> You restore the pmds as is, and in ctxswin we replay the interrupt.
> You get to the interrupt handler which calls:
> - freeze
> - arch_stop
> - stop_active
>
> And that's where the interesting statement is (arch/powerpc/perfmon.c):
>
>
> static void pfm_stop_active(struct task_struct *task,
> struct pfm_context *ctx, struct
> pfm_event_set *set)
> {
> struct pfm_arch_pmu_info *arch_info;
>
> arch_info = pfm_pmu_info();
> BUG_ON(!arch_info->disable_counters || !arch_info->
get_ovfl_pmds);
>
> arch_info->disable_counters(ctx, set);
>
> if (set->npend_ovfls)
> return;
>
> arch_info->get_ovfl_pmds(ctx, set);
> }
>
> The key is the if (set->npend_ovfls) test. You do not inspect the
> registers again. Otherwise, I agree, you would
> not detect an overflow. The generic handler does not inspect the
> pmds, it simply increments the 64-bit register
> value just like you are suggesting in your patch.
>
> Hope this clarifies what is going on.
Yes, that does. Thank you.
Do you agree that the code should only write the least 31 significant bits
to the PMD in restore? It seems to me that it should.
Regards,
- Corey,
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
