Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200908044228.61197-1-namhyung@kernel.org> <20200910091542.GD1627030@krava>
 <20200910111020.GA4018363@kernel.org>
In-Reply-To: <20200910111020.GA4018363@kernel.org>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Thu, 10 Sep 2020 22:32:44 +0900
Message-ID: <CAM9d7cjvid5zoAKnUtHMqwgMTA7FLAbh9fqamCZg5E-_pvD7qQ@mail.gmail.com>
Subject: Re: [PATCHSET 0/4] perf stat: Add --multiply-cgroup option
To:     Arnaldo Carvalho de Melo <acme@kernel.org>
Cc:     Jiri Olsa <jolsa@redhat.com>, Ingo Molnar <mingo@kernel.org>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Stephane Eranian <eranian@google.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Andi Kleen <andi@firstfloor.org>,
        Ian Rogers <irogers@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Arnaldo,

On Thu, Sep 10, 2020 at 8:10 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Thu, Sep 10, 2020 at 11:15:42AM +0200, Jiri Olsa escreveu:
> > On Tue, Sep 08, 2020 at 01:42:24PM +0900, Namhyung Kim wrote:
> > > When we profile cgroup events with perf stat, it's very annoying to
> > > specify events and cgroups on the command line as it requires the
> > > mapping between events and cgroups.  (Note that perf record can use
> > > cgroup sampling but it's not usable for perf stat).
>
> > > I guess most cases we just want to use a same set of events (N) for
> > > all cgroups (M), but we need to specify NxM events and NxM cgroups.
> > > This is not good especially when profiling large number of cgroups:
> > > say M=200.
>
> > > So I added --multiply-cgroup option to make it easy for that case.  It
> > > will create NxM events from N events and M cgroups.  One more upside
> > > is that it can handle metrics too.
>
> > agreed that it's PITA to use -G option ;-)
>
> yeah, its great that someone is looking at cgroups improvements, thanks
> Namyung, its great to have you working on this!

Thanks! :)

>
> More below.
>
> > > For example, the following example measures IPC metric for 3 cgroups
>
> > >   $ cat perf-multi-cgrp.sh
> > >   #!/bin/sh
>
> > >   METRIC=${1:-IPC}
> > >   CGROUP_DIR=/sys/fs/cgroup/perf_event
>
> > >   sudo mkdir $CGROUP_DIR/A $CGROUP_DIR/B $CGROUP_DIR/C
>
> > >   # add backgroupd workload for each cgroup
> > >   echo $$ | sudo tee $CGROUP_DIR/A/cgroup.procs > /dev/null
> > >   yes > /dev/null &
> > >   echo $$ | sudo tee $CGROUP_DIR/B/cgroup.procs > /dev/null
> > >   yes > /dev/null &
> > >   echo $$ | sudo tee $CGROUP_DIR/C/cgroup.procs > /dev/null
> > >   yes > /dev/null &
>
> > >   # run 'perf stat' in the root cgroup
> > >   echo $$ | sudo tee $CGROUP_DIR/cgroup.procs > /dev/null
> > >   perf stat -a -M $METRIC --multiply-cgroup -G A,B,C sleep 1
> >
> > would it be easier to have new option for this? like:
> >
> >   perf stat -a -M $METRIC --for-cgroup A,B,C
> >   perf stat -a -M $METRIC --for-each-cgroup A,B,C
> >   perf stat -a -M $METRIC --attach-cgroup A,B,C
> >   perf stat -a -M $METRIC --attach-to-cgroup A,B,C

Looks good.  I like the --for-each-cgroup.
Then we should make it and -G mutually exclusive IMHO.

> >
> > I'm still not sure how the --multiply-cgroup deals with empty
> > cgroup A,,C but looks like we don't need this behaviour now?

Yep, it can handle such case and bind the events to the root cgroup.

>
> Yeah, I also didn't like the --multiply-cgroup thing, perhaps we can use
> a per-event term? or per group, for example:
>
>   perf stat -a -M $METRIC/cgroups=A,B,C/
>   perf stat -a -e '{cycles,instructions,cache-misses}/cgroups=A,B,C/'
>
> Allowing wildcards or regexps would help with some use cases.
>
> We already have several terms that allows us to control per event knobs,
> this would be one more.

At some point, we can support this kind of flexibility, but it'd make the code
complex when multiple events or metrics have different cgroup membership.
So I'd like to do the simple case (all events are expanded to same cgroups)
first..

Thanks
Namhyung