Re: [RFC][PATCH 12/12 v3] x86/nmi: Perform a safe NMI stack trace on all CPUs

Steven Rostedt
2014-11-07T18:56:27Z

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");
> > + }
> > + }
> > +
>