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

[Song Liu <songliubraving@fb.com>]

> On Dec 12, 2019, at 7:39 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Fri, Dec 06, 2019 at 04:24:47PM -0800, Song Liu wrote:
> 
>> @@ -2174,6 +2410,14 @@ __perf_remove_from_context(struct perf_event *event,
>> 		update_cgrp_time_from_cpuctx(cpuctx);
>> 	}
>> 
>> +	if (event->dup_master == event) {
>> +		if (ctx->is_active)
>> +			ctx_resched(cpuctx, cpuctx->task_ctx,
>> +				    get_event_type(event), NULL, event);
>> +		else
>> +			perf_event_remove_dup(event, ctx);
>> +	}
>> +
>> 	event_sched_out(event, cpuctx, ctx);
>> 	if (flags & DETACH_GROUP)
>> 		perf_group_detach(event);
>> @@ -2241,6 +2485,14 @@ static void __perf_event_disable(struct perf_event *event,
>> 		update_cgrp_time_from_event(event);
>> 	}
>> 
>> +	if (event->dup_master == event) {
>> +		if (ctx->is_active)
>> +			ctx_resched(cpuctx, cpuctx->task_ctx,
>> +				    get_event_type(event), NULL, event);
>> +		else
>> +			perf_event_remove_dup(event, ctx);
>> +	}
>> +
>> 	if (event == event->group_leader)
>> 		group_sched_out(event, cpuctx, ctx);
>> 	else
> 
>> @@ -2544,7 +2793,9 @@ 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,
>> +			struct perf_event *event_del_dup)
>> {
>> 	enum event_type_t ctx_event_type;
>> 	bool cpu_event = !!(event_type & EVENT_CPU);
>> @@ -2574,6 +2825,18 @@ 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);
>> +	}
>> +
>> +	if (event_del_dup) {
>> +		if (event_del_dup->ctx->is_active)
>> +			ctx_sched_out(event_del_dup->ctx, cpuctx, EVENT_ALL);
>> +		perf_event_remove_dup(event_del_dup, event_del_dup->ctx);
>> +	}
>> +
>> 	perf_event_sched_in(cpuctx, task_ctx, current);
>> 	perf_pmu_enable(cpuctx->ctx.pmu);
>> }
> 
> Yuck!
> 
> Why do you do a full reschedule when you take out a master?

If there is active slave using this master, we need to schedule out
them before removing the master. 

We can improve the check though. We only need to do it if the master
is in state PERF_EVENT_STATE_ENABLED. 

Or we can add a different function to only schedule out slaves. 

Thanks,
Song