Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Thu, Jul 28, 2016 at 10:15:52AM -0600, Mathieu Poirier wrote:
> On 27 July 2016 at 13:26, Jiri Olsa  wrote:
> > On Wed, Jul 27, 2016 at 11:59:50AM -0600, Mathieu Poirier wrote:
> >
> > SNIP
> >
> >> > -PE_DRV_CFG_TERM
> >> > +'@' PE_NAME '=' PE_NAME
> >> >  {
> >> > struct parse_events_term *term;
> >> >
> >> > ABORT_ON(parse_events_term__str(, 
> >> > PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> >> > -   $1, $1, &@1, NULL));
> >> > +   $2, $4, &@2, &@4));
> >> > $$ = term;
> >> >  }
> >> >
> >>
> >> The problem here is that the correlation between the first and the
> >> second PE_NAME is lost and instead of seeing "PE_NAME=PE_NAME", the
> >> kernel only gets the value associated with the second PE_NAME.
> >>
> >> For example,
> >>
> >> -e event/@cfg1=value1,@cfg2=value2/ ...
> >>
> >> The above code will send "value1" and "value2" to the kernel driver
> >> where there is no way to know what configurable the values correspond
> >
> > hum.. you get the 'cfg1' and 'cfg2' strings in $1 no?
> 
> Indeed you do.
> 
> Macro ADD_CONFIG_TERM in function get_config_terms() only account for
> the __val parameter and struct parse_events_term::config is completely
> ignored.  We could concatenate the fields before calling
> ADD_CONFIG_TERM() but between that and freeing the reserved memory, I
> think it is cleaner to let flex do the work.

ah you need that whole string in one piece.. ok
let's do it by your original way then

please add some comments for in drv_str function describing
the usage of @ and the fact we're doing this because we need
whole assignment as single string

thanks,
jirka

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Thu, Jul 28, 2016 at 10:15:52AM -0600, Mathieu Poirier wrote:
> On 27 July 2016 at 13:26, Jiri Olsa  wrote:
> > On Wed, Jul 27, 2016 at 11:59:50AM -0600, Mathieu Poirier wrote:
> >
> > SNIP
> >
> >> > -PE_DRV_CFG_TERM
> >> > +'@' PE_NAME '=' PE_NAME
> >> >  {
> >> > struct parse_events_term *term;
> >> >
> >> > ABORT_ON(parse_events_term__str(, 
> >> > PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> >> > -   $1, $1, &@1, NULL));
> >> > +   $2, $4, &@2, &@4));
> >> > $$ = term;
> >> >  }
> >> >
> >>
> >> The problem here is that the correlation between the first and the
> >> second PE_NAME is lost and instead of seeing "PE_NAME=PE_NAME", the
> >> kernel only gets the value associated with the second PE_NAME.
> >>
> >> For example,
> >>
> >> -e event/@cfg1=value1,@cfg2=value2/ ...
> >>
> >> The above code will send "value1" and "value2" to the kernel driver
> >> where there is no way to know what configurable the values correspond
> >
> > hum.. you get the 'cfg1' and 'cfg2' strings in $1 no?
> 
> Indeed you do.
> 
> Macro ADD_CONFIG_TERM in function get_config_terms() only account for
> the __val parameter and struct parse_events_term::config is completely
> ignored.  We could concatenate the fields before calling
> ADD_CONFIG_TERM() but between that and freeing the reserved memory, I
> think it is cleaner to let flex do the work.

ah you need that whole string in one piece.. ok
let's do it by your original way then

please add some comments for in drv_str function describing
the usage of @ and the fact we're doing this because we need
whole assignment as single string

thanks,
jirka

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 27 July 2016 at 13:26, Jiri Olsa  wrote:
> On Wed, Jul 27, 2016 at 11:59:50AM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> > -PE_DRV_CFG_TERM
>> > +'@' PE_NAME '=' PE_NAME
>> >  {
>> > struct parse_events_term *term;
>> >
>> > ABORT_ON(parse_events_term__str(, 
>> > PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>> > -   $1, $1, &@1, NULL));
>> > +   $2, $4, &@2, &@4));
>> > $$ = term;
>> >  }
>> >
>>
>> The problem here is that the correlation between the first and the
>> second PE_NAME is lost and instead of seeing "PE_NAME=PE_NAME", the
>> kernel only gets the value associated with the second PE_NAME.
>>
>> For example,
>>
>> -e event/@cfg1=value1,@cfg2=value2/ ...
>>
>> The above code will send "value1" and "value2" to the kernel driver
>> where there is no way to know what configurable the values correspond
>
> hum.. you get the 'cfg1' and 'cfg2' strings in $1 no?

Indeed you do.

Macro ADD_CONFIG_TERM in function get_config_terms() only account for
the __val parameter and struct parse_events_term::config is completely
ignored.  We could concatenate the fields before calling
ADD_CONFIG_TERM() but between that and freeing the reserved memory, I
think it is cleaner to let flex do the work.

Mathieu

>
> jirka
>
>> to.  To go around that we'd have to concatenate $2 and $4 in function
>> parse_events_term__str() (or new_term()) when @type_term ==
>> PARSE_EVENTS__TERM_TYPE_DRV_CFG, something that definitely looks
>> hackish to me.
>>
>> Thanks,
>> Mathieu

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 27 July 2016 at 13:26, Jiri Olsa  wrote:
> On Wed, Jul 27, 2016 at 11:59:50AM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> > -PE_DRV_CFG_TERM
>> > +'@' PE_NAME '=' PE_NAME
>> >  {
>> > struct parse_events_term *term;
>> >
>> > ABORT_ON(parse_events_term__str(, 
>> > PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>> > -   $1, $1, &@1, NULL));
>> > +   $2, $4, &@2, &@4));
>> > $$ = term;
>> >  }
>> >
>>
>> The problem here is that the correlation between the first and the
>> second PE_NAME is lost and instead of seeing "PE_NAME=PE_NAME", the
>> kernel only gets the value associated with the second PE_NAME.
>>
>> For example,
>>
>> -e event/@cfg1=value1,@cfg2=value2/ ...
>>
>> The above code will send "value1" and "value2" to the kernel driver
>> where there is no way to know what configurable the values correspond
>
> hum.. you get the 'cfg1' and 'cfg2' strings in $1 no?

Indeed you do.

Macro ADD_CONFIG_TERM in function get_config_terms() only account for
the __val parameter and struct parse_events_term::config is completely
ignored.  We could concatenate the fields before calling
ADD_CONFIG_TERM() but between that and freeing the reserved memory, I
think it is cleaner to let flex do the work.

Mathieu

>
> jirka
>
>> to.  To go around that we'd have to concatenate $2 and $4 in function
>> parse_events_term__str() (or new_term()) when @type_term ==
>> PARSE_EVENTS__TERM_TYPE_DRV_CFG, something that definitely looks
>> hackish to me.
>>
>> Thanks,
>> Mathieu

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Wed, Jul 27, 2016 at 11:59:50AM -0600, Mathieu Poirier wrote:

SNIP

> > -PE_DRV_CFG_TERM
> > +'@' PE_NAME '=' PE_NAME
> >  {
> > struct parse_events_term *term;
> >
> > ABORT_ON(parse_events_term__str(, 
> > PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> > -   $1, $1, &@1, NULL));
> > +   $2, $4, &@2, &@4));
> > $$ = term;
> >  }
> >
> 
> The problem here is that the correlation between the first and the
> second PE_NAME is lost and instead of seeing "PE_NAME=PE_NAME", the
> kernel only gets the value associated with the second PE_NAME.
> 
> For example,
> 
> -e event/@cfg1=value1,@cfg2=value2/ ...
> 
> The above code will send "value1" and "value2" to the kernel driver
> where there is no way to know what configurable the values correspond

hum.. you get the 'cfg1' and 'cfg2' strings in $1 no?

jirka

> to.  To go around that we'd have to concatenate $2 and $4 in function
> parse_events_term__str() (or new_term()) when @type_term ==
> PARSE_EVENTS__TERM_TYPE_DRV_CFG, something that definitely looks
> hackish to me.
> 
> Thanks,
> Mathieu

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Wed, Jul 27, 2016 at 11:59:50AM -0600, Mathieu Poirier wrote:

