Re: [PATCH v2 06/15] SUNRPC: Trace the rpc_create_args

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2022-06-06 at 10:51 -0400, Chuck Lever wrote:
> Pass the upper layer's rpc_create_args to the rpc_clnt_new()
> tracepoint so additional parts of the upper layer's request can be
> recorded.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  include/trace/events/sunrpc.h |   53 +++++++++++++++++++++++++++++++++--------
>  net/sunrpc/clnt.c             |    2 +-
>  2 files changed, 44 insertions(+), 11 deletions(-)
> 
> diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
> index a66aa1f59ed8..986e135e314f 100644
> --- a/include/trace/events/sunrpc.h
> +++ b/include/trace/events/sunrpc.h
> @@ -139,36 +139,69 @@ DEFINE_RPC_CLNT_EVENT(release);
>  DEFINE_RPC_CLNT_EVENT(replace_xprt);
>  DEFINE_RPC_CLNT_EVENT(replace_xprt_err);
>  
> +TRACE_DEFINE_ENUM(RPC_XPRTSEC_NONE);
> +TRACE_DEFINE_ENUM(RPC_XPRTSEC_TLS_X509);
> +TRACE_DEFINE_ENUM(RPC_XPRTSEC_TLS_PSK);
> +
> +#define rpc_show_xprtsec_policy(policy)					\
> +	__print_symbolic(policy,					\
> +		{ RPC_XPRTSEC_NONE,		"none" },		\
> +		{ RPC_XPRTSEC_TLS_X509,		"tls-x509" },		\
> +		{ RPC_XPRTSEC_TLS_PSK,		"tls-psk" })
> +
> +#define rpc_show_create_flags(flags)					\
> +	__print_flags(flags, "|",					\
> +		{ RPC_CLNT_CREATE_HARDRTRY,	"HARDRTRY" },		\
> +		{ RPC_CLNT_CREATE_AUTOBIND,	"AUTOBIND" },		\
> +		{ RPC_CLNT_CREATE_NONPRIVPORT,	"NONPRIVPORT" },	\
> +		{ RPC_CLNT_CREATE_NOPING,	"NOPING" },		\
> +		{ RPC_CLNT_CREATE_DISCRTRY,	"DISCRTRY" },		\
> +		{ RPC_CLNT_CREATE_QUIET,	"QUIET" },		\
> +		{ RPC_CLNT_CREATE_INFINITE_SLOTS,			\
> +						"INFINITE_SLOTS" },	\
> +		{ RPC_CLNT_CREATE_NO_IDLE_TIMEOUT,			\
> +						"NO_IDLE_TIMEOUT" },	\
> +		{ RPC_CLNT_CREATE_NO_RETRANS_TIMEOUT,			\
> +						"NO_RETRANS_TIMEOUT" },	\
> +		{ RPC_CLNT_CREATE_SOFTERR,	"SOFTERR" },		\
> +		{ RPC_CLNT_CREATE_REUSEPORT,	"REUSEPORT" })
> +
>  TRACE_EVENT(rpc_clnt_new,
>  	TP_PROTO(
>  		const struct rpc_clnt *clnt,
>  		const struct rpc_xprt *xprt,
> -		const char *program,
> -		const char *server
> +		const struct rpc_create_args *args
>  	),
>  
> -	TP_ARGS(clnt, xprt, program, server),
> +	TP_ARGS(clnt, xprt, args),
>  
>  	TP_STRUCT__entry(
>  		__field(unsigned int, client_id)
> +		__field(unsigned long, xprtsec)
> +		__field(unsigned long, flags)
> +		__string(program, clnt->cl_program->name)
> +		__string(server, xprt->servername)
>  		__string(addr, xprt->address_strings[RPC_DISPLAY_ADDR])
>  		__string(port, xprt->address_strings[RPC_DISPLAY_PORT])
> -		__string(program, program)
> -		__string(server, server)
>  	),
>  
>  	TP_fast_assign(
>  		__entry->client_id = clnt->cl_clid;
> +		__entry->xprtsec = args->xprtsec;
> +		__entry->flags = args->flags;
> +		__assign_str(program, clnt->cl_program->name);
> +		__assign_str(server, xprt->servername);
>  		__assign_str(addr, xprt->address_strings[RPC_DISPLAY_ADDR]);
>  		__assign_str(port, xprt->address_strings[RPC_DISPLAY_PORT]);
> -		__assign_str(program, program);
> -		__assign_str(server, server);
>  	),
>  
> -	TP_printk("client=" SUNRPC_TRACE_CLID_SPECIFIER
> -		  " peer=[%s]:%s program=%s server=%s",
> +	TP_printk("client=" SUNRPC_TRACE_CLID_SPECIFIER " peer=[%s]:%s"
> +		" program=%s server=%s xprtsec=%s flags=%s",
>  		__entry->client_id, __get_str(addr), __get_str(port),
> -		__get_str(program), __get_str(server))
> +		__get_str(program), __get_str(server),
> +		rpc_show_xprtsec_policy(__entry->xprtsec),
> +		rpc_show_create_flags(__entry->flags)
> +	)
>  );
>  
>  TRACE_EVENT(rpc_clnt_new_err,
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 6dcc88d45f5d..0ca86c92968f 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -435,7 +435,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args,
>  	if (parent)
>  		refcount_inc(&parent->cl_count);
>  
> -	trace_rpc_clnt_new(clnt, xprt, program->name, args->servername);
> +	trace_rpc_clnt_new(clnt, xprt, args);
>  	return clnt;
>  
>  out_no_path:
> 
> 

All of these tracing patches seem like a reasonable thing to add on
their own, IMO. It might be good to move them and some of the bugfixes
to a separate series and get those merged ahead of the parts that add
TLS support (which are more controversial).

Reviewed-by: Jeff Layton <jlayton@kernel.org>