Re: [PATCH 13/18] tracing: Add array type to function based events

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 9 Feb 2018 10:17:45 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> > + # echo 1 > events/functions/ip_rcv/enable
> > + # cat trace
> > +    <idle>-0     [003] ..s3   219.813582: __netif_receive_skb_core->ip_rcv(skb=ffff880118195e00, perm_addr=b4,b5,2f,ce,18,65)
> > +    <idle>-0     [003] ..s3   219.813595: __netif_receive_skb_core->ip_rcv(skb=ffff880118195e00, perm_addr=b4,b5,2f,ce,18,65)
> > +    <idle>-0     [003] ..s3   220.115053: __netif_receive_skb_core->ip_rcv(skb=ffff880118195c00, perm_addr=b4,b5,2f,ce,18,65)
> > +    <idle>-0     [003] ..s3   220.115293: __netif_receive_skb_core->ip_rcv(skb=ffff880118195c00, perm_addr=b4,b5,2f,ce,18,65)  
> 
> What about adding braces to indicate array type like below?
> 
> ... ip_rcv(skb=ffff880118195c00, perm_addr={b4,b5,2f,ce,18,65})
> 

That's a nice idea, I'll add it.

> > +	case FUNC_STATE_ARRAY:
> >  	case FUNC_STATE_BRACKET:
> > -		WARN_ON(!fevent->last_arg);
> > +		if (WARN_ON(!fevent->last_arg))
> > +			break;
> >  		ret = kstrtoul(token, 0, &val);
> >  		if (ret)
> >  			break;
> > -		val *= fevent->last_arg->size;
> > -		fevent->last_arg->indirect = val ^ INDIRECT_FLAG;
> > -		return FUNC_STATE_INDIRECT;
> > +		if (state == FUNC_STATE_BRACKET) {
> > +			val *= fevent->last_arg->size;
> > +			fevent->last_arg->indirect = val ^ INDIRECT_FLAG;
> > +			return FUNC_STATE_INDIRECT;
> > +		}
> > +		if (val <= 0)
> > +			break;  
> 
> The val is unsigned long type.

I probably should make it a cap it for the array, as arrays that are
too big will simply fail to allocate on the ring buffer.

But it should only check for zero.

> 
> 
> > +		fevent->last_arg->array = val;
> > +		type = kasprintf(GFP_KERNEL, "%s[%d]", fevent->last_arg->type, (unsigned)val);  
> 
> s/%d/%lu/  and no need to cast it.

Sure.

> 
> 
> > +		if (!type)
> > +			break;
> > +		kfree(fevent->last_arg->type);
> > +		fevent->last_arg->type = type;
> > +		/*
> > +		 * arg_offset has already been updated once by size.
> > +		 * This update needs to account for that (hence the "- 1").
> > +		 */
> > +		fevent->arg_offset += fevent->last_arg->size * (fevent->last_arg->array - 1);
> > +		return FUNC_STATE_ARRAY_SIZE;
> > +
> > +	case FUNC_STATE_ARRAY_SIZE:
> > +		if (token[0] != ']')
> > +			break;
> > +		return FUNC_STATE_ARRAY_END;
> >  
> >  	case FUNC_STATE_INDIRECT:
> >  		if (token[0] != ']')
> > @@ -453,6 +485,10 @@ static long long get_arg(struct func_arg *arg, unsigned long val)
> >  
> >  	val = val + (arg->indirect ^ INDIRECT_FLAG);
> >  
> > +	/* Arrays do their own indirect reads */
> > +	if (arg->array)
> > +		return val;
> > +  
> 
> Not sure about this.  After this change it would make 'x64[1] foo' and
> 'x64[1] foo[0]' equivalent, right?

Yeah, I may need to re-think this. I originally had the "array"
use the "indirect" code, but I'm thinking that isn't necessary.

Thanks for the input.

-- Steve