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

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

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.

Much appreciated,
John

> thanks,
> jirka
>
> .
>