On Tue, 28 Jun 2016 17:34:34 +0200
Jiri Olsa <jo...@kernel.org> wrote:
> When using trace_printk for cpumask I've got wrong results,
> some bitmaps were completely different from what I expected.
>
> Currently you get wrong results when using trace_printk
> on local cpumask, like:
>
> void test(void)
> {
> struct cpumask mask;
> ...
> trace_printk("mask '%*pbl'\n", cpumask_pr_args(&mask));
> }
>
> The reason is that trace_printk stores the data into binary
> buffer (pointer for cpumask), which is read after via read
> handler of trace/trace_pipe files. At that time pointer for
> local cpumask is no longer valid and you get wrong data.
>
> Fixing this by storing complete cpumask into tracing buffer.
The thing is, this is basically true with all pointer derivatives
(just look at the list of options under pointer()). I probably should
make a trace_printk() that doesn't default to the binary print, to
handle things like this.
trace_printk_ptr()?
Or even just see if I can find a way that detects this in the fmt
string. Hmm, that probably can't be done at compile time :-/
>
> Cc: Steven Rostedt <rost...@goodmis.org>
> Signed-off-by: Jiri Olsa <jo...@kernel.org>
> ---
> lib/vsprintf.c | 41 ++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 0967771d8f7f..f21d68e1b5fc 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -388,7 +388,8 @@ enum format_type {
> struct printf_spec {
> unsigned int type:8; /* format_type enum */
> signed int field_width:24; /* width of output field */
> - unsigned int flags:8; /* flags to number() */
> + unsigned int flags:7; /* flags to number() */
> + unsigned int cpumask:1; /* pointer to cpumask flag */
Why not just add this as another flag? There's one left. I'm not sure
gcc does nice things with bit fields not a multiple of 8.
-- Steve
> unsigned int base:8; /* number base, 8, 10 or 16 only */
> signed int precision:16; /* # of digits/chars */
> } __packed;
> @@ -1864,6 +1865,7 @@ qualifier:
>
> case 'p':
> spec->type = FORMAT_TYPE_PTR;
> + spec->cpumask = fmt[1] == 'b';
> return ++fmt - start;
>
> case '%':
> @@ -2338,7 +2340,23 @@ do {
> \
> }
>
> case FORMAT_TYPE_PTR:
> - save_arg(void *);
> + if (spec.cpumask) {
> + /*
> + * Store entire cpumask directly to buffer
> + * instead of storing just a pointer.
> + */
> + struct cpumask *mask = va_arg(args, void *);
> +
> + str = PTR_ALIGN(str, sizeof(u32));
> +
> + if (str + sizeof(*mask) <= end)
> + cpumask_copy((struct cpumask *) str,
> mask);
> +
> + str += sizeof(*mask);
> + } else {
> + save_arg(void *);
> + }
> +
> /* skip all alphanumeric pointer suffixes */
> while (isalnum(*fmt))
> fmt++;
> @@ -2490,12 +2508,25 @@ int bstr_printf(char *buf, size_t size, const char
> *fmt, const u32 *bin_buf)
> break;
> }
>
> - case FORMAT_TYPE_PTR:
> - str = pointer(fmt, str, end, get_arg(void *), spec);
> + case FORMAT_TYPE_PTR: {
> + void *ptr;
> +
> + if (spec.cpumask) {
> + /*
> + * Load cpumask directly from buffer.
> + */
> + args = PTR_ALIGN(args, sizeof(u32));
> + ptr = (void *) args;
> + args += sizeof(struct cpumask);
> + } else {
> + ptr = get_arg(void *);
> + }
> +
> + str = pointer(fmt, str, end, ptr, spec);
> while (isalnum(*fmt))
> fmt++;
> break;
> -
> + }
> case FORMAT_TYPE_PERCENT_CHAR:
> if (str < end)
> *str = '%';
