Re: [PATCHSET 0/4] perf stat: Add --multiply-cgroup option

From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>,
	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>
Subject: Re: [PATCHSET 0/4] perf stat: Add --multiply-cgroup option
Date: Thu, 10 Sep 2020 08:10:20 -0300	[thread overview]
Message-ID: <20200910111020.GA4018363@kernel.org> (raw)
In-Reply-To: <20200910091542.GD1627030@krava>

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!

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
> 
> 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?

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.

- Arnaldo