| Message ID | 20190910084707.18380-2-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v6,01/12] tools lib traceevent: Convert remaining %p[fF] users to %p[sS] | expand |
On Tue, Sep 10, 2019 at 11:46:56AM +0300, Sakari Ailus wrote: > There are no in-kernel %p[fF] users left. Convert the traceevent tool, > too, to align with the kernel. > function. The _tep_ argument is the trace event parser context. The _name_ is > -the name of the function, the string is copied internally. The _addr_ is > -the start address of the function. The _mod_ is the kernel module > -the function may be in (NULL for none). > +the name of the function, the string is copied internally. The _addr_ is the > +start address of the function. The _mod_ is the kernel module the function may > +be in (NULL for none). This seems a churn of reformatting. > - if (asprintf(&format, "%%pf: (NO FORMAT FOUND at %llx)\n", addr) < 0) > + if (asprintf(&format, "%%ps: (NO FORMAT FOUND at %llx)\n", > + addr) < 0) Splitting line also seems a churn to me. But this one is up to maintainers. Other than above, looks good to me, thanks!
On Tue, 10 Sep 2019 11:46:56 +0300 Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > There are no in-kernel %p[fF] users left. Convert the traceevent tool, > too, to align with the kernel. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > Cc: Tzvetomir Stoyanov <tstoyanov@vmware.com> > Cc: linux-trace-devel@vger.kernel.org > Cc: Jiri Olsa <jolsa@redhat.com> > Cc: Namhyung Kim <namhyung@kernel.org> > --- > .../Documentation/libtraceevent-func_apis.txt | 10 +++++----- > tools/lib/traceevent/event-parse.c | 7 +++---- > 2 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt b/tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt > index 38bfea30a5f64..f6aca0df2151a 100644 > --- a/tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt > +++ b/tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt > @@ -59,12 +59,12 @@ parser context. > > The _tep_register_function()_ function registers a function name mapped to an > address and (optional) module. This mapping is used in case the function tracer > -or events have "%pF" or "%pS" parameter in its format string. It is common to > -pass in the kallsyms function names with their corresponding addresses with this > +or events have "%pS" parameter in its format string. It is common to pass in > +the kallsyms function names with their corresponding addresses with this > function. The _tep_ argument is the trace event parser context. The _name_ is > -the name of the function, the string is copied internally. The _addr_ is > -the start address of the function. The _mod_ is the kernel module > -the function may be in (NULL for none). > +the name of the function, the string is copied internally. The _addr_ is the > +start address of the function. The _mod_ is the kernel module the function may > +be in (NULL for none). > > The _tep_register_print_string()_ function registers a string by the address > it was stored in the kernel. Some strings internal to the kernel with static > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c > index b36b536a9fcba..1d7927ff32660 100644 > --- a/tools/lib/traceevent/event-parse.c > +++ b/tools/lib/traceevent/event-parse.c > @@ -4335,8 +4335,6 @@ static struct tep_print_arg *make_bprint_args(char *fmt, void *data, int size, s > switch (*ptr) { > case 's': > case 'S': > - case 'f': > - case 'F': This file is used to parse output from older kernels, so remove this hunk. It's not just for the lastest kernel. We must maintain backward compatibility here too. If there use to be a usage of this, then we must keep it until the kernels are no longer used (perhaps 7 years?) > case 'x': > break; > default: > @@ -4455,12 +4453,13 @@ get_bprint_format(void *data, int size > __maybe_unused, > printk = find_printk(tep, addr); > if (!printk) { > - if (asprintf(&format, "%%pf: (NO FORMAT FOUND at > %llx)\n", addr) < 0) > + if (asprintf(&format, "%%ps: (NO FORMAT FOUND at > %llx)\n", > + addr) < 0) Remove the line break. I hate the 80 character limit especially when it makes the code look worse. Like it does here. -- Steve > return NULL; > return format; > } > > - if (asprintf(&format, "%s: %s", "%pf", printk->printk) < 0) > + if (asprintf(&format, "%s: %s", "%ps", printk->printk) < 0) > return NULL; > > return format;
On Tue, 2019-09-10 at 07:18 -0400, Steven Rostedt wrote: > On Tue, 10 Sep 2019 11:46:56 +0300 > Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > > There are no in-kernel %p[fF] users left. Convert the traceevent tool, > > too, to align with the kernel. [] > > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c [] > > @@ -4335,8 +4335,6 @@ static struct tep_print_arg *make_bprint_args(char *fmt, void *data, int size, s > > switch (*ptr) { > > case 's': > > case 'S': > > - case 'f': > > - case 'F': > > This file is used to parse output from older kernels, so remove this hunk. > > It's not just for the lastest kernel. We must maintain backward > compatibility here too. If there use to be a usage of this, then we > must keep it until the kernels are no longer used (perhaps 7 years?) That argues for not using "%pfw" at all for some number of years. Perhaps the '%pfw' should be '%pnfw' for 'name' and 'fwnode'
On Tue, 10 Sep 2019 10:18:44 -0700 Joe Perches <joe@perches.com> wrote: > > It's not just for the lastest kernel. We must maintain backward > > compatibility here too. If there use to be a usage of this, then we > > must keep it until the kernels are no longer used (perhaps 7 years?) > > That argues for not using "%pfw" at all for some number of years. > > Perhaps the '%pfw' should be '%pnfw' for 'name' and 'fwnode' -ENOCOMPREHENSION -- Steve
On Tue, 2019-09-10 at 14:26 -0400, Steven Rostedt wrote: > On Tue, 10 Sep 2019 10:18:44 -0700 > Joe Perches <joe@perches.com> wrote: > > > > It's not just for the lastest kernel. We must maintain backward > > > compatibility here too. If there use to be a usage of this, then we > > > must keep it until the kernels are no longer used (perhaps 7 years?) > > > > That argues for not using "%pfw" at all for some number of years. > > > > Perhaps the '%pfw' should be '%pnfw' for 'name' and 'fwnode' > > -ENOCOMPREHENSION Perhaps you were not copied on the whole series. https://lore.kernel.org/lkml/20190910084707.18380-1-sakari.ailus@linux.intel.com/ As I understand it, Sakair Ailus is proposing to obsolete the current vsprintf "%p[Ff]" extension and replace the usage with a new "%pfw" extension which would emit the name of a pointer to "struct fwnode {}". https://lore.kernel.org/lkml/20190910084707.18380-10-sakari.ailus@linux.intel.com/ If reusing "%pf<foo>" is a problem, then instead it might be reasonable to have a new "%pn<foo>" for that use instead. btw: Is there kernel version information available in trace output files? If so, it might be reasonable to change the tooling there instead.
On Tue, 10 Sep 2019 11:42:06 -0700 Joe Perches <joe@perches.com> wrote: > On Tue, 2019-09-10 at 14:26 -0400, Steven Rostedt wrote: > > On Tue, 10 Sep 2019 10:18:44 -0700 > > Joe Perches <joe@perches.com> wrote: > > > > > > It's not just for the lastest kernel. We must maintain backward > > > > compatibility here too. If there use to be a usage of this, then we > > > > must keep it until the kernels are no longer used (perhaps 7 years?) > > > > > > That argues for not using "%pfw" at all for some number of years. > > > > > > Perhaps the '%pfw' should be '%pnfw' for 'name' and 'fwnode' > > > > -ENOCOMPREHENSION > > Perhaps you were not copied on the whole series. > > https://lore.kernel.org/lkml/20190910084707.18380-1-sakari.ailus@linux.intel.com/ Thanks for the link. > > As I understand it, Sakair Ailus is proposing to > obsolete the current vsprintf "%p[Ff]" extension > and replace the usage with a new "%pfw" extension > which would emit the name of a pointer to "struct fwnode {}". > > https://lore.kernel.org/lkml/20190910084707.18380-10-sakari.ailus@linux.intel.com/ > > If reusing "%pf<foo>" is a problem, then instead > it might be reasonable to have a new "%pn<foo>" for > that use instead. > > btw: > > Is there kernel version information available in > trace output files? Not really. This is just a library that parses the trace event formats, there's not kernel versions passed in, but we do use variations in formats and such to determine what is supported. > > If so, it might be reasonable to change the tooling > there instead. > Actually, I think we could just look to see if "%pfw" is used and fall to that, otherwise consider it an older kernel and do it the original way. -- Steve
On Tue, 2019-09-10 at 15:03 -0400, Steven Rostedt wrote: > On Tue, 10 Sep 2019 11:42:06 -0700 [] > > btw: > > > > Is there kernel version information available in > > trace output files? > > Not really. This is just a library that parses the trace event formats, > there's not kernel versions passed in, but we do use variations in > formats and such to determine what is supported. > > > If so, it might be reasonable to change the tooling > > there instead. > > > > Actually, I think we could just look to see if "%pfw" is used and fall > to that, otherwise consider it an older kernel and do it the original > way. Well, if you think that works, OK great. But could that work? How would an individual trace record know if another trace record used %pfw? Perhaps not reusing %pf, marking it reserved for a period of years, and using another unused prefix %p<type> like %pnfw may be simpler.
Hi Joe, Steven, On Tue, Sep 10, 2019 at 12:44:03PM -0700, Joe Perches wrote: > On Tue, 2019-09-10 at 15:03 -0400, Steven Rostedt wrote: > > On Tue, 10 Sep 2019 11:42:06 -0700 > [] > > > btw: > > > > > > Is there kernel version information available in > > > trace output files? > > > > Not really. This is just a library that parses the trace event formats, > > there's not kernel versions passed in, but we do use variations in > > formats and such to determine what is supported. > > > > > If so, it might be reasonable to change the tooling > > > there instead. > > > > > > > Actually, I think we could just look to see if "%pfw" is used and fall > > to that, otherwise consider it an older kernel and do it the original > > way. > > Well, if you think that works, OK great. > > But could that work? > How would an individual trace record know if > another trace record used %pfw? > > Perhaps not reusing %pf, marking it reserved > for a period of years, and using another unused > prefix %p<type> like %pnfw may be simpler. %p[Ff]w does not exist (I grepped for it) in older kernels since v3.0. So kernel support for %p[fF] and %pfw are mutually exclusive. If you're ok with that, I could change the patch to check %pf isn't followed by 'w', in order to support %pf on older kernels. Although that still does not address using older tooling on newer kernels with support for %pfw. If you think that's an issue, I'll opt for another extension than %pfw, which I chose originally since it's memorable --- fw for fwnode (names, paths, and probably more in the future).
On Mon, 16 Sep 2019 14:41:59 +0300 Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Well, if you think that works, OK great. > > > > But could that work? > > How would an individual trace record know if > > another trace record used %pfw? > > > > Perhaps not reusing %pf, marking it reserved > > for a period of years, and using another unused > > prefix %p<type> like %pnfw may be simpler. > > %p[Ff]w does not exist (I grepped for it) in older kernels since v3.0. So > kernel support for %p[fF] and %pfw are mutually exclusive. If you're ok > with that, I could change the patch to check %pf isn't followed by 'w', > in order to support %pf on older kernels. I think that's what I suggested to do. > > Although that still does not address using older tooling on newer kernels > with support for %pfw. That should be fine. I don't think it will crash those tools, they will just give out wrong information, and if people complain, we can try to get them to use the newer version of those tools ;-) (hopefully they don't complain to Linus). > > If you think that's an issue, I'll opt for another extension than %pfw, > which I chose originally since it's memorable --- fw for fwnode (names, > paths, and probably more in the future). > I'm fine with the switch, as long as newer tools know how to handle it. Make sure we also add a comment in the Linux kernel code that states that older kernels use to have 'f' and 'F' and that new tools look for 'fw' to denote that this isn't an older kernel. This way, people will hopefully not add another 'fX' pointer name. -- Steve
Hi Steven, On Mon, Sep 16, 2019 at 10:37:55AM -0400, Steven Rostedt wrote: > > If you think that's an issue, I'll opt for another extension than %pfw, > > which I chose originally since it's memorable --- fw for fwnode (names, > > paths, and probably more in the future). > > > > I'm fine with the switch, as long as newer tools know how to handle it. > > Make sure we also add a comment in the Linux kernel code that states > that older kernels use to have 'f' and 'F' and that new tools look for > 'fw' to denote that this isn't an older kernel. This way, people will > hopefully not add another 'fX' pointer name. Good point. I'll add a comment on this to make_bprint_args() in tools/lib/traceevent/event-parse.c as well as in vsprintf.c.
diff --git a/tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt b/tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt index 38bfea30a5f64..f6aca0df2151a 100644 --- a/tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt +++ b/tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt @@ -59,12 +59,12 @@ parser context. The _tep_register_function()_ function registers a function name mapped to an address and (optional) module. This mapping is used in case the function tracer -or events have "%pF" or "%pS" parameter in its format string. It is common to -pass in the kallsyms function names with their corresponding addresses with this +or events have "%pS" parameter in its format string. It is common to pass in +the kallsyms function names with their corresponding addresses with this function. The _tep_ argument is the trace event parser context. The _name_ is -the name of the function, the string is copied internally. The _addr_ is -the start address of the function. The _mod_ is the kernel module -the function may be in (NULL for none). +the name of the function, the string is copied internally. The _addr_ is the +start address of the function. The _mod_ is the kernel module the function may +be in (NULL for none). The _tep_register_print_string()_ function registers a string by the address it was stored in the kernel. Some strings internal to the kernel with static diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c index b36b536a9fcba..1d7927ff32660 100644 --- a/tools/lib/traceevent/event-parse.c +++ b/tools/lib/traceevent/event-parse.c @@ -4335,8 +4335,6 @@ static struct tep_print_arg *make_bprint_args(char *fmt, void *data, int size, s switch (*ptr) { case 's': case 'S': - case 'f': - case 'F': case 'x': break; default: @@ -4455,12 +4453,13 @@ get_bprint_format(void *data, int size __maybe_unused, printk = find_printk(tep, addr); if (!printk) { - if (asprintf(&format, "%%pf: (NO FORMAT FOUND at %llx)\n", addr) < 0) + if (asprintf(&format, "%%ps: (NO FORMAT FOUND at %llx)\n", + addr) < 0) return NULL; return format; } - if (asprintf(&format, "%s: %s", "%pf", printk->printk) < 0) + if (asprintf(&format, "%s: %s", "%ps", printk->printk) < 0) return NULL; return format;
There are no in-kernel %p[fF] users left. Convert the traceevent tool, too, to align with the kernel. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Tzvetomir Stoyanov <tstoyanov@vmware.com> Cc: linux-trace-devel@vger.kernel.org Cc: Jiri Olsa <jolsa@redhat.com> Cc: Namhyung Kim <namhyung@kernel.org> --- .../Documentation/libtraceevent-func_apis.txt | 10 +++++----- tools/lib/traceevent/event-parse.c | 7 +++---- 2 files changed, 8 insertions(+), 9 deletions(-)