Re: [PATCH] perf_events: improve x86 event scheduling (v5)

[Frederic Weisbecker <fweisbec@gmail.com>]

On Mon, Jan 18, 2010 at 10:58:01AM +0200, Stephane Eranian wrote:
> +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:
> +	x86_event_sched_out(leader, cpuctx, cpu);
> +	n0  = 1;
> +	list_for_each_entry(sub, &leader->sibling_list, group_entry) {
> +		if (sub->state == PERF_EVENT_STATE_ACTIVE) {
> +			x86_event_sched_out(sub, cpuctx, cpu);
> +			if (++n0 == n1)
> +				break;
> +		}
> +	}
> +	return ret;



Looking at all these numerous places where this whole constraint
and ad hoc scheduling machine tries to catch up with what the
core can already do (handle non-x86 events, revert in failure,
first handle leader, then handle the rest, etc...), I wonder
if this hw_group_sched_in() based design is a good idea.

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?

I guess this would simplify all this code, avoid it to run through
the list of events, handle software events, revert partial enabled
by itself etc...

Hm?