Re: [PATCH v3] tracing: kdb: Allow ftdump to skip all but the last few lines

[Doug Anderson]

Hi,

On Fri, Mar 15, 2019 at 2:08 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> > > The get_total_entries() is the faster approach to get the count, but in
> > > either case, the count should end up the same.
> >
> > If you're OK with going back to the super slow mechanism in v1 I can
> > do that and we can be guaranteed we're consistent.  Presumably it
> > can't be _that_ slow because we're going to use the same mechanism to
> > skip the lines later.
> >
> > So, if you agree, I'll send out a v4 that looks like v1 except that it
> > disables / enables tracing directly in kdb_ftdump() so it stays
> > disabled for both calls.
> >
> >
> > [1] https://lkml.kernel.org/r/20190305233150.159633-1-dianders@xxxxxxxxxxxx
> >
>
> But this part of the patch:
>
> > -static void ftrace_dump_buf(int skip_lines, long cpu_file)
> > +static int ftrace_dump_buf(int skip_lines, long cpu_file, bool quiet)
> >  {
> >       /* use static because iter can be a bit big for the stack */
> >       static struct trace_iterator iter;
> > @@ -39,7 +39,9 @@ static void ftrace_dump_buf(int skip_lines, long cpu_file)
> >       /* don't look at user memory in panic mode */
> >       tr->trace_flags &= ~TRACE_ITER_SYM_USEROBJ;
> >
> > -     kdb_printf("Dumping ftrace buffer:\n");
> > +     if (!quiet)
> > +             kdb_printf("Dumping ftrace buffer (skipping %d lines):\n",
> > +                        skip_lines);
> >
> >       /* reset all but tr, trace, and overruns */
> >       memset(&iter.seq, 0,
> > @@ -66,25 +68,29 @@ static void ftrace_dump_buf(int skip_lines, long cpu_file)
> >       }
> >
> >       while (trace_find_next_entry_inc(&iter)) {
> > -             if (!cnt)
> > -                     kdb_printf("---------------------------------\n");
> > -             cnt++;
> > -
> > -             if (!skip_lines) {
> > -                     print_trace_line(&iter);
> > -                     trace_printk_seq(&iter.seq);
> > -             } else {
> > -                     skip_lines--;
> > +             if (!quiet) {
> > +                     if (!cnt)
> > +                             kdb_printf("---------------------------------\n");
> > +
> > +                     if (!skip_lines) {
> > +                             print_trace_line(&iter);
> > +                             trace_printk_seq(&iter.seq);
> > +                     } else {
> > +                             skip_lines--;
>
> How do you know that trace_printk_seq() didn't produce more than one line?
>
> If the event is a stack dump, you need to read the seq, and count the
> number of '\n' that are added.
>
> The cnt in this code is no different than the get_total_entries() that
> I suggested.

I had a little bit of a hard time figuring out if print_trace_line()
plus trace_printk_seq() always printed one line or always printed one
entry.  I guess the point I was making was that in v1 of my patch it
wouldn't matter because the pseudo code looked like:

1. Count how many "things" would be printed, but don't printed them.

2. Use math to figure out how many "things" to skip given that we want
to print the last N "things".

3. Skip the "things" the math told us to and then print the last N "things".

...but it sounds like it _must_ print one entry because we're looping
over trace_find_next_entry_inc().  That means that the existing "skip
lines" that predates my patch should actually be "skip entries".  I'd
be happy adjusting the help text (and local variable name) so that
"skip_lines" is instead "skip_entries".  I think that would avoid
confusion everywhere.  It doesn't change behavior but just documents
the existing behavior.


With that I can use your optimized path assuming you can confirm that
"tr->trace_flags &= ~TRACE_ITER_SYM_USEROBJ" doesn't affect how many
entries will be iterated over by trace_find_next_entry_inc().

-Doug