Hi Peter, Thanks for your review!
On Thu, Apr 15, 2021 at 11:51 PM Peter Zijlstra <pet...@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
