Hi Stephane, Thanks for your review.
Well, I had seen two calls to pfm_power6_enable_counters in a row, so I was concerned that the two calls to *enable_counters could overlap, and both would see that the timer is not currently active, and both would call hrtimer start on the same timer. A race condition. The case for needing the lock on the *disable_counters call is incorrect, I realize now. I was initially thinking that if the cancel happened in between the hrtimer_active and hrtimer_start call, that would be bad. But that can't happen, because the timer would have to be inactive in order to call start, meaning that no cancel would take place. One concern, though, would be if there is a case where we can get two calls to disable_counters in a row, attempting to cancel the timer twice would be bad, I think. To prevent that, I could add another "hrtimer_active" check in *disable_counters. I will look at the further simplificaiton you suggested, thanks. - Corey stephane eranian <eran...@googlemail.com> wrote on 01/15/2009 02:43:01 PM: > 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 enteringthe timer > callback while executing that code? > > > On Wed, Jan 14, 2009 at 11:00 PM, Corey Ashford > <cjash...@linux.vnet.ibm.com> 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 > > cjash...@us.ibm.com > > > > > > > > (*) 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 > > perfmon2-devel@lists.sourceforge.net > > 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 > perfmon2-devel@lists.sourceforge.net > 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 perfmon2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