SNIP

> > -PE_DRV_CFG_TERM
> > +'@' PE_NAME '=' PE_NAME
> >  {
> > struct parse_events_term *term;
> >
> > ABORT_ON(parse_events_term__str(, 
> > PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> > -   $1, $1, &@1, NULL));
> > +   $2, $4, &@2, &@4));
> > $$ = term;
> >  }
> >
> 
> The problem here is that the correlation between the first and the
> second PE_NAME is lost and instead of seeing "PE_NAME=PE_NAME", the
> kernel only gets the value associated with the second PE_NAME.
> 
> For example,
> 
> -e event/@cfg1=value1,@cfg2=value2/ ...
> 
> The above code will send "value1" and "value2" to the kernel driver
> where there is no way to know what configurable the values correspond

hum.. you get the 'cfg1' and 'cfg2' strings in $1 no?

jirka

> to.  To go around that we'd have to concatenate $2 and $4 in function
> parse_events_term__str() (or new_term()) when @type_term ==
> PARSE_EVENTS__TERM_TYPE_DRV_CFG, something that definitely looks
> hackish to me.
> 
> Thanks,
> Mathieu

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 26 July 2016 at 14:41, Jiri Olsa  wrote:
> On Fri, Jul 22, 2016 at 12:24:48PM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> > diff --git a/tools/perf/util/parse-events.y 
>> > b/tools/perf/util/parse-events.y
>> > --- a/tools/perf/util/parse-events.y
>> > +++ b/tools/perf/util/parse-events.y
>> > @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
>> > $$ = term;
>> >  }
>> >  |
>> > -PE_DRV_CFG_TERM
>> > +'@' PE_DRV_CFG_TERM
>> >  {
>> > struct parse_events_term *term;
>> >
>> > ABORT_ON(parse_events_term__str(, 
>> > PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>> > -   $1, $1, &@1, NULL));
>> > +   $2, $2, &@2, NULL));
>> > $$ = term;
>> >  }
>> >
>>
>> I've been experimenting with the above solution and it is not yielding
>> the results one might think at first glance.
>>
>> If we use the example: -e event/@cfg1/ ...
>>
>> First if we leave things exactly the way they are suggested in the
>> code snippet flex doesn't know what do to with the '@' character and
>> returns an error.  To deal with that a new clause
>>
>> "@"{ return '@'; }
>>
>> can be inserted in the config state.  But that doesn't link '@' with
>> 'cfg1', and 'cfg1' gets interpreted as a PE_NAME.  Introducing a new
>> state upon hitting '@' would get us around that but we are moving away
>> from our initial goal of keeping things simple.
>
> hum, then how about keeping the flex atoms simple like for the
> other terms and do something like below.. untested ;-)
>
> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 1f7e11a6c5b3..8ba228e1c150 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
> return token;
>  }
>
> -static int drv_str(yyscan_t scanner, int token)
> -{
> -   YYSTYPE *yylval = parse_events_get_lval(scanner);
> -   char *text = parse_events_get_text(scanner);
> -
> -   /* Strip off the '@' */
> -   yylval->str = strdup(text + 1);
> -   return token;
> -}
> -
>  #define REWIND(__alloc)\
>  do {   \
> YYSTYPE *__yylval = parse_events_get_lval(yyscanner);   \
> @@ -134,7 +124,6 @@ num_hex 0x[a-fA-F0-9]+
>  num_raw_hex[a-fA-F0-9]+
>  name   [a-zA-Z_*?][a-zA-Z0-9_*?.]*
>  name_minus [a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
> -drv_cfg_term   [a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
>  /* If you add a modifier you need to update check_modifier() */
>  modifier_event [ukhpPGHSDI]+
>  modifier_bp[rwx]{1,3}
> @@ -216,11 +205,11 @@ no-inherit{ return term(yyscanner, 
> PARSE_EVENTS__TERM_TYPE_NOINHERIT); }
>  overwrite  { return term(yyscanner, 
> PARSE_EVENTS__TERM_TYPE_OVERWRITE); }
>  no-overwrite   { return term(yyscanner, 
> PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
>  ,  { return ','; }
> +"@"{ return '@'; }
>  "/"{ BEGIN(INITIAL); return '/'; }
>  {name_minus}   { return str(yyscanner, PE_NAME); }
>  \[all\]{ return PE_ARRAY_ALL; }
>  "["{ BEGIN(array); return '['; }
> -@{drv_cfg_term}{ return drv_str(yyscanner, PE_DRV_CFG_TERM); 
> }
>  }
>
>  {
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 879115f93edc..7e03e93dabca 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
> $$ = term;
>  }
>  |
> -PE_DRV_CFG_TERM
> +'@' PE_NAME '=' PE_NAME
>  {
> struct parse_events_term *term;
>
> ABORT_ON(parse_events_term__str(, 
> PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> -   $1, $1, &@1, NULL));
> +   $2, $4, &@2, &@4));
> $$ = term;
>  }
>

The problem here is that the correlation between the first and the
second PE_NAME is lost and instead of seeing "PE_NAME=PE_NAME", the
kernel only gets the value associated with the second PE_NAME.

For example,

-e event/@cfg1=value1,@cfg2=value2/ ...

The above code will send "value1" and "value2" to the kernel driver
where there is no way to know what configurable the values correspond
to.  To go around that we'd have to concatenate $2 and $4 in function
parse_events_term__str() (or new_term()) when @type_term ==
PARSE_EVENTS__TERM_TYPE_DRV_CFG, something that definitely looks
hackish to me.

Thanks,
Mathieu

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 26 July 2016 at 14:41, Jiri Olsa  wrote:
> On Fri, Jul 22, 2016 at 12:24:48PM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> > diff --git a/tools/perf/util/parse-events.y 
>> > b/tools/perf/util/parse-events.y
>> > --- a/tools/perf/util/parse-events.y
>> > +++ b/tools/perf/util/parse-events.y
>> > @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
>> > $$ = term;
>> >  }
>> >  |
>> > -PE_DRV_CFG_TERM
>> > +'@' PE_DRV_CFG_TERM
>> >  {
>> > struct parse_events_term *term;
>> >
>> > ABORT_ON(parse_events_term__str(, 
>> > PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>> > -   $1, $1, &@1, NULL));
>> > +   $2, $2, &@2, NULL));
>> > $$ = term;
>> >  }
>> >
>>
>> I've been experimenting with the above solution and it is not yielding
>> the results one might think at first glance.
>>
>> If we use the example: -e event/@cfg1/ ...
>>
>> First if we leave things exactly the way they are suggested in the
>> code snippet flex doesn't know what do to with the '@' character and
>> returns an error.  To deal with that a new clause
>>
>> "@"{ return '@'; }
>>
>> can be inserted in the config state.  But that doesn't link '@' with
>> 'cfg1', and 'cfg1' gets interpreted as a PE_NAME.  Introducing a new
>> state upon hitting '@' would get us around that but we are moving away
>> from our initial goal of keeping things simple.
>
> hum, then how about keeping the flex atoms simple like for the
> other terms and do something like below.. untested ;-)
>
> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 1f7e11a6c5b3..8ba228e1c150 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
> return token;
>  }
>
> -static int drv_str(yyscan_t scanner, int token)
> -{
> -   YYSTYPE *yylval = parse_events_get_lval(scanner);
> -   char *text = parse_events_get_text(scanner);
> -
> -   /* Strip off the '@' */
> -   yylval->str = strdup(text + 1);
> -   return token;
> -}
> -
>  #define REWIND(__alloc)\
>  do {   \
> YYSTYPE *__yylval = parse_events_get_lval(yyscanner);   \
> @@ -134,7 +124,6 @@ num_hex 0x[a-fA-F0-9]+
>  num_raw_hex[a-fA-F0-9]+
>  name   [a-zA-Z_*?][a-zA-Z0-9_*?.]*
>  name_minus [a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
> -drv_cfg_term   [a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
>  /* If you add a modifier you need to update check_modifier() */
>  modifier_event [ukhpPGHSDI]+
>  modifier_bp[rwx]{1,3}
> @@ -216,11 +205,11 @@ no-inherit{ return term(yyscanner, 
> PARSE_EVENTS__TERM_TYPE_NOINHERIT); }
>  overwrite  { return term(yyscanner, 
> PARSE_EVENTS__TERM_TYPE_OVERWRITE); }
>  no-overwrite   { return term(yyscanner, 
> PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
>  ,  { return ','; }
> +"@"{ return '@'; }
>  "/"{ BEGIN(INITIAL); return '/'; }
>  {name_minus}   { return str(yyscanner, PE_NAME); }
>  \[all\]{ return PE_ARRAY_ALL; }
>  "["{ BEGIN(array); return '['; }
> -@{drv_cfg_term}{ return drv_str(yyscanner, PE_DRV_CFG_TERM); 
> }
>  }
>
>  {
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 879115f93edc..7e03e93dabca 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
> $$ = term;
>  }
>  |
> -PE_DRV_CFG_TERM
> +'@' PE_NAME '=' PE_NAME
>  {
> struct parse_events_term *term;
>
> ABORT_ON(parse_events_term__str(, 
> PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> -   $1, $1, &@1, NULL));
> +   $2, $4, &@2, &@4));
> $$ = term;
>  }
>

The problem here is that the correlation between the first and the
second PE_NAME is lost and instead of seeing "PE_NAME=PE_NAME", the
kernel only gets the value associated with the second PE_NAME.

For example,

-e event/@cfg1=value1,@cfg2=value2/ ...

The above code will send "value1" and "value2" to the kernel driver
where there is no way to know what configurable the values correspond
to.  To go around that we'd have to concatenate $2 and $4 in function
parse_events_term__str() (or new_term()) when @type_term ==
PARSE_EVENTS__TERM_TYPE_DRV_CFG, something that definitely looks
hackish to me.

Thanks,
Mathieu

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Fri, Jul 22, 2016 at 12:24:48PM -0600, Mathieu Poirier wrote:

SNIP

> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
> > $$ = term;
> >  }
> >  |
> > -PE_DRV_CFG_TERM
> > +'@' PE_DRV_CFG_TERM
> >  {
> > struct parse_events_term *term;
> >
> > ABORT_ON(parse_events_term__str(, 
> > PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> > -   $1, $1, &@1, NULL));
> > +   $2, $2, &@2, NULL));
> > $$ = term;
> >  }
> >
> 
> I've been experimenting with the above solution and it is not yielding
> the results one might think at first glance.
> 
> If we use the example: -e event/@cfg1/ ...
> 
> First if we leave things exactly the way they are suggested in the
> code snippet flex doesn't know what do to with the '@' character and
> returns an error.  To deal with that a new clause
> 
> "@"{ return '@'; }
> 
> can be inserted in the config state.  But that doesn't link '@' with
> 'cfg1', and 'cfg1' gets interpreted as a PE_NAME.  Introducing a new
> state upon hitting '@' would get us around that but we are moving away
> from our initial goal of keeping things simple.

