Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1450884 > unrolled thread
| Started by | Jiri Olsa <jolsa@redhat.com> |
|---|---|
| First post | 2016-07-26 22:50 +0200 |
| Last post | 2016-07-28 19:00 +0200 |
| Articles | 5 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration Jiri Olsa <jolsa@redhat.com> - 2016-07-26 22:50 +0200
Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration Mathieu Poirier <mathieu.poirier@linaro.org> - 2016-07-27 20:00 +0200
Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration Jiri Olsa <jolsa@redhat.com> - 2016-07-27 21:30 +0200
Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration Mathieu Poirier <mathieu.poirier@linaro.org> - 2016-07-28 18:20 +0200
Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration Jiri Olsa <jolsa@redhat.com> - 2016-07-28 19:00 +0200
| From | Jiri Olsa <jolsa@redhat.com> |
|---|---|
| Date | 2016-07-26 22:50 +0200 |
| Subject | Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration |
| Message-ID | <rZhsB-6N1-9@gated-at.bofh.it> |
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(&term, 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); }
}
<mem>{
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(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
- $1, $1, &@1, NULL));
+ $2, $4, &@2, &@4));
$$ = term;
}
[toc] | [next] | [standalone]
| From | Mathieu Poirier <mathieu.poirier@linaro.org> |
|---|---|
| Date | 2016-07-27 20:00 +0200 |
| Subject | Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration |
| Message-ID | <rZBhE-2x1-9@gated-at.bofh.it> |
| In reply to | #1450884 |
On 26 July 2016 at 14:41, Jiri Olsa <jolsa@redhat.com> 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(&term, 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); }
> }
>
> <mem>{
> 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(&term, 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
[toc] | [prev] | [next] | [standalone]
| From | Jiri Olsa <jolsa@redhat.com> |
|---|---|
| Date | 2016-07-27 21:30 +0200 |
| Message-ID | <rZCGJ-3Dk-1@gated-at.bofh.it> |
| In reply to | #1451441 |
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(&term, 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
[toc] | [prev] | [next] | [standalone]
| From | Mathieu Poirier <mathieu.poirier@linaro.org> |
|---|---|
| Date | 2016-07-28 18:20 +0200 |
| Subject | Re: [PATCH V2 3/6] perf tools: add infrastructure for PMU specific configuration |
| Message-ID | <rZWcq-ek-11@gated-at.bofh.it> |
| In reply to | #1451480 |
On 27 July 2016 at 13:26, Jiri Olsa <jolsa@redhat.com> 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(&term, 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
[toc] | [prev] | [next] | [standalone]
| From | Jiri Olsa <jolsa@redhat.com> |
|---|---|
| Date | 2016-07-28 19:00 +0200 |
| Message-ID | <rZWP7-s6-3@gated-at.bofh.it> |
| In reply to | #1452002 |
On Thu, Jul 28, 2016 at 10:15:52AM -0600, Mathieu Poirier wrote:
> On 27 July 2016 at 13:26, Jiri Olsa <jolsa@redhat.com> 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(&term, 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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web