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 <[email protected]>
> Reported-by: Johannes Jensen <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> ---
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
