diff mbox series

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

Message ID 1560521283-73314-3-git-send-email-john.garry@huawei.com
State New
Headers show
Series Perf uncore PMU event alias support for Hisi hip08 ARM64 platform | expand

Commit Message

John Garry June 14, 2019, 2:08 p.m. UTC
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(-)

-- 
2.17.1

Comments

Arnaldo Carvalho de Melo June 14, 2019, 2:50 p.m. UTC | #1
Em Fri, Jun 14, 2019 at 10:08:00PM +0800, John Garry escreveu:
> 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 ','.


Looks ok, but would be good to have some Reviewed-by attached as I'm not
super familiar with the PMU oddities, Jiri, can you please review this,
somebody else?

Thanks,

- Arnaldo
 
> 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;

> +

> +	/*

> +	 * uncore alias may be from different PMU with common

> +	 * prefix or matching tokens.

> +	 */

> +	tok = strtok_r(str, ",", &tmp);

> +	if (strncmp(pmu_name, tok, strlen(tok))) {

> +		res = false;

> +		goto out;

> +	}

> +

> +	for (; tok; name += strlen(tok), tok = strtok_r(NULL, ",", &tmp)) {

> +		name = strstr(name, tok);

> +		if (!name) {

> +			res = false;

> +			goto out;

> +		}

> +	}

> +

> +	res = true;

> +out:

> +	free(str);

> +	return res;

> +}

> +

>  /*

>   * From the pmu_events_map, find the table of PMU events that corresponds

>   * to the current running CPU. Then, add all PMU events from that table

> @@ -730,12 +763,8 @@ static void pmu_add_cpu_aliases(struct list_head *head, struct perf_pmu *pmu)

>  			break;

>  		}

>  

> -		/*

> -		 * uncore alias may be from different PMU

> -		 * with common prefix

> -		 */

>  		if (pmu_is_uncore(name) &&

> -		    !strncmp(pname, name, strlen(pname)))

> +		    pmu_uncore_alias_match(pname, name))

>  			goto new_alias;

>  

>  		if (strcmp(pname, name))

> -- 

> 2.17.1


-- 

- Arnaldo
Jiri Olsa June 16, 2019, 9:58 a.m. UTC | #2
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;

> +

> +	/*

> +	 * uncore alias may be from different PMU with common

> +	 * prefix or matching tokens.

> +	 */

> +	tok = strtok_r(str, ",", &tmp);

> +	if (strncmp(pmu_name, tok, strlen(tok))) {


if tok is NULL in here we crash

> +		res = false;

> +		goto out;

> +	}

> +

> +	for (; tok; name += strlen(tok), tok = strtok_r(NULL, ",", &tmp)) {


why is name shifted in here?

jirka

> +		name = strstr(name, tok);

> +		if (!name) {

> +			res = false;

> +			goto out;

> +		}

> +	}

> +

> +	res = true;

> +out:

> +	free(str);

> +	return res;

> +}

> +

>  /*

>   * From the pmu_events_map, find the table of PMU events that corresponds

>   * to the current running CPU. Then, add all PMU events from that table

> @@ -730,12 +763,8 @@ static void pmu_add_cpu_aliases(struct list_head *head, struct perf_pmu *pmu)

>  			break;

>  		}

>  

> -		/*

> -		 * uncore alias may be from different PMU

> -		 * with common prefix

> -		 */

>  		if (pmu_is_uncore(name) &&

> -		    !strncmp(pname, name, strlen(pname)))

> +		    pmu_uncore_alias_match(pname, name))

>  			goto new_alias;

>  

>  		if (strcmp(pname, name))

> -- 

> 2.17.1

>
John Garry June 17, 2019, 9:06 a.m. UTC | #3
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;

>> +

>> +	/*

>> +	 * uncore alias may be from different PMU with common

>> +	 * prefix or matching tokens.

>> +	 */

>> +	tok = strtok_r(str, ",", &tmp);

