Received-SPF: pass (google.com: domain of linux-nfs+bounces-3038-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240424104112.1053-1-chenhx.fnst@fujitsu.com> <76BBD76E-9CDC-4B97-81E0-72839DF63498@redhat.com>
In-Reply-To: <76BBD76E-9CDC-4B97-81E0-72839DF63498@redhat.com>
From: Anna Schumaker <anna@kernel.org>
Date: Fri, 26 Apr 2024 09:16:34 -0400
Message-ID: <CAFX2JfkGsUQS9PEkzmMDiv0jrq0Xt1RqpfzJ1tpnBi2TKEZn7g@mail.gmail.com>
Subject: Re: [PATCH] SUNRPC: rpc_show_tasks: add an empty list check
To: Benjamin Coddington <bcodding@redhat.com>
Cc: Chen Hanxiao <chenhx.fnst@fujitsu.com>, 
	Trond Myklebust <trond.myklebust@hammerspace.com>, 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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 24, 2024 at 4:07=E2=80=AFPM Benjamin Coddington <bcodding@redha=
t.com> wrote:
>
> 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 =3D 0;
> >       struct sunrpc_net *sn =3D 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.

I looked into list_empty(), and it's using READ_ONCE() internally so
it should be okay to use outside of the lock. Having said that, this
function is only used by sunrpc/sysctl.c, so it's not a path I would
think needs to be heavily optimized.

Anna

>
> Ben
>