Received-SPF: pass (google.com: domain of linux-nfs+bounces-2984-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Benjamin Coddington <bcodding@redhat.com>
To: Chen Hanxiao <chenhx.fnst@fujitsu.com>
Cc: Trond Myklebust <trond.myklebust@hammerspace.com>,
 Anna Schumaker <anna@kernel.org>, Chuck Lever <chuck.lever@oracle.com>,
 Jeff Layton <jlayton@kernel.org>, Neil Brown <neilb@suse.de>,
 Olga Kornievskaia <kolga@netapp.com>, Dai Ngo <Dai.Ngo@oracle.com>,
 Tom Talpey <tom@talpey.com>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH] SUNRPC: rpc_show_tasks: add an empty list check
Date: Wed, 24 Apr 2024 16:07:06 -0400
Message-ID: <76BBD76E-9CDC-4B97-81E0-72839DF63498@redhat.com>
In-Reply-To: <20240424104112.1053-1-chenhx.fnst@fujitsu.com>
References: <20240424104112.1053-1-chenhx.fnst@fujitsu.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On 24 Apr 2024, at 6:41, Chen Hanxiao wrote:

> add an empty list check, so we can get rid of some useless
> list iterate or spin locks.
>
> Signed-off-by: Chen Hanxiao <chenhx.fnst@fujitsu.com>
> ---
>  net/sunrpc/clnt.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 28f3749f6dc6..749317587bb3 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -3345,8 +3345,13 @@ void rpc_show_tasks(struct net *net)
>  	int header = 0;
>  	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
>
> +	if (list_empty(&sn->all_clients))
> +		return;
> +
>  	spin_lock(&sn->rpc_client_lock);
>  	list_for_each_entry(clnt, &sn->all_clients, cl_clients) {
> +		if (list_empty(&clnt->cl_tasks))
> +			continue;
>  		spin_lock(&clnt->cl_lock);
>  		list_for_each_entry(task, &clnt->cl_tasks, tk_task) {
>  			if (!header) {
> -- 
> 2.39.1


Why optimize this?  Can you show the locks are contended?  Its probably
fine, but using list_empty outside of the lock has a bad smell to me.

Ben