Message-ID: <20200110125944.GJ2844@hirez.programming.kicks-ass.net>
Date:   Fri, 10 Jan 2020 13:59:44 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Song Liu <songliubraving@...com>
Cc:     linux-kernel@...r.kernel.org, kernel-team@...com,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Alexey Budankov <alexey.budankov@...ux.intel.com>,
        Namhyung Kim <namhyung@...nel.org>, Tejun Heo <tj@...nel.org>
Subject: Re: [PATCH v9] perf: Sharing PMU counters across compatible events

On Tue, Dec 17, 2019 at 09:59:48AM -0800, Song Liu wrote:

This is starting to look good, find a few comments below.

>  include/linux/perf_event.h |  13 +-
>  kernel/events/core.c       | 363 ++++++++++++++++++++++++++++++++-----
>  2 files changed, 332 insertions(+), 44 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 6d4c22aee384..45a346ee33d2 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -547,7 +547,9 @@ enum perf_event_state {
>  	PERF_EVENT_STATE_ERROR		= -2,
>  	PERF_EVENT_STATE_OFF		= -1,
>  	PERF_EVENT_STATE_INACTIVE	=  0,
> -	PERF_EVENT_STATE_ACTIVE		=  1,
> +	/* the hw PMC is enabled, but this event is not counting */
> +	PERF_EVENT_STATE_ENABLED	=  1,
> +	PERF_EVENT_STATE_ACTIVE		=  2,
>  };

It's probably best to extend the comment above instead of adding a
comment for one of the states.

>  
>  struct file;

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4ff86d57f9e5..7d4b6ac46de5 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -1657,6 +1657,181 @@ perf_event_groups_next(struct perf_event *event)
>  		event = rb_entry_safe(rb_next(&event->group_node),	\
>  				typeof(*event), group_node))
>  
> +static inline bool perf_event_can_share(struct perf_event *event)
> +{
> +	/* only share hardware counting events */
> +	return !is_sampling_event(event);
> +	return !is_software_event(event) && !is_sampling_event(event);

One of those return statements is too many; I'm thinking you meant to
only have the second.

> +}
> +

> +/* After adding a event to the ctx, try find compatible event(s). */
> +static void perf_event_setup_dup(struct perf_event *event,
> +				 struct perf_event_context *ctx)
> +
> +{
> +	struct perf_event *tmp;
> +
> +	if (event->dup_master ||
> +	    event->state != PERF_EVENT_STATE_INACTIVE ||
> +	    !perf_event_can_share(event))
> +		return;
> +
> +	/* look for dup with other events */
> +	list_for_each_entry(tmp, &ctx->event_list, event_entry) {
> +		WARN_ON_ONCE(tmp->state > PERF_EVENT_STATE_INACTIVE);
> +
> +		if (tmp == event ||
> +		    tmp->state != PERF_EVENT_STATE_INACTIVE ||
> +		    !perf_event_can_share(tmp) ||
> +		    !perf_event_compatible(event, tmp))
> +			continue;
> +
> +		/* first dup, pick tmp as the master */
> +		if (!tmp->dup_master)
> +			perf_event_init_dup_master(tmp);
> +
> +		event->dup_master = tmp->dup_master;
> +		break;
> +	}
> +}
> +
> +static int event_pmu_add(struct perf_event *event,
> +			 struct perf_event_context *ctx);
> +
> +/* Remove dup_master for the event */
> +static void perf_event_remove_dup(struct perf_event *event,
> +				  struct perf_cpu_context *cpuctx,
> +				  struct perf_event_context *ctx)
> +
> +{
> +	struct perf_event *tmp, *new_master;
> +	int count, active_count;
> +
> +	/* no sharing */
> +	if (!event->dup_master)
> +		return;
> +
> +	WARN_ON_ONCE(event->state < PERF_EVENT_STATE_OFF ||
> +		     event->state > PERF_EVENT_STATE_ENABLED);
> +
> +	/* this event is not the master */
> +	if (event->dup_master != event) {
> +		event_sync_dup_count(event, event->dup_master);
> +		event->dup_master = NULL;
> +		return;
> +	}
> +
> +	/* this event is the master */
> +	count = 0;
> +	new_master = NULL;
> +	list_for_each_entry(tmp, &ctx->event_list, event_entry) {
> +		if (tmp->dup_master != event || tmp == event)
> +			continue;
> +		if (!new_master)
> +			new_master = tmp;
> +		if (tmp->state == PERF_EVENT_STATE_ACTIVE) {
> +			event_sync_dup_count(tmp, event);
> +			tmp->dup_base_count = local64_read(&new_master->count);
> +			tmp->dup_base_child_count =
> +				atomic64_read(&new_master->child_count);
> +		}
> +		tmp->dup_master = new_master;
> +		count++;
> +	}
> +
> +	active_count = event->dup_active_count;
> +	perf_event_exit_dup_master(event);
> +
> +	if (!count)
> +		return;
> +
> +	if (count == 1) {
> +		/* no more sharing */
> +		new_master->dup_master = NULL;
> +	} else {
> +		perf_event_init_dup_master(new_master);
> +		new_master->dup_active_count = active_count;
> +	}
> +
> +	if (active_count) {

Would it make sense to do something like:

		new_master->hw.idx = event->hw.idx;

That should ensure x86_schedule_events() can do with the fast path;
after all, we're adding back the 'same' event. If we do this; this wants
a comment though.

> +		WARN_ON_ONCE(event->pmu->add(new_master, PERF_EF_START));

For consistency that probably ought to be:

		new_master->pmu->add(new_master, PERF_EF_START);

> +		if (new_master->state == PERF_EVENT_STATE_INACTIVE)
> +			new_master->state = PERF_EVENT_STATE_ENABLED;

If this really should not be perf_event_set_state() we need a comment
explaining why -- I think I see, but it's still early and I've not had
nearly enough tea to wake me up.

> +	}
> +}
> +
>  /*
>   * Add an event from the lists for its context.
>   * Must be called with ctx->mutex and ctx->lock held.
> @@ -1902,7 +2077,8 @@ perf_aux_output_match(struct perf_event *event, struct perf_event *aux_event)
>  static void put_event(struct perf_event *event);
>  static void event_sched_out(struct perf_event *event,
>  			    struct perf_cpu_context *cpuctx,
> -			    struct perf_event_context *ctx);
> +			    struct perf_event_context *ctx,
> +			    bool remove_dup);
>  
>  static void perf_put_aux_event(struct perf_event *event)
>  {

>  static void
>  event_sched_out(struct perf_event *event,
>  		  struct perf_cpu_context *cpuctx,
> -		  struct perf_event_context *ctx)
> +		struct perf_event_context *ctx,
> +		bool remove_dup)
>  {
>  	enum perf_event_state state = PERF_EVENT_STATE_INACTIVE;
>  
>  	WARN_ON_ONCE(event->ctx != ctx);
>  	lockdep_assert_held(&ctx->lock);
>  
> -	if (event->state != PERF_EVENT_STATE_ACTIVE)
> +	if (event->state < PERF_EVENT_STATE_ENABLED) {
> +		if (remove_dup)
> +			perf_event_remove_dup(event, cpuctx, ctx);
>  		return;
> +	}
>  
>  	/*
>  	 * Asymmetry; we only schedule events _IN_ through ctx_sched_in(), but
> @@ -2106,15 +2343,20 @@ event_sched_out(struct perf_event *event,
>  
>  	perf_pmu_disable(event->pmu);
>  
> -	event->pmu->del(event, 0);
> +	event_pmu_del(event, ctx, remove_dup);
>  	event->oncpu = -1;
>  
>  	if (READ_ONCE(event->pending_disable) >= 0) {
>  		WRITE_ONCE(event->pending_disable, -1);
>  		state = PERF_EVENT_STATE_OFF;
> -	}
> +	} else if (event->dup_master == event &&
> +		   event->dup_active_count)
> +		state = PERF_EVENT_STATE_ENABLED;
>  	perf_event_set_state(event, state);
>  
> +	if (remove_dup)
> +		perf_event_remove_dup(event, cpuctx, ctx);
> +
>  	if (!is_software_event(event))
>  		cpuctx->active_oncpu--;
>  	if (!--ctx->nr_active)

> @@ -2174,7 +2426,7 @@ __perf_remove_from_context(struct perf_event *event,
>  		update_cgrp_time_from_cpuctx(cpuctx);
>  	}
>  
> -	event_sched_out(event, cpuctx, ctx);
> +	event_sched_out(event, cpuctx, ctx, true);
>  	if (flags & DETACH_GROUP)
>  		perf_group_detach(event);
>  	list_del_event(event, ctx);
> @@ -2242,9 +2494,9 @@ static void __perf_event_disable(struct perf_event *event,
>  	}
>  
>  	if (event == event->group_leader)
> -		group_sched_out(event, cpuctx, ctx);
> +		group_sched_out(event, cpuctx, ctx, true);
>  	else
> -		event_sched_out(event, cpuctx, ctx);
> +		event_sched_out(event, cpuctx, ctx, true);
>  
>  	perf_event_set_state(event, PERF_EVENT_STATE_OFF);
>  }

So the above event_sched_out(.remove_dup) is very inconsistent with the
below ctx_resched(.event_add_dup).

> @@ -2544,7 +2793,8 @@ static void perf_event_sched_in(struct perf_cpu_context *cpuctx,
>   */
>  static void ctx_resched(struct perf_cpu_context *cpuctx,
>  			struct perf_event_context *task_ctx,
> -			enum event_type_t event_type)
> +			enum event_type_t event_type,
> +			struct perf_event *event_add_dup)
>  {
>  	enum event_type_t ctx_event_type;
>  	bool cpu_event = !!(event_type & EVENT_CPU);
> @@ -2574,6 +2824,12 @@ static void ctx_resched(struct perf_cpu_context *cpuctx,
>  	else if (ctx_event_type & EVENT_PINNED)
>  		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
>  
> +	if (event_add_dup) {
> +		if (event_add_dup->ctx->is_active)
> +			ctx_sched_out(event_add_dup->ctx, cpuctx, EVENT_ALL);
> +		perf_event_setup_dup(event_add_dup, event_add_dup->ctx);
> +	}
> +
>  	perf_event_sched_in(cpuctx, task_ctx, current);
>  	perf_pmu_enable(cpuctx->ctx.pmu);
>  }

> @@ -2642,9 +2898,10 @@ static int  __perf_install_in_context(void *info)
>  	if (reprogram) {
>  		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
>  		add_event_to_ctx(event, ctx);
> -		ctx_resched(cpuctx, task_ctx, get_event_type(event));
> +		ctx_resched(cpuctx, task_ctx, get_event_type(event), event);
>  	} else {
>  		add_event_to_ctx(event, ctx);
> +		perf_event_setup_dup(event, ctx);
>  	}
>  
>  unlock:
> @@ -2789,8 +3046,10 @@ static void __perf_event_enable(struct perf_event *event,
>  
>  	perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
>  
> -	if (!ctx->is_active)
> +	if (!ctx->is_active) {
> +		perf_event_setup_dup(event, ctx);
>  		return;
> +	}
>  
>  	if (!event_filter_match(event)) {
>  		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
> @@ -2801,7 +3060,7 @@ static void __perf_event_enable(struct perf_event *event,
>  	 * If the event is in a group and isn't the group leader,
>  	 * then don't put it on unless the group is on.
>  	 */
> -	if (leader != event && leader->state != PERF_EVENT_STATE_ACTIVE) {
> +	if (leader != event && leader->state <= PERF_EVENT_STATE_INACTIVE) {
>  		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
>  		return;
>  	}
> @@ -2810,7 +3069,7 @@ static void __perf_event_enable(struct perf_event *event,
>  	if (ctx->task)
>  		WARN_ON_ONCE(task_ctx != ctx);
>  
> -	ctx_resched(cpuctx, task_ctx, get_event_type(event));
> +	ctx_resched(cpuctx, task_ctx, get_event_type(event), event);
>  }
>  
>  /*

We basically need:

 * perf_event_setup_dup() after add_event_to_ctx(), but before *sched_in()
   - perf_install_in_context()
   - perf_event_enable()
   - inherit_event()

 * perf_event_remove_dup() after *sched_out(), but before list_del_event()
   - perf_remove_from_context()
   - perf_event_disable()

AFAICT we can do that without changing *sched_out() and ctx_resched(),
with probably less lines changed over all.

> @@ -4051,6 +4310,9 @@ static void __perf_event_read(void *info)
>  
>  static inline u64 perf_event_count(struct perf_event *event)
>  {
> +	if (event->dup_master == event)
> +		return local64_read(&event->master_count) +
> +			atomic64_read(&event->master_child_count);

Wants {}

>  	return local64_read(&event->count) + atomic64_read(&event->child_count);
>  }

Powered by blists - more mailing lists