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: <20210921181349.199780-1-irogers@google.com> <YUudR8ST7qGYdFQE@krava>
In-Reply-To: <YUudR8ST7qGYdFQE@krava>
From:   Ian Rogers <irogers@google.com>
Date:   Wed, 22 Sep 2021 15:59:11 -0700
Message-ID: <CAP-5=fVWGT0=0k0eG0c-6wEGod=49SVsumJRN2=SxmgOo5A4dw@mail.gmail.com>
Subject: Re: [PATCH v2] perf evlist: Remove group option.
To:     Jiri Olsa <jolsa@redhat.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
        eranian@google.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Sep 22, 2021 at 2:17 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Sep 21, 2021 at 11:13:49AM -0700, Ian Rogers wrote:
>
> SNIP
>
> > diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> > index 8feef3a05af7..9cd79513eebb 100644
> > --- a/tools/perf/util/python.c
> > +++ b/tools/perf/util/python.c
> > @@ -1109,14 +1109,6 @@ static PyObject *pyrf_evlist__open(struct pyrf_evlist *pevlist,
> >                                  PyObject *args, PyObject *kwargs)
> >  {
> >       struct evlist *evlist = &pevlist->evlist;
> > -     int group = 0;
> > -     static char *kwlist[] = { "group", NULL };
> > -
> > -     if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|OOii", kwlist, &group))
> > -             return NULL;
> > -
> > -     if (group)
> > -             evlist__set_leader(evlist);
> >
> >       if (evlist__open(evlist) < 0) {
> >               PyErr_SetFromErrno(PyExc_OSError);
> > diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
> > index bff669b615ee..9e694db7c7ee 100644
> > --- a/tools/perf/util/record.c
> > +++ b/tools/perf/util/record.c
> > @@ -99,13 +99,6 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
> >       bool use_comm_exec;
> >       bool sample_id = opts->sample_id;
> >
> > -     /*
> > -      * Set the evsel leader links before we configure attributes,
> > -      * since some might depend on this info.
> > -      */
> > -     if (opts->group)
> > -             evlist__set_leader(evlist);
> > -
>
> I don't mind erasing that, but just curious if you're going
> to add something which would clash with this? it does not
> look too complex for the code.. but still, let's remove it
> if it's in 'legacy mode' for this long ;-)

I've nothing conflicting but I came across old tutorials describing it
and I suspect usage of it is likely broken - in particular mixing
--group with -e {}.

Thanks,
Ian

> thanks,
> jirka
>
>
> >       if (evlist->core.cpus->map[0] < 0)
> >               opts->no_inherit = true;
> >
> > diff --git a/tools/perf/util/record.h b/tools/perf/util/record.h
> > index 68f471d9a88b..d71dee9ce41c 100644
> > --- a/tools/perf/util/record.h
> > +++ b/tools/perf/util/record.h
> > @@ -13,7 +13,6 @@ struct option;
> >
> >  struct record_opts {
> >       struct target target;
> > -     bool          group;
> >       bool          inherit_stat;
> >       bool          no_buffering;
> >       bool          no_inherit;
> > --
> > 2.33.0.464.g1972c5931b-goog
> >
>