hum, then how about keeping the flex atoms simple like for the
other terms and do something like below.. untested ;-)

thanks,
jirka


---
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 1f7e11a6c5b3..8ba228e1c150 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
return token;
 }
 
-static int drv_str(yyscan_t scanner, int token)
-{
-   YYSTYPE *yylval = parse_events_get_lval(scanner);
-   char *text = parse_events_get_text(scanner);
-
-   /* Strip off the '@' */
-   yylval->str = strdup(text + 1);
-   return token;
-}
-
 #define REWIND(__alloc)\
 do {   \
YYSTYPE *__yylval = parse_events_get_lval(yyscanner);   \
@@ -134,7 +124,6 @@ num_hex 0x[a-fA-F0-9]+
 num_raw_hex[a-fA-F0-9]+
 name   [a-zA-Z_*?][a-zA-Z0-9_*?.]*
 name_minus [a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
-drv_cfg_term   [a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
 /* If you add a modifier you need to update check_modifier() */
 modifier_event [ukhpPGHSDI]+
 modifier_bp[rwx]{1,3}
@@ -216,11 +205,11 @@ no-inherit{ return term(yyscanner, 
PARSE_EVENTS__TERM_TYPE_NOINHERIT); }
 overwrite  { return term(yyscanner, 
PARSE_EVENTS__TERM_TYPE_OVERWRITE); }
 no-overwrite   { return term(yyscanner, 
PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
 ,  { return ','; }
+"@"{ return '@'; }
 "/"{ BEGIN(INITIAL); return '/'; }
 {name_minus}   { return str(yyscanner, PE_NAME); }
 \[all\]{ return PE_ARRAY_ALL; }
 "["{ BEGIN(array); return '['; }
-@{drv_cfg_term}{ return drv_str(yyscanner, PE_DRV_CFG_TERM); }
 }
 
 {
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 879115f93edc..7e03e93dabca 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
$$ = term;
 }
 |
-PE_DRV_CFG_TERM
+'@' PE_NAME '=' PE_NAME
 {
struct parse_events_term *term;
 
ABORT_ON(parse_events_term__str(, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
-   $1, $1, &@1, NULL));
+   $2, $4, &@2, &@4));
$$ = term;
 }
 

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Fri, Jul 22, 2016 at 12:24:48PM -0600, Mathieu Poirier wrote:

SNIP

> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
> > $$ = term;
> >  }
> >  |
> > -PE_DRV_CFG_TERM
> > +'@' PE_DRV_CFG_TERM
> >  {
> > struct parse_events_term *term;
> >
> > ABORT_ON(parse_events_term__str(, 
> > PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> > -   $1, $1, &@1, NULL));
> > +   $2, $2, &@2, NULL));
> > $$ = term;
> >  }
> >
> 
> I've been experimenting with the above solution and it is not yielding
> the results one might think at first glance.
> 
> If we use the example: -e event/@cfg1/ ...
> 
> First if we leave things exactly the way they are suggested in the
> code snippet flex doesn't know what do to with the '@' character and
> returns an error.  To deal with that a new clause
> 
> "@"{ return '@'; }
> 
> can be inserted in the config state.  But that doesn't link '@' with
> 'cfg1', and 'cfg1' gets interpreted as a PE_NAME.  Introducing a new
> state upon hitting '@' would get us around that but we are moving away
> from our initial goal of keeping things simple.

hum, then how about keeping the flex atoms simple like for the
other terms and do something like below.. untested ;-)

thanks,
jirka


---
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 1f7e11a6c5b3..8ba228e1c150 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
return token;
 }
 
