Message-ID: <CAM9d7chrHYNOB4ShJ=34WwXOUY-grXhkiW_wursywTH1FbZdvA@mail.gmail.com>
Date:   Fri, 16 Apr 2021 08:48:12 +0900
From:   Namhyung Kim <namhyung@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Stephane Eranian <eranian@...gle.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Ian Rogers <irogers@...gle.com>,
        Song Liu <songliubraving@...com>, Tejun Heo <tj@...nel.org>,
        kernel test robot <lkp@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>
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@...radead.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

Powered by blists - more mailing lists