On 16.11.12 13:00:30, Jacob Shin wrote:
> On Fri, Nov 16, 2012 at 07:43:44PM +0100, Robert Richter wrote:
> > On 15.11.12 15:31:53, Jacob Shin wrote:
> > > @@ -156,31 +161,28 @@ static inline int amd_pmu_addr_offset(int index)
> > > if (offset)
> > > return offset;
> > >
> > > - if (!cpu_has_perfctr_core)
> > > + if (!cpu_has_perfctr_core) {
> > > offset = index;
> > > - else
> > > + ncore = AMD64_NUM_COUNTERS;
> > > + } else {
First calculation:
> > > offset = index << 1;
> > > + ncore = AMD64_NUM_COUNTERS_CORE;
> > > + }
> > > +
> > > + /* find offset of NB counters with respect to x86_pmu.eventsel */
> > > + if (cpu_has_perfctr_nb) {
> > > + if (index >= ncore && index < (ncore + AMD64_NUM_COUNTERS_NB))
Second calculation:
> > > + offset = (MSR_F15H_NB_PERF_CTL - x86_pmu.eventsel) +
> > > + ((index - ncore) << 1);
> > > + }
> >
> > There is duplicate calculatoin of offset in some cases. Better we
> > avoid this.
>
> Which cases? The code calculates the offset for a given index the very
> first time it is called, stores it, and uses that stored offset from
> then on. My [PATCH 3/4] sets that up.
One case above.
It looks like the paths should be defined more clearly.
> > > @@ -323,6 +368,16 @@ __amd_get_nb_event_constraints(struct cpu_hw_events
> > > *cpuc, struct perf_event *ev
> > > if (new == -1)
> > > return &emptyconstraint;
> > >
> > > + /* set up interrupts to be delivered only to this core */
> > > + if (cpu_has_perfctr_nb) {
> > > + struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> > > +
> > > + hwc->config |= AMD_PERFMON_EVENTSEL_INT_CORE_ENABLE;
> > > + hwc->config &= ~AMD_PERFMON_EVENTSEL_INT_CORE_SEL_MASK;
> > > + hwc->config |= (0ULL | (c->cpu_core_id)) <<
> > > + AMD_PERFMON_EVENTSEL_INT_CORE_SEL_SHIFT;
> > > + }
> >
> > Looks like a hack to me. The constaints handler is only supposed to
> > determine constraints and not to touch anything in the event's
> > structure. This should be done later when setting up hwc->config in
> > amd_nb_event_config() or so.
>
> Hm.. is the hwc->config called after constraints have been set up
> already? If so, I'll change it ..
Should be, since the hw register can be setup only after the counter
is selected.
>
> >
> > I also do not think that smp_processor_id() is the right thing to do
> > here. Since cpu_hw_events is per-cpu the cpu is already selected.
>
> Yeah, I could not figure out how to get the cpu number from cpuc. Is
> there a container_of kind of thing that I can do to get the cpu number
> ?
At some point event->cpu is assigned, I think.
-Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
