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

[Doug Anderson]

Hi,

On Fri, Mar 15, 2019 at 11:41 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Fri, 15 Mar 2019 10:45:28 -0700
> Douglas Anderson <dianders@xxxxxxxxxxxx> wrote:
>
>
> > Changes in v3:
> > - Optimize counting as per Steven Rostedt.
> > - Down to 1 patch since patch #1 from v2 landed.
> >
> >  kernel/trace/trace_kdb.c | 38 +++++++++++++++++++++++++++++---------
> >  1 file changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c
> > index 810d78a8d14c..cc6ca6c0d6de 100644
> > --- a/kernel/trace/trace_kdb.c
> > +++ b/kernel/trace/trace_kdb.c
> > @@ -17,7 +17,7 @@
> >  #include "trace.h"
> >  #include "trace_output.h"
> >
> > -static void ftrace_dump_buf(int skip_lines, long cpu_file)
> > +static int ftrace_dump_buf(int skip_lines, long cpu_file, bool quiet)
>
> Not quite what I wanted you to do.
>
>
> >  {
> >       /* use static because iter can be a bit big for the stack */
> >       static struct trace_iterator iter;
> > @@ -39,8 +39,6 @@ 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");
> > -
> >       /* reset all but tr, trace, and overruns */
> >       memset(&iter.seq, 0,
> >                  sizeof(struct trace_iterator) -
> > @@ -55,6 +53,9 @@ static void ftrace_dump_buf(int skip_lines, long cpu_file)
> >                                                cpu, GFP_ATOMIC);
> >                       ring_buffer_read_start(iter.buffer_iter[cpu]);
> >                       tracing_iter_reset(&iter, cpu);
> > +
> > +                     cnt +=
> > +                     ring_buffer_entries_cpu(iter.trace_buffer->buffer, cpu);
>
> BTW, 80 char limit is a guideline not a rule. Don't break a line to
> make it look worse. But that doesn't matter because this isn't the
> change I meant.

Yeah, I didn't like the look of it either but decided to match the
style of the function, which uses nearly the same solution for word
wrapping a few lines above.  :-)  I'm happy to just break 80
characters though...


> >               }
> >       } else {
> >               iter.cpu_file = cpu_file;
> > @@ -63,13 +64,21 @@ static void ftrace_dump_buf(int skip_lines, long cpu_file)
> >                                                cpu_file, GFP_ATOMIC);
> >               ring_buffer_read_start(iter.buffer_iter[cpu_file]);
> >               tracing_iter_reset(&iter, cpu_file);
> > +
> > +             cnt += ring_buffer_entries_cpu(iter.trace_buffer->buffer,
> > +                                            cpu_file);
> >       }
> >
> > -     while (trace_find_next_entry_inc(&iter)) {
> > -             if (!cnt)
> > -                     kdb_printf("---------------------------------\n");
> > -             cnt++;
> > +     if (quiet)
> > +             goto out;
> > +
> > +     kdb_printf("Dumping ftrace buffer (skipping %d lines):\n",
> > +                skip_lines);
> > +
> > +     if (cnt)
> > +             kdb_printf("---------------------------------\n");
> >
> > +     while (trace_find_next_entry_inc(&iter)) {
> >               if (!skip_lines) {
> >                       print_trace_line(&iter);
> >                       trace_printk_seq(&iter.seq);
> > @@ -99,6 +108,8 @@ static void ftrace_dump_buf(int skip_lines, long cpu_file)
> >                       iter.buffer_iter[cpu] = NULL;
> >               }
> >       }
> > +
> > +     return cnt;
> >  }
> >
> >  /*
> > @@ -109,6 +120,7 @@ static int kdb_ftdump(int argc, const char **argv)
> >       int skip_lines = 0;
> >       long cpu_file;
> >       char *cp;
> > +     int cnt;
> >
> >       if (argc > 2)
> >               return KDB_ARGCOUNT;
> > @@ -129,7 +141,14 @@ static int kdb_ftdump(int argc, const char **argv)
> >       }
> >
> >       kdb_trap_printk++;
> > -     ftrace_dump_buf(skip_lines, cpu_file);
> > +
> > +     /* A negative skip_lines means skip all but the last lines */
> > +     if (skip_lines < 0) {
> > +             cnt = ftrace_dump_buf(0, cpu_file, true);
>
> Actually, you can add a function in trace.c that exposes
> get_total_entries:
>
> unsigned long trace_total_entries(struct trace_array *tr)
> {
>         unsigned long total, entries;
>
>         if (!tr)
>                 tr = &global_trace;
>
>         get_total_entries(tr->trace_buffer, &total, &entries);
>
>         return entries;
> }
>
> and then do:
>                 cnt = trace_total_entries(NULL);

OK.  I guess I'll need to figure out how to add an argument to limit
it to just one CPU too since the kdb command allows you to specify a
single CPU or all CPUs.  I think the best way would mean adding an
argument to get_total_entries().  ...or I can just change the kdb
command to not allow specifying a CPU?  Which do you prefer?  I
personally haven't ever used the feature to just print the ftrace
buffer for a certain CPU so I'd be OK removing it.


> Don't modify ftrace_dump_buf()

I still kinda prefer modifying ftrace_dump_buf() just because it's
pretty important that the "count" we come up with match pretty exactly
the count that ftrace_dump_buf() will come up with.  My worry probably
comes from my lack of experience with the internals of ftrace but, for
instance, I see things like "tr->trace_flags &=
~TRACE_ITER_SYM_USEROBJ" in ftrace_dump_buf() and I worry that it will
affect the count.  ...but if you tell me that I need not worry about
things like that then I won't.  :-)


Thanks for your help!

-Doug