>> +	if (strncmp(pmu_name, tok, strlen(tok))) {

>


Hi Jirka,

> 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?

>> +		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'.

Thanks,
John

>

> jirka

>

>> +		name = strstr(name, tok);

>> +		if (!name) {

>> +			res = false;

>> +			goto out;

>> +		}

>> +	}

>> +

>> +	res = true;

>> +out:

>> +	free(str);

>> +	return res;

>> +}

>> +

>>  /*

>>   * From the pmu_events_map, find the table of PMU events that corresponds

>>   * to the current running CPU. Then, add all PMU events from that table

>> @@ -730,12 +763,8 @@ static void pmu_add_cpu_aliases(struct list_head *head, struct perf_pmu *pmu)

>>  			break;

>>  		}

>>

>> -		/*

>> -		 * uncore alias may be from different PMU

>> -		 * with common prefix

>> -		 */

>>  		if (pmu_is_uncore(name) &&

>> -		    !strncmp(pname, name, strlen(pname)))

>> +		    pmu_uncore_alias_match(pname, name))

>>  			goto new_alias;

>>

>>  		if (strcmp(pname, name))

>> --

>> 2.17.1

>>

>

> .

>
Jiri Olsa June 20, 2019, 6:25 p.m. UTC | #4
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;

> > > +

> > > +	/*

> > > +	 * uncore alias may be from different PMU with common

> > > +	 * prefix or matching tokens.

> > > +	 */

> > > +	tok = strtok_r(str, ",", &tmp);

> > > +	if (strncmp(pmu_name, tok, strlen(tok))) {

> > 

> 

> 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?
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

and name is still pmu with no ',' ... please make this or
proper version that in some comment

thanks,
jirka
John Garry June 21, 2019, 10:42 a.m. UTC | #5
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

>

> .

>
John Garry June 27, 2019, 4:27 p.m. UTC | #6
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
Jiri Olsa June 27, 2019, 4:33 p.m. UTC | #7
On Thu, Jun 27, 2019 at 05:27:32PM +0100, John Garry wrote:

SNIP

> > 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,


sorry, forgot about this one.. will check soon

jirka
Jiri Olsa June 28, 2019, 10:40 a.m. UTC | #8
On Thu, Jun 27, 2019 at 05:27:32PM +0100, John Garry wrote:

SNIP

> > > 

> > > 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.


ok

> > 

> > > 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


ok, plz put something like above into comment

thanks,
jirka
John Garry June 28, 2019, 10:45 a.m. UTC | #9
On 28/06/2019 11:40, Jiri Olsa wrote:
> On Thu, Jun 27, 2019 at 05:27:32PM +0100, John Garry wrote:

>

> SNIP

>

>>>>

>>>> 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.

>

> ok

>

>>>

>>>> 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

>


Hi jirka,

> ok, plz put something like above into comment

>


ok, can do.

Thanks again,
John

> thanks,

> jirka

>

> .

>
diff mbox series

Patch

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;
+
+	/*
+	 * uncore alias may be from different PMU with common
+	 * prefix or matching tokens.
+	 */
+	tok = strtok_r(str, ",", &tmp);
+	if (strncmp(pmu_name, tok, strlen(tok))) {
+		res = false;
+		goto out;
+	}
+
+	for (; tok; name += strlen(tok), tok = strtok_r(NULL, ",", &tmp)) {
+		name = strstr(name, tok);
+		if (!name) {
+			res = false;
+			goto out;
+		}
+	}
+
+	res = true;
+out:
+	free(str);
+	return res;
+}
+
 /*
  * From the pmu_events_map, find the table of PMU events that corresponds
  * to the current running CPU. Then, add all PMU events from that table
@@ -730,12 +763,8 @@  static void pmu_add_cpu_aliases(struct list_head *head, struct perf_pmu *pmu)
 			break;
 		}
 
-		/*
-		 * uncore alias may be from different PMU
-		 * with common prefix
-		 */
 		if (pmu_is_uncore(name) &&
-		    !strncmp(pname, name, strlen(pname)))
+		    pmu_uncore_alias_match(pname, name))
 			goto new_alias;
 
 		if (strcmp(pname, name))