Re: [PATCH] perf: decouple unthrottling and rotating

[Mark Rutland]

On Wed, Jan 07, 2015 at 03:01:54PM +0000, Mark Rutland wrote:
> Currently the adjusments made as part of perf_event_task_tick use the
> percpu rotation lists to iterate over any active PMU contexts, but these
> are not used by the context rotation code, having been replaced by
> separate (per-context) hrtimer callbacks. However, some manipulation of
> the rotation lists (i.e. removal of contexts) has remained in
> perf_rotate_context. This leads to the following issues:
> 
> * Contexts are not always removed from the rotation lists. Removal of
>   PMUs which have been placed in rotation lists, but have not been
>   removed by a hrtimer callback can result in corruption of the rotation
>   lists (when memory backing the context is freed).
> 
>   This has been observed to result in hangs when PMU drivers built as
>   modules are inserted and removed around the creation of events for
>   said PMUs.
> 
> * Contexts which do not require rotation may be removed from the
>   rotation lists as a result of a hrtimer, and will not be considered by
>   the unthrottling code in perf_event_task_tick.
> 
> This patch solves these issues by moving any and all removal of contexts
> from rotation lists to only occur when the final event is removed from a
> context, mirroring the addition which only occurs when the first event
> is added to a context. The vestigal manipulation of the rotation lists
> is removed from perf_event_rotate_context.
> 
> As the rotation_list variables are not used for rotation, these are
> renamed to active_ctx_list, which better matches their current function.
> perf_pmu_rotate_{start,stop} are renamed to
> perf_pmu_ctx_{activate,deactivate}.

Any comments?

Thanks,
Mark.

> Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
> Reported-by: Johannes Jensen <johannes.jensen@xxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Cc: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
> Cc: Paul Mackerras <paulus@xxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> ---
>  include/linux/perf_event.h |  2 +-
>  kernel/events/core.c       | 64 ++++++++++++++++++++--------------------------
>  2 files changed, 29 insertions(+), 37 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 486e84c..4ca17f1 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -530,7 +530,7 @@ struct perf_cpu_context {
>  	int				exclusive;
>  	struct hrtimer			hrtimer;
>  	ktime_t				hrtimer_interval;
> -	struct list_head		rotation_list;
> +	struct list_head		active_ctx_list;
>  	struct pmu			*unique_pmu;
>  	struct perf_cgroup		*cgrp;
>  };
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4faccf3..ac0b9c8 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -872,22 +872,33 @@ void perf_pmu_enable(struct pmu *pmu)
>  		pmu->pmu_enable(pmu);
>  }
>  
> -static DEFINE_PER_CPU(struct list_head, rotation_list);
> +static DEFINE_PER_CPU(struct list_head, active_ctx_list);
>  
>  /*
> - * perf_pmu_rotate_start() and perf_rotate_context() are fully serialized
> - * because they're strictly cpu affine and rotate_start is called with IRQs
> - * disabled, while rotate_context is called from IRQ context.
> + * perf_pmu_ctx_activate(), perf_pmu_ctx_deactivate(), and
> + * perf_even_task_tick() are fully serialized because they're strictly cpu
> + * affine and perf_pmu_ctx{activate,deactiveate} are called with IRQs disabled,
> + * while perf_event_task_tick is called from IRQ context.
>   */
> -static void perf_pmu_rotate_start(struct pmu *pmu)
> +static void perf_pmu_ctx_activate(struct pmu *pmu)
>  {
>  	struct perf_cpu_context *cpuctx = this_cpu_ptr(pmu->pmu_cpu_context);
> -	struct list_head *head = this_cpu_ptr(&rotation_list);
> +	struct list_head *head = this_cpu_ptr(&active_ctx_list);
>  
>  	WARN_ON(!irqs_disabled());
>  
> -	if (list_empty(&cpuctx->rotation_list))
> -		list_add(&cpuctx->rotation_list, head);
> +	WARN_ON(!list_empty(&cpuctx->active_ctx_list));
> +
> +	list_add(&cpuctx->active_ctx_list, head);
> +}
> +
> +static void perf_pmu_ctx_deactivate(struct pmu *pmu)
> +{
> +	struct perf_cpu_context *cpuctx = this_cpu_ptr(pmu->pmu_cpu_context);
> +
> +	WARN_ON(!irqs_disabled());
> +
> +	list_del_init(&cpuctx->active_ctx_list);
>  }
>  
>  static void get_ctx(struct perf_event_context *ctx)
> @@ -1156,7 +1167,7 @@ list_add_event(struct perf_event *event, struct perf_event_context *ctx)
>  
>  	list_add_rcu(&event->event_entry, &ctx->event_list);
>  	if (!ctx->nr_events)
> -		perf_pmu_rotate_start(ctx->pmu);
> +		perf_pmu_ctx_activate(ctx->pmu);
>  	ctx->nr_events++;
>  	if (event->attr.inherit_stat)
>  		ctx->nr_stat++;
> @@ -1324,6 +1335,8 @@ list_del_event(struct perf_event *event, struct perf_event_context *ctx)
>  		ctx->nr_stat--;
>  
>  	list_del_rcu(&event->event_entry);
> +	if (!ctx->nr_events)
> +		perf_pmu_ctx_deactivate(ctx->pmu);
>  
>  	if (event->group_leader == event)
>  		list_del_init(&event->group_entry);
> @@ -2612,12 +2625,6 @@ static void perf_event_context_sched_in(struct perf_event_context *ctx,
>  
>  	perf_pmu_enable(ctx->pmu);
>  	perf_ctx_unlock(cpuctx, ctx);
> -
> -	/*
> -	 * Since these rotations are per-cpu, we need to ensure the
> -	 * cpu-context we got scheduled on is actually rotating.
> -	 */
> -	perf_pmu_rotate_start(ctx->pmu);
>  }
>  
>  /*
> @@ -2906,24 +2913,22 @@ static void rotate_ctx(struct perf_event_context *ctx)
>  }
>  
>  /*
> - * perf_pmu_rotate_start() and perf_rotate_context() are fully serialized
> + * perf_pmu_ctx_activate() and perf_rotate_context() are fully serialized
>   * because they're strictly cpu affine and rotate_start is called with IRQs
>   * disabled, while rotate_context is called from IRQ context.
>   */
>  static int perf_rotate_context(struct perf_cpu_context *cpuctx)
>  {
>  	struct perf_event_context *ctx = NULL;
> -	int rotate = 0, remove = 1;
> +	int rotate = 0;
>  
>  	if (cpuctx->ctx.nr_events) {
> -		remove = 0;
>  		if (cpuctx->ctx.nr_events != cpuctx->ctx.nr_active)
>  			rotate = 1;
>  	}
>  
>  	ctx = cpuctx->task_ctx;
>  	if (ctx && ctx->nr_events) {
> -		remove = 0;
>  		if (ctx->nr_events != ctx->nr_active)
>  			rotate = 1;
>  	}
> @@ -2947,8 +2952,6 @@ static int perf_rotate_context(struct perf_cpu_context *cpuctx)
>  	perf_pmu_enable(cpuctx->ctx.pmu);
>  	perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
>  done:
> -	if (remove)
> -		list_del_init(&cpuctx->rotation_list);
>  
>  	return rotate;
>  }
> @@ -2966,7 +2969,7 @@ bool perf_event_can_stop_tick(void)
>  
>  void perf_event_task_tick(void)
>  {
> -	struct list_head *head = this_cpu_ptr(&rotation_list);
> +	struct list_head *head = this_cpu_ptr(&active_ctx_list);
>  	struct perf_cpu_context *cpuctx, *tmp;
>  	struct perf_event_context *ctx;
>  	int throttled;
> @@ -2976,7 +2979,7 @@ void perf_event_task_tick(void)
>  	__this_cpu_inc(perf_throttled_seq);
>  	throttled = __this_cpu_xchg(perf_throttled_count, 0);
>  
> -	list_for_each_entry_safe(cpuctx, tmp, head, rotation_list) {
> +	list_for_each_entry_safe(cpuctx, tmp, head, active_ctx_list) {
>  		ctx = &cpuctx->ctx;
>  		perf_adjust_freq_unthr_context(ctx, throttled);
>  
> @@ -6784,7 +6787,7 @@ skip_type:
>  
>  		__perf_cpu_hrtimer_init(cpuctx, cpu);
>  
> -		INIT_LIST_HEAD(&cpuctx->rotation_list);
> +		INIT_LIST_HEAD(&cpuctx->active_ctx_list);
>  		cpuctx->unique_pmu = pmu;
>  	}
>  
> @@ -8133,7 +8136,7 @@ static void __init perf_event_init_all_cpus(void)
>  	for_each_possible_cpu(cpu) {
>  		swhash = &per_cpu(swevent_htable, cpu);
>  		mutex_init(&swhash->hlist_mutex);
> -		INIT_LIST_HEAD(&per_cpu(rotation_list, cpu));
> +		INIT_LIST_HEAD(&per_cpu(active_ctx_list, cpu));
>  	}
>  }
>  
> @@ -8154,22 +8157,11 @@ static void perf_event_init_cpu(int cpu)
>  }
>  
>  #if defined CONFIG_HOTPLUG_CPU || defined CONFIG_KEXEC
> -static void perf_pmu_rotate_stop(struct pmu *pmu)
> -{
> -	struct perf_cpu_context *cpuctx = this_cpu_ptr(pmu->pmu_cpu_context);
> -
> -	WARN_ON(!irqs_disabled());
> -
> -	list_del_init(&cpuctx->rotation_list);
> -}
> -
>  static void __perf_event_exit_context(void *__info)
>  {
>  	struct remove_event re = { .detach_group = true };
>  	struct perf_event_context *ctx = __info;
>  
> -	perf_pmu_rotate_stop(ctx->pmu);
> -
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(re.event, &ctx->event_list, event_entry)
>  		__perf_remove_from_context(&re);
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/