Messages in this thread | | | Date | Wed, 23 Mar 2022 09:27:58 +0800 | Subject | Re: [Phishing Risk] [External] Re: [PATCH v2 2/6] perf/core: Introduce percpu perf_cgroup | From | Chengming Zhou <> |
| |
Hi Namhyung,
On 2022/3/23 6:18 上午, Namhyung Kim wrote: > On Tue, Mar 22, 2022 at 5:10 AM Chengming Zhou > <zhouchengming@bytedance.com> wrote: >> >> Although we don't have incosistency problem any more, we can >> have other problem like: >> >> CPU1 CPU2 >> (in context_switch) (attach running task) >> prev->cgroups = cgrp2 >> perf_cgroup_sched_switch(prev, next) >> cgrp2 == cgrp2 is True >> >> If perf_cgroup of prev task changes from cgrp1 to cgrp2, >> perf_cgroup_sched_switch() will skip perf_cgroup_switch(), >> so the CPU would still schedule the cgrp1 events, but we should >> schedule the cgrp2 events. > > Ah ok, now I see the problem in changing prev->cgroup too. > >> >> The reason of this problem is that we shouldn't use the changeable >> prev->cgroups to decide whether skip perf_cgroup_switch(). >> >> This patch introduces a percpu perf_cgroup to cache the perf_cgroup >> that scheduled in cpuctxes, which later used to compare with the >> perf_cgroup of next task to decide whether skip perf_cgroup_switch(). >> >> Since the perf_cgroup_switch() can be called after the context switch, >> the cgroup events might be scheduled already. So we put the comparison >> of perf_cgroups in perf_cgroup_switch(), and delete the unused function >> perf_cgroup_sched_switch(). >> >> We must clear the percpu perf_cgroup cache when the last cgroup event >> disabled. >> >> Fixes: a8d757ef076f ("perf events: Fix slow and broken cgroup context switch code") >> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> >> --- >> kernel/events/core.c | 63 ++++++++++++++++---------------------------- >> 1 file changed, 22 insertions(+), 41 deletions(-) >> >> diff --git a/kernel/events/core.c b/kernel/events/core.c >> index 8b5cf2aedfe6..848a3bfa9513 100644 >> --- a/kernel/events/core.c >> +++ b/kernel/events/core.c >> @@ -826,6 +826,7 @@ perf_cgroup_set_timestamp(struct task_struct *task, >> } >> } >> >> +static DEFINE_PER_CPU(struct perf_cgroup *, cpu_perf_cgroup); >> static DEFINE_PER_CPU(struct list_head, cgrp_cpuctx_list); >> >> /* >> @@ -833,6 +834,7 @@ static DEFINE_PER_CPU(struct list_head, cgrp_cpuctx_list); >> */ >> static void perf_cgroup_switch(struct task_struct *task) >> { >> + struct perf_cgroup *cgrp; >> struct perf_cpu_context *cpuctx, *tmp; >> struct list_head *list; >> unsigned long flags; >> @@ -843,11 +845,21 @@ static void perf_cgroup_switch(struct task_struct *task) >> */ >> local_irq_save(flags); >> >> + cgrp = perf_cgroup_from_task(task, NULL); >> + if (cgrp == __this_cpu_read(cpu_perf_cgroup)) >> + goto out; >> + >> + __this_cpu_write(cpu_perf_cgroup, cgrp); >> + >> list = this_cpu_ptr(&cgrp_cpuctx_list); >> list_for_each_entry_safe(cpuctx, tmp, list, cgrp_cpuctx_entry) { >> WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0); >> >> perf_ctx_lock(cpuctx, cpuctx->task_ctx); >> + >> + if (cpuctx->cgrp == cgrp) > > Missing perf_ctx_unlock().
Thank you, will fix next version.
> > Thanks, > Namhyung > >> + continue; >> + >> perf_pmu_disable(cpuctx->ctx.pmu); >> >> cpu_ctx_sched_out(cpuctx, EVENT_ALL); >> @@ -855,14 +867,11 @@ static void perf_cgroup_switch(struct task_struct *task) >> * must not be done before ctxswout due >> * to event_filter_match() in event_sched_out() >> */ >> - cpuctx->cgrp = perf_cgroup_from_task(task, >> - &cpuctx->ctx); >> + cpuctx->cgrp = cgrp; >> /* >> * set cgrp before ctxsw in to allow >> * event_filter_match() to not have to pass >> * task around >> - * we pass the cpuctx->ctx to perf_cgroup_from_task() >> - * because cgroup events are only per-cpu >> */ >> cpu_ctx_sched_in(cpuctx, EVENT_ALL, task); >> >> @@ -870,35 +879,10 @@ static void perf_cgroup_switch(struct task_struct *task) >> perf_ctx_unlock(cpuctx, cpuctx->task_ctx); >> } >> >> +out: >> local_irq_restore(flags); >> }
| |