perf_events: improve x86 event scheduling (v5)

arch/x86/kernel/cpu/perf_event.c +50 -0

+int hw_perf_group_sched_in(struct perf_event *leader,
+	       struct perf_cpu_context *cpuctx,
+	       struct perf_event_context *ctx, int cpu)
+{
+	struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
+	struct perf_event *sub;
+	int assign[X86_PMC_IDX_MAX];
+	int n0, n1, ret;
+
+	/* n0 = total number of events */
+	n0 = collect_events(cpuc, leader, true);
+	if (n0 < 0)
+		return n0;
+
+	ret = x86_schedule_events(cpuc, n0, assign);
+	if (ret)
+		return ret;
+
+	ret = x86_event_sched_in(leader, cpuctx, cpu);
+	if (ret)
+		return ret;
+
+	n1 = 1;
+	list_for_each_entry(sub, &leader->sibling_list, group_entry) {
+		if (sub->state != PERF_EVENT_STATE_OFF) {
+			ret = x86_event_sched_in(sub, cpuctx, cpu);
+			if (ret)
+				goto undo;
+			++n1;
+		}
+	}
+	/*
+	 * copy new assignment, now we know it is possible
+	 * will be used by hw_perf_enable()
+	 */
+	memcpy(cpuc->assign, assign, n0*sizeof(int));
+
+	cpuc->n_events  = n0;
+	cpuc->n_added   = n1;
+	ctx->nr_active += n1;
+
+	/*
+	 * 1 means successful and events are active
+	 * This is not quite true because we defer
+	 * actual activation until hw_perf_enable() but
+	 * this way we* ensure caller won't try to enable
+	 * individual events
+	 */
+	return 1;
+undo:

Shouldn't we actually use the core based pmu->enable(),disable() model called from kernel/perf_event.c:event_sched_in(), like every other events, where we can fill up the queue of hardware events to be scheduled, and then call a hw_check_constraints() when we finish a group scheduling?

Quoted 6 lines · Frederic Weisbecker

Shouldn't we actually use the core based pmu->enable(),disable() model called from kernel/perf_event.c:event_sched_in(), like every other events, where we can fill up the queue of hardware events to be scheduled, and then call a hw_check_constraints() when we finish a group scheduling?

Well the thing that makes hw_perf_group_sched_in() useful is that you can add a bunch of events and not have to reschedule for each one, but instead do a single schedule pass.

That said you do have a point, maybe we can express this particular thing differently.. maybe a pre and post group call like:

 void hw_perf_group_sched_in_begin(struct pmu *pmu) int  hw_perf_group_sched_in_end(struct pmu *pmu)

That way we know we need to track more state for rollback and can give the pmu implementation leeway to delay scheduling/availablility tests.

Paul, would that work for you too?

Then there's still the question of having events of multiple hw pmus in a single group, I'd be perfectly fine with saying that's not allowed, what to others think?

Quoted 5 lines

Then there's still the question of having events of multiple hw pmus in a single group, I'd be perfectly fine with saying that's not allowed, what to others think?

I have seen requests for measuring both core and uncore PMU events together for instance. It all depends on how uncore PMU will be managed.

Quoted 6 lines

That said you do have a point, maybe we can express this particular thing differently.. maybe a pre and post group call like:

void hw_perf_group_sched_in_begin(struct pmu *pmu) int hw_perf_group_sched_in_end(struct pmu *pmu)

The issue with hw_perf_group_sched_in() is that because we do not know when we are done scheduling, we have to defer actual activation until hw_perf_enable(). But we have to still mark the events as ACTIVE, otherwise things go wrong in the generic layer and for non-PMU events. That leads to partial duplication of event_sched_in()/event_sched_out() in the PMU specific layer.

As Frederic pointed out, the more natural way would be to simply rely on event_sched_in()/event_sched_out() and the rollback logic and just drop hw_perf_group_sched_in() which is there as an optimization and not for correctness. Scheduling can be done incrementally from the event_sched_in() function.

Quoted 3 lines

That way we know we need to track more state for rollback and can give the pmu implementation leeway to delay scheduling/availablility tests.

Rollback would still be handled by the generic code, wouldn't it?

Quoted 6 lines · Frederic Weisbecker

Shouldn't we actually use the core based pmu->enable(),disable() model called from kernel/perf_event.c:event_sched_in(), like every other events, where we can fill up the queue of hardware events to be scheduled, and then call a hw_check_constraints() when we finish a group scheduling?

Well the thing that makes hw_perf_group_sched_in() useful is that you can add a bunch of events and not have to reschedule for each one, but instead do a single schedule pass.

That said you do have a point, maybe we can express this particular thing differently.. maybe a pre and post group call like:

void hw_perf_group_sched_in_begin(struct pmu *pmu) int hw_perf_group_sched_in_end(struct pmu *pmu)

That way we know we need to track more state for rollback and can give the pmu implementation leeway to delay scheduling/availablility tests.

Then there's still the question of having events of multiple hw pmus in a single group, I'd be perfectly fine with saying that's not allowed, what to others think?

Well in appearance, things go through one pass.

But actually not, there is a first iteration that collects the events (walking trhough the group list, filtering soft events), a second iteration that check the constraints and schedule (but not apply) the events.

And thereafter we schedule soft events (and revert the whole if needed).

This is a one pass from group_sched_in() POV but at the cost of reimplementating what the core does wrt soft events and iterations. And not only is it reinventing the wheel, it also produces more iterations than we need.

If we were using the common pmu->enable() from group/event_sched_in(), that would build the collection, with only one iteration through the group list (instead of one to collect, and one for the software events).

And the constraints can be validated in a second explicit iteration through hw_check_constraint(), like it's currently done explicitly from hw_perf_group_sched_in() that calls x86_schedule_event().

The fact is we have with this patch a _lot_ of iterations each time x86 get scheduled. This is really a lot for a fast path. But considering the dynamic cpu events / task events series we can have, I don't see other alternatives.

Do you mean this:

hw_perf_group_sched_in_begin(&x86_pmu);

for_each_event(event, group) {
        event->enable();        //do the collection here
}

if (hw_perf_group_sched_in_end(&x86_pmu)) {
        rollback...
}

That requires to know in advance if we have hardware pmu in the list though (can be a flag in the group).

Quoted 22 lines · Frederic Weisbecker

Well in appearance, things go through one pass.

But actually not, there is a first iteration that collects the events (walking trhough the group list, filtering soft events), a second iteration that check the constraints and schedule (but not apply) the events.

And thereafter we schedule soft events (and revert the whole if needed).

This is a one pass from group_sched_in() POV but at the cost of reimplementating what the core does wrt soft events and iterations. And not only is it reinventing the wheel, it also produces more iterations than we need.

If we were using the common pmu->enable() from group/event_sched_in(), that would build the collection, with only one iteration through the group list (instead of one to collect, and one for the software events).

Quoted 3 lines

And the constraints can be validated in a second explicit iteration through hw_check_constraint(), like it's currently done explicitly from hw_perf_group_sched_in() that calls x86_schedule_event().

Thing is, we cannot do that, because we currently require ->enable() to report schedulability. Now we could add an argument to ->enable, or add callbacks like I suggested to convey that state.

Quoted 4 lines

The fact is we have with this patch a _lot_ of iterations each time x86 get scheduled. This is really a lot for a fast path. But considering the dynamic cpu events / task events series we can have, I don't see other alternatives.

Luckily it tries to use a previous configuration, so in practise the schedule phase is real quick amortized O(1) as long as we don't change the set.

Quoted 13 lines

Do you mean this:

hw_perf_group_sched_in_begin(&x86_pmu);

for_each_event(event, group) {
        event->enable();        //do the collection here
}

if (hw_perf_group_sched_in_end(&x86_pmu)) {
        rollback...
}

Quoted 2 lines

That requires to know in advance if we have hardware pmu in the list though (can be a flag in the group).

Good point, but your proposed hw_check_constraint() call needs to know the exact same.

Quoted 6 lines

Quoted 3 lines

That requires to know in advance if we have hardware pmu in the list though (can be a flag in the group).

Good point, but your proposed hw_check_constraint() call needs to know the exact same.

True. Whatever model we use anyway, both implement the same idea.

Quoted 8 lines · Frederic Weisbecker

Quoted 6 lines

Quoted 3 lines

That requires to know in advance if we have hardware pmu in the list though (can be a flag in the group).

Good point, but your proposed hw_check_constraint() call needs to know the exact same.

Quoted 1 line

True. Whatever model we use anyway, both implement the same idea.

Hmm, we seem to already have that problem (which would indicate another bug in the hw-breakpoint stuff), how do you deal with hw_perf_{enable,disable}() for the breakpoints?

Quoted 9 lines · Peter Zijlstra

Quoted 8 lines · Frederic Weisbecker

Quoted 6 lines

Quoted 3 lines

That requires to know in advance if we have hardware pmu in the list though (can be a flag in the group).

Good point, but your proposed hw_check_constraint() call needs to know the exact same.

Quoted 5 lines

Quoted 1 line

True. Whatever model we use anyway, both implement the same idea.

Hmm, we seem to already have that problem (which would indicate another bug in the hw-breakpoint stuff), how do you deal with hw_perf_{enable,disable}() for the breakpoints?

We don't have ordering constraints for breakpoints, only constraints on the number of available registers.

So we check the constraints when a breakpoint registers. The enable/disable then (is supposed to) always succeed on breakpoint pmu, except for flexible breakpoints that can make it or not, but no need to overwrite group scheduling for that.

Quoted 20 lines · Frederic Weisbecker

Quoted 9 lines · Peter Zijlstra

Quoted 8 lines · Frederic Weisbecker

Quoted 6 lines

Quoted 3 lines

That requires to know in advance if we have hardware pmu in the list though (can be a flag in the group).

Good point, but your proposed hw_check_constraint() call needs to know the exact same.

Quoted 5 lines

Quoted 1 line

True. Whatever model we use anyway, both implement the same idea.

Hmm, we seem to already have that problem (which would indicate another bug in the hw-breakpoint stuff), how do you deal with hw_perf_{enable,disable}() for the breakpoints?

We don't have ordering constraints for breakpoints, only constraints on the number of available registers.

Quoted 4 lines

So we check the constraints when a breakpoint registers. The enable/disable then (is supposed to) always succeed on breakpoint pmu, except for flexible breakpoints that can make it or not, but no need to overwrite group scheduling for that.

hw_perf_{enable,disable} are unrelated to groups.

Right hw_perf_enable/disable have no action on breakpoint events. These were somehow considered as software events until now.

That raises the question: why perf_disable() only takes care of hardware events? Very few software events can trigger between perf_disable() and perf_enable() sections though.

May be I should handle breakpoints there.

Quoted 7 lines · Frederic Weisbecker

Right hw_perf_enable/disable have no action on breakpoint events. These were somehow considered as software events until now.

That raises the question: why perf_disable() only takes care of hardware events? Very few software events can trigger between perf_disable() and perf_enable() sections though.

Quoted 1 line

May be I should handle breakpoints there.

OK, so maybe I'm not understanding the breakpoint stuff correctly, why is it modeled as a software pmu? It has resource constraints like a hardware pmu.

Quoted 9 lines · Peter Zijlstra

Quoted 7 lines · Frederic Weisbecker

Right hw_perf_enable/disable have no action on breakpoint events. These were somehow considered as software events until now.

That raises the question: why perf_disable() only takes care of hardware events? Very few software events can trigger between perf_disable() and perf_enable() sections though.

Quoted 5 lines

Quoted 1 line

May be I should handle breakpoints there.

OK, so maybe I'm not understanding the breakpoint stuff correctly, why is it modeled as a software pmu? It has resource constraints like a hardware pmu.

It doesn't use the software pmu, it uses its own. But what kind of properties can it share with other hardware events?

It has constraints that only need to be checked when we register the event. It has also constraint on enable time but nothing tricky that requires an overwritten group scheduling.

It has constraints that only need to be checked when we register the event. It has also constraint on enable time but nothing tricky that requires an overwritten group scheduling.

Quoted 3 lines · Frederic Weisbecker

It has constraints that only need to be checked when we register the event. It has also constraint on enable time but nothing tricky that requires an overwritten group scheduling.

The fact that ->enable() can fail makes it a hardware counter. Software counters cannot fail enable.

Having multiple groups of failable events (multiple hardware pmus) can go wrong with the current core in interesting ways, look for example at __perf_event_sched_in():

It does:

	int can_add_hw = 1;

	...

	list_for_each_entry(event, &ctx->flexible_groups, group_entry) {
		/* Ignore events in OFF or ERROR state */
		if (event->state <= PERF_EVENT_STATE_OFF)
			continue;
		/*
		 * Listen to the 'cpu' scheduling filter constraint
		 * of events:
		 */
		if (event->cpu != -1 && event->cpu != cpu)
			continue;

		if (group_can_go_on(event, cpuctx, can_add_hw))
			if (group_sched_in(event, cpuctx, ctx, cpu))
				can_add_hw = 0;
	}

Now, if you look at that logic you'll see that it assumes there's one hw device since it only has one can_add_hw state. So if your hw_breakpoint pmu starts to fail we'll also stop adding counters to the cpu pmu (for lack of a better name) and vs.

This might be fixable by using per-cpu struct pmu variables.

I'm going to try and move all the weak hw_perf_* functions into struct pmu and create a notifier like callchain for them so we can have proper per pmu state, and then use that to fix these things up.

However I'm afraid its far to late to push any of that into .33, which means .33 will have rather funny behaviour once the breakpoints start getting used.

Hrmph, so I read some of that hw_breakpoint stuff, and now I'm sorta confused, it looks like ->enable should never fail, but that means you cannot overcommit breakpoints, which doesn't fit the perf model nicely.

Also, I see you set an ->unthrottle, but then don't implement it, but comment it as todo, which is strange because that implies its broken. If there's an ->unthrottle method it will throttle, so if its todo, the safest thing is to not set it.

/me mutters something and goes look at something else for a while.

Quoted 4 lines

Also, I see you set an ->unthrottle, but then don't implement it, but comment it as todo, which is strange because that implies its broken. If there's an ->unthrottle method it will throttle, so if its todo, the safest thing is to not set it.

Yeah, that's because I have a too vague idea on what is the purpose of the unthrottle() callback.

I've read the concerned codes that call this, several times, and I still can't figure out what happens there, not sure what is meant by throttle or unthrottle there :-/

Quoted 6 lines

Quoted 4 lines

Also, I see you set an ->unthrottle, but then don't implement it, but comment it as todo, which is strange because that implies its broken. If there's an ->unthrottle method it will throttle, so if its todo, the safest thing is to not set it.

Quoted 6 lines

Yeah, that's because I have a too vague idea on what is the purpose of the unthrottle() callback.

I've read the concerned codes that call this, several times, and I still can't figure out what happens there, not sure what is meant by throttle or unthrottle there :-/

OK, so not setting it is relatively safe.

As to what it does, it has to undo everything you do when perf_event_overflow() returns true, which happens when ->unthrottle is set and we get more than sysctl_perf_event_sample_rate/HZ events in a jiffy.

If you look at the x86 implementation, you'll see that we simply disable the hardware counter when the overflow call returns true, so the unthrottle() callback simply enables it again.

Quoted 3 lines · Frederic Weisbecker

It has constraints that only need to be checked when we register the event. It has also constraint on enable time but nothing tricky that requires an overwritten group scheduling.

The fact that ->enable() can fail makes it a hardware counter. Software counters cannot fail enable.

Having multiple groups of failable events (multiple hardware pmus) can go wrong with the current core in interesting ways, look for example at __perf_event_sched_in():

It does:

	int can_add_hw = 1;

	...

	list_for_each_entry(event, &ctx->flexible_groups, group_entry) {
		/* Ignore events in OFF or ERROR state */
		if (event->state <= PERF_EVENT_STATE_OFF)
			continue;
		/*
		 * Listen to the 'cpu' scheduling filter constraint
		 * of events:
		 */
		if (event->cpu != -1 && event->cpu != cpu)
			continue;

		if (group_can_go_on(event, cpuctx, can_add_hw))
			if (group_sched_in(event, cpuctx, ctx, cpu))
				can_add_hw = 0;
	}

Now, if you look at that logic you'll see that it assumes there's one hw device since it only has one can_add_hw state. So if your hw_breakpoint pmu starts to fail we'll also stop adding counters to the cpu pmu (for lack of a better name) and vs.

This might be fixable by using per-cpu struct pmu variables.

However I'm afraid its far to late to push any of that into .33, which means .33 will have rather funny behaviour once the breakpoints start getting used.

Quoted 9 lines

Quoted 9 lines

Do you mean this:

hw_perf_group_sched_in_begin(&x86_pmu);

for_each_event(event, group) {
        event->enable();        //do the collection here }

Quoted 7 lines

Quoted 6 lines

if (hw_perf_group_sched_in_end(&x86_pmu)) {
        rollback...
}

That requires to know in advance if we have hardware pmu in the list though (can be a flag in the group).

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Do you mean this:

hw_perf_group_sched_in_begin(&x86_pmu);

for_each_event(event, group) {
        event->enable();        //do the collection here
}

if (hw_perf_group_sched_in_end(&x86_pmu)) {
        rollback...
}

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

That requires to know in advance if we have hardware pmu in the list though (can be a flag in the group).

I don't think this model can work without scheduling for each event.

Imagine the situation where you have more events than you have counters. At each tick you:

Quoted 19 lines · Stephane Eranian

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Do you mean this:

hw_perf_group_sched_in_begin(&x86_pmu);

for_each_event(event, group) {
        event->enable();        //do the collection here }

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

if (hw_perf_group_sched_in_end(&x86_pmu)) {
        rollback...
}

That requires to know in advance if we have hardware pmu in the list though (can be a flag in the group).

I don't think this model can work without scheduling for each event.

Quoted 2 lines

Imagine the situation where you have more events than you have counters. At each tick you:

No it wont indeed, but it will work for where we now use hw_perf_group_sched_in() without having to replicate lots of code.

For the cases you mention I see no other way than to try and schedule each event individually.

Are you suggesting a speculative approach where you first try simply accumulate then schedule and if this fails, then restart the whole loop but this time adding and scheduling each event individually? For groups, you'd have to fail the group if one of its events fails.

Quoted 4 lines · Stephane Eranian

Are you suggesting a speculative approach where you first try simply accumulate then schedule and if this fails, then restart the whole loop but this time adding and scheduling each event individually? For groups, you'd have to fail the group if one of its events fails.

No, I'm only talking about groups. The complaint from frederic was that current hw_perf_group_sched_in() implementations have to basically replicate all of the group_sched_in() and event_sched_in() stuff, which seems wasteful.

So I was thinking of an alternative interface that would give the same end result but not as much code replication.

I'm now leaning towards adding a parameter to ->enable() to postpone schedulability and add a hw_perf_validate() like call.

With that I'm also looking at what would be the sanest way to multiplex all the current weak hw_perf* functions in the light of multiple pmu implementations.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Do you mean this:

hw_perf_group_sched_in_begin(&x86_pmu);

for_each_event(event, group) {
        event->enable();        //do the collection here }

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

if (hw_perf_group_sched_in_end(&x86_pmu)) {
        rollback...
}

That requires to know in advance if we have hardware pmu in the list though (can be a flag in the group).

I don't think this model can work without scheduling for each event.

Imagine the situation where you have more events than you have counters. At each tick you:
- disable all events
- rotate the list
- collect events from the list
- schedule events
- activate

Collection is the accumulation of events until you have as many as you have counters
given you defer scheduling until the end (see loop above).

But that does not mean you can schedule what you have accumulated. And then what do you do, i.e., rollback to what?

With incremental, you can skip a group that is conflicting with the groups already
accumulated. What hw_perf_group_sched_in() gives you is simply a way to do incremental on a whole event group at once.

Given the perf_event model, I believe you have no other way but to do incremental
scheduling of events. That is the only way you guarantee you maximize the use of the PMU. Regardless of that, the scheduling model has a bias towards smaller and less constrained event groups.

Quoted 27 lines · Stephane Eranian

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Do you mean this:

hw_perf_group_sched_in_begin(&x86_pmu);

for_each_event(event, group) {
        event->enable();        //do the collection here }

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

if (hw_perf_group_sched_in_end(&x86_pmu)) {
        rollback...
}

That requires to know in advance if we have hardware pmu in the list though (can be a flag in the group).

I don't think this model can work without scheduling for each event.

Imagine the situation where you have more events than you have counters. At each tick you:
   - disable all events
   - rotate the list
   - collect events from the list
   - schedule events
   - activate

Quoted 6 lines

Collection is the accumulation of events until you have as many as you have counters
given you defer scheduling until the end (see loop above).

But that does not mean you can schedule what you have accumulated. And then what do you do, i.e., rollback to what?

If the scheduling validation fails, then you just need to rollback the whole group.

That's sensibly what you did in your patch, right? Except the loop is now handled by the core code.

I don't understand why that can't be done with the above model. In your patch we iterate through the whole group, collect events, and schedule them.

With the above, the collection is just done on enable(), and the scheduling is done with the new pmu callbacks.

The thing is sensibly the same, where is the obstacle?

Quoted 7 lines

If the scheduling validation fails, then you just need to rollback the whole group.

That's sensibly what you did in your patch, right? Except the loop is now handled by the core code.

Ok, I think I missed where you were actually placing that loop. So you want to do this in group_sched_in(), right?

Quoted 5 lines

I don't understand why that can't be done with the above model. In your patch we iterate through the whole group, collect events, and schedule them.

Quoted 5 lines

With the above, the collection is just done on enable(), and the scheduling is done with the new pmu callbacks.

The thing is sensibly the same, where is the obstacle?

There is none. You've just hoisted the some of the code from hw_perf_group_sched_in().

Quoted 4 lines

Then there's still the question of having events of multiple hw pmus in a single group, I'd be perfectly fine with saying that's not allowed, what to others think?

I guess we need that. It can be insteresting to couple hardware counters with memory accesses...or whatever.

Perf stat combines cache miss counting with page faults, cpu clock counters.

We shouldn't limit such possibilities for technical/cleanliness reasons. We should rather adapt.

Quoted 6 lines · Frederic Weisbecker

Quoted 4 lines

Then there's still the question of having events of multiple hw pmus in a single group, I'd be perfectly fine with saying that's not allowed, what to others think?

Quoted 2 lines

I guess we need that. It can be insteresting to couple hardware counters with memory accesses...or whatever.

That really depends on how easy it is to correlate events from the various pmus. This case could indeed do that, but the core vs uncore tihng is a lot less clear.

Quoted 2 lines

Perf stat combines cache miss counting with page faults, cpu clock counters.

perf stat also doesn't use groups and it still works quite nicely.

Quoted 2 lines

We shouldn't limit such possibilities for technical/cleanliness reasons. We should rather adapt.

Maybe, I'm not a very big fan of groups myself, but they are clearly useful within a pmu, measuring cache misses through total-access for example, but the use between pmus is questionable.

Not sure what you both mean by this core VS uncore thing :) Is it about hardware counters that apply to single hardware threads or shared among them inside a same core?

Quoted 3 lines · Frederic Weisbecker

Not sure what you both mean by this core VS uncore thing :) Is it about hardware counters that apply to single hardware threads or shared among them inside a same core?

Yeah intel has two PMUs, one per logical cpu and one per socket/node. We currently don't support the node one yet.

So the thing with these socket wide things is is that they are not able to tell where the event originated, so relating that back to a task is only possible when there's only one task running.

+ }

        x86_pmu.enable_all();

}

Quoted 6 lines · Stephane Eranian

 void hw_perf_enable(void)
 {

+               cpuc->n_added = 0;
+               perf_events_lapic_init();

Just wondering, why do we need that lapic_init() there?

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines · Stephane Eranian

void hw_perf_enable(void)
{

Quoted 5 lines

arch/x86/kernel/cpu/perf_event.c static void amd_pmu_enable_all(void) · line 1441

Quoted 2 lines

arch/x86/kernel/cpu/perf_event.c +2 -0

+               cpuc->n_added = 0;
+               perf_events_lapic_init();

Just wondering, why do we need that lapic_init() there?

I think I picked it up from x86_pmu_enable(). I don't think you necessarily need it here. Not clear to me why it was in x86_pmu_enable() to begin with.

I will post a new version of the patch which fixes some bugs and also implements true fast path (reuse of previous assignment). It turned out, things were a bit more complicated than what I had in v5.

Quoted 9 lines · Stephane Eranian

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines · Stephane Eranian

 void hw_perf_enable(void)
 {

+               cpuc->n_added = 0;
+               perf_events_lapic_init();

Quoted 5 lines

Quoted 2 lines

Just wondering, why do we need that lapic_init() there?

I think I picked it up from x86_pmu_enable(). I don't think you necessarily need it here. Not clear to me why it was in x86_pmu_enable() to begin with.

Right, wondering about the same ;-)

Quoted 3 lines

I will post a new version of the patch which fixes some bugs and also implements true fast path (reuse of previous assignment). It turned out, things were a bit more complicated than what I had in v5.

Could you post a diff against your previous version, I just picked up -v5 and tidied a few things up and send it mingo wards (although it appears he hasn't picked it up yet).