On Fri, 9 Feb 2018 10:17:45 +0900
Namhyung Kim <[email protected]> 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
