| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAD=FV=WUnmMxKZrH-Jfz-VocieOu-Cfsi1aLpB8G5FnLM+_W_w@mail.gmail.com>
Date: Fri, 15 Mar 2019 12:55:31 -0700
From: Doug Anderson <dianders@...omium.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Ingo Molnar <mingo@...hat.com>,
Jason Wessel <jason.wessel@...driver.com>,
Daniel Thompson <daniel.thompson@...aro.org>,
kgdb-bugreport@...ts.sourceforge.net,
Brian Norris <briannorris@...omium.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] tracing: kdb: Allow ftdump to skip all but the last
few lines
Hi,
On Fri, Mar 15, 2019 at 11:41 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Fri, 15 Mar 2019 10:45:28 -0700
> Douglas Anderson <dianders@...omium.org> 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
Powered by blists - more mailing lists