linux-kernel - Re: [PATCH] perf_events: AMD event scheduling (v2)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1265300375.22001.102.camel@laptop>
Date:	Thu, 04 Feb 2010 17:19:35 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	Stephane Eranian <eranian@...gle.com>
Cc:	linux-kernel@...r.kernel.org, mingo@...e.hu, paulus@...ba.org,
	davem@...emloft.net, fweisbec@...il.com, robert.richter@....com,
	perfmon2-devel@...ts.sf.net, eranian@...il.com
Subject: Re: [PATCH] perf_events: AMD event scheduling (v2)

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/