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

From: Doug Anderson
Date: Fri Mar 15 2019 - 17:28:56 EST


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