Messages in this thread | | | Date | Fri, 7 Nov 2014 13:56:09 -0500 | From | Steven Rostedt <> | Subject | Re: [RFC][PATCH 12/12 v3] x86/nmi: Perform a safe NMI stack trace on all CPUs |
| |
On Thu, 6 Nov 2014 19:41:55 +0100 Petr Mladek <pmladek@suse.cz> wrote:
> > /* "in progress" flag of arch_trigger_all_cpu_backtrace */ > > static unsigned long backtrace_flag; > > > > +static void print_seq_line(struct nmi_seq_buf *s, int last, int pos) > > I would rename the arguments: > > "last -> first" > "pos -> last" > > or maybe better would be to pass first positon and len.
I switched it to "start" and "end" to not be confused by the last_i that is being passed in.
> > > +{ > > + const char *buf = s->buffer + last; > > + > > + printk("%.*s", (pos - last) + 1, buf); > > +} >
> > > > + /* > > + * Now that all the NMIs have triggered, we can dump out their > > + * back traces safely to the console. > > + */ > > + for_each_cpu(cpu, printtrace_mask) { > > + int last_i = 0; > > + > > + s = &per_cpu(nmi_print_seq, cpu); > > + len = s->seq.len; > > If there is an seq_buf overflow, the len might be size + 1, so we need to do: > > len = min(s->seq.len, s->size); > > Well, we should create a function for this in seq_buf.h. > Alternatively, we might reconsider the overflow state, > use len == size and extra "overflow" flag in the seq_buf struct. > > > > + if (!len) > > + continue; > > + > > + /* Print line by line. */ > > + for (i = 0; i < len; i++) { > > + if (s->buffer[i] == '\n') { > > + print_seq_line(s, last_i, i); > > + last_i = i + 1; > > + } > > + } > > > > + if (last_i < i - 1) { > > IMHO, this should be: > > if (last_i < i) > > because last_i = i + 1. Otherwise, we would ignore state when there is > one character after a new line. For example, imagine the following: > > buffer = "a\nb"; > len = 3; > > it will end with: > > last_i = 2; > i = 3; > > and we still need to print the "b".
Well, we really don't *need* to ;-)
But for correctness sake, I agree, it should be last_i < i.
> > > + print_seq_line(s, last_i, i); > > If I get it correctly, (i == len) here and "printk_seq_line" > print_seq_line() prints the characters including "pos" value. > So, we should call: > > print_seq_line(s, last_i, i - 1)
Right that was wrong. Actually, I think the best answer would be:
print_seq_line(s, last_i, len - 1);
This removes the variable 'i'. Probably should add a comment here too that reminds the reviewer that print_seq_line() prints up to and including the last index.
Note, my current code also has:
len = seq_buf_used(&s->seq);
where we don't need to worry about the semantics of seq_buf internals.
-- Steve
> > > + pr_cont("\n"); > > + } > > + } > > + >
| |