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

[Petr Mladek]

On Fri 2014-11-07 13:56:09, Steven Rostedt wrote:
> On Thu, 6 Nov 2014 19:41:55 +0100
> Petr Mladek <pmladek@xxxxxxx> 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.

I like it.

> > 
> > > +{
> > > +	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.

I agree that one more character does not make much difference but
it might save someones day :-)

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

Yup

> 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.

Yes, the comment is worth having.

> 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.

Perfect

Thanks a lot for working on it. Please, resend this patch once you are
happy with it.

Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/