Messages in this thread | | | From | Song Liu <> | Subject | Re: [PATCH 1/2] perf/core: Share an event with multiple cgroups | Date | Sun, 28 Mar 2021 17:17:03 +0000 |
| |
> On Mar 23, 2021, at 9:21 AM, Namhyung Kim <namhyung@kernel.org> wrote: > > As we can run many jobs (in container) on a big machine, we want to > measure each job's performance during the run. To do that, the > perf_event can be associated to a cgroup to measure it only. > > However such cgroup events need to be opened separately and it causes > significant overhead in event multiplexing during the context switch > as well as resource consumption like in file descriptors and memory > footprint. > > As a cgroup event is basically a cpu event, we can share a single cpu > event for multiple cgroups. All we need is a separate counter (and > two timing variables) for each cgroup. I added a hash table to map > from cgroup id to the attached cgroups. > > With this change, the cpu event needs to calculate a delta of event > counter values when the cgroups of current and the next task are > different. And it attributes the delta to the current task's cgroup. > > This patch adds two new ioctl commands to perf_event for light-weight > cgroup event counting (i.e. perf stat). > > * PERF_EVENT_IOC_ATTACH_CGROUP - it takes a buffer consists of a > 64-bit array to attach given cgroups. The first element is a > number of cgroups in the buffer, and the rest is a list of cgroup > ids to add a cgroup info to the given event. > > * PERF_EVENT_IOC_READ_CGROUP - it takes a buffer consists of a 64-bit > array to get the event counter values. The first element is size > of the array in byte, and the second element is a cgroup id to > read. The rest is to save the counter value and timings. > > This attaches all cgroups in a single syscall and I didn't add the > DETACH command deliberately to make the implementation simple. The > attached cgroup nodes would be deleted when the file descriptor of the > perf_event is closed. > > Cc: Tejun Heo <tj@kernel.org> > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > include/linux/perf_event.h | 22 ++ > include/uapi/linux/perf_event.h | 2 + > kernel/events/core.c | 474 ++++++++++++++++++++++++++++++-- > 3 files changed, 471 insertions(+), 27 deletions(-)
[...]
> @@ -4461,6 +4787,8 @@ static void __perf_event_init_context(struct perf_event_context *ctx) > INIT_LIST_HEAD(&ctx->event_list); > INIT_LIST_HEAD(&ctx->pinned_active); > INIT_LIST_HEAD(&ctx->flexible_active); > + INIT_LIST_HEAD(&ctx->cgrp_ctx_entry); > + INIT_LIST_HEAD(&ctx->cgrp_node_list);
I guess we need ifdef CONFIG_CGROUP_PERF here?
> refcount_set(&ctx->refcount, 1); > } > > @@ -4851,6 +5179,8 @@ static void _free_event(struct perf_event *event) > if (is_cgroup_event(event)) > perf_detach_cgroup(event); > > + perf_event_destroy_cgroup_nodes(event); > + > if (!event->parent) { > if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN) > put_callchain_buffers();
[...]
> +static void perf_sched_enable(void) > +{ > + /* > + * We need the mutex here because static_branch_enable() > + * must complete *before* the perf_sched_count increment > + * becomes visible. > + */ > + if (atomic_inc_not_zero(&perf_sched_count)) > + return;
Why don't we use perf_cgroup_events for the new use case?
> + > + mutex_lock(&perf_sched_mutex); > + if (!atomic_read(&perf_sched_count)) { > + static_branch_enable(&perf_sched_events); > + /* > + * Guarantee that all CPUs observe they key change and > + * call the perf scheduling hooks before proceeding to > + * install events that need them. > + */ > + synchronize_rcu(); > + } > + /* > + * Now that we have waited for the sync_sched(), allow further > + * increments to by-pass the mutex. > + */ > + atomic_inc(&perf_sched_count); > + mutex_unlock(&perf_sched_mutex); > +} > + > static long perf_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > { > struct perf_event *event = file->private_data; > struct perf_event_context *ctx; > + bool do_sched_enable = false; > long ret; > > /* Treat ioctl like writes as it is likely a mutating operation. */ > @@ -5595,9 +6006,19 @@ static long perf_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > return ret; > > ctx = perf_event_ctx_lock(event); > + /* ATTACH_CGROUP requires context switch callback */ > + if (cmd == PERF_EVENT_IOC_ATTACH_CGROUP && !event_has_cgroup_node(event)) > + do_sched_enable = true; > ret = _perf_ioctl(event, cmd, arg); > perf_event_ctx_unlock(event, ctx); > > + /* > + * Due to the circular lock dependency, it cannot call > + * static_branch_enable() under the ctx->mutex. > + */ > + if (do_sched_enable && ret >= 0) > + perf_sched_enable(); > + > return ret; > } > > @@ -11240,33 +11661,8 @@ static void account_event(struct perf_event *event) > if (event->attr.text_poke) > atomic_inc(&nr_text_poke_events); > > - if (inc) { > - /* > - * We need the mutex here because static_branch_enable() > - * must complete *before* the perf_sched_count increment > - * becomes visible. > - */ > - if (atomic_inc_not_zero(&perf_sched_count)) > - goto enabled; > - > - mutex_lock(&perf_sched_mutex); > - if (!atomic_read(&perf_sched_count)) { > - static_branch_enable(&perf_sched_events); > - /* > - * Guarantee that all CPUs observe they key change and > - * call the perf scheduling hooks before proceeding to > - * install events that need them. > - */ > - synchronize_rcu(); > - } > - /* > - * Now that we have waited for the sync_sched(), allow further > - * increments to by-pass the mutex. > - */ > - atomic_inc(&perf_sched_count); > - mutex_unlock(&perf_sched_mutex); > - } > -enabled: > + if (inc) > + perf_sched_enable(); > > account_event_cpu(event, event->cpu); > > @@ -13008,6 +13404,7 @@ static void __init perf_event_init_all_cpus(void) > > #ifdef CONFIG_CGROUP_PERF > INIT_LIST_HEAD(&per_cpu(cgrp_cpuctx_list, cpu)); > + INIT_LIST_HEAD(&per_cpu(cgroup_ctx_list, cpu)); > #endif > INIT_LIST_HEAD(&per_cpu(sched_cb_list, cpu)); > } > @@ -13218,6 +13615,28 @@ static int perf_cgroup_css_online(struct cgroup_subsys_state *css) > return 0; > } > > +static int __perf_cgroup_update_node(void *info) > +{ > + struct task_struct *task = info; > + > + rcu_read_lock(); > + cgroup_node_sched_out(task); > + rcu_read_unlock(); > + > + return 0; > +} > + > +static int perf_cgroup_can_attach(struct cgroup_taskset *tset) > +{ > + struct task_struct *task; > + struct cgroup_subsys_state *css; > + > + cgroup_taskset_for_each(task, css, tset) > + task_function_call(task, __perf_cgroup_update_node, task); > + > + return 0; > +}
Could you please explain why we need this logic in can_attach?
Thanks, Song
| |