Corey,
I looked at the patch. It is better than the first version with the
per-cpu info.
I think it could be simplified a bit more by dropping:
struct pfm_arch_context *ctx_arch;
>From the struct pmc5_6_per_cpu. You can get to the context,
even in the hrtimer handler, via
ctx = __get_cpu_var(pmu_ctx);
ctx_arch = pfm_ctx_arch(ctx);
I am not sure I understand why you need a spinlock in the per-cpu struct.
The hrtimer is per-cpu, so is the structure. You lock/unlock in enable_counters
and disable_counters. Although it could be possible for two threads to try and
access the context at the same, there is already a context spinlock to serialize
execution, thus there is no risk of having one thread in enable_counters and
another in disable counters. Are you trying to prevent from entering the timer
callback while executing that code?
On Wed, Jan 14, 2009 at 11:00 PM, Corey Ashford
<[email protected]> wrote:
> Hello,
>
> I have prepared a new version of the fix to Power6's timers which are used
> for sampling(*) PMU registers 5 & 6. In addition to Stephane's comments
> about using DEFINE_PER_CPU, I have made a few additional changes:
>
> 1) I consolidated all of the per cpu data used for the sampling, including
> the hrtimer, into one DEFINE_PER_CPU data structure.
>
> 2) Because calls to pfm_power6_enable_counters and
> pfm_power6_disable_counters are not perfectly paired (extra calls to
> pfm_power6_enable_counters occur from pfm_restart), I needed some extra code
> to know when it's safe to call hrtimer_start on a timer and when it's not
> necessary. In order to do this correctly without race conditions, I added a
> spinlock on the timer to make sure that it's dealt with synchronously.
>
> 3) In the previous patch, the initialization of the hrtimers was done in
> pfm_power6_enable_counters. I have changed that to be done once during
> start-up of the module; all hrtimers are initialzed when the module is
> loaded. This will improve performance a little, and makes the code cleaner,
> I think. I used the "on_each_cpu" function to initialize each of the
> per-cpu timers (as well as the spin locks).
>
> Having said this, I may have been too conservative by using the spin lock.
> If it's the case that the enable and disable functions are not reentrant on
> the same CPU, the spin locks are not necessary. So, Stephane, if you are
> convinced that the spin locks are not necessary in this patch, I'd be happy
> to submit a v3 version without them.
>
> Please have a look at the patch and let me know if you have
> comments/issue/suggestions, etc.
>
> Thanks for your consideration,
>
>
> - Corey
>
> Corey Ashford
> Software Engineer
> IBM Linux Technology Center, Linux Toolchain
> Beaverton, OR
> 503-578-3507
> [email protected]
>
>
>
> (*) The term "sampling" is used rather poorly here. I don't mean perfmon
> sampling, but rather code that samples these free-running counters into
> virtual registers that can be started and stopped like counters 1-4 on
> thread context switches. Counters 5 & 6 cannot be stopped via hardware.
>
> ------------------------------------------------------------------------------
> This SF.net email is sponsored by:
> SourcForge Community
> SourceForge wants to tell your story.
> http://p.sf.net/sfu/sf-spreadtheword
> _______________________________________________
> perfmon2-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
>
>
------------------------------------------------------------------------------
This SF.net email is sponsored by:
SourcForge Community
SourceForge wants to tell your story.
http://p.sf.net/sfu/sf-spreadtheword
_______________________________________________
perfmon2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
