Re: [PATCH] sunrpc: Fix trace events to store data in the struct

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 24 Feb 2015 11:47:56 +0000
James Hogan <james.hogan@imgtec.com> wrote:



>  	TP_printk("xprt=0x%p addr=%pIScp pid=%d flags=%s", __entry->xprt,
> -		(struct sockaddr *)&__entry->xprt->xpt_remote,

There's actually nothing wrong with the above even if xprt is NULL.
It's not dereferencing the structure, it is just getting the address of
what would be dereference.

> -		__entry->rqst ? __entry->rqst->rq_task->pid : 0,
> -		show_svc_xprt_flags(__entry->xprt->xpt_flags))
> +		(struct sockaddr *)&__entry->ss,

The above is meaningless. You just printed the address of the ring
buffer and this will be different (and useless) every time.

> +		__entry->pid,
> +		show_svc_xprt_flags(__entry->flags))
>  );
>  
>  TRACE_EVENT(svc_xprt_dequeue,
> @@ -562,17 +566,21 @@ TRACE_EVENT(svc_handle_xprt,
>  
>  	TP_STRUCT__entry(
>  		__field(struct svc_xprt *, xprt)
> +		__field_struct(struct sockaddr_storage, ss)
> +		__field(unsigned long, flags);
>  		__field(int, len)
>  	),
>  
>  	TP_fast_assign(
>  		__entry->xprt = xprt;
> +		xprt ? memcpy(&__entry->ss, &xprt->xpt_remote, sizeof(__entry->ss)) : memset(&__entry->ss, 0, sizeof(__entry->ss));
> +		__entry->flags = xprt ? xprt->xpt_flags : 0;
>  		__entry->len = len;
>  	),
>  
>  	TP_printk("xprt=0x%p addr=%pIScp len=%d flags=%s", __entry->xprt,
> -		(struct sockaddr *)&__entry->xprt->xpt_remote, __entry->len,
> -		show_svc_xprt_flags(__entry->xprt->xpt_flags))
> +		(struct sockaddr *)&__entry->ss, __entry->len,

Ditto.

Don't use field_struct() unless you really know what you are doing.
This is copying the entire struct into the ring buffer and only using
the address of that struct. Which not only is useless, but wastes a lot
of space in the ring buffer.

-- Steve

> +		show_svc_xprt_flags(__entry->flags))
>  );
>  #endif /* _TRACE_SUNRPC_H */
>