On Fri, 8 Dec 2023 15:41:01 +0800
Ze Gao <zegao2021@xxxxxxxxx> wrote:
> > diff --git a/plugins/plugin_sched_switch.c b/plugins/plugin_sched_switch.c
> > index e0986ac9cc3d..bde68c4b602d 100644
> > --- a/plugins/plugin_sched_switch.c
> > +++ b/plugins/plugin_sched_switch.c
> > @@ -9,13 +9,136 @@
> > #include "event-parse.h"
> > #include "trace-seq.h"
> >
> > -static void write_state(struct trace_seq *s, int val)
> > +static const char *convert_sym(struct tep_print_flag_sym *sym)
> > {
> > - const char states[] = "SDTtXZPI";
> > + static char save_states[33];
> > +
> > + memset(save_states, 0, sizeof(save_states));
> > +
> > + /* This is the flags for the prev_state_field, now make them into a string */
> > + for (; sym; sym = sym->next) {
> > + long bitmask = strtoul(sym->value, NULL, 0);
> > + int i;
> > +
> > + for (i = 0; !(bitmask & 1); i++)
> > + bitmask >>= 1;
>
> It's no big deal but I think glibc has a faster version of ffs here
> we can directly call that one instead.
I'm use to using ffs in the kernel, but I'm always weary of glibc. No real
reason, just a personal reaction. Anyway, this is far from a fast path if
it's only calculated when hitting a new prev_state field. Although this can
happen often with host/guest tracing.
I figured I'd optimize that in another patch.
>
> > + if (i > 32)
> > + continue; // warn?
> > +
> > + save_states[i] = sym->str[0];
> > + }
> > +
> > + return save_states;
> > +}
> > +
> > +static void write_state(struct trace_seq *s, struct tep_format_field *field,
> > + struct tep_record *record)
> > +{
> > + static struct tep_format_field *prev_state_field;
> > + static const char *states;
> > + unsigned long long val;
> > int found = 0;
> > + int len;
> > int i;
> >
> > - for (i = 0; i < (sizeof(states) - 1); i++) {
> > + if (!field)
> > + return;
> > +
> > + if (!states || field != prev_state_field) {
> > + states = get_states(field);
> > + if (!states)
> > + states = "SDTtXZPI";
> > + printf("states = '%s'\n", states);
>
> printf here introduces some noises when I use perf script to inspect
> the raw events,
> and it always shows this "out-of-nowhere" message in the first
> sched_switch record.
> Still, no big deal, and better without this one.
Bah, that was for debugging. I forgot to nuke it. That's what I get when I
work on user space tooling while compiling the kernel for updates I'm doing
there. :-p
>
> > + prev_state_field = field;
> > + }
> > +
> > + tep_read_number_field(field, record->data, &val);
> > +
> > + len = strlen(states);
> > + for (i = 0; i < len; i++) {
> > if (!(val & (1 << i)))
> > continue;
>
> I think here is another case to consider, the preemption state. it's
> exposed as well IIRC.
> and the kernel denotes this as "R+", maybe we can follow that
> convention by adding
> checks like:
>
> if (!found) {
> trace_seq_putc(s, 'R');
> if (val)
> trace_seq_putc(s, ‘+’);
> }
>
> Just in case, if you were not intentional to make it as it is now.
Sounds good. I'd do that in a separate patch too.
>
> > @@ -99,8 +222,8 @@ static int sched_switch_handler(struct trace_seq *s,
> > if (tep_get_field_val(s, event, "prev_prio", record, &val, 1) == 0)
> > trace_seq_printf(s, "[%d] ", (int) val);
> >
> > - if (tep_get_field_val(s, event, "prev_state", record, &val, 1) == 0)
> > - write_state(s, val);
> > + field = tep_find_any_field(event, "prev_state");
> > + write_state(s, field, record);
> >
> > trace_seq_puts(s, " ==> ");
> >
> > --
> > 2.42.0
> >
>
> Overall, I did the tests on my settings, and the patch worked. It's much more
> robust in finding and validating TEP_PRINT_FLAG than the one I posted and
> This is the first time I know we are capable of doing analyses on multiple data
> records simultaneously. Learned a lot!
>
> Last but the least, I had trouble applying the diff to the branch libtraceevent
> initially but had to copy and paste on my own. Just FYI. Still no big deal :)
I have three patches I have yet to push on top of this one. I'm way behind
in my user space work. As mentioned above, I tend to screw up when I try to
focus both on the kernel and user space libraries (unless I'm working on
both because they are related, but that's not currently the case).
Hopefully this will get done before the end of the year.
Thanks!
-- Steve
|