-static int drv_str(yyscan_t scanner, int token)
-{
-   YYSTYPE *yylval = parse_events_get_lval(scanner);
-   char *text = parse_events_get_text(scanner);
-
-   /* Strip off the '@' */
-   yylval->str = strdup(text + 1);
-   return token;
-}
-
 #define REWIND(__alloc)\
 do {   \
YYSTYPE *__yylval = parse_events_get_lval(yyscanner);   \
@@ -134,7 +124,6 @@ num_hex 0x[a-fA-F0-9]+
 num_raw_hex[a-fA-F0-9]+
 name   [a-zA-Z_*?][a-zA-Z0-9_*?.]*
 name_minus [a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
-drv_cfg_term   [a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
 /* If you add a modifier you need to update check_modifier() */
 modifier_event [ukhpPGHSDI]+
 modifier_bp[rwx]{1,3}
@@ -216,11 +205,11 @@ no-inherit{ return term(yyscanner, 
PARSE_EVENTS__TERM_TYPE_NOINHERIT); }
 overwrite  { return term(yyscanner, 
PARSE_EVENTS__TERM_TYPE_OVERWRITE); }
 no-overwrite   { return term(yyscanner, 
PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
 ,  { return ','; }
+"@"{ return '@'; }
 "/"{ BEGIN(INITIAL); return '/'; }
 {name_minus}   { return str(yyscanner, PE_NAME); }
 \[all\]{ return PE_ARRAY_ALL; }
 "["{ BEGIN(array); return '['; }
-@{drv_cfg_term}{ return drv_str(yyscanner, PE_DRV_CFG_TERM); }
 }
 
 {
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 879115f93edc..7e03e93dabca 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
$$ = term;
 }
 |
-PE_DRV_CFG_TERM
+'@' PE_NAME '=' PE_NAME
 {
struct parse_events_term *term;
 
ABORT_ON(parse_events_term__str(, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
-   $1, $1, &@1, NULL));
+   $2, $4, &@2, &@4));
$$ = term;
 }
 

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 21 July 2016 at 08:54, Jiri Olsa  wrote:
> On Thu, Jul 21, 2016 at 08:44:53AM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> >> diff --git a/tools/perf/util/parse-events.l 
>> >> b/tools/perf/util/parse-events.l
>> >> index 7a2519435da0..1f7e11a6c5b3 100644
>> >> --- a/tools/perf/util/parse-events.l
>> >> +++ b/tools/perf/util/parse-events.l
>> >> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
>> >>   return token;
>> >>  }
>> >>
>> >> +static int drv_str(yyscan_t scanner, int token)
>> >> +{
>> >> + YYSTYPE *yylval = parse_events_get_lval(scanner);
>> >> + char *text = parse_events_get_text(scanner);
>> >> +
>> >> + /* Strip off the '@' */
>> >> + yylval->str = strdup(text + 1);
>> >> + return token;
>> >> +}
>> >
>> > why don't let bison parse this with rule like:
>> >
>> > | '@' PE_DRV_CFG_TERM
>> > {
>> > ...
>> > }
>> >
>> >
>> > you could omit the drv_str function then
>>
>> I simply thought it was simple and clean to do it that way - and it
>> follows the trend already in place.  Are you sure you want me to move
>> this to bison?  Either way we have to add code...
>>
>> Many thanks for the review,
>> Mathieu
>
> hum, i might be missing something, but what I meant
> was something like below
>
> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 1f7e11a6c5b3..9b00f9b9caa2 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
> return token;
>  }
>
> -static int drv_str(yyscan_t scanner, int token)
> -{
> -   YYSTYPE *yylval = parse_events_get_lval(scanner);
> -   char *text = parse_events_get_text(scanner);
> -
> -   /* Strip off the '@' */
> -   yylval->str = strdup(text + 1);
> -   return token;
> -}
> -
>  #define REWIND(__alloc)\
>  do {   \
> YYSTYPE *__yylval = parse_events_get_lval(yyscanner);   \
> @@ -220,7 +210,7 @@ no-overwrite{ return term(yyscanner, 
> PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
>  {name_minus}   { return str(yyscanner, PE_NAME); }
>  \[all\]{ return PE_ARRAY_ALL; }
>  "["{ BEGIN(array); return '['; }
> -@{drv_cfg_term}{ return drv_str(yyscanner, PE_DRV_CFG_TERM); 
> }
> +{drv_cfg_term} { return PE_DRV_CFG_TERM; }
>  }
>
>  {
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
> $$ = term;
>  }
>  |
> -PE_DRV_CFG_TERM
> +'@' PE_DRV_CFG_TERM
>  {
> struct parse_events_term *term;
>
> ABORT_ON(parse_events_term__str(, 
> PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> -   $1, $1, &@1, NULL));
> +   $2, $2, &@2, NULL));
> $$ = term;
>  }
>

I've been experimenting with the above solution and it is not yielding
the results one might think at first glance.

If we use the example: -e event/@cfg1/ ...

First if we leave things exactly the way they are suggested in the
code snippet flex doesn't know what do to with the '@' character and
returns an error.  To deal with that a new clause

"@"{ return '@'; }

can be inserted in the config state.  But that doesn't link '@' with
'cfg1', and 'cfg1' gets interpreted as a PE_NAME.  Introducing a new
state upon hitting '@' would get us around that but we are moving away
from our initial goal of keeping things simple.

Another solution is to have something like this:

drv_cfg_term@[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?

But then we need to strip off the '@' character in the parse-events.y
file, which isn't pretty given that it can be done so easily with the
solution I had before.

So I'm a little puzzled on how to proceed here, but I'm well aware
that my flex/bison knowledge might also be too shallow...

Thanks,
Mathieu

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 21 July 2016 at 08:54, Jiri Olsa  wrote:
> On Thu, Jul 21, 2016 at 08:44:53AM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> >> diff --git a/tools/perf/util/parse-events.l 
>> >> b/tools/perf/util/parse-events.l
>> >> index 7a2519435da0..1f7e11a6c5b3 100644
>> >> --- a/tools/perf/util/parse-events.l
>> >> +++ b/tools/perf/util/parse-events.l
>> >> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
>> >>   return token;
>> >>  }
>> >>
>> >> +static int drv_str(yyscan_t scanner, int token)
>> >> +{
>> >> + YYSTYPE *yylval = parse_events_get_lval(scanner);
>> >> + char *text = parse_events_get_text(scanner);
>> >> +
>> >> + /* Strip off the '@' */
>> >> + yylval->str = strdup(text + 1);
>> >> + return token;
>> >> +}
>> >
>> > why don't let bison parse this with rule like:
>> >
>> > | '@' PE_DRV_CFG_TERM
>> > {
>> > ...
>> > }
>> >
>> >
>> > you could omit the drv_str function then
>>
>> I simply thought it was simple and clean to do it that way - and it
>> follows the trend already in place.  Are you sure you want me to move
>> this to bison?  Either way we have to add code...
>>
>> Many thanks for the review,
>> Mathieu
>
> hum, i might be missing something, but what I meant
> was something like below
>
> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 1f7e11a6c5b3..9b00f9b9caa2 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
> return token;
>  }
>
> -static int drv_str(yyscan_t scanner, int token)
> -{
> -   YYSTYPE *yylval = parse_events_get_lval(scanner);
> -   char *text = parse_events_get_text(scanner);
> -
> -   /* Strip off the '@' */
> -   yylval->str = strdup(text + 1);
> -   return token;
> -}
> -
>  #define REWIND(__alloc)\
>  do {   \
> YYSTYPE *__yylval = parse_events_get_lval(yyscanner);   \
> @@ -220,7 +210,7 @@ no-overwrite{ return term(yyscanner, 
> PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
>  {name_minus}   { return str(yyscanner, PE_NAME); }
>  \[all\]{ return PE_ARRAY_ALL; }
>  "["{ BEGIN(array); return '['; }
> -@{drv_cfg_term}{ return drv_str(yyscanner, PE_DRV_CFG_TERM); 
> }
> +{drv_cfg_term} { return PE_DRV_CFG_TERM; }
>  }
>
>  {
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
> $$ = term;
>  }
>  |
> -PE_DRV_CFG_TERM
> +'@' PE_DRV_CFG_TERM
>  {
> struct parse_events_term *term;
>
> ABORT_ON(parse_events_term__str(, 
> PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> -   $1, $1, &@1, NULL));
> +   $2, $2, &@2, NULL));
> $$ = term;
>  }
>

I've been experimenting with the above solution and it is not yielding
the results one might think at first glance.

If we use the example: -e event/@cfg1/ ...

First if we leave things exactly the way they are suggested in the
code snippet flex doesn't know what do to with the '@' character and
returns an error.  To deal with that a new clause

"@"{ return '@'; }

can be inserted in the config state.  But that doesn't link '@' with
'cfg1', and 'cfg1' gets interpreted as a PE_NAME.  Introducing a new
state upon hitting '@' would get us around that but we are moving away
from our initial goal of keeping things simple.

Another solution is to have something like this:

drv_cfg_term@[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?

But then we need to strip off the '@' character in the parse-events.y
file, which isn't pretty given that it can be done so easily with the
solution I had before.

So I'm a little puzzled on how to proceed here, but I'm well aware
that my flex/bison knowledge might also be too shallow...

Thanks,
Mathieu

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 21 July 2016 at 08:54, Jiri Olsa  wrote:
> On Thu, Jul 21, 2016 at 08:44:53AM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> >> diff --git a/tools/perf/util/parse-events.l 
>> >> b/tools/perf/util/parse-events.l
>> >> index 7a2519435da0..1f7e11a6c5b3 100644
>> >> --- a/tools/perf/util/parse-events.l
>> >> +++ b/tools/perf/util/parse-events.l
>> >> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
>> >>   return token;
>> >>  }
>> >>
>> >> +static int drv_str(yyscan_t scanner, int token)
>> >> +{
>> >> + YYSTYPE *yylval = parse_events_get_lval(scanner);
>> >> + char *text = parse_events_get_text(scanner);
>> >> +
>> >> + /* Strip off the '@' */
>> >> + yylval->str = strdup(text + 1);
>> >> + return token;
>> >> +}
>> >
>> > why don't let bison parse this with rule like:
>> >
>> > | '@' PE_DRV_CFG_TERM
>> > {
>> > ...
>> > }
>> >
>> >
>> > you could omit the drv_str function then
>>
>> I simply thought it was simple and clean to do it that way - and it
>> follows the trend already in place.  Are you sure you want me to move
>> this to bison?  Either way we have to add code...
>>
>> Many thanks for the review,
>> Mathieu
>
> hum, i might be missing something, but what I meant
> was something like below
>
> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 1f7e11a6c5b3..9b00f9b9caa2 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
> return token;
>  }
>
> -static int drv_str(yyscan_t scanner, int token)
> -{
> -   YYSTYPE *yylval = parse_events_get_lval(scanner);
> -   char *text = parse_events_get_text(scanner);
> -
> -   /* Strip off the '@' */
> -   yylval->str = strdup(text + 1);
> -   return token;
> -}
> -
>  #define REWIND(__alloc)\
>  do {   \
> YYSTYPE *__yylval = parse_events_get_lval(yyscanner);   \
> @@ -220,7 +210,7 @@ no-overwrite{ return term(yyscanner, 
> PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
>  {name_minus}   { return str(yyscanner, PE_NAME); }
>  \[all\]{ return PE_ARRAY_ALL; }
>  "["{ BEGIN(array); return '['; }
> -@{drv_cfg_term}{ return drv_str(yyscanner, PE_DRV_CFG_TERM); 
> }
> +{drv_cfg_term} { return PE_DRV_CFG_TERM; }
>  }
>
>  {
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 879115f93edc..b7af1b834fda 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
> $$ = term;
>  }
>  |
> -PE_DRV_CFG_TERM
> +'@' PE_DRV_CFG_TERM
>  {
> struct parse_events_term *term;
>
> ABORT_ON(parse_events_term__str(, 
> PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> -   $1, $1, &@1, NULL));
> +   $2, $2, &@2, NULL));
> $$ = term;
>  }
>

Yes, this might just work too.  Let me play around a little to make
sure all the cases are covered.

Thanks,
Mathieu

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 21 July 2016 at 08:54, Jiri Olsa  wrote:
> On Thu, Jul 21, 2016 at 08:44:53AM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> >> diff --git a/tools/perf/util/parse-events.l 
>> >> b/tools/perf/util/parse-events.l
>> >> index 7a2519435da0..1f7e11a6c5b3 100644
>> >> --- a/tools/perf/util/parse-events.l
>> >> +++ b/tools/perf/util/parse-events.l
>> >> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
>> >>   return token;
>> >>  }
>> >>
>> >> +static int drv_str(yyscan_t scanner, int token)
>> >> +{
>> >> + YYSTYPE *yylval = parse_events_get_lval(scanner);
>> >> + char *text = parse_events_get_text(scanner);
>> >> +
>> >> + /* Strip off the '@' */
>> >> + yylval->str = strdup(text + 1);
>> >> + return token;
>> >> +}
>> >
>> > why don't let bison parse this with rule like:
>> >
>> > | '@' PE_DRV_CFG_TERM
>> > {
>> > ...
>> > }
>> >
>> >
>> > you could omit the drv_str function then
>>
>> I simply thought it was simple and clean to do it that way - and it
>> follows the trend already in place.  Are you sure you want me to move
>> this to bison?  Either way we have to add code...
>>
>> Many thanks for the review,
>> Mathieu
>
> hum, i might be missing something, but what I meant
> was something like below
>
> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 1f7e11a6c5b3..9b00f9b9caa2 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
> return token;
>  }
>
> -static int drv_str(yyscan_t scanner, int token)
> -{
> -   YYSTYPE *yylval = parse_events_get_lval(scanner);
> -   char *text = parse_events_get_text(scanner);
> -
> -   /* Strip off the '@' */
> -   yylval->str = strdup(text + 1);
> -   return token;
> -}
> -
>  #define REWIND(__alloc)\
>  do {   \
> YYSTYPE *__yylval = parse_events_get_lval(yyscanner);   \
> @@ -220,7 +210,7 @@ no-overwrite{ return term(yyscanner, 
> PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
>  {name_minus}   { return str(yyscanner, PE_NAME); }
>  \[all\]{ return PE_ARRAY_ALL; }
>  "["{ BEGIN(array); return '['; }
> -@{drv_cfg_term}{ return drv_str(yyscanner, PE_DRV_CFG_TERM); 
> }
> +{drv_cfg_term} { return PE_DRV_CFG_TERM; }
>  }
>
>  {
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 879115f93edc..b7af1b834fda 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
> $$ = term;
>  }
>  |
> -PE_DRV_CFG_TERM
> +'@' PE_DRV_CFG_TERM
>  {
> struct parse_events_term *term;
>
> ABORT_ON(parse_events_term__str(, 
> PARSE_EVENTS__TERM_TYPE_DRV_CFG,
> -   $1, $1, &@1, NULL));
> +   $2, $2, &@2, NULL));
> $$ = term;
>  }
>

Yes, this might just work too.  Let me play around a little to make
sure all the cases are covered.

Thanks,
Mathieu

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Thu, Jul 21, 2016 at 08:44:53AM -0600, Mathieu Poirier wrote:

SNIP

> >> diff --git a/tools/perf/util/parse-events.l 
> >> b/tools/perf/util/parse-events.l
> >> index 7a2519435da0..1f7e11a6c5b3 100644
> >> --- a/tools/perf/util/parse-events.l
> >> +++ b/tools/perf/util/parse-events.l
> >> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
> >>   return token;
> >>  }
> >>
> >> +static int drv_str(yyscan_t scanner, int token)
> >> +{
> >> + YYSTYPE *yylval = parse_events_get_lval(scanner);
> >> + char *text = parse_events_get_text(scanner);
> >> +
> >> + /* Strip off the '@' */
> >> + yylval->str = strdup(text + 1);
> >> + return token;
> >> +}
> >
> > why don't let bison parse this with rule like:
> >
> > | '@' PE_DRV_CFG_TERM
> > {
> > ...
> > }
> >
> >
> > you could omit the drv_str function then
> 
> I simply thought it was simple and clean to do it that way - and it
> follows the trend already in place.  Are you sure you want me to move
> this to bison?  Either way we have to add code...
> 
> Many thanks for the review,
> Mathieu

hum, i might be missing something, but what I meant
was something like below

thanks,
jirka


---
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 1f7e11a6c5b3..9b00f9b9caa2 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
return token;
 }
 
-static int drv_str(yyscan_t scanner, int token)
-{
-   YYSTYPE *yylval = parse_events_get_lval(scanner);
-   char *text = parse_events_get_text(scanner);
-
-   /* Strip off the '@' */
-   yylval->str = strdup(text + 1);
-   return token;
-}
-
 #define REWIND(__alloc)\
 do {   \
YYSTYPE *__yylval = parse_events_get_lval(yyscanner);   \
@@ -220,7 +210,7 @@ no-overwrite{ return term(yyscanner, 
PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
 {name_minus}   { return str(yyscanner, PE_NAME); }
 \[all\]{ return PE_ARRAY_ALL; }
 "["{ BEGIN(array); return '['; }
-@{drv_cfg_term}{ return drv_str(yyscanner, PE_DRV_CFG_TERM); }
+{drv_cfg_term} { return PE_DRV_CFG_TERM; }
 }
 
 {
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 879115f93edc..b7af1b834fda 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
$$ = term;
 }
 |
-PE_DRV_CFG_TERM
+'@' PE_DRV_CFG_TERM
 {
struct parse_events_term *term;
 
ABORT_ON(parse_events_term__str(, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
-   $1, $1, &@1, NULL));
+   $2, $2, &@2, NULL));
$$ = term;
 }
 

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Thu, Jul 21, 2016 at 08:44:53AM -0600, Mathieu Poirier wrote:

SNIP

> >> diff --git a/tools/perf/util/parse-events.l 
> >> b/tools/perf/util/parse-events.l
> >> index 7a2519435da0..1f7e11a6c5b3 100644
> >> --- a/tools/perf/util/parse-events.l
> >> +++ b/tools/perf/util/parse-events.l
> >> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
> >>   return token;
> >>  }
> >>
> >> +static int drv_str(yyscan_t scanner, int token)
> >> +{
> >> + YYSTYPE *yylval = parse_events_get_lval(scanner);
> >> + char *text = parse_events_get_text(scanner);
> >> +
> >> + /* Strip off the '@' */
> >> + yylval->str = strdup(text + 1);
> >> + return token;
> >> +}
> >
> > why don't let bison parse this with rule like:
> >
> > | '@' PE_DRV_CFG_TERM
> > {
> > ...
> > }
> >
> >
> > you could omit the drv_str function then
> 
> I simply thought it was simple and clean to do it that way - and it
> follows the trend already in place.  Are you sure you want me to move
> this to bison?  Either way we have to add code...
> 
> Many thanks for the review,
> Mathieu

hum, i might be missing something, but what I meant
was something like below

thanks,
jirka


---
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 1f7e11a6c5b3..9b00f9b9caa2 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -53,16 +53,6 @@ static int str(yyscan_t scanner, int token)
return token;
 }
 
-static int drv_str(yyscan_t scanner, int token)
-{
-   YYSTYPE *yylval = parse_events_get_lval(scanner);
-   char *text = parse_events_get_text(scanner);
-
-   /* Strip off the '@' */
-   yylval->str = strdup(text + 1);
-   return token;
-}
-
 #define REWIND(__alloc)\
 do {   \
YYSTYPE *__yylval = parse_events_get_lval(yyscanner);   \
@@ -220,7 +210,7 @@ no-overwrite{ return term(yyscanner, 
PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
 {name_minus}   { return str(yyscanner, PE_NAME); }
 \[all\]{ return PE_ARRAY_ALL; }
 "["{ BEGIN(array); return '['; }
-@{drv_cfg_term}{ return drv_str(yyscanner, PE_DRV_CFG_TERM); }
+{drv_cfg_term} { return PE_DRV_CFG_TERM; }
 }
 
 {
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 879115f93edc..b7af1b834fda 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -602,12 +602,12 @@ PE_NAME array '=' PE_VALUE
$$ = term;
 }
 |
-PE_DRV_CFG_TERM
+'@' PE_DRV_CFG_TERM
 {
struct parse_events_term *term;
 
ABORT_ON(parse_events_term__str(, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
-   $1, $1, &@1, NULL));
+   $2, $2, &@2, NULL));
$$ = term;
 }
 

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 21 July 2016 at 01:47, Jiri Olsa  wrote:
> On Wed, Jul 20, 2016 at 02:38:17PM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
>> index d1edbf8cc66a..8d09a976fca8 100644
>> --- a/tools/perf/util/parse-events.h
>> +++ b/tools/perf/util/parse-events.h
>> @@ -71,6 +71,7 @@ enum {
>>   PARSE_EVENTS__TERM_TYPE_MAX_STACK,
>>   PARSE_EVENTS__TERM_TYPE_NOOVERWRITE,
>>   PARSE_EVENTS__TERM_TYPE_OVERWRITE,
>> + PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>>   __PARSE_EVENTS__TERM_TYPE_NR,
>>  };
>>
>> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
>> index 7a2519435da0..1f7e11a6c5b3 100644
>> --- a/tools/perf/util/parse-events.l
>> +++ b/tools/perf/util/parse-events.l
>> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
>>   return token;
>>  }
>>
>> +static int drv_str(yyscan_t scanner, int token)
>> +{
>> + YYSTYPE *yylval = parse_events_get_lval(scanner);
>> + char *text = parse_events_get_text(scanner);
>> +
>> + /* Strip off the '@' */
>> + yylval->str = strdup(text + 1);
>> + return token;
>> +}
>
> why don't let bison parse this with rule like:
>
> | '@' PE_DRV_CFG_TERM
> {
> ...
> }
>
>
> you could omit the drv_str function then

I simply thought it was simple and clean to do it that way - and it
follows the trend already in place.  Are you sure you want me to move
this to bison?  Either way we have to add code...

Many thanks for the review,
Mathieu

>
> thanks,
> jirka

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

On 21 July 2016 at 01:47, Jiri Olsa  wrote:
> On Wed, Jul 20, 2016 at 02:38:17PM -0600, Mathieu Poirier wrote:
>
> SNIP
>
>> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
>> index d1edbf8cc66a..8d09a976fca8 100644
>> --- a/tools/perf/util/parse-events.h
>> +++ b/tools/perf/util/parse-events.h
>> @@ -71,6 +71,7 @@ enum {
>>   PARSE_EVENTS__TERM_TYPE_MAX_STACK,
>>   PARSE_EVENTS__TERM_TYPE_NOOVERWRITE,
>>   PARSE_EVENTS__TERM_TYPE_OVERWRITE,
>> + PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>>   __PARSE_EVENTS__TERM_TYPE_NR,
>>  };
>>
>> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
>> index 7a2519435da0..1f7e11a6c5b3 100644
>> --- a/tools/perf/util/parse-events.l
>> +++ b/tools/perf/util/parse-events.l
>> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
>>   return token;
>>  }
>>
>> +static int drv_str(yyscan_t scanner, int token)
>> +{
>> + YYSTYPE *yylval = parse_events_get_lval(scanner);
>> + char *text = parse_events_get_text(scanner);
>> +
>> + /* Strip off the '@' */
>> + yylval->str = strdup(text + 1);
>> + return token;
>> +}
>
> why don't let bison parse this with rule like:
>
> | '@' PE_DRV_CFG_TERM
> {
> ...
> }
>
>
> you could omit the drv_str function then

I simply thought it was simple and clean to do it that way - and it
follows the trend already in place.  Are you sure you want me to move
this to bison?  Either way we have to add code...

Many thanks for the review,
Mathieu

>
> thanks,
> jirka

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Wed, Jul 20, 2016 at 02:38:17PM -0600, Mathieu Poirier wrote:

SNIP

