Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever" <cel@kernel.org>
Cc:     linux-nfs@vger.kernel.org, "Chuck Lever" <chuck.lever@oracle.com>,
        lorenzo@kernel.org, jlayton@redhat.com, david@fromorbit.com
Subject: Re: [PATCH v2 0/9] SUNRPC service thread scheduler optimizations
In-reply-to: <168842897573.139194.15893960758088950748.stgit@manet.1015granger.net>
References: <168842897573.139194.15893960758088950748.stgit@manet.1015granger.net>
Date:   Wed, 05 Jul 2023 11:08:32 +1000
Message-id: <168851931219.8939.14382911673248383020@noble.neil.brown.name>
Precedence: bulk


I've been pondering this scheduling mechanism in sunrpc/svc some more,
and I wonder if rather than optimising the search, we should eliminate
it.

Instead we could have a linked list of idle threads using llist.h

svc_enqueue_xprt calls llist_del_first() and if the result is not NULL,
that thread is deemed busy (because it isn't on the list) and is woken.

choose_victim() could also use llist_del_first().  If nothing is there
it could set a flag which gets cleared by the next thread to go idle.
That thread exits ..  or something.  Some interlock would be needed but
it shouldn't be too hard.

svc_exit_thread would have difficulty removing itself from the idle
list, if it wasn't busy..  Possibly we could disallow that case (I think
sending a signal to a thread can make it exit while idle).
Alternately we could use llist_del_all() to clear the list, then wake
them all up so that they go back on the list if there is nothing to do
and if they aren't trying to exit.  That is fairly heavy handed, but
isn't a case that we need to optimise.

If you think that might be worth pursuing, I could have a go at writing
the patch - probably on top of all the cleanups in your series before
the xarray is added.

I also wonder if we should avoid waking too many threads up at once.
If multiple events happen in quick succession, we currently wake up
multiple threads and if there is any scheduling delay (which is expected
based on Commit 22700f3c6df5 ("SUNRPC: Improve ordering of transport processi=
ng"))
then by the time the threads wake up, there may no longer be work to do
as another thread might have gone idle and taken the work.

Instead we could have a limit on the number of threads waking up -
possibly 1 or 3.  If the counter is maxed out, don't do a wake up.
When a thread wakes up, it decrements the counter, dequeues some work,
and if there is more to do, then it queues another task.
I imagine the same basic protocol would be used for creating new threads
when load is high - start just one at a time, though maybe a new thread
would handle a first request before possibly starting another thread.

But this is a stretch goal - not the main focus.

Thanks,
NeilBrown