Messages in this thread | | | From | Song Liu <> | Subject | Re: [PATCH v9] perf: Sharing PMU counters across compatible events | Date | Thu, 16 Jan 2020 23:59:13 +0000 |
| |
Hi Peter,
> On Jan 10, 2020, at 9:37 AM, Song Liu <songliubraving@fb.com> wrote: > >>> >>> @@ -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). > > [...] > >>> @@ -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()
Quick question:
For the remove_dup() path, if we do it after *_sched_out(), we will need to disable-then-enable the pmu for one extra time. In current version, we only call perf_event_remove_dup() in event_sched_out(), where extra disable/enable is not necessary. Is it a good tradeoff to add one extra disable-enable for cleaner code?
Thanks, Song
| |