Re: [RFC/PATCH] lib/vsprintf: Add support to store cpumask

[Rasmus Villemoes]

On Tue, Jun 28 2016, Jiri Olsa <jolsa@xxxxxxxxxx> 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.
>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> ---
>  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 */
>  	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) {

As I hinted in the other mail, I think it's better just to put the
fmt[1]=='b' here and not change struct printf_spec.

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

A cpumask is an array of longs. Why is u32-alignment enough for that?
cpumask_copy may end up compiling to a simple "*dst = *src", and even if
this is a memcpy(), the same 4-but-possibly-not-8 byte aligned
pointer is created below in bstr_printf which is then passed on to
pointer() and then bitmap_* which certainly expects an unsigned long*. 

Rasmus