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

[James Clark <james.clark@arm.com>]

On 26/06/2024 05:13, Namhyung Kim wrote:
> 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
> 

I don't feel too strongly about this one, unless anyone really is still
using my_pmu[01].

For patch 2 I was going to resend with Ian's suggested-by tag, but I
think we should try to get that one in as a test is failing.