Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 13 Sep 2022 10:17:08 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Rob Herring <robh@kernel.org>
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>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        James Clark <james.clark@arm.com>,
        linux-perf-users <linux-perf-users@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] perf: Skip and warn on unknown format 'configN' attrs
Message-ID: <Yx/oJDvan6ctGYtX@leoy-yangtze.lan>
References: <20220909204509.2169512-1-robh@kernel.org>
 <Yx8LgqUtDh5TwWmV@leoy-huanghe.lan>
 <CAL_JsqJGrUrahj-T+k7T9GAF+T8=y03YUTG2V+v52styFFDW_Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAL_JsqJGrUrahj-T+k7T9GAF+T8=y03YUTG2V+v52styFFDW_Q@mail.gmail.com>
Precedence: bulk

On Mon, Sep 12, 2022 at 08:55:32AM -0500, Rob Herring wrote:

[...]

> > If this is the case,
> > it's good to add a fixes tag like below?
> >
> > Fixes: cd82a32e9924 ("perf tools: Add perf pmu object to access pmu format definition")
> 
> Probably not too important given this is from 2012.

Which means since 2012 there have no requirement for config3 :)

AFAICT, perf tool should be forward compatible, so I think it's good
for this patch to be landed on (old) LTS kernels.

> > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > ---
> > > v2:
> > >  - Rework YACC code to handle configN formats in C code
> > >  - Add a warning when an unknown configN attr is found
> > >
> > > v1: https://lore.kernel.org/all/20220901184709.2179309-1-robh@kernel.org/
> > > ---
> > >  tools/perf/util/pmu.c |  6 ++++++
> > >  tools/perf/util/pmu.l |  2 --
> > >  tools/perf/util/pmu.y | 15 ++++-----------
> > >  3 files changed, 10 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > > index 89655d53117a..6757db7d559c 100644
> > > --- a/tools/perf/util/pmu.c
> > > +++ b/tools/perf/util/pmu.c
> > > @@ -1475,6 +1475,12 @@ int perf_pmu__new_format(struct list_head *list, char *name,
> > >  {
> > >       struct perf_pmu_format *format;
> > >
> > > +     if (config > PERF_PMU_FORMAT_VALUE_CONFIG2) {
> >
> > It's good to add a new macro PERF_PMU_FORMAT_VALUE_CONFIG_END in
> > util/pmu.h. Then at here we can check the condition:
> 
> Sure.
> 
> >
> >        if (config >= PERF_PMU_FORMAT_VALUE_CONFIG_END) {
> >
> > > +             pr_warning("WARNING: format '%s' requires 'config%d' which is not supported by this version of perf!\n",
> > > +                        name, config);
> >
> > ... so at here we can print log like:
> >
> >        pr_warning("WARNING: format '%s' requires 'config%d' which is not "
> >                   "supported by this version of perf (maximum config%d)!\n",
> >                   name, config, PERF_PMU_FORMAT_VALUE_CONFIG_END - 1);
> 
> I was trying to keep it to one line and given configN isn't expanded
> frequently it should be simple enough to figure out what version you
> need.

It's fine for me.

> > The rest of this patch is fine for me.
> >
> > As a side topic, I know you want to support the SPEv1.2 feature with
> > config3, seems to me a complete patch set series should include the
> > changes for supporting config3 as well.  This can give more clear view
> > for how to fix incompatibility issue between old and new kernels, and
> > how to support config3 in the latest kernel, but it's fine for me if
> > you want to split into two patch sets.
> 
> I've sent out the SPE kernel support separately. I was told by Arnaldo
> to split perf tool and kernel side changes.

To be clear, it's right to split kernel and userspace parts into two
patch set, but for userspace change I cannot find you sent patch
to support config3 in perf tool, something in the [1] is missed,
otherwise I don't think you provide complete support for config3 (and
thus we have no chance to use new event format).

Thanks,
Leo

[1] https://termbin.com/vdph