| Message ID | 20240424104112.1053-1-chenhx.fnst@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SUNRPC: rpc_show_tasks: add an empty list check | expand |
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
On Wed, Apr 24, 2024 at 4:07 PM Benjamin Coddington <bcodding@redhat.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 = 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 >
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) {
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(+)