Messages in this thread | | | From | Doug Anderson <> | Date | Fri, 15 Mar 2019 14:28:39 -0700 | Subject | Re: [PATCH v3] tracing: kdb: Allow ftdump to skip all but the last few lines |
| |
Hi,
On Fri, Mar 15, 2019 at 2:08 PM Steven Rostedt <rostedt@goodmis.org> 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@chromium.org > > > > 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
| |