Re: [PATCH v2 2/5] perf pmu: Support more complex PMU event aliasing

[John Garry <john.garry@huawei.com>]

On 21/06/2019 11:42, John Garry wrote:
> On 20/06/2019 19:25, Jiri Olsa wrote:
>> On Mon, Jun 17, 2019 at 10:06:08AM +0100, John Garry wrote:
>>> On 16/06/2019 10:58, Jiri Olsa wrote:
>>>> On Fri, Jun 14, 2019 at 10:08:00PM +0800, John Garry wrote:
>>>>> The jevent "Unit" field is used for uncore PMU alias definition.
>>>>>
>>>>> The form uncore_pmu_example_X is supported, where "X" is a wildcard,
>>>>> to support multiple instances of the same PMU in a system.
>>>>>
>>>>> Unfortunately this format not suitable for all uncore PMUs; take
>>>>> the Hisi
>>>>> DDRC uncore PMU for example, where the name is in the form
>>>>> hisi_scclX_ddrcY.
>>>>>
>>>>> For the current jevent parsing, we would be required to hardcode an
>>>>> uncore
>>>>> alias translation for each possible value of X. This is not scalable.
>>>>>
>>>>> Instead, add support for "Unit" field in the form "hisi_sccl,ddrc",
>>>>> where
>>>>> we can match by hisi_scclX and ddrcY. Tokens in Unit field are
>>>>> delimited by ','.
>>>>>
>>>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>>>> ---
>>>>>  tools/perf/util/pmu.c | 39 ++++++++++++++++++++++++++++++++++-----
>>>>>  1 file changed, 34 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>>>>> index 7e7299fee550..bc71c60589b5 100644
>>>>> --- a/tools/perf/util/pmu.c
>>>>> +++ b/tools/perf/util/pmu.c
>>>>> @@ -700,6 +700,39 @@ struct pmu_events_map
>>>>> *perf_pmu__find_map(struct perf_pmu *pmu)
>>>>>      return map;
>>>>>  }
>>>>>
>>>>> +static bool pmu_uncore_alias_match(const char *pmu_name, const
>>>>> char *name)
>>>>> +{
>>>>> +    char *tmp, *tok, *str;
>>>>> +    bool res;
>>>>> +
>>>>> +    str = strdup(pmu_name);
>>>>> +    if (!str)
>>>>> +        return false;
>
> Hi Jirka,
>
>>>>> +
>>>>> +    /*
>>>>> +     * uncore alias may be from different PMU with common
>>>>> +     * prefix or matching tokens.
>>>>> +     */
>>>>> +    tok = strtok_r(str, ",", &tmp);
>
> If str contains no delimiter, then it returns str in tok.
>
>>>>> +    if (strncmp(pmu_name, tok, strlen(tok))) {
>
> So this above check covers the case of str with and without a delimiter.
>
>>>>
>>>
>>> Hi Jirka,
>>
>> heya,
>> sry for late reply
>>
>>>
>>>> if tok is NULL in here we crash
>>>>
>>>
>>> As I see, tok could not be NULL. If str contains no delimiters, then
>>> we just
>>> return same as str in tok.
>>>
>>> Can you see tok being NULL?
>>
>> well, if there's no ',' in the str it returns NULL, right?
>
> No, it would return str in tok.
>
>> and IIUC this function is still called for standard uncore
>> pmu names
>>
>>>
>>>>> +        res = false;
>>>>> +        goto out;
>>>>> +    }
>>>>> +
>>>>> +    for (; tok; name += strlen(tok), tok = strtok_r(NULL, ",",
>>>>> &tmp)) {
>>>>
>>>> why is name shifted in here?
>>>
>>> I want to ensure that we match the tokens in order and also guard
>>> against
>>> possible repeated token matches in 'name'.
>>
>> i might not understand this correctly.. so
>>
>> str is the alias name that can contain ',' now, like:
>>   hisi_sccl,ddrc
>
> For example of pmu_nmame=hisi_sccl,ddrc and pmu=hisi_sccl1_ddrc0, we
> match in this sequence:
>
> loop 1. tok=hisi_sccl name=hisi_sccl1_ddrc0
> loop 2. tok=ddrc name=ddrc0
> loop 3. tok=NULL -> breakout and return true
>
> A couple of notes:
> a. loop 1. could be omitted, but the code becomes a bit more complicated
> 2. I don't have to advance name. But then we would match something like
> hisi_ddrc0_sccl1. Maybe this is ok.
>
>>
>> and name is still pmu with no ',' ...
>> please make this or
>> proper version that in some comment
>>
>
> I didn't really get your meaning here. Please check my replies and see
> if you have further doubt or concern.
>

Hi Jirka,

I was just wondering if you have any further comments or questions here?

Much appreciated,
John