On Wed, Apr 24, 2024 at 4:07 PM Benjamin Coddington <bcodding@xxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
> > 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.
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
>
