Messages in this thread | | | From | Namhyung Kim <> | Date | Fri, 16 Apr 2021 08:48:12 +0900 | Subject | Re: [PATCH v3 1/2] perf/core: Share an event with multiple cgroups |
| |
Hi Peter,
Thanks for your review!
On Thu, Apr 15, 2021 at 11:51 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Tue, Apr 13, 2021 at 08:53:36AM -0700, Namhyung Kim 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 > > git grep "This patch" Documentation/
Ok, will change.
> > > 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. > > WTH is a cgroup-id? The syscall takes a fd to the path, why have two > different ways?
As you know, we already use cgroup-id for sampling. Yeah we can do it with the fd but one of the point in this patch is to reduce the number of file descriptors. :)
Also, having cgroup-id is good to match with the result (from read) as it contains the cgroup information.
> > > * 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. > > :-( > > So basically you're doing a whole seconds cgroup interface, one that > violates the one counter per file premise and lives off of ioctl()s.
Right, but I'm not sure that we really want a separate event for each cgroup if underlying hardware events are all the same.
> > *IF* we're going to do something like this, I feel we should explore the > whole vector-per-fd concept before proceeding. Can we make it less yuck > (less special ioctl() and more regular file ops. Can we apply the > concept to more things?
Ideally it'd do without keeping file descriptors open. Maybe we can make the vector accept various types like vector-per-cgroup_id or so.
> > The second patch extends the ioctl() to be more read() like, instead of > doing the sane things and extending read() by adding PERF_FORMAT_VECTOR > or whatever. In fact, this whole second ioctl() doesn't make sense to > have if we do indeed want to do vector-per-fd.
One of the upside of the ioctl() is that we can pass cgroup-id to read. Probably we can keep the index in the vector and set the file offset with it. Or else just read the whole vector, and then it has a cgroup-id in the output like PERF_FORMAT_CGROUP?
> > Also, I suppose you can already fake this, by having a > SW_CGROUP_SWITCHES (sorry, I though I picked those up, done now) event
Thanks!
> with PERF_SAMPLE_READ|PERF_SAMPLE_CGROUP and PERF_FORMAT_GROUP in a > group with a bunch of events. Then the buffer will fill with the values > you use here.
Right, I'll do an experiment with it.
> > Yes, I suppose it has higher overhead, but you get the data you want > without having to do terrible things like this.
That's true. And we don't need many things in the perf record like synthesizing task/mmap info. Also there's a risk we can miss some samples for some reason.
Another concern is that it'd add huge slow down in the perf event open as it creates a mixed sw/hw group. The synchronized_rcu in the move_cgroup path caused significant problems in my environment as it adds up in proportion to the number of cpus.
> > > > > Lots of random comments below.
Thanks for the review, I'll reply in a separate thread.
Namhyung
| |