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

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 28 Jun 2016 17:34:34 +0200
Jiri Olsa <jolsa@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 <rostedt@goodmis.org>
> Signed-off-by: Jiri Olsa <jolsa@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 = '%';