Date: Fri, 7 Nov 2014 13:56:09 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Petr Mladek <pmladek@suse.cz>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Jiri Kosina <jkosina@suse.cz>, "H. Peter Anvin" <hpa@zytor.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Subject: Re: [RFC][PATCH 12/12 v3] x86/nmi: Perform a safe NMI stack trace
 on all CPUs
Message-ID: <20141107135609.7ccdd3ce@gandalf.local.home>
In-Reply-To: <20141106184155.GB28294@dhcp128.suse.cz>
References: <20141104155237.228431433@goodmis.org>
 <20141104160223.310714394@goodmis.org>
 <20141106184155.GB28294@dhcp128.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org

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

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