On Thu, 2010-02-04 at 17:05 +0100, Stephane Eranian wrote:
> >> @@ -2250,10 +2261,144 @@ intel_get_event_constraints(struct cpu_hw_events
> >> *cpuc, struct perf_event *event
> >> return &unconstrained;
> >> }
> >>
> >> +/*
> >> + * AMD64 events are detected based on their event codes.
> >> + */
> >> +static inline int amd_is_nb_event(struct hw_perf_event *hwc)
> >> +{
> >> + u64 val = hwc->config;
> >
> > & K7_EVNTSEL_EVENT_MASK ?
>
> Yes, except that:
> -#define K7_EVNTSEL_EVENT_MASK 0x7000000FFULL
> +#define K7_EVNTSEL_EVENT_MASK 0xF000000FFULL
Ah, indeed.
> >> + *
> >> + * try to alllcate same counter as before if
> >> + * event has already been assigned once. Otherwise,
> >> + * try to use free counter k obtained during the 1st
> >> + * pass above.
> >> + */
> >> + i = j = hwc->idx != -1 ? hwc->idx : (k == -1 ? 0 : k);
> >
> > That's patently unreadable, and I'm not sure what happens if we failed
> > to find an eligible spot in the above loop, should we not somehow jump
> > out and return emptyconstraint?
> >
> I have clarified the nested if. The goal of the first loop is to check
> if the event is already present (explained why this is possible in the
> comment). We may not have scanned all the slots. Furthermore, there
> may be concurrent scans going on on other CPUs. The first pass tries
> to find an empty slot, it does not reserve it. The second loop is the actual
> allocation. We speculate the slot we found in the first pass is still
> available.
> If the second loops fails, then we return emptyconstraints.
Ah, now I see, yes indeed.
> >> + do {
> >> + old = cmpxchg(nb->owners+i, NULL, event);
> >> + if (!old)
> >> + break;
> >> + if (++i == x86_pmu.num_events)
> >> + i = 0;
> >> + } while (i != j);
> >> +skip:
> >> + if (!old)
> >> + return &nb->event_constraints[i];
> >> + return &emptyconstraint;
> >> }
> >> +
> >> + /*
> >> + * function may be called too early in the
> >> + * boot process, in which case nb_id is bogus
> >> + *
> >> + * for BSP, there is an explicit call from
> >> + * amd_pmu_init()
> >> + */
> >
> > I keep getting flash-backs to doom's graphics engine every time I see
> > BSP..
> >
> So how do you call the initial boot processor?
boot cpu, or x86 specific, cpu0.
>
> >> + nb_id = amd_get_nb_id(cpu);
> >> + if (nb_id == BAD_APICID)
> >> + return;
> >> +
> >> + cpu1 = &per_cpu(cpu_hw_events, cpu);
> >> + cpu1->amd_nb = NULL;
> >> +
> >> + raw_spin_lock(&amd_nb_lock);
> >> +
> >> + for_each_online_cpu(i) {
> >> + cpu2 = &per_cpu(cpu_hw_events, i);
> >> + nb = cpu2->amd_nb;
> >> + if (!nb)
> >> + continue;
> >> + if (nb->nb_id == nb_id)
> >> + goto found;
> >> + }
> >> +
> >> + nb = amd_alloc_nb(cpu, nb_id);
> >> + if (!nb) {
> >> + pr_err("perf_events: failed to allocate NB storage for
> >> CPU%d\n", cpu);
> >> + raw_spin_unlock(&amd_nb_lock);
> >> + return;
> >> + }
> >> +found:
> >> + nb->refcnt++;
> >> + cpu1->amd_nb = nb;
> >> +
> >> + raw_spin_unlock(&amd_nb_lock);
> >
> > Can't this be simplified by using the cpu to node mask?
>
> You mean to find the NB that corresponds to a CPU?
Yep, saves having to poke at all cpus.
> > Also, I think this is buggy in that:
> >
> > perf_disable();
> > event->pmu->disable(event);
> > ...
> > event->pmu->enable(event);
> > perf_enable();
> >
> > can now fail, I think we need to move the put_event_constraint() from
> > x86_pmu_disable() into x86_perf_enable() or something.
>
> Constraints are reserved during x86_scheduling(), not during enable().
> So if you had a conflict it was detected earlier than that.
What I'm worried about is:
CPU A and B are of the same NorthBridge and all node counters are taken.
CPU-A CPU-B
perf_disable();
event->pmu->disable(event)
x86_pmu.put_event_constraint() /* free slot n */
event->pmu->enable(event);
x86_schedule_events();
x86_pmu.get_event_constraint() /* grab slot n */
event->pmu->enable(event)
x86_schedule_events()
x86_pmu.get_event_constraint() /* FAIL */
perf_enable();
That means you cannot disable/enable the same event within a
perf_disable/enable section.
------------------------------------------------------------------------------
The Planet: dedicated and managed hosting, cloud storage, colocation
Stay online with enterprise data centers and the best network in the business
Choose flexible plans and management services without long-term contracts
Personal 24x7 support from experience hosting pros just a phone call away.
http://p.sf.net/sfu/theplanet-com
_______________________________________________
perfmon2-devel mailing list
perfmon2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