> +static int get_config_terms(struct list_head *head_config,
> + struct list_head *head_terms)
> +{
>   struct parse_events_term *term;
>  
>   list_for_each_entry(term, head_config, list) {
>   switch (term->type_term) {
>   case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
> - ADD_CONFIG_TERM(PERIOD, period, term->val.num);
> + ADD_CONFIG_TERM(PERIOD, period,
> + term->val.num, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
> - ADD_CONFIG_TERM(FREQ, freq, term->val.num);
> + ADD_CONFIG_TERM(FREQ, freq, term->val.num, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_TIME:
> - ADD_CONFIG_TERM(TIME, time, term->val.num);
> + ADD_CONFIG_TERM(TIME, time, term->val.num, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
> - ADD_CONFIG_TERM(CALLGRAPH, callgraph, term->val.str);
> + ADD_CONFIG_TERM(CALLGRAPH, callgraph,
> + term->val.str, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
> - ADD_CONFIG_TERM(STACK_USER, stack_user, term->val.num);
> + ADD_CONFIG_TERM(STACK_USER, stack_user,
> + term->val.num, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_INHERIT:
> - ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 1 : 
> 0);
> + ADD_CONFIG_TERM(INHERIT, inherit,
> + term->val.num ? 1 : 0, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
> - ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 0 : 
> 1);
> + ADD_CONFIG_TERM(INHERIT, inherit,
> + term->val.num ? 0 : 1, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
> - ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
> + ADD_CONFIG_TERM(MAX_STACK, max_stack,
> + term->val.num, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> - ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 
> : 0);
> + ADD_CONFIG_TERM(OVERWRITE, overwrite,
> + term->val.num ? 1 : 0, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> - ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 
> : 1);
> + ADD_CONFIG_TERM(OVERWRITE, overwrite,
> + term->val.num ? 0 : 1, head_terms);
>   break;

I think it'd be better to have all terms altogether,
and handle the 'case PARSE_EVENTS__TERM_TYPE_DRV_CFG:'
in here, rather than in separate function

also ADD_CONFIG_TERM macro could stay local to the
function with no need of adding new head arguments

thanks,
jirka

>   default:
>   break;
> @@ -1142,6 +1158,21 @@ do {   
> \
>   return 0;
>  }
>  
> +static int get_drv_config_terms(struct list_head *head_config,
> + struct list_head *head_terms)
> +{
> + struct parse_events_term *term;
> +
> + list_for_each_entry(term, head_config, list) {
> + if (term->type_term != PARSE_EVENTS__TERM_TYPE_DRV_CFG)
> + continue;
> +
> + ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str, head_terms);
> + }
> +
> + return 0;
> +}

SNIP

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Wed, Jul 20, 2016 at 02:38:17PM -0600, Mathieu Poirier wrote:

SNIP

> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index d1edbf8cc66a..8d09a976fca8 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -71,6 +71,7 @@ enum {
>   PARSE_EVENTS__TERM_TYPE_MAX_STACK,
>   PARSE_EVENTS__TERM_TYPE_NOOVERWRITE,
>   PARSE_EVENTS__TERM_TYPE_OVERWRITE,
> + PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>   __PARSE_EVENTS__TERM_TYPE_NR,
>  };
>  
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 7a2519435da0..1f7e11a6c5b3 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
>   return token;
>  }
>  
> +static int drv_str(yyscan_t scanner, int token)
> +{
> + YYSTYPE *yylval = parse_events_get_lval(scanner);
> + char *text = parse_events_get_text(scanner);
> +
> + /* Strip off the '@' */
> + yylval->str = strdup(text + 1);
> + return token;
> +}

why don't let bison parse this with rule like:

| '@' PE_DRV_CFG_TERM
{
...
}


you could omit the drv_str function then

thanks,
jirka

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Wed, Jul 20, 2016 at 02:38:17PM -0600, Mathieu Poirier wrote:
> This patchset adds PMU driver specific configuration to the parser
> infrastructure by preceding any term with the '@' letter.  As such
> doing something like:
> 
> perf record -e some_event/@cfg1,@cfg2=config/ ...
> 
> will see 'cfg1' and 'cfg2=config' being added to the list of evsel config
> terms.  Token 'cfg1' and 'cfg2=config' are not processed in user space
> and are meant to be interpreted by the PMU driver.

please also update the docs, like 'Documentation/perf-record.txt --event option'

thanks,
jirka

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Wed, Jul 20, 2016 at 02:38:17PM -0600, Mathieu Poirier wrote:

SNIP

> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index d1edbf8cc66a..8d09a976fca8 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -71,6 +71,7 @@ enum {
>   PARSE_EVENTS__TERM_TYPE_MAX_STACK,
>   PARSE_EVENTS__TERM_TYPE_NOOVERWRITE,
>   PARSE_EVENTS__TERM_TYPE_OVERWRITE,
> + PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>   __PARSE_EVENTS__TERM_TYPE_NR,
>  };
>  
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 7a2519435da0..1f7e11a6c5b3 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -53,6 +53,16 @@ static int str(yyscan_t scanner, int token)
>   return token;
>  }
>  
> +static int drv_str(yyscan_t scanner, int token)
> +{
> + YYSTYPE *yylval = parse_events_get_lval(scanner);
> + char *text = parse_events_get_text(scanner);
> +
> + /* Strip off the '@' */
> + yylval->str = strdup(text + 1);
> + return token;
> +}

why don't let bison parse this with rule like:

| '@' PE_DRV_CFG_TERM
{
...
}


you could omit the drv_str function then

thanks,
jirka

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Wed, Jul 20, 2016 at 02:38:17PM -0600, Mathieu Poirier wrote:
> This patchset adds PMU driver specific configuration to the parser
> infrastructure by preceding any term with the '@' letter.  As such
> doing something like:
> 
> perf record -e some_event/@cfg1,@cfg2=config/ ...
> 
> will see 'cfg1' and 'cfg2=config' being added to the list of evsel config
> terms.  Token 'cfg1' and 'cfg2=config' are not processed in user space
> and are meant to be interpreted by the PMU driver.

please also update the docs, like 'Documentation/perf-record.txt --event option'

thanks,
jirka

Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Jiri Olsa]

On Wed, Jul 20, 2016 at 02:38:17PM -0600, Mathieu Poirier wrote:

SNIP

> +static int get_config_terms(struct list_head *head_config,
> + struct list_head *head_terms)
> +{
>   struct parse_events_term *term;
>  
>   list_for_each_entry(term, head_config, list) {
>   switch (term->type_term) {
>   case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
> - ADD_CONFIG_TERM(PERIOD, period, term->val.num);
> + ADD_CONFIG_TERM(PERIOD, period,
> + term->val.num, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
> - ADD_CONFIG_TERM(FREQ, freq, term->val.num);
> + ADD_CONFIG_TERM(FREQ, freq, term->val.num, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_TIME:
> - ADD_CONFIG_TERM(TIME, time, term->val.num);
> + ADD_CONFIG_TERM(TIME, time, term->val.num, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
> - ADD_CONFIG_TERM(CALLGRAPH, callgraph, term->val.str);
> + ADD_CONFIG_TERM(CALLGRAPH, callgraph,
> + term->val.str, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
> - ADD_CONFIG_TERM(STACK_USER, stack_user, term->val.num);
> + ADD_CONFIG_TERM(STACK_USER, stack_user,
> + term->val.num, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_INHERIT:
> - ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 1 : 
> 0);
> + ADD_CONFIG_TERM(INHERIT, inherit,
> + term->val.num ? 1 : 0, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
> - ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 0 : 
> 1);
> + ADD_CONFIG_TERM(INHERIT, inherit,
> + term->val.num ? 0 : 1, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
> - ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
> + ADD_CONFIG_TERM(MAX_STACK, max_stack,
> + term->val.num, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> - ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 
> : 0);
> + ADD_CONFIG_TERM(OVERWRITE, overwrite,
> + term->val.num ? 1 : 0, head_terms);
>   break;
>   case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> - ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 
> : 1);
> + ADD_CONFIG_TERM(OVERWRITE, overwrite,
> + term->val.num ? 0 : 1, head_terms);
>   break;

I think it'd be better to have all terms altogether,
and handle the 'case PARSE_EVENTS__TERM_TYPE_DRV_CFG:'
in here, rather than in separate function

also ADD_CONFIG_TERM macro could stay local to the
function with no need of adding new head arguments

thanks,
jirka

>   default:
>   break;
> @@ -1142,6 +1158,21 @@ do {   
> \
>   return 0;
>  }
>  
> +static int get_drv_config_terms(struct list_head *head_config,
> + struct list_head *head_terms)
> +{
> + struct parse_events_term *term;
> +
> + list_for_each_entry(term, head_config, list) {
> + if (term->type_term != PARSE_EVENTS__TERM_TYPE_DRV_CFG)
> + continue;
> +
> + ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str, head_terms);
> + }
> +
> + return 0;
> +}

SNIP

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

This patchset adds PMU driver specific configuration to the parser
infrastructure by preceding any term with the '@' letter.  As such
doing something like:

perf record -e some_event/@cfg1,@cfg2=config/ ...

will see 'cfg1' and 'cfg2=config' being added to the list of evsel config
terms.  Token 'cfg1' and 'cfg2=config' are not processed in user space
and are meant to be interpreted by the PMU driver.

First the lexer/parser are supplemented with the required definitions to
recognise the driver specific configuration.  From there they are simply
added to the list of event terms.  The bulk of the work is done in
function "parse_events_add_pmu()" where driver config event terms are
added to a new list of driver config terms, which in turn spliced with
the event's new driver configuration list.

Signed-off-by: Mathieu Poirier 
---
 tools/perf/util/evsel.c|  1 +
 tools/perf/util/evsel.h|  4 +++
 tools/perf/util/parse-events.c | 76 +++---
 tools/perf/util/parse-events.h |  1 +
 tools/perf/util/parse-events.l | 12 +++
 tools/perf/util/parse-events.y | 11 ++
 6 files changed, 85 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 8c54df61fe64..b16f1621ce3e 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -231,6 +231,7 @@ void perf_evsel__init(struct perf_evsel *evsel,
evsel->bpf_fd  = -1;
INIT_LIST_HEAD(>node);
INIT_LIST_HEAD(>config_terms);
+   INIT_LIST_HEAD(>drv_config_terms);
perf_evsel__object.init(evsel);
evsel->sample_size = __perf_evsel__sample_size(attr->sample_type);
perf_evsel__calc_id_pos(evsel);
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 8a4a6c9f1480..e25fd5e4c740 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -46,6 +46,7 @@ enum {
PERF_EVSEL__CONFIG_TERM_INHERIT,
PERF_EVSEL__CONFIG_TERM_MAX_STACK,
PERF_EVSEL__CONFIG_TERM_OVERWRITE,
+   PERF_EVSEL__CONFIG_TERM_DRV_CFG,
PERF_EVSEL__CONFIG_TERM_MAX,
 };
 
@@ -57,6 +58,7 @@ struct perf_evsel_config_term {
u64 freq;
booltime;
char*callgraph;
+   char*drv_cfg;
u64 stack_user;
int max_stack;
boolinherit;
@@ -79,6 +81,7 @@ struct perf_evsel_config_term {
  *  PERF_SAMPLE_IDENTIFIER) in a non-sample event i.e. if sample_id_all
  *  is used there is an id sample appended to non-sample events
  * @priv:   And what is in its containing unnamed union are tool specific
+ * @drv_config_terms: List of configurables sent directly to the PMU driver
  */
 struct perf_evsel {
struct list_headnode;
@@ -125,6 +128,7 @@ struct perf_evsel {
char*group_name;
boolcmdline_group_boundary;
struct list_headconfig_terms;
+   struct list_headdrv_config_terms;
int bpf_fd;
 };
 
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 6c913c3914fb..697a21c11fc5 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -303,7 +303,8 @@ static struct perf_evsel *
 __add_event(struct list_head *list, int *idx,
struct perf_event_attr *attr,
char *name, struct cpu_map *cpus,
-   struct list_head *config_terms)
+   struct list_head *config_terms,
+   struct list_head *drv_config_terms)
 {
struct perf_evsel *evsel;
 
@@ -322,6 +323,10 @@ __add_event(struct list_head *list, int *idx,
if (config_terms)
list_splice(config_terms, >config_terms);
 
+   if (drv_config_terms)
+   list_splice(drv_config_terms, >drv_config_terms);
+
+
list_add_tail(>node, list);
return evsel;
 }
@@ -330,7 +335,8 @@ static int add_event(struct list_head *list, int *idx,
 struct perf_event_attr *attr, char *name,
 struct list_head *config_terms)
 {
-   return __add_event(list, idx, attr, name, NULL, config_terms) ? 0 : 
-ENOMEM;
+   return __add_event(list, idx, attr, name,
+  NULL, config_terms, NULL) ? 0 : -ENOMEM;
 }
 
 static int parse_aliases(char *str, const char 
*names[][PERF_EVSEL__MAX_ALIASES], int size)
@@ -904,6 +910,7 @@ static const char 
*config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = {
[PARSE_EVENTS__TERM_TYPE_MAX_STACK] = "max-stack",
[PARSE_EVENTS__TERM_TYPE_OVERWRITE] = "overwrite",
[PARSE_EVENTS__TERM_TYPE_NOOVERWRITE]   = "no-overwrite",
+   [PARSE_EVENTS__TERM_TYPE_DRV_CFG]   = "driver-config",
 };
 
 static bool config_term_shrinked;
@@ -1034,7 +1041,8 @@ static int config_term_pmu(struct perf_event_attr *attr,
   

[PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration

[Mathieu Poirier]

This patchset adds PMU driver specific configuration to the parser
infrastructure by preceding any term with the '@' letter.  As such
doing something like:

perf record -e some_event/@cfg1,@cfg2=config/ ...

will see 'cfg1' and 'cfg2=config' being added to the list of evsel config
terms.  Token 'cfg1' and 'cfg2=config' are not processed in user space
and are meant to be interpreted by the PMU driver.

First the lexer/parser are supplemented with the required definitions to
recognise the driver specific configuration.  From there they are simply
added to the list of event terms.  The bulk of the work is done in
function "parse_events_add_pmu()" where driver config event terms are
added to a new list of driver config terms, which in turn spliced with
the event's new driver configuration list.

Signed-off-by: Mathieu Poirier 
---
 tools/perf/util/evsel.c|  1 +
 tools/perf/util/evsel.h|  4 +++
 tools/perf/util/parse-events.c | 76 +++---
 tools/perf/util/parse-events.h |  1 +
 tools/perf/util/parse-events.l | 12 +++
 tools/perf/util/parse-events.y | 11 ++
 6 files changed, 85 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 8c54df61fe64..b16f1621ce3e 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -231,6 +231,7 @@ void perf_evsel__init(struct perf_evsel *evsel,
evsel->bpf_fd  = -1;
INIT_LIST_HEAD(>node);
INIT_LIST_HEAD(>config_terms);
+   INIT_LIST_HEAD(>drv_config_terms);
perf_evsel__object.init(evsel);
evsel->sample_size = __perf_evsel__sample_size(attr->sample_type);
perf_evsel__calc_id_pos(evsel);
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 8a4a6c9f1480..e25fd5e4c740 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -46,6 +46,7 @@ enum {
PERF_EVSEL__CONFIG_TERM_INHERIT,
PERF_EVSEL__CONFIG_TERM_MAX_STACK,
PERF_EVSEL__CONFIG_TERM_OVERWRITE,
+   PERF_EVSEL__CONFIG_TERM_DRV_CFG,
PERF_EVSEL__CONFIG_TERM_MAX,
 };
 
@@ -57,6 +58,7 @@ struct perf_evsel_config_term {
u64 freq;
booltime;
char*callgraph;
+   char*drv_cfg;
u64 stack_user;
int max_stack;
boolinherit;
@@ -79,6 +81,7 @@ struct perf_evsel_config_term {
  *  PERF_SAMPLE_IDENTIFIER) in a non-sample event i.e. if sample_id_all
  *  is used there is an id sample appended to non-sample events
  * @priv:   And what is in its containing unnamed union are tool specific
+ * @drv_config_terms: List of configurables sent directly to the PMU driver
  */
 struct perf_evsel {
struct list_headnode;
@@ -125,6 +128,7 @@ struct perf_evsel {
char*group_name;
boolcmdline_group_boundary;
struct list_headconfig_terms;
+   struct list_headdrv_config_terms;
int bpf_fd;
 };
 
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 6c913c3914fb..697a21c11fc5 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -303,7 +303,8 @@ static struct perf_evsel *
 __add_event(struct list_head *list, int *idx,
struct perf_event_attr *attr,
char *name, struct cpu_map *cpus,
-   struct list_head *config_terms)
+   struct list_head *config_terms,
+   struct list_head *drv_config_terms)
 {
struct perf_evsel *evsel;
 
@@ -322,6 +323,10 @@ __add_event(struct list_head *list, int *idx,
if (config_terms)
list_splice(config_terms, >config_terms);
 
+   if (drv_config_terms)
+   list_splice(drv_config_terms, >drv_config_terms);
+
+
list_add_tail(>node, list);
return evsel;
 }
@@ -330,7 +335,8 @@ static int add_event(struct list_head *list, int *idx,
 struct perf_event_attr *attr, char *name,
 struct list_head *config_terms)
 {
-   return __add_event(list, idx, attr, name, NULL, config_terms) ? 0 : 
-ENOMEM;
+   return __add_event(list, idx, attr, name,
+  NULL, config_terms, NULL) ? 0 : -ENOMEM;
 }
 
 static int parse_aliases(char *str, const char 
*names[][PERF_EVSEL__MAX_ALIASES], int size)
@@ -904,6 +910,7 @@ static const char 
*config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = {
[PARSE_EVENTS__TERM_TYPE_MAX_STACK] = "max-stack",
[PARSE_EVENTS__TERM_TYPE_OVERWRITE] = "overwrite",
[PARSE_EVENTS__TERM_TYPE_NOOVERWRITE]   = "no-overwrite",
+   [PARSE_EVENTS__TERM_TYPE_DRV_CFG]   = "driver-config",
 };
 
 static bool config_term_shrinked;
@@ -1034,7 +1041,8 @@ static int config_term_pmu(struct perf_event_attr *attr,
   struct