Re: [PATCH] perf_events: fix event scheduling issues introduced by transactional API

[Lin Ming <ming.m.lin@intel.com>]

On Mon, 2010-05-24 at 22:40 +0800, Stephane Eranian wrote:
> The transactional API patch between the generic and model-specific
> code introduced several important bugs with event scheduling, at
> least on X86. If you had pinned events, e.g., watchdog,  and were
> over-committing the PMU, you would get bogus counts. The bug was
> showing up on Intel CPU because events would move around more
> often that on AMD. But the problem also existed on AMD, though
> harder to expose.
> 
> The issues were:
> 
>  - group_sched_in() was missing a cancel_txn() in the error path
> 
>  - cpuc->n_added was not properly maintained, leading to missing
>    actions in hw_perf_enable(), i.e., n_running being 0. You cannot
>    update n_added until you know the transaction has succeeded. In
>    case of failed transaction n_added was not adjusted back.
> 
>  - in case of failed transactions, event_sched_out() was called
>    and eventually invoked x86_disable_event() to touch the HW reg.
>    But with transactions, on X86, event_sched_in() does not touch
>    HW registers, it simply collects events into a list. Thus, you

event_sched_in always does not touch HW registers, both with and without
transactions.

>    could end up calling x86_disable_event() on a counter which
>    did not correspond to the current event when idx != -1.
> 
> The patch modifies the generic and X86 code to avoid all those
> problems.
> 
> First, n_added is now ONLY updated once we know the transaction has
> succeeded.
> 
> Second, we encapsulate the event_sched_in() and event_sched_out() in
> group_sched_in() inside the transaction. That way, in the X6 code, we
> can detect that we are being asked to stop an event which was not yet
> started by checking cpuc->group_flag.
> 
> With this patch, you can now overcommit the PMU even with pinned
> system-wide events present and still get valid counts.
> 
> Signed-off-by: Stephane Eranian <eranian@google.com>
> 
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index fd4db0d..25dfd30 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -105,6 +105,7 @@ struct cpu_hw_events {
>  	int			enabled;
>  
>  	int			n_events;
> +	int			n_events_trans;
>  	int			n_added;
>  	int			assign[X86_PMC_IDX_MAX]; /* event to counter assignment */
>  	u64			tags[X86_PMC_IDX_MAX];
> @@ -591,7 +592,7 @@ void hw_perf_disable(void)
>  	if (!cpuc->enabled)
>  		return;
>  
> -	cpuc->n_added = 0;
> +	cpuc->n_added = cpuc->n_events_trans = 0;
>  	cpuc->enabled = 0;
>  	barrier();
>  
> @@ -820,7 +821,7 @@ void hw_perf_enable(void)
>  		 * apply assignment obtained either from
>  		 * hw_perf_group_sched_in() or x86_pmu_enable()
>  		 *
> -		 * step1: save events moving to new counters
> +		 * step1: stop-save old events moving to new counters
>  		 * step2: reprogram moved events into new counters
>  		 */
>  		for (i = 0; i < n_running; i++) {
> @@ -982,7 +983,7 @@ static int x86_pmu_enable(struct perf_event *event)
>  
>  out:
>  	cpuc->n_events = n;
> -	cpuc->n_added += n - n0;
> +	cpuc->n_events_trans += n - n0;
>  
>  	return 0;
>  }
> @@ -1070,7 +1071,7 @@ static void x86_pmu_stop(struct perf_event *event)
>  	struct hw_perf_event *hwc = &event->hw;
>  	int idx = hwc->idx;
>  
> -	if (!__test_and_clear_bit(idx, cpuc->active_mask))
> +	if (idx == -1 || !__test_and_clear_bit(idx, cpuc->active_mask))
>  		return;
>  
>  	x86_pmu.disable(event);
> @@ -1087,14 +1088,30 @@ static void x86_pmu_stop(struct perf_event *event)
>  static void x86_pmu_disable(struct perf_event *event)
>  {
>  	struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
> +	bool no_trans;
>  	int i;
>  
> -	x86_pmu_stop(event);
> +	/*
> +	 * when coming from a transaction, then the event has not
> +	 * actually been scheduled in yet. It has only been collected
> +	 * (collect_events), thus we cannot apply a full disable:
> +	 * - put_constraints() assumes went thru schedule_events()
> +	 * - x86_pmu_stop() assumes went thru x86_pmu_start() because
> +	 *   maybe idx != -1 and thus we may overwrite another the wrong
> +	 *   counter (e.g, pinned event).
> +	 *
> +	 * When called from a transaction, we need to un-collect the
> +	 * event, i..e, remove the event from event_list[]
> +	 */
> +	no_trans = !(cpuc->group_flag & PERF_EVENT_TXN_STARTED);
> +
> +	if (no_trans)
> +		x86_pmu_stop(event);
>  
>  	for (i = 0; i < cpuc->n_events; i++) {
>  		if (event == cpuc->event_list[i]) {
>  
> -			if (x86_pmu.put_event_constraints)
> +			if (no_trans && x86_pmu.put_event_constraints)
>  				x86_pmu.put_event_constraints(cpuc, event);
>  
>  			while (++i < cpuc->n_events)
> @@ -1104,7 +1121,8 @@ static void x86_pmu_disable(struct perf_event *event)
>  			break;
>  		}
>  	}
> -	perf_event_update_userpage(event);
> +	if (no_trans)
> +		perf_event_update_userpage(event);
>  }
>  
>  static int x86_pmu_handle_irq(struct pt_regs *regs)
> @@ -1418,7 +1436,8 @@ static int x86_pmu_commit_txn(const struct pmu *pmu)
>  	 * will be used by hw_perf_enable()
>  	 */
>  	memcpy(cpuc->assign, assign, n*sizeof(int));
> -
> +	cpuc->n_added += cpuc->n_events_trans;
> +	cpuc->n_events_trans = 0;
>  	return 0;
>  }
>  
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index 7e3bcf1..b0ccf4b 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -652,8 +652,11 @@ group_sched_in(struct perf_event *group_event,
>  	if (txn)
>  		pmu->start_txn(pmu);
>  
> -	if (event_sched_in(group_event, cpuctx, ctx))
> +	if (event_sched_in(group_event, cpuctx, ctx)) {
> +		if (txn)
> +			pmu->cancel_txn(pmu);
>  		return -EAGAIN;
> +	}

Can't imagine I forgot to call cancel_txn here.
Thanks very much.

>  
>  	/*
>  	 * Schedule in siblings as one group (if any):
> @@ -675,8 +678,6 @@ group_sched_in(struct perf_event *group_event,
>  	}
>  
>  group_error:
> -	if (txn)
> -		pmu->cancel_txn(pmu);
>  
>  	/*
>  	 * Groups can be scheduled in as one unit only, so undo any
> @@ -689,6 +690,9 @@ group_error:
>  	}
>  	event_sched_out(group_event, cpuctx, ctx);
>  
> +	if (txn)
> +		pmu->cancel_txn(pmu);
> +
>  	return -EAGAIN;
>  }
>  

Looks good to me.

Thanks,
Lin Ming