Re: [PATCH v8] perf: Sharing PMU counters across compatible events

[Song Liu]

> On Dec 12, 2019, at 5:49 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> 
> On Fri, Dec 06, 2019 at 04:24:47PM -0800, Song Liu wrote:
> 
>> @@ -750,6 +752,16 @@ struct perf_event {
>> 	void *security;
>> #endif
>> 	struct list_head		sb_list;
>> +
>> +	/* for PMU sharing */
>> +	struct perf_event		*dup_master;
>> +	/* check event_sync_dup_count() for the use of dup_base_* */
>> +	u64				dup_base_count;
>> +	u64				dup_base_child_count;
>> +	/* when this event is master,  read from master*count */
>> +	local64_t			master_count;
>> +	atomic64_t			master_child_count;
>> +	int				dup_active_count;
>> #endif /* CONFIG_PERF_EVENTS */
>> };
> 
>> +/* PMU sharing aware version of event->pmu->add() */
>> +static int event_pmu_add(struct perf_event *event,
>> +			 struct perf_event_context *ctx)
>> +{
>> +	struct perf_event *master;
>> +	int ret;
>> +
>> +	/* no sharing, just do event->pmu->add() */
>> +	if (!event->dup_master)
>> +		return event->pmu->add(event, PERF_EF_START);
> 
> Possibly we should look at the location of perf_event::dup_master to be
> in a hot cacheline. Because I'm thinking you just added a guaranteed
> miss here.

I am not quite sure the best location for these. How about:

diff --git i/include/linux/perf_event.h w/include/linux/perf_event.h
index 7d49f9251621..218cc7f75775 100644
--- i/include/linux/perf_event.h
+++ w/include/linux/perf_event.h
@@ -643,6 +643,16 @@ struct perf_event {
        local64_t                       count;
        atomic64_t                      child_count;

+       /* for PMU sharing */
+       struct perf_event               *dup_master;
+       /* check event_sync_dup_count() for the use of dup_base_* */
+       u64                             dup_base_count;
+       u64                             dup_base_child_count;
+       /* when this event is master,  read from master*count */
+       local64_t                       master_count;
+       atomic64_t                      master_child_count;
+       int                             dup_active_count;
+
        /*
         * These are the total time in nanoseconds that the event
         * has been enabled (i.e. eligible to run, and the task has

?

> 
>> +
>> +	master = event->dup_master;
>> +
>> +	if (!master->dup_active_count) {
>> +		ret = event->pmu->add(master, PERF_EF_START);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (master != event)
>> +			perf_event_set_state(master, PERF_EVENT_STATE_ENABLED);
>> +	}
>> +
>> +	master->dup_active_count++;
>> +	master->pmu->read(master);
>> +	event->dup_base_count = local64_read(&master->count);
>> +	event->dup_base_child_count = atomic64_read(&master->child_count);
>> +	return 0;
>> +}
> 
>> +/* PMU sharing aware version of event->pmu->del() */
>> +static void event_pmu_del(struct perf_event *event,
>> +			  struct perf_event_context *ctx)
>> +{
>> +	struct perf_event *master;
>> +
>> +	if (event->dup_master == NULL) {
>> +		event->pmu->del(event, 0);
>> +		return;
>> +	}
> 
> How about you write it exactly like the add version:
> 
> 	if (!event->dup_master)
> 		return event->pmu->del(event, 0);
> 
> ?

Sure, will fix in v9. 

Thanks,
Song
> 
>> +
>> +	master = event->dup_master;
>> +	event_sync_dup_count(event, master);
>> +	if (--master->dup_active_count == 0) {
>> +		event->pmu->del(master, 0);
>> +		perf_event_set_state(master, PERF_EVENT_STATE_INACTIVE);
>> +	} else if (master == event) {
>> +		perf_event_set_state(master, PERF_EVENT_STATE_ENABLED);
>> +	}
>> +}
>> +
>> +/* PMU sharing aware version of event->pmu->read() */
>> +static void event_pmu_read(struct perf_event *event)
>> +{
>> +	if (event->dup_master == NULL) {
>> +		event->pmu->read(event);
>> +		return;
>> +	}
> 
> And here too.
> 
>> +	event_sync_dup_count(event, event->dup_master);
>> +}