| Message ID | 20191016063920.20791-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/lib/traceevent, perf tools: Handle %pU format correctly | expand |
On 16.10.19 г. 9:39 ч., Qu Wenruo wrote: > [BUG] > For btrfs related events, there is a field for fsid, but perf never > parse it correctly. > > # perf trace -e btrfS:qgroup_meta_convert xfs_io -f -c "pwrite 0 4k" \ > /mnt/btrfs/file1 > 0.000 xfs_io/77915 btrfs:qgroup_meta_reserve:(nil)U: refroot=5(FS_TREE) type=0x0 diff=2 > ^^^^^^ Not a correct UUID > ... > > [CAUSE] > The pretty_print() function doesn't handle the %pU format correctly. > In fact it doesn't handle %pU as uuid at all. > > [FIX] > Add a new functiono, print_uuid_arg(), to handle %pU correctly. > > Now perf trace can at least print fsid correctly: > 0.000 xfs_io/79619 btrfs:qgroup_meta_reserve:23ad1511-dd83-47d4-a79c-e96625a15a6e refroot=5(FS_TREE) type=0x0 diff=2 > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > Please note in above case, the @type and @diff are not properly showed. > That's another problem, will be addressed in later patches. > --- > tools/lib/traceevent/event-parse.c | 38 ++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c > index d948475585ce..4f730ed527b0 100644 > --- a/tools/lib/traceevent/event-parse.c > +++ b/tools/lib/traceevent/event-parse.c > @@ -18,6 +18,7 @@ > #include <errno.h> > #include <stdint.h> > #include <limits.h> > +#include <linux/uuid.h> > #include <linux/time64.h> > > #include <netinet/in.h> > @@ -4508,6 +4509,33 @@ get_bprint_format(void *data, int size __maybe_unused, > return format; > } > > +static void print_uuid_arg(struct trace_seq *s, void *data, int size, > + struct tep_event *event, struct tep_print_arg *arg) > +{ > + const char *fmt; > + unsigned char *buf; > + > + fmt = "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x"; > + if (arg->type != TEP_PRINT_FIELD) { > + trace_seq_printf(s, "ARG TYPE NOT FIELID but %d", arg->type); > + return; > + } > + > + if (!arg->field.field) { > + arg->field.field = tep_find_any_field(event, arg->field.name); > + if (!arg->field.field) { > + do_warning("%s: field %s not found", > + __func__, arg->field.name); > + return; > + } > + } > + buf = data + arg->field.field->offset; > + > + trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5], > + buf[6], buf[7], buf[8], buf[9], buf[10], buf[11], buf[12], > + buf[13], buf[14], buf[15]); > +} > + > static void print_mac_arg(struct trace_seq *s, int mac, void *data, int size, > struct tep_event *event, struct tep_print_arg *arg) > { > @@ -5074,6 +5102,16 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e > arg = arg->next; > break; > } > + } else if (*ptr == 'U') { > + /* Those finetunings are ignored for now */ I think this comment is cryptic. What do you mean by "finetunings"? > + if (isalpha(ptr[1])) > + ptr += 2; > + else > + ptr++; > + > + print_uuid_arg(s, data, size, event, arg); > + arg = arg->next; > + break; > } > > /* fall through */ >
On 2019/10/16 下午3:39, Nikolay Borisov wrote: > > > On 16.10.19 г. 9:39 ч., Qu Wenruo wrote: >> [BUG] >> For btrfs related events, there is a field for fsid, but perf never >> parse it correctly. >> >> # perf trace -e btrfS:qgroup_meta_convert xfs_io -f -c "pwrite 0 4k" \ >> /mnt/btrfs/file1 >> 0.000 xfs_io/77915 btrfs:qgroup_meta_reserve:(nil)U: refroot=5(FS_TREE) type=0x0 diff=2 >> ^^^^^^ Not a correct UUID >> ... >> >> [CAUSE] >> The pretty_print() function doesn't handle the %pU format correctly. >> In fact it doesn't handle %pU as uuid at all. >> >> [FIX] >> Add a new functiono, print_uuid_arg(), to handle %pU correctly. >> >> Now perf trace can at least print fsid correctly: >> 0.000 xfs_io/79619 btrfs:qgroup_meta_reserve:23ad1511-dd83-47d4-a79c-e96625a15a6e refroot=5(FS_TREE) type=0x0 diff=2 >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> Please note in above case, the @type and @diff are not properly showed. >> That's another problem, will be addressed in later patches. >> --- >> tools/lib/traceevent/event-parse.c | 38 ++++++++++++++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> >> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c >> index d948475585ce..4f730ed527b0 100644 >> --- a/tools/lib/traceevent/event-parse.c >> +++ b/tools/lib/traceevent/event-parse.c >> @@ -18,6 +18,7 @@ >> #include <errno.h> >> #include <stdint.h> >> #include <limits.h> >> +#include <linux/uuid.h> >> #include <linux/time64.h> >> >> #include <netinet/in.h> >> @@ -4508,6 +4509,33 @@ get_bprint_format(void *data, int size __maybe_unused, >> return format; >> } >> >> +static void print_uuid_arg(struct trace_seq *s, void *data, int size, >> + struct tep_event *event, struct tep_print_arg *arg) >> +{ >> + const char *fmt; >> + unsigned char *buf; >> + >> + fmt = "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x"; >> + if (arg->type != TEP_PRINT_FIELD) { >> + trace_seq_printf(s, "ARG TYPE NOT FIELID but %d", arg->type); >> + return; >> + } >> + >> + if (!arg->field.field) { >> + arg->field.field = tep_find_any_field(event, arg->field.name); >> + if (!arg->field.field) { >> + do_warning("%s: field %s not found", >> + __func__, arg->field.name); >> + return; >> + } >> + } >> + buf = data + arg->field.field->offset; >> + >> + trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5], >> + buf[6], buf[7], buf[8], buf[9], buf[10], buf[11], buf[12], >> + buf[13], buf[14], buf[15]); >> +} >> + >> static void print_mac_arg(struct trace_seq *s, int mac, void *data, int size, >> struct tep_event *event, struct tep_print_arg *arg) >> { >> @@ -5074,6 +5102,16 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e >> arg = arg->next; >> break; >> } >> + } else if (*ptr == 'U') { >> + /* Those finetunings are ignored for now */ > > I think this comment is cryptic. What do you mean by "finetunings"? Oh, the original planned check is: if (ptr[1] == 'b' || ptr[1] == 'B' || ptr[1] == 'l' || ptr[1] == 'L') Those options are finetunes for endian and upper/lower characters. However I just choose to be lazy and use isalpha() to check them in one line. I will change the comment to make it more clear. Thanks, Qu > >> + if (isalpha(ptr[1])) >> + ptr += 2; >> + else >> + ptr++; >> + >> + print_uuid_arg(s, data, size, event, arg); >> + arg = arg->next; >> + break; >> } >> >> /* fall through */ >>
On Wed, 16 Oct 2019 14:39:20 +0800 Qu Wenruo <wqu@suse.com> wrote: > +static void print_uuid_arg(struct trace_seq *s, void *data, int size, > + struct tep_event *event, struct tep_print_arg *arg) > +{ > + const char *fmt; > + unsigned char *buf; > + > + fmt = "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x"; > + if (arg->type != TEP_PRINT_FIELD) { > + trace_seq_printf(s, "ARG TYPE NOT FIELID but %d", arg->type); > + return; > + } > + > + if (!arg->field.field) { > + arg->field.field = tep_find_any_field(event, arg->field.name); > + if (!arg->field.field) { > + do_warning("%s: field %s not found", > + __func__, arg->field.name); > + return; > + } > + } > + buf = data + arg->field.field->offset; You also need to make sure the data field is not smaller than 16 bytes. if (arg->field.field->size < 16) { trace_seq_puts(s, "INVALIDUUID"); return; } > + > + trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5], > + buf[6], buf[7], buf[8], buf[9], buf[10], buf[11], buf[12], > + buf[13], buf[14], buf[15]); > +} > + Hmm, I know print_mac_addr() does something similar as this, but this is getting a bit extreme (too many arguments!). What about doing: for (i = 0; i < 4; i++) trace_seq_printf(s, "%02x", buf++); for (i = 0; i < 3; i++) trace_seq_printf(s, "-%02x%02x", buf[i * 2], buf[i * 2 + 1]); buf += 6; trace_seq_putc(s, '-'); for (i = 0; i < 6; i++) trace_seq_printf(s, "%02x", buf++); Hmm, not sure the above is better, but having that many arguments just looks ugly to me. -- Steve
On 2019/10/16 下午10:54, Steven Rostedt wrote: > On Wed, 16 Oct 2019 14:39:20 +0800 > Qu Wenruo <wqu@suse.com> wrote: > >> +static void print_uuid_arg(struct trace_seq *s, void *data, int size, >> + struct tep_event *event, struct tep_print_arg *arg) >> +{ >> + const char *fmt; >> + unsigned char *buf; >> + >> + fmt = "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x"; >> + if (arg->type != TEP_PRINT_FIELD) { >> + trace_seq_printf(s, "ARG TYPE NOT FIELID but %d", arg->type); >> + return; >> + } >> + >> + if (!arg->field.field) { >> + arg->field.field = tep_find_any_field(event, arg->field.name); >> + if (!arg->field.field) { >> + do_warning("%s: field %s not found", >> + __func__, arg->field.name); >> + return; >> + } >> + } >> + buf = data + arg->field.field->offset; > > You also need to make sure the data field is not smaller than 16 bytes. > > if (arg->field.field->size < 16) { > trace_seq_puts(s, "INVALIDUUID"); > return; > } > Oh, forgot that sanity check. >> + >> + trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5], >> + buf[6], buf[7], buf[8], buf[9], buf[10], buf[11], buf[12], >> + buf[13], buf[14], buf[15]); >> +} >> + > > Hmm, I know print_mac_addr() does something similar as this, but this > is getting a bit extreme (too many arguments!). What about doing: > > for (i = 0; i < 4; i++) > trace_seq_printf(s, "%02x", buf++); > > for (i = 0; i < 3; i++) > trace_seq_printf(s, "-%02x%02x", buf[i * 2], buf[i * 2 + 1]); > > buf += 6; > > trace_seq_putc(s, '-'); > > for (i = 0; i < 6; i++) > trace_seq_printf(s, "%02x", buf++); > > > > Hmm, not sure the above is better, but having that many arguments just > looks ugly to me. Indeed, I'll update the patchset to make it more sane. Thanks, Qu > > -- Steve >
diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c index d948475585ce..4f730ed527b0 100644 --- a/tools/lib/traceevent/event-parse.c +++ b/tools/lib/traceevent/event-parse.c @@ -18,6 +18,7 @@ #include <errno.h> #include <stdint.h> #include <limits.h> +#include <linux/uuid.h> #include <linux/time64.h> #include <netinet/in.h> @@ -4508,6 +4509,33 @@ get_bprint_format(void *data, int size __maybe_unused, return format; } +static void print_uuid_arg(struct trace_seq *s, void *data, int size, + struct tep_event *event, struct tep_print_arg *arg) +{ + const char *fmt; + unsigned char *buf; + + fmt = "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x"; + if (arg->type != TEP_PRINT_FIELD) { + trace_seq_printf(s, "ARG TYPE NOT FIELID but %d", arg->type); + return; + } + + if (!arg->field.field) { + arg->field.field = tep_find_any_field(event, arg->field.name); + if (!arg->field.field) { + do_warning("%s: field %s not found", + __func__, arg->field.name); + return; + } + } + buf = data + arg->field.field->offset; + + trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5], + buf[6], buf[7], buf[8], buf[9], buf[10], buf[11], buf[12], + buf[13], buf[14], buf[15]); +} + static void print_mac_arg(struct trace_seq *s, int mac, void *data, int size, struct tep_event *event, struct tep_print_arg *arg) { @@ -5074,6 +5102,16 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e arg = arg->next; break; } + } else if (*ptr == 'U') { + /* Those finetunings are ignored for now */ + if (isalpha(ptr[1])) + ptr += 2; + else + ptr++; + + print_uuid_arg(s, data, size, event, arg); + arg = arg->next; + break; } /* fall through */
[BUG] For btrfs related events, there is a field for fsid, but perf never parse it correctly. # perf trace -e btrfS:qgroup_meta_convert xfs_io -f -c "pwrite 0 4k" \ /mnt/btrfs/file1 0.000 xfs_io/77915 btrfs:qgroup_meta_reserve:(nil)U: refroot=5(FS_TREE) type=0x0 diff=2 ^^^^^^ Not a correct UUID ... [CAUSE] The pretty_print() function doesn't handle the %pU format correctly. In fact it doesn't handle %pU as uuid at all. [FIX] Add a new functiono, print_uuid_arg(), to handle %pU correctly. Now perf trace can at least print fsid correctly: 0.000 xfs_io/79619 btrfs:qgroup_meta_reserve:23ad1511-dd83-47d4-a79c-e96625a15a6e refroot=5(FS_TREE) type=0x0 diff=2 Signed-off-by: Qu Wenruo <wqu@suse.com> --- Please note in above case, the @type and @diff are not properly showed. That's another problem, will be addressed in later patches. --- tools/lib/traceevent/event-parse.c | 38 ++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)