Re: [RFC PATCH v2 1/3] tracing: Show real address for trace event arguments

[Masami Hiramatsu]

On Thu, 15 Oct 2020 09:09:55 -0400
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:

> On Thu, 15 Oct 2020 18:00:08 +0900
> Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
> 
> > +#define STATIC_FMT_BUF_SIZE	128
> > +static char static_fmt_buf[STATIC_FMT_BUF_SIZE];
> > +
> > +const char *trace_event_format(struct trace_iterator *iter, const char *fmt)
> > +{
> > +	const char *p, *new_fmt;
> > +	char *q;
> > +
> > +	if (WARN_ON_ONCE(!fmt))
> > +		return fmt;
> > +retry:
> > +	p = fmt;
> > +	new_fmt = q = iter->fmt;
> > +	while (*p) {
> > +		if (unlikely(q - new_fmt + 3 > iter->fmt_size)) {
> > +			/* expand format buffer if needed */
> > +			if (iter->fmt == static_fmt_buf)
> > +				return fmt;
> > +
> > +			iter->fmt_size = iter->fmt_size ? iter->fmt_size * 2
> > +							: STATIC_FMT_BUF_SIZE;
> 
> Doubling may be too much, as fmt strings are not going to grow
> exponentially.

OK, then I just make it "+= STATIC_FMT_BUF_SIZE".

Actually, as far as I can see, the format string should be shorter than
128 bytes. If we see it is longer than that, it will be 2 lines in event
definition. And 128 + 128 = 256, which will be longer than 3 lines in 80
columns wide terminal. I guess no one want to write it. :)

> 
> > +			kfree(iter->fmt);
> > +			iter->fmt = kmalloc(iter->fmt_size, GFP_KERNEL);
> 
> Need to test the return value of kmalloc. But I would use realloc instead,
> and continue:

Good catch!

> 
> 			char *tmp_fmt;
> 			unsigned int tmp_size;
> 
> 			tmp_size = iter->fmt_size + STATIC_FMT_BUF_SIZE;
> 
> 			tmp_fmt = krealloc(iter->fmt, tmp_size, GFP_KERNEL);
> 			if (!tmp_fmt)
> 				return fmt;
> 
> 			q += tmp_fmt - new_fmt;
> 			new_fmt = iter->fmt = tmp_fmt;
> 			iter->fmt_size = tmp_size;
> 
> > +			goto retry;
> 
> Then you don't even need the retry, and just continue processing.
> 
> Also, if it fails to allocate, you still have the iter->fmt and fmt_size
> intact and correct.

OK, let me update it.

Thank you,

> 
> -- Steve
> 
> > +		}
> > +		*q++ = *p++;
> > +		/* Replace %p with %px */
> > +		if (p[-1] == '%') {
> > +			if (p[0] == '%') {
> > +				*q++ = *p++;
> > +			} else if (p[0] == 'p' && !isalnum(p[1])) {
> > +				*q++ = *p++;
> > +				*q++ = 'x';
> > +			}
> > +		}
> > +	}
> > +	*q = '\0';
> > +
> > +	return new_fmt;
> > +}
> > +


-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>