2024-04-24 10:44:09

by Chen Hanxiao

Subject: [PATCH] SUNRPC: rpc_show_tasks: add an empty list check

add an empty list check, so we can get rid of some useless
list iterate or spin locks.

Signed-off-by: Chen Hanxiao <[email protected]>
---
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

2024-04-24 20:16:47

by Benjamin Coddington

Subject: Re: [PATCH] SUNRPC: rpc_show_tasks: add an empty list check

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 <[email protected]>
> ---
> 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

2024-04-26 13:16:53

by Anna Schumaker

Subject: Re: [PATCH] SUNRPC: rpc_show_tasks: add an empty list check

On Wed, Apr 24, 2024 at 4:07 PM Benjamin Coddington <[email protected]> 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 <[email protected]>
> > ---
> > 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
>