Re: [PATCH 1/2] perf pmu: Restore full PMU name wildcard support

[Namhyung Kim <namhyung@kernel.org>]

On Tue, Jun 18, 2024 at 04:47:01PM +0100, James Clark wrote:
> 
> 
> On 18/06/2024 16:25, Ian Rogers wrote:
> > On Tue, Jun 18, 2024, 8:06 AM James Clark <james.clark@arm.com> wrote:
> > 
> >>
> >>
> >> On 18/06/2024 15:23, Ian Rogers wrote:
> >>> On Tue, Jun 18, 2024, 3:59 AM James Clark <james.clark@arm.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 17/06/2024 22:25, Ian Rogers wrote:
> >>>>> On Mon, Jun 17, 2024, 6:44 AM James Clark <james.clark@arm.com> wrote:
> >>>>>
> >>>>>> Commit b2b9d3a3f021 ("perf pmu: Support wildcards on pmu name in
> >> dynamic
> >>>>>> pmu events") gives the following example for wildcarding a subset of
> >>>>>> PMUs:
> >>>>>>
> >>>>>>   E.g., in a system with the following dynamic pmus:
> >>>>>>
> >>>>>>         mypmu_0
> >>>>>>         mypmu_1
> >>>>>>         mypmu_2
> >>>>>>         mypmu_4
> >>>>>>
> >>>>>>   perf stat -e mypmu_[01]/<config>/
> >>>>>>
> >>>>>> Since commit f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()"),
> >> only
> >>>>>> "*" has been supported, removing the ability to subset PMUs, even
> >> though
> >>>>>> parse-events.l still supports ? and [] characters.
> >>>>>>
> >>>>>> Fix it by using fnmatch() when any glob character is detected and add
> >> a
> >>>>>> test which covers that and other scenarios of
> >>>>>> perf_pmu__match_ignoring_suffix().
> >>>>>>
> >>>>>> Fixes: f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()")
> >>>>>> Signed-off-by: James Clark <james.clark@arm.com>
> >>>>>>
> >>>>>
> >>>>> We use regular expression matching elsewhere rather than fnmatch. We
> >> can
> >>>>> also precompile the matchers using lex. I'm not sure we shouldn't be
> >>>>> looking for an opportunity to remove fnmatch rather than expand upon
> >> it.
> >>>>>
> >>>>> Thanks,
> >>>>> Ian
> >>>>>
> >>>>
> >>>> Presumably you mean we can do the removal of fnmatch after this fix goes
> >>>> in,
> >>>> rather than instead of? Because this is a user facing change in
> >> behaviour
> >>>> but the removal of fnmatch would be an non-user facing code refactor?
> >>>>
> >>>> It's technically not an "expansion" because we always used fnmatch and
> >> the
> >>>> linked commit hasn't made it to a release yet.
> >>>>
> >>>
> >>> The main place the expansion gets added is parse-events.c, previously
> >>> parse-events.y. If we're adding the expansion ourselves then we can
> >> choose
> >>> the form we add it. Some coming servers will have 100s of PMUs and so I'm
> >>> worried about the scanning cost when a PMU isn't specified.
> >>>
> >>> Thanks,
> >>> Ian
> >>>
> >>
> >> I think I might not be following. If a PMU isn't specified then
> >> perf_pmu__match() is never called so no cost is incurred. I also don't
> >> add any new calls to fnmatch().
> >>
> >> I only updated the gate on whether the existing fnmatch() is called from
> >> "*" to "*[?". So it only happens when one of those characters is in the
> >> PMU name, but it already happens when '*' is in the name.
> >>
> > 
> > Right. I'm not saying there is anything wrong in the change or an
> > additional cost, what the issue is is that currently only really '*'
> > requires fnmatch and that's because the event parsing adds it. It could
> 
> It's not only '*' that requires it, see the example I added in the
> commit message:
> 
>   ./perf stat -e mypmu_[01]/<config>/
> 
> '?' was supported before as well which could be useful.
> 
> > similarly add '.*' if we did regular expression matching. By expanding what
> > we pass to fnmatch from the command line the more committed we are to
> > fnmatch rather than regular expressions - which is what we use everywhere
> > else in the code. So maybe it was a feature that this wasn't working.
> > 
> 
> But we haven't had a release of Perf yet where more is passed to
> fnmatch(). Before f91fa2ae6360 ("perf pmu: Refactor perf_pmu__match()")
> everything was passed to fnmatch(). After that unreleased commit only
> things with '*' are. Now with this change only "*?[", so it's less not more.
> 
> I don't think there is any commitment to keep it, we can always remove
> fnmatch in the future. But it looks like a mistake to me because the
> title says "refactor" when it actually removes a feature.

Ian, are you ok with this now?

Thanks,
Namhyung