| Message ID | 20210812085929.54832-6-y.karadz@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | libtraceevent: Optimize the print of tep fields | expand |
On Thu, 12 Aug 2021 11:59:29 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > The new method can print only a subset of the unique data fields of > the trace event. The print format is derived from the parsing tokens > (tep_print_parse objects) of the event. > > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > src/event-parse.c | 26 ++++++++++++++++++++++---- > src/event-parse.h | 3 +++ > 2 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/src/event-parse.c b/src/event-parse.c > index 0795135..645506e 100644 > --- a/src/event-parse.c > +++ b/src/event-parse.c > @@ -5455,20 +5455,38 @@ void tep_print_field(struct trace_seq *s, void *data, > _tep_print_field(s, data, field, NULL); > } > > -void tep_print_fields(struct trace_seq *s, void *data, > - int size __maybe_unused, struct tep_event *event) > +static inline void > +print_selected_fields(struct trace_seq *s, void *data, > + struct tep_event *event, > + unsigned long long ignore_mask) > { > struct tep_print_parse *parse = event->print_fmt.print_cache; > struct tep_format_field *field; > + unsigned long long field_mask = 1; > > field = event->format.fields; > - while (field) { > + for (;field; field = field->next, field_mask *= 2) { The above should be: for(; field; field = field->next, field_mask <<= 1) { As a shift should be done with a shift operator and not a multiplication. Other than that, the rest looks good. I'll go ahead and pull in patches 1 and 2. -- Steve > + if (field_mask & ignore_mask) > + continue; > + > trace_seq_printf(s, " %s=", field->name); > _tep_print_field(s, data, field, &parse); > - field = field->next; > } > } > > +void tep_print_selected_fields(struct trace_seq *s, void *data, As the above is an API, it needs a kernel doc type comment, and also an addition to the man pages. The man page may be a separate patch. -- Steve > + struct tep_event *event, > + unsigned long long ignore_mask) > +{ > + print_selected_fields(s, data, event, ignore_mask); > +} > + > +void tep_print_fields(struct trace_seq *s, void *data, > + int size __maybe_unused, struct tep_event *event) > +{ > + print_selected_fields(s, data, event, 0); > +} > + > static int print_function(struct trace_seq *s, const char *format, > void *data, int size, struct tep_event *event, > struct tep_print_arg *arg) > diff --git a/src/event-parse.h b/src/event-parse.h > index d4a876f..e3638cf 100644 > --- a/src/event-parse.h > +++ b/src/event-parse.h > @@ -545,6 +545,9 @@ int tep_cmdline_pid(struct tep_handle *tep, struct tep_cmdline *cmdline); > > void tep_print_field(struct trace_seq *s, void *data, > struct tep_format_field *field); > +void tep_print_selected_fields(struct trace_seq *s, void *data, > + struct tep_event *event, > + unsigned long long ignore_mask); > void tep_print_fields(struct trace_seq *s, void *data, > int size __maybe_unused, struct tep_event *event); > int tep_strerror(struct tep_handle *tep, enum tep_errno errnum,
On 19.08.21 г. 19:55, Steven Rostedt wrote: > On Thu, 12 Aug 2021 11:59:29 +0300 > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > >> The new method can print only a subset of the unique data fields of >> the trace event. The print format is derived from the parsing tokens >> (tep_print_parse objects) of the event. >> >> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> >> --- >> src/event-parse.c | 26 ++++++++++++++++++++++---- >> src/event-parse.h | 3 +++ >> 2 files changed, 25 insertions(+), 4 deletions(-) >> >> diff --git a/src/event-parse.c b/src/event-parse.c >> index 0795135..645506e 100644 >> --- a/src/event-parse.c >> +++ b/src/event-parse.c >> @@ -5455,20 +5455,38 @@ void tep_print_field(struct trace_seq *s, void *data, >> _tep_print_field(s, data, field, NULL); >> } >> >> -void tep_print_fields(struct trace_seq *s, void *data, >> - int size __maybe_unused, struct tep_event *event) >> +static inline void >> +print_selected_fields(struct trace_seq *s, void *data, >> + struct tep_event *event, >> + unsigned long long ignore_mask) >> { >> struct tep_print_parse *parse = event->print_fmt.print_cache; >> struct tep_format_field *field; >> + unsigned long long field_mask = 1; >> >> field = event->format.fields; >> - while (field) { >> + for (;field; field = field->next, field_mask *= 2) { > > The above should be: > > for(; field; field = field->next, field_mask <<= 1) { > > As a shift should be done with a shift operator and not a > multiplication. > > Other than that, the rest looks good. I'll go ahead and pull in patches > 1 and 2. > > -- Steve > > >> + if (field_mask & ignore_mask) >> + continue; >> + >> trace_seq_printf(s, " %s=", field->name); >> _tep_print_field(s, data, field, &parse); >> - field = field->next; >> } >> } >> >> +void tep_print_selected_fields(struct trace_seq *s, void *data, > > As the above is an API, it needs a kernel doc type comment, and also an > addition to the man pages. The man page may be a separate patch. > I was thinking of maybe changing the second argument of the function to void tep_print_selected_fields(struct trace_seq *s, struct tep_record *record, struct tep_event *event, unsigned long long ignore_mask) > -- Steve > > >> + struct tep_event *event, >> + unsigned long long ignore_mask) >> +{ >> + print_selected_fields(s, data, event, ignore_mask); respectively here we will have print_selected_fields(s, record->data, event, ignore_mask); >> +} This way the call will look cleaner. tep_print_selected_fields(s, record, event, mask); instead of tep_print_selected_fields(s, record->data, event, mask); But on the other hand, this will make the new API inconsistent with the existing "tep_print_fields()" API bellow. What do you think? Thanks! Yordan >> + >> +void tep_print_fields(struct trace_seq *s, void *data, >> + int size __maybe_unused, struct tep_event *event) >> +{ >> + print_selected_fields(s, data, event, 0); >> +} >> + >> static int print_function(struct trace_seq *s, const char *format, >> void *data, int size, struct tep_event *event, >> struct tep_print_arg *arg) >> diff --git a/src/event-parse.h b/src/event-parse.h >> index d4a876f..e3638cf 100644 >> --- a/src/event-parse.h >> +++ b/src/event-parse.h >> @@ -545,6 +545,9 @@ int tep_cmdline_pid(struct tep_handle *tep, struct tep_cmdline *cmdline); >> >> void tep_print_field(struct trace_seq *s, void *data, >> struct tep_format_field *field); >> +void tep_print_selected_fields(struct trace_seq *s, void *data, >> + struct tep_event *event, >> + unsigned long long ignore_mask); >> void tep_print_fields(struct trace_seq *s, void *data, >> int size __maybe_unused, struct tep_event *event); >> int tep_strerror(struct tep_handle *tep, enum tep_errno errnum, >
On Fri, 20 Aug 2021 16:36:12 +0300 Yordan Karadzhov <y.karadz@gmail.com> wrote: if (field_mask & ignore_mask) > >> + continue; > >> + > >> trace_seq_printf(s, " %s=", field->name); > >> _tep_print_field(s, data, field, &parse); > >> - field = field->next; > >> } > >> } > >> > >> +void tep_print_selected_fields(struct trace_seq *s, void *data, > > > > As the above is an API, it needs a kernel doc type comment, and also an > > addition to the man pages. The man page may be a separate patch. > > > > I was thinking of maybe changing the second argument of the function to > > void tep_print_selected_fields(struct trace_seq *s, > struct tep_record *record, > struct tep_event *event, > unsigned long long ignore_mask) > > > > -- Steve > > > > > >> + struct tep_event *event, > >> + unsigned long long ignore_mask) > >> +{ > >> + print_selected_fields(s, data, event, ignore_mask); > > respectively here we will have > > print_selected_fields(s, record->data, event, ignore_mask); > >> +} > > This way the call will look cleaner. > > tep_print_selected_fields(s, record, event, mask); > > instead of > > tep_print_selected_fields(s, record->data, event, mask); > > But on the other hand, this will make the new API inconsistent with the existing > "tep_print_fields()" API bellow. I think we rushed the libtraceevent APIs :-( There's a lot of them I hate, and I agree, passing record would have made more sense. I've been thinking of reworking a lot of them, but we need to add new APIs. tep_print_record_fields() tep_print_record_selected_fields() ?? -- Steve
On 20.08.21 г. 17:35, Steven Rostedt wrote: > On Fri, 20 Aug 2021 16:36:12 +0300 > Yordan Karadzhov <y.karadz@gmail.com> wrote: > if (field_mask & ignore_mask) >>>> + continue; >>>> + >>>> trace_seq_printf(s, " %s=", field->name); >>>> _tep_print_field(s, data, field, &parse); >>>> - field = field->next; >>>> } >>>> } >>>> >>>> +void tep_print_selected_fields(struct trace_seq *s, void *data, >>> >>> As the above is an API, it needs a kernel doc type comment, and also an >>> addition to the man pages. The man page may be a separate patch. >>> >> >> I was thinking of maybe changing the second argument of the function to >> >> void tep_print_selected_fields(struct trace_seq *s, >> struct tep_record *record, >> struct tep_event *event, >> unsigned long long ignore_mask) >> >> >>> -- Steve >>> >>> >>>> + struct tep_event *event, >>>> + unsigned long long ignore_mask) >>>> +{ >>>> + print_selected_fields(s, data, event, ignore_mask); >> >> respectively here we will have >> >> print_selected_fields(s, record->data, event, ignore_mask); >>>> +} >> >> This way the call will look cleaner. >> >> tep_print_selected_fields(s, record, event, mask); >> >> instead of >> >> tep_print_selected_fields(s, record->data, event, mask); >> >> But on the other hand, this will make the new API inconsistent with the existing >> "tep_print_fields()" API bellow. > > I think we rushed the libtraceevent APIs :-( > > There's a lot of them I hate, and I agree, passing record would have made more sense. > > I've been thinking of reworking a lot of them, but we need to add new APIs. > > tep_print_record_fields() > tep_print_record_selected_fields() OK I can add those two. Thanks! Y. > > ?? > > -- Steve >
On Fri, 20 Aug 2021 18:05:37 +0300 Yordan Karadzhov <y.karadz@gmail.com> wrote: > > I've been thinking of reworking a lot of them, but we need to add new APIs. > > > > tep_print_record_fields() > > tep_print_record_selected_fields() > > > OK I can add those two. Actually, let's call it: tep_record_print_fields() tep_record_print_selected_fields() This way we can use 'tep_record_" prefix for functions that affect tep_records. -- Steve
diff --git a/src/event-parse.c b/src/event-parse.c index 0795135..645506e 100644 --- a/src/event-parse.c +++ b/src/event-parse.c @@ -5455,20 +5455,38 @@ void tep_print_field(struct trace_seq *s, void *data, _tep_print_field(s, data, field, NULL); } -void tep_print_fields(struct trace_seq *s, void *data, - int size __maybe_unused, struct tep_event *event) +static inline void +print_selected_fields(struct trace_seq *s, void *data, + struct tep_event *event, + unsigned long long ignore_mask) { struct tep_print_parse *parse = event->print_fmt.print_cache; struct tep_format_field *field; + unsigned long long field_mask = 1; field = event->format.fields; - while (field) { + for (;field; field = field->next, field_mask *= 2) { + if (field_mask & ignore_mask) + continue; + trace_seq_printf(s, " %s=", field->name); _tep_print_field(s, data, field, &parse); - field = field->next; } } +void tep_print_selected_fields(struct trace_seq *s, void *data, + struct tep_event *event, + unsigned long long ignore_mask) +{ + print_selected_fields(s, data, event, ignore_mask); +} + +void tep_print_fields(struct trace_seq *s, void *data, + int size __maybe_unused, struct tep_event *event) +{ + print_selected_fields(s, data, event, 0); +} + static int print_function(struct trace_seq *s, const char *format, void *data, int size, struct tep_event *event, struct tep_print_arg *arg) diff --git a/src/event-parse.h b/src/event-parse.h index d4a876f..e3638cf 100644 --- a/src/event-parse.h +++ b/src/event-parse.h @@ -545,6 +545,9 @@ int tep_cmdline_pid(struct tep_handle *tep, struct tep_cmdline *cmdline); void tep_print_field(struct trace_seq *s, void *data, struct tep_format_field *field); +void tep_print_selected_fields(struct trace_seq *s, void *data, + struct tep_event *event, + unsigned long long ignore_mask); void tep_print_fields(struct trace_seq *s, void *data, int size __maybe_unused, struct tep_event *event); int tep_strerror(struct tep_handle *tep, enum tep_errno errnum,
The new method can print only a subset of the unique data fields of the trace event. The print format is derived from the parsing tokens (tep_print_parse objects) of the event. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- src/event-parse.c | 26 ++++++++++++++++++++++---- src/event-parse.h | 3 +++ 2 files changed, 25 insertions(+), 4 deletions(-)