On Tue, 15 Aug 2023, Chuck Lever wrote:
> On Wed, Aug 02, 2023 at 05:34:42PM +1000, NeilBrown wrote:
> > Rather than searching a list of threads to find an idle one, having a
> > list of idle threads allows an idle thread to be found immediately.
> >
> > This adds some spin_lock calls which is not ideal, but as the hold-time
> > is tiny it is still faster than searching a list.
>
> Keep in mind that b1691bc03d4e ("sunrpc: convert to lockless lookup
> of queued server threads") did the opposite because that very
> spin_lock was highly contended. I am skeptical of the above claim
> without lock_stat data... but that's sort of moot as this is a
> temporary situation, as you point out next.
The old code did a lot more writes in the spin-locked region than this
code - so more hold-time.
But as you say - we would need data rather than speculation if this were
to be more than an interim state.
>
>
> > A future patch will
> > remove them using llist.h. This involves some subtlety and so is left
> > to a separate patch.
>
> Since I haven't seen that patch yet, I'm reserving judgement about
> whether and how these two changes might be merged.
I'll try to send the remainder of the series today.
>
>
> > This removes the need for the RQ_BUSY flag. The rqst is "busy"
> > precisely when it is not on the "idle" list.
>
> I've been having some trouble with this one. The server system
> deadlocks hard as soon as the NFS server starts. I tracked it down
> this morning: this patch never initialized the sp_idle_threads
> list_head.
Whoops. Looks like I didn't test this particular intermediate state.
>
> I will apply this patch (with one-line fix) and the patch that
> removes SP_CONGESTED once I hear from the client folks on the
> "integrate backchannel" patch.
Thanks,
NeilBrown
