knfsd performance

knfsd performance

[Trond Myklebust]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

I recently back ported Neil's lwq code and sunrpc server changes to our
5.15.130 based kernel in the hope of improving the performance for our
data servers.

Our performance team recently ran a fio workload on a client that was
doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
(infiniband) against that resulting server. I've attached the resulting
flame graph from a perf profile run on the server side.

Is anyone else seeing this massive contention for the spin lock in
__lwq_dequeue? As you can see, it appears to be dwarfing all the other
nfsd activity on the system in question here, being responsible for 45%
of all the perf hits.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com



[-- Attachment #2: 5.15.130-200.pd.124.el8.x86_64.dsx.svg --]
[-- Type: image/svg+xml, Size: 432471 bytes --]

Re: knfsd performance

[Chuck Lever III]

> On Jun 18, 2024, at 2:32 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> I recently back ported Neil's lwq code and sunrpc server changes to our
> 5.15.130 based kernel in the hope of improving the performance for our
> data servers.
> 
> Our performance team recently ran a fio workload on a client that was
> doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
> (infiniband) against that resulting server. I've attached the resulting
> flame graph from a perf profile run on the server side.
> 
> Is anyone else seeing this massive contention for the spin lock in
> __lwq_dequeue? As you can see, it appears to be dwarfing all the other
> nfsd activity on the system in question here, being responsible for 45%
> of all the perf hits.

I haven't seen that, but I've been working on other issues.

What's the nfsd thread count on your test server? Have you
seen a similar impact on 6.10 kernels ?

--
Chuck Lever

Re: knfsd performance

[Trond Myklebust]

On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
> 
> 
> > On Jun 18, 2024, at 2:32 PM, Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > 
> > I recently back ported Neil's lwq code and sunrpc server changes to
> > our
> > 5.15.130 based kernel in the hope of improving the performance for
> > our
> > data servers.
> > 
> > Our performance team recently ran a fio workload on a client that
> > was
> > doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
> > (infiniband) against that resulting server. I've attached the
> > resulting
> > flame graph from a perf profile run on the server side.
> > 
> > Is anyone else seeing this massive contention for the spin lock in
> > __lwq_dequeue? As you can see, it appears to be dwarfing all the
> > other
> > nfsd activity on the system in question here, being responsible for
> > 45%
> > of all the perf hits.
> 
> I haven't seen that, but I've been working on other issues.
> 
> What's the nfsd thread count on your test server? Have you
> seen a similar impact on 6.10 kernels ?
> 

640 knfsd threads. The machine was a supermicro 2029BT-HNR with 2xIntel
6150, 384GB of memory and 6xWDC SN840.

Unfortunately, the machine was a loaner, so cannot compare to 6.10.
That's why I was asking if anyone has seen anything similar.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: knfsd performance

[Jeff Layton]

On Tue, 2024-06-18 at 18:32 +0000, Trond Myklebust wrote:
> I recently back ported Neil's lwq code and sunrpc server changes to
> our
> 5.15.130 based kernel in the hope of improving the performance for
> our
> data servers.
> 
> Our performance team recently ran a fio workload on a client that was
> doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
> (infiniband) against that resulting server. I've attached the
> resulting
> flame graph from a perf profile run on the server side.
> 
> Is anyone else seeing this massive contention for the spin lock in
> __lwq_dequeue? As you can see, it appears to be dwarfing all the
> other
> nfsd activity on the system in question here, being responsible for
> 45%
> of all the perf hits.
> 
> 

I haven't spent much time on performance testing since I keep getting
involved in bugs. It looks like that's just the way lwq works. From the
comments in lib/lwq.c:

 * Entries are dequeued using a spinlock to protect against multiple
 * access.  The llist is staged in reverse order, and refreshed
 * from the llist when it exhausts.
 *
 * This is particularly suitable when work items are queued in BH or
 * IRQ context, and where work items are handled one at a time by
 * dedicated threads.

...we have dedicated threads, but we usually have a lot of them, so
that lock ends up being pretty contended.

Is the box you're testing on NUMA-enabled? Setting the server for
pool_mode=pernode might be worth an experiment. At least you'd have
more than one lwq and less cross-node chatter. You could also try
pool_mode=percpu, but that's rumored to not be as helpful.

Maybe we need to consider some other lockless queueing mechanism longer
term, but I'm not sure how possible that is.
-- 
Jeff Layton <jlayton@kernel.org>

Re: knfsd performance

[Chuck Lever III]

> On Jun 18, 2024, at 3:29 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Jun 18, 2024, at 2:32 PM, Trond Myklebust
>>> <trondmy@hammerspace.com> wrote:
>>> 
>>> I recently back ported Neil's lwq code and sunrpc server changes to
>>> our
>>> 5.15.130 based kernel in the hope of improving the performance for
>>> our
>>> data servers.
>>> 
>>> Our performance team recently ran a fio workload on a client that
>>> was
>>> doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
>>> (infiniband) against that resulting server. I've attached the
>>> resulting
>>> flame graph from a perf profile run on the server side.
>>> 
>>> Is anyone else seeing this massive contention for the spin lock in
>>> __lwq_dequeue? As you can see, it appears to be dwarfing all the
>>> other
>>> nfsd activity on the system in question here, being responsible for
>>> 45%
>>> of all the perf hits.
>> 
>> I haven't seen that, but I've been working on other issues.
>> 
>> What's the nfsd thread count on your test server? Have you
>> seen a similar impact on 6.10 kernels ?
>> 
> 
> 640 knfsd threads. The machine was a supermicro 2029BT-HNR with 2xIntel
> 6150, 384GB of memory and 6xWDC SN840.
> 
> Unfortunately, the machine was a loaner, so cannot compare to 6.10.
> That's why I was asking if anyone has seen anything similar.

If this system had more than one NUMA node, then using
svc's "numa pool" mode might have helped.


--
Chuck Lever

Re: knfsd performance

[Trond Myklebust]

On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
> 
> 
> > On Jun 18, 2024, at 3:29 PM, Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > 
> > On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
> > > 
> > > 
> > > > On Jun 18, 2024, at 2:32 PM, Trond Myklebust
> > > > <trondmy@hammerspace.com> wrote:
> > > > 
> > > > I recently back ported Neil's lwq code and sunrpc server
> > > > changes to
> > > > our
> > > > 5.15.130 based kernel in the hope of improving the performance
> > > > for
> > > > our
> > > > data servers.
> > > > 
> > > > Our performance team recently ran a fio workload on a client
> > > > that
> > > > was
> > > > doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
> > > > (infiniband) against that resulting server. I've attached the
> > > > resulting
> > > > flame graph from a perf profile run on the server side.
> > > > 
> > > > Is anyone else seeing this massive contention for the spin lock
> > > > in
> > > > __lwq_dequeue? As you can see, it appears to be dwarfing all
> > > > the
> > > > other
> > > > nfsd activity on the system in question here, being responsible
> > > > for
> > > > 45%
> > > > of all the perf hits.
> > > 
> > > I haven't seen that, but I've been working on other issues.
> > > 
> > > What's the nfsd thread count on your test server? Have you
> > > seen a similar impact on 6.10 kernels ?
> > > 
> > 
> > 640 knfsd threads. The machine was a supermicro 2029BT-HNR with
> > 2xIntel
> > 6150, 384GB of memory and 6xWDC SN840.
> > 
> > Unfortunately, the machine was a loaner, so cannot compare to 6.10.
> > That's why I was asking if anyone has seen anything similar.
> 
> If this system had more than one NUMA node, then using
> svc's "numa pool" mode might have helped.
> 

Interesting. I had forgotten about that setting.

Just out of curiosity, is there any reason why we might not want to
default to that mode on a NUMA enabled system?

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: knfsd performance

[Chuck Lever III]

> On Jun 18, 2024, at 3:50 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Jun 18, 2024, at 3:29 PM, Trond Myklebust
>>> <trondmy@hammerspace.com> wrote:
>>> 
>>> On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
>>>> 
>>>> 
>>>>> On Jun 18, 2024, at 2:32 PM, Trond Myklebust
>>>>> <trondmy@hammerspace.com> wrote:
>>>>> 
>>>>> I recently back ported Neil's lwq code and sunrpc server
>>>>> changes to
>>>>> our
>>>>> 5.15.130 based kernel in the hope of improving the performance
>>>>> for
>>>>> our
>>>>> data servers.
>>>>> 
>>>>> Our performance team recently ran a fio workload on a client
>>>>> that
>>>>> was
>>>>> doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
>>>>> (infiniband) against that resulting server. I've attached the
>>>>> resulting
>>>>> flame graph from a perf profile run on the server side.
>>>>> 
>>>>> Is anyone else seeing this massive contention for the spin lock
>>>>> in
>>>>> __lwq_dequeue? As you can see, it appears to be dwarfing all
>>>>> the
>>>>> other
>>>>> nfsd activity on the system in question here, being responsible
>>>>> for
>>>>> 45%
>>>>> of all the perf hits.
>>>> 
>>>> I haven't seen that, but I've been working on other issues.
>>>> 
>>>> What's the nfsd thread count on your test server? Have you
>>>> seen a similar impact on 6.10 kernels ?
>>>> 
>>> 
>>> 640 knfsd threads. The machine was a supermicro 2029BT-HNR with
>>> 2xIntel
>>> 6150, 384GB of memory and 6xWDC SN840.
>>> 
>>> Unfortunately, the machine was a loaner, so cannot compare to 6.10.
>>> That's why I was asking if anyone has seen anything similar.
>> 
>> If this system had more than one NUMA node, then using
>> svc's "numa pool" mode might have helped.
>> 
> 
> Interesting. I had forgotten about that setting.
> 
> Just out of curiosity, is there any reason why we might not want to
> default to that mode on a NUMA enabled system?

Can't think of one off hand. Maybe back in the day it was
hard to tell when you were actually /on/ a NUMA system.

Copying Dave to see if he has any recollection.


--
Chuck Lever

Re: knfsd performance

[Jeff Layton]

On Tue, 2024-06-18 at 19:54 +0000, Chuck Lever III wrote:
> 
> 
> > On Jun 18, 2024, at 3:50 PM, Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > 
> > On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
> > > 
> > > 
> > > > On Jun 18, 2024, at 3:29 PM, Trond Myklebust
> > > > <trondmy@hammerspace.com> wrote:
> > > > 
> > > > On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
> > > > > 
> > > > > 
> > > > > > On Jun 18, 2024, at 2:32 PM, Trond Myklebust
> > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > 
> > > > > > I recently back ported Neil's lwq code and sunrpc server
> > > > > > changes to
> > > > > > our
> > > > > > 5.15.130 based kernel in the hope of improving the
> > > > > > performance
> > > > > > for
> > > > > > our
> > > > > > data servers.
> > > > > > 
> > > > > > Our performance team recently ran a fio workload on a
> > > > > > client
> > > > > > that
> > > > > > was
> > > > > > doing 100% NFSv3 reads in O_DIRECT mode over an RDMA
> > > > > > connection
> > > > > > (infiniband) against that resulting server. I've attached
> > > > > > the
> > > > > > resulting
> > > > > > flame graph from a perf profile run on the server side.
> > > > > > 
> > > > > > Is anyone else seeing this massive contention for the spin
> > > > > > lock
> > > > > > in
> > > > > > __lwq_dequeue? As you can see, it appears to be dwarfing
> > > > > > all
> > > > > > the
> > > > > > other
> > > > > > nfsd activity on the system in question here, being
> > > > > > responsible
> > > > > > for
> > > > > > 45%
> > > > > > of all the perf hits.
> > > > > 
> > > > > I haven't seen that, but I've been working on other issues.
> > > > > 
> > > > > What's the nfsd thread count on your test server? Have you
> > > > > seen a similar impact on 6.10 kernels ?
> > > > > 
> > > > 
> > > > 640 knfsd threads. The machine was a supermicro 2029BT-HNR with
> > > > 2xIntel
> > > > 6150, 384GB of memory and 6xWDC SN840.
> > > > 
> > > > Unfortunately, the machine was a loaner, so cannot compare to
> > > > 6.10.
> > > > That's why I was asking if anyone has seen anything similar.
> > > 
> > > If this system had more than one NUMA node, then using
> > > svc's "numa pool" mode might have helped.
> > > 
> > 
> > Interesting. I had forgotten about that setting.
> > 
> > Just out of curiosity, is there any reason why we might not want to
> > default to that mode on a NUMA enabled system?
> 
> Can't think of one off hand. Maybe back in the day it was
> hard to tell when you were actually /on/ a NUMA system.
> 
> Copying Dave to see if he has any recollection.
> 

It's at least partly because of the klunkiness of the old pool_threads
interface: You have to bring up the server first using the "threads"
procfile, and then you can actually bring up threads in the various
pools using pool_threads.

Same for shutdown. You have to bring down the pool_threads first and
then you can bring down the final thread and the rest of the server
with it. Why it was designed this way, I have NFC.

The new nfsdctl tool and netlink interfaces should make this simpler in
the future. You'll be able to set the pool-mode in /etc/nfs.conf and
configure a list of per-pool thread counts in there too. Once we have
that, I think we'll be in a better position to consider doing it by
default.

Eventually we'd like to make the thread poos dynamic, at which point
making that the default becomes much simpler from an administrative
standpoint.
-- 
Jeff Layton <jlayton@kernel.org>

Re: knfsd performance

[NeilBrown]

On Wed, 19 Jun 2024, Jeff Layton wrote:
> On Tue, 2024-06-18 at 18:32 +0000, Trond Myklebust wrote:
> > I recently back ported Neil's lwq code and sunrpc server changes to
> > our
> > 5.15.130 based kernel in the hope of improving the performance for
> > our
> > data servers.
> > 
> > Our performance team recently ran a fio workload on a client that was
> > doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
> > (infiniband) against that resulting server. I've attached the
> > resulting
> > flame graph from a perf profile run on the server side.
> > 
> > Is anyone else seeing this massive contention for the spin lock in
> > __lwq_dequeue? As you can see, it appears to be dwarfing all the
> > other
> > nfsd activity on the system in question here, being responsible for
> > 45%
> > of all the perf hits.
> > 
> > 
> 
> I haven't spent much time on performance testing since I keep getting
> involved in bugs. It looks like that's just the way lwq works. From the
> comments in lib/lwq.c:
> 
>  * Entries are dequeued using a spinlock to protect against multiple
>  * access.  The llist is staged in reverse order, and refreshed
>  * from the llist when it exhausts.
>  *
>  * This is particularly suitable when work items are queued in BH or
>  * IRQ context, and where work items are handled one at a time by
>  * dedicated threads.
> 
> ...we have dedicated threads, but we usually have a lot of them, so
> that lock ends up being pretty contended.
> 
> Is the box you're testing on NUMA-enabled? Setting the server for
> pool_mode=pernode might be worth an experiment. At least you'd have
> more than one lwq and less cross-node chatter. You could also try
> pool_mode=percpu, but that's rumored to not be as helpful.
> 
> Maybe we need to consider some other lockless queueing mechanism longer
> term, but I'm not sure how possible that is.

I spent a lot of thought trying to come up with a lockless dequeue and
failed.  I could do it if we had generic load-locked/store-conditional
primitives, but that requires hardware support to be efficient.
compare-and-exchange cannot do it.
The core of the problem is that dequeue requires concurrent control of
the "head" pointer and the selected entry.  cmpxchg can only control one
address at a time.

If the lock is highly contended, then sharding is likely the best option
- across numa nodes or across some other subset of CPUs.

NeilBrown

Re: knfsd performance

[NeilBrown]

On Wed, 19 Jun 2024, Jeff Layton wrote:
> On Tue, 2024-06-18 at 19:54 +0000, Chuck Lever III wrote:
> > 
> > 
> > > On Jun 18, 2024, at 3:50 PM, Trond Myklebust
> > > <trondmy@hammerspace.com> wrote:
> > > 
> > > On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
> > > > 
> > > > 
> > > > > On Jun 18, 2024, at 3:29 PM, Trond Myklebust
> > > > > <trondmy@hammerspace.com> wrote:
> > > > > 
> > > > > On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
> > > > > > 
> > > > > > 
> > > > > > > On Jun 18, 2024, at 2:32 PM, Trond Myklebust
> > > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > > 
> > > > > > > I recently back ported Neil's lwq code and sunrpc server
> > > > > > > changes to
> > > > > > > our
> > > > > > > 5.15.130 based kernel in the hope of improving the
> > > > > > > performance
> > > > > > > for
> > > > > > > our
> > > > > > > data servers.
> > > > > > > 
> > > > > > > Our performance team recently ran a fio workload on a
> > > > > > > client
> > > > > > > that
> > > > > > > was
> > > > > > > doing 100% NFSv3 reads in O_DIRECT mode over an RDMA
> > > > > > > connection
> > > > > > > (infiniband) against that resulting server. I've attached
> > > > > > > the
> > > > > > > resulting
> > > > > > > flame graph from a perf profile run on the server side.
> > > > > > > 
> > > > > > > Is anyone else seeing this massive contention for the spin
> > > > > > > lock
> > > > > > > in
> > > > > > > __lwq_dequeue? As you can see, it appears to be dwarfing
> > > > > > > all
> > > > > > > the
> > > > > > > other
> > > > > > > nfsd activity on the system in question here, being
> > > > > > > responsible
> > > > > > > for
> > > > > > > 45%
> > > > > > > of all the perf hits.
> > > > > > 
> > > > > > I haven't seen that, but I've been working on other issues.
> > > > > > 
> > > > > > What's the nfsd thread count on your test server? Have you
> > > > > > seen a similar impact on 6.10 kernels ?
> > > > > > 
> > > > > 
> > > > > 640 knfsd threads. The machine was a supermicro 2029BT-HNR with
> > > > > 2xIntel
> > > > > 6150, 384GB of memory and 6xWDC SN840.
> > > > > 
> > > > > Unfortunately, the machine was a loaner, so cannot compare to
> > > > > 6.10.
> > > > > That's why I was asking if anyone has seen anything similar.
> > > > 
> > > > If this system had more than one NUMA node, then using
> > > > svc's "numa pool" mode might have helped.
> > > > 
> > > 
> > > Interesting. I had forgotten about that setting.
> > > 
> > > Just out of curiosity, is there any reason why we might not want to
> > > default to that mode on a NUMA enabled system?
> > 
> > Can't think of one off hand. Maybe back in the day it was
> > hard to tell when you were actually /on/ a NUMA system.
> > 
> > Copying Dave to see if he has any recollection.
> > 
> 
> It's at least partly because of the klunkiness of the old pool_threads
> interface: You have to bring up the server first using the "threads"
> procfile, and then you can actually bring up threads in the various
> pools using pool_threads.
> 
> Same for shutdown. You have to bring down the pool_threads first and
> then you can bring down the final thread and the rest of the server
> with it. Why it was designed this way, I have NFC.
> 
> The new nfsdctl tool and netlink interfaces should make this simpler in
> the future. You'll be able to set the pool-mode in /etc/nfs.conf and
> configure a list of per-pool thread counts in there too. Once we have
> that, I think we'll be in a better position to consider doing it by
> default.
> 
> Eventually we'd like to make the thread poos dynamic, at which point
> making that the default becomes much simpler from an administrative
> standpoint.

I agree that dynamic thread pools will make numa management simpler.
Greg Banks did the numa work for SGI - I wonder where he is now.  He was
at fastmail 10 years ago..

The idea was to bind network interfaces to numa nodes with interrupt
routing.  There was no expectation that work would be distributed evenly
across all nodes. Some might be dedicated to non-nfs work.  So there was
expected to be non-trivial configuration for both IRQ routing and
threads-per-node.  If we can make threads-per-node demand-based, then
half the problem goes away.

We could even default to one-thread-pool-per-CPU if there are more than
X cpus....

NeilBrown

Re: knfsd performance

[Chuck Lever III]

> On Jun 18, 2024, at 7:17 PM, NeilBrown <neilb@suse.de> wrote:
> 
> On Wed, 19 Jun 2024, Jeff Layton wrote:
>> On Tue, 2024-06-18 at 19:54 +0000, Chuck Lever III wrote:
>>> 
>>> 
>>>> On Jun 18, 2024, at 3:50 PM, Trond Myklebust
>>>> <trondmy@hammerspace.com> wrote:
>>>> 
>>>> On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
>>>>> 
>>>>> 
>>>>>> On Jun 18, 2024, at 3:29 PM, Trond Myklebust
>>>>>> <trondmy@hammerspace.com> wrote:
>>>>>> 
>>>>>> On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>> On Jun 18, 2024, at 2:32 PM, Trond Myklebust
>>>>>>>> <trondmy@hammerspace.com> wrote:
>>>>>>>> 
>>>>>>>> I recently back ported Neil's lwq code and sunrpc server
>>>>>>>> changes to
>>>>>>>> our
>>>>>>>> 5.15.130 based kernel in the hope of improving the
>>>>>>>> performance
>>>>>>>> for
>>>>>>>> our
>>>>>>>> data servers.
>>>>>>>> 
>>>>>>>> Our performance team recently ran a fio workload on a
>>>>>>>> client
>>>>>>>> that
>>>>>>>> was
>>>>>>>> doing 100% NFSv3 reads in O_DIRECT mode over an RDMA
>>>>>>>> connection
>>>>>>>> (infiniband) against that resulting server. I've attached
>>>>>>>> the
>>>>>>>> resulting
>>>>>>>> flame graph from a perf profile run on the server side.
>>>>>>>> 
>>>>>>>> Is anyone else seeing this massive contention for the spin
>>>>>>>> lock
>>>>>>>> in
>>>>>>>> __lwq_dequeue? As you can see, it appears to be dwarfing
>>>>>>>> all
>>>>>>>> the
>>>>>>>> other
>>>>>>>> nfsd activity on the system in question here, being
>>>>>>>> responsible
>>>>>>>> for
>>>>>>>> 45%
>>>>>>>> of all the perf hits.
>>>>>>> 
>>>>>>> I haven't seen that, but I've been working on other issues.
>>>>>>> 
>>>>>>> What's the nfsd thread count on your test server? Have you
>>>>>>> seen a similar impact on 6.10 kernels ?
>>>>>>> 
>>>>>> 
>>>>>> 640 knfsd threads. The machine was a supermicro 2029BT-HNR with
>>>>>> 2xIntel
>>>>>> 6150, 384GB of memory and 6xWDC SN840.
>>>>>> 
>>>>>> Unfortunately, the machine was a loaner, so cannot compare to
>>>>>> 6.10.
>>>>>> That's why I was asking if anyone has seen anything similar.
>>>>> 
>>>>> If this system had more than one NUMA node, then using
>>>>> svc's "numa pool" mode might have helped.
>>>>> 
>>>> 
>>>> Interesting. I had forgotten about that setting.
>>>> 
>>>> Just out of curiosity, is there any reason why we might not want to
>>>> default to that mode on a NUMA enabled system?
>>> 
>>> Can't think of one off hand. Maybe back in the day it was
>>> hard to tell when you were actually /on/ a NUMA system.
>>> 
>>> Copying Dave to see if he has any recollection.
>>> 
>> 
>> It's at least partly because of the klunkiness of the old pool_threads
>> interface: You have to bring up the server first using the "threads"
>> procfile, and then you can actually bring up threads in the various
>> pools using pool_threads.
>> 
>> Same for shutdown. You have to bring down the pool_threads first and
>> then you can bring down the final thread and the rest of the server
>> with it. Why it was designed this way, I have NFC.
>> 
>> The new nfsdctl tool and netlink interfaces should make this simpler in
>> the future. You'll be able to set the pool-mode in /etc/nfs.conf and
>> configure a list of per-pool thread counts in there too. Once we have
>> that, I think we'll be in a better position to consider doing it by
>> default.
>> 
>> Eventually we'd like to make the thread poos dynamic, at which point
>> making that the default becomes much simpler from an administrative
>> standpoint.
> 
> I agree that dynamic thread pools will make numa management simpler.
> Greg Banks did the numa work for SGI - I wonder where he is now.  He was
> at fastmail 10 years ago..

Dave (cc'd) designed it with Greg, Greg implemented it.


> The idea was to bind network interfaces to numa nodes with interrupt
> routing.  There was no expectation that work would be distributed evenly
> across all nodes. Some might be dedicated to non-nfs work.  So there was
> expected to be non-trivial configuration for both IRQ routing and
> threads-per-node.  If we can make threads-per-node demand-based, then
> half the problem goes away.

Network devices (and storage devices) are affined to one
NUMA node. If the nfsd threads are not on the same node
as the network device, there is a significant penalty.

I have a two-node system here, and it performs consistently
well when I put it in pool-mode=numa and affine the network
device's IRQs to one node.

It even works with two network devices (one per node) --
each device gets its own set of nfsd threads.

I don't think the pool_mode needs to be demand based. If
the system is a NUMA system, it makes sense to split up
the thread pools and put our pencils down. The only other
step that is needed is proper IRQ affinity settings for
the network devices.


> We could even default to one-thread-pool-per-CPU if there are more than
> X cpus....

I've never seen a performance improvement in the per-cpu
pool mode, fwiw.


--
Chuck Lever

Re: knfsd performance

[Jeff Layton]

On Tue, 2024-06-18 at 23:26 +0000, Chuck Lever III wrote:
> 
> > On Jun 18, 2024, at 7:17 PM, NeilBrown <neilb@suse.de> wrote:
> > 
> > On Wed, 19 Jun 2024, Jeff Layton wrote:
> > > On Tue, 2024-06-18 at 19:54 +0000, Chuck Lever III wrote:
> > > > 
> > > > 
> > > > > On Jun 18, 2024, at 3:50 PM, Trond Myklebust
> > > > > <trondmy@hammerspace.com> wrote:
> > > > > 
> > > > > On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
> > > > > > 
> > > > > > 
> > > > > > > On Jun 18, 2024, at 3:29 PM, Trond Myklebust
> > > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > > 
> > > > > > > On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > On Jun 18, 2024, at 2:32 PM, Trond Myklebust
> > > > > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > > > > 
> > > > > > > > > I recently back ported Neil's lwq code and sunrpc server
> > > > > > > > > changes to
> > > > > > > > > our
> > > > > > > > > 5.15.130 based kernel in the hope of improving the
> > > > > > > > > performance
> > > > > > > > > for
> > > > > > > > > our
> > > > > > > > > data servers.
> > > > > > > > > 
> > > > > > > > > Our performance team recently ran a fio workload on a
> > > > > > > > > client
> > > > > > > > > that
> > > > > > > > > was
> > > > > > > > > doing 100% NFSv3 reads in O_DIRECT mode over an RDMA
> > > > > > > > > connection
> > > > > > > > > (infiniband) against that resulting server. I've attached
> > > > > > > > > the
> > > > > > > > > resulting
> > > > > > > > > flame graph from a perf profile run on the server side.
> > > > > > > > > 
> > > > > > > > > Is anyone else seeing this massive contention for the spin
> > > > > > > > > lock
> > > > > > > > > in
> > > > > > > > > __lwq_dequeue? As you can see, it appears to be dwarfing
> > > > > > > > > all
> > > > > > > > > the
> > > > > > > > > other
> > > > > > > > > nfsd activity on the system in question here, being
> > > > > > > > > responsible
> > > > > > > > > for
> > > > > > > > > 45%
> > > > > > > > > of all the perf hits.
> > > > > > > > 
> > > > > > > > I haven't seen that, but I've been working on other issues.
> > > > > > > > 
> > > > > > > > What's the nfsd thread count on your test server? Have you
> > > > > > > > seen a similar impact on 6.10 kernels ?
> > > > > > > > 
> > > > > > > 
> > > > > > > 640 knfsd threads. The machine was a supermicro 2029BT-HNR with
> > > > > > > 2xIntel
> > > > > > > 6150, 384GB of memory and 6xWDC SN840.
> > > > > > > 
> > > > > > > Unfortunately, the machine was a loaner, so cannot compare to
> > > > > > > 6.10.
> > > > > > > That's why I was asking if anyone has seen anything similar.
> > > > > > 
> > > > > > If this system had more than one NUMA node, then using
> > > > > > svc's "numa pool" mode might have helped.
> > > > > > 
> > > > > 
> > > > > Interesting. I had forgotten about that setting.
> > > > > 
> > > > > Just out of curiosity, is there any reason why we might not want to
> > > > > default to that mode on a NUMA enabled system?
> > > > 
> > > > Can't think of one off hand. Maybe back in the day it was
> > > > hard to tell when you were actually /on/ a NUMA system.
> > > > 
> > > > Copying Dave to see if he has any recollection.
> > > > 
> > > 
> > > It's at least partly because of the klunkiness of the old pool_threads
> > > interface: You have to bring up the server first using the "threads"
> > > procfile, and then you can actually bring up threads in the various
> > > pools using pool_threads.
> > > 
> > > Same for shutdown. You have to bring down the pool_threads first and
> > > then you can bring down the final thread and the rest of the server
> > > with it. Why it was designed this way, I have NFC.
> > > 
> > > The new nfsdctl tool and netlink interfaces should make this simpler in
> > > the future. You'll be able to set the pool-mode in /etc/nfs.conf and
> > > configure a list of per-pool thread counts in there too. Once we have
> > > that, I think we'll be in a better position to consider doing it by
> > > default.
> > > 
> > > Eventually we'd like to make the thread poos dynamic, at which point
> > > making that the default becomes much simpler from an administrative
> > > standpoint.
> > 
> > I agree that dynamic thread pools will make numa management simpler.
> > Greg Banks did the numa work for SGI - I wonder where he is now.  He was
> > at fastmail 10 years ago..
> 
> Dave (cc'd) designed it with Greg, Greg implemented it.
> 
> 
> > The idea was to bind network interfaces to numa nodes with interrupt
> > routing.  There was no expectation that work would be distributed evenly
> > across all nodes. Some might be dedicated to non-nfs work.  So there was
> > expected to be non-trivial configuration for both IRQ routing and
> > threads-per-node.  If we can make threads-per-node demand-based, then
> > half the problem goes away.
> 
> Network devices (and storage devices) are affined to one
> NUMA node. If the nfsd threads are not on the same node
> as the network device, there is a significant penalty.
> 
> I have a two-node system here, and it performs consistently
> well when I put it in pool-mode=numa and affine the network
> device's IRQs to one node.
> 
> It even works with two network devices (one per node) --
> each device gets its own set of nfsd threads.
> 
> I don't think the pool_mode needs to be demand based. If
> the system is a NUMA system, it makes sense to split up
> the thread pools and put our pencils down. The only other
> step that is needed is proper IRQ affinity settings for
> the network devices.
> 

Having them be demand-based is a nice-to-have. Right now, you need to
know how many thread pools (it's not always trivial to tell) and you
have and decide how many threads each gets. There is some cost to
getting that wrong too.

An on-demand thread pool takes a lot of the guesswork out of the
equation (assuming we can get the behavior right, of course).

> 
> > We could even default to one-thread-pool-per-CPU if there are more than
> > X cpus....
> 
> I've never seen a performance improvement in the per-cpu
> pool mode, fwiw.
> 
> 
> --
> Chuck Lever
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: knfsd performance

[Chuck Lever III]

> On Jun 18, 2024, at 7:33 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Tue, 2024-06-18 at 23:26 +0000, Chuck Lever III wrote:
>> 
>>> On Jun 18, 2024, at 7:17 PM, NeilBrown <neilb@suse.de> wrote:
>>> 
>>> On Wed, 19 Jun 2024, Jeff Layton wrote:
>>>> On Tue, 2024-06-18 at 19:54 +0000, Chuck Lever III wrote:
>>>>> 
>>>>> 
>>>>>> On Jun 18, 2024, at 3:50 PM, Trond Myklebust
>>>>>> <trondmy@hammerspace.com> wrote:
>>>>>> 
>>>>>> On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>> On Jun 18, 2024, at 3:29 PM, Trond Myklebust
>>>>>>>> <trondmy@hammerspace.com> wrote:
>>>>>>>> 
>>>>>>>> On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> On Jun 18, 2024, at 2:32 PM, Trond Myklebust
>>>>>>>>>> <trondmy@hammerspace.com> wrote:
>>>>>>>>>> 
>>>>>>>>>> I recently back ported Neil's lwq code and sunrpc server
>>>>>>>>>> changes to
>>>>>>>>>> our
>>>>>>>>>> 5.15.130 based kernel in the hope of improving the
>>>>>>>>>> performance
>>>>>>>>>> for
>>>>>>>>>> our
>>>>>>>>>> data servers.
>>>>>>>>>> 
>>>>>>>>>> Our performance team recently ran a fio workload on a
>>>>>>>>>> client
>>>>>>>>>> that
>>>>>>>>>> was
>>>>>>>>>> doing 100% NFSv3 reads in O_DIRECT mode over an RDMA
>>>>>>>>>> connection
>>>>>>>>>> (infiniband) against that resulting server. I've attached
>>>>>>>>>> the
>>>>>>>>>> resulting
>>>>>>>>>> flame graph from a perf profile run on the server side.
>>>>>>>>>> 
>>>>>>>>>> Is anyone else seeing this massive contention for the spin
>>>>>>>>>> lock
>>>>>>>>>> in
>>>>>>>>>> __lwq_dequeue? As you can see, it appears to be dwarfing
>>>>>>>>>> all
>>>>>>>>>> the
>>>>>>>>>> other
>>>>>>>>>> nfsd activity on the system in question here, being
>>>>>>>>>> responsible
>>>>>>>>>> for
>>>>>>>>>> 45%
>>>>>>>>>> of all the perf hits.
>>>>>>>>> 
>>>>>>>>> I haven't seen that, but I've been working on other issues.
>>>>>>>>> 
>>>>>>>>> What's the nfsd thread count on your test server? Have you
>>>>>>>>> seen a similar impact on 6.10 kernels ?
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> 640 knfsd threads. The machine was a supermicro 2029BT-HNR with
>>>>>>>> 2xIntel
>>>>>>>> 6150, 384GB of memory and 6xWDC SN840.
>>>>>>>> 
>>>>>>>> Unfortunately, the machine was a loaner, so cannot compare to
>>>>>>>> 6.10.
>>>>>>>> That's why I was asking if anyone has seen anything similar.
>>>>>>> 
>>>>>>> If this system had more than one NUMA node, then using
>>>>>>> svc's "numa pool" mode might have helped.
>>>>>>> 
>>>>>> 
>>>>>> Interesting. I had forgotten about that setting.
>>>>>> 
>>>>>> Just out of curiosity, is there any reason why we might not want to
>>>>>> default to that mode on a NUMA enabled system?
>>>>> 
>>>>> Can't think of one off hand. Maybe back in the day it was
>>>>> hard to tell when you were actually /on/ a NUMA system.
>>>>> 
>>>>> Copying Dave to see if he has any recollection.
>>>>> 
>>>> 
>>>> It's at least partly because of the klunkiness of the old pool_threads
>>>> interface: You have to bring up the server first using the "threads"
>>>> procfile, and then you can actually bring up threads in the various
>>>> pools using pool_threads.
>>>> 
>>>> Same for shutdown. You have to bring down the pool_threads first and
>>>> then you can bring down the final thread and the rest of the server
>>>> with it. Why it was designed this way, I have NFC.
>>>> 
>>>> The new nfsdctl tool and netlink interfaces should make this simpler in
>>>> the future. You'll be able to set the pool-mode in /etc/nfs.conf and
>>>> configure a list of per-pool thread counts in there too. Once we have
>>>> that, I think we'll be in a better position to consider doing it by
>>>> default.
>>>> 
>>>> Eventually we'd like to make the thread poos dynamic, at which point
>>>> making that the default becomes much simpler from an administrative
>>>> standpoint.
>>> 
>>> I agree that dynamic thread pools will make numa management simpler.
>>> Greg Banks did the numa work for SGI - I wonder where he is now.  He was
>>> at fastmail 10 years ago..
>> 
>> Dave (cc'd) designed it with Greg, Greg implemented it.
>> 
>> 
>>> The idea was to bind network interfaces to numa nodes with interrupt
>>> routing.  There was no expectation that work would be distributed evenly
>>> across all nodes. Some might be dedicated to non-nfs work.  So there was
>>> expected to be non-trivial configuration for both IRQ routing and
>>> threads-per-node.  If we can make threads-per-node demand-based, then
>>> half the problem goes away.
>> 
>> Network devices (and storage devices) are affined to one
>> NUMA node. If the nfsd threads are not on the same node
>> as the network device, there is a significant penalty.
>> 
>> I have a two-node system here, and it performs consistently
>> well when I put it in pool-mode=numa and affine the network
>> device's IRQs to one node.
>> 
>> It even works with two network devices (one per node) --
>> each device gets its own set of nfsd threads.
>> 
>> I don't think the pool_mode needs to be demand based. If
>> the system is a NUMA system, it makes sense to split up
>> the thread pools and put our pencils down. The only other
>> step that is needed is proper IRQ affinity settings for
>> the network devices.
>> 
> 
> Having them be demand-based is a nice-to-have. Right now, you need to
> know how many thread pools (it's not always trivial to tell) and you
> have and decide how many threads each gets. There is some cost to
> getting that wrong too.

I misread Neil's suggestion. I thought he meant that
the pool_mode setting would be demand-based. I don't
have a problem with a demand-based thread count, and
the demand should be estimated per pool, IMO.

The current pool_mode=numa setting knows how many
pools to set up: it's the same number as the there
are NUMA nodes on the system. That probably should
not be changed. nfsdctl can look at the pool_mode
setting and know how many pools there are, can't it?


> An on-demand thread pool takes a lot of the guesswork out of the
> equation (assuming we can get the behavior right, of course).
> 
>> 
>>> We could even default to one-thread-pool-per-CPU if there are more than
>>> X cpus....
>> 
>> I've never seen a performance improvement in the per-cpu
>> pool mode, fwiw.
>> 
>> 
>> --
>> Chuck Lever
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>


--
Chuck Lever

Re: knfsd performance

[Dave Chinner]

On Tue, Jun 18, 2024 at 07:54:43PM +0000, Chuck Lever III wrote  > On Jun 18, 2024, at 3:50 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> > 
> > On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
> >> 
> >> 
> >>> On Jun 18, 2024, at 3:29 PM, Trond Myklebust
> >>> <trondmy@hammerspace.com> wrote:
> >>> 
> >>> On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
> >>>> 
> >>>> 
> >>>>> On Jun 18, 2024, at 2:32 PM, Trond Myklebust
> >>>>> <trondmy@hammerspace.com> wrote:
> >>>>> 
> >>>>> I recently back ported Neil's lwq code and sunrpc server
> >>>>> changes to
> >>>>> our
> >>>>> 5.15.130 based kernel in the hope of improving the performance
> >>>>> for
> >>>>> our
> >>>>> data servers.
> >>>>> 
> >>>>> Our performance team recently ran a fio workload on a client
> >>>>> that
> >>>>> was
> >>>>> doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
> >>>>> (infiniband) against that resulting server. I've attached the
> >>>>> resulting
> >>>>> flame graph from a perf profile run on the server side.
> >>>>> 
> >>>>> Is anyone else seeing this massive contention for the spin lock
> >>>>> in
> >>>>> __lwq_dequeue? As you can see, it appears to be dwarfing all
> >>>>> the
> >>>>> other
> >>>>> nfsd activity on the system in question here, being responsible
> >>>>> for
> >>>>> 45%
> >>>>> of all the perf hits.

Ouch. __lwq_dequeue() runs llist_reverse_order() under a spinlock.

llist_reverse_order() is an O(n) algorithm involving full length
linked list traversal. IOWs, it's a worst case cache miss algorithm
running under a spin lock. And then consider what happens when
enqueue processing is faster than dequeue processing.

This means the depth of the queue grows, and ultimate length of the
queue is unbound. Because fo the batch processing nature of lwq -
it takes ->new, reverses it and places it in ->ready - the length of
the list that needs reversing ends up growing every batch that
we queue faster than we dequeue. Unbound processing queues are bad
even when they have O(1) behaviour. lwq has O(n) worst case
behaviour, and that makes this even worse...

Regardless, The current lwq could be slightly improved - the
lockless enqueue competes for the same cacheline as the dequeue
serialisation lock.

struct lwq {
        spinlock_t              lock;
        struct llist_node       *ready;         /* entries to be dequeued */
        struct llist_head       new;            /* entries being enqueued */
};

Adding __cacheline_aligned_in_smp to ->new (the enqueue side) might
help reduce this enqueue/dequeue cacheline contention a bit by
separating them onto different cachelines. That will push the point
of catastrophic breakdown out a little bit, not solve the issue of
queue depth based batch processing on the dequeue side.

I suspect a lockless ring buffer might be a more scalable solution
for the nfsd...

> >>>> I haven't seen that, but I've been working on other issues.
> >>>> 
> >>>> What's the nfsd thread count on your test server? Have you
> >>>> seen a similar impact on 6.10 kernels ?
> >>>> 
> >>> 
> >>> 640 knfsd threads. The machine was a supermicro 2029BT-HNR with
> >>> 2xIntel
> >>> 6150, 384GB of memory and 6xWDC SN840.
> >>> 
> >>> Unfortunately, the machine was a loaner, so cannot compare to 6.10.
> >>> That's why I was asking if anyone has seen anything similar.
> >> 
> >> If this system had more than one NUMA node, then using
> >> svc's "numa pool" mode might have helped.

It's a dual socket machine so it has at least 2 physical nodes. Of
course, the bios has to be configured to expose this as a NUMA
machine and not a "legacy SMP" machine for the OS to know that, but
I'm pretty sure that's been the typical server bios defaults for
quite a few years now.

Even if it wasn't a dual socket machine, the CPU itself is a NUMA
architecture.  i.e. just about every server x86-64 CPU sold these
days is a NUMA SOC - neither core-to-core or core-to-memory latency
is uniform within a socket these days. Desktop CPUs are also well
down this track, too.

Intel exposes the details of the topology within a socket via the
bios option known as "sub-numa clustering". This exposes the full
sub-socket CPU and memory topology to the OS, so it is fully aware
of both the on-chip and off-chip topology.

Using sub-numa clustering means we don't end up with 32+ CPUS to a
per-socket numa node. Numa scalability algorithms largely rely on
keeping the cores-per-numa-node ratio in check. Sub-numa
clustering enables the OS to keep this ratio down to reasonable
levels.

> > Interesting. I had forgotten about that setting.
> > 
> > Just out of curiosity, is there any reason why we might not want to
> > default to that mode on a NUMA enabled system?
> 
> Can't think of one off hand. Maybe back in the day it was
> hard to tell when you were actually /on/ a NUMA system.

As per above, I would assume that the kernel is *always* running on
a NUMA machine. IOWs, if CONFIG_NUMA is enabled (which it is on just
about every distro kernel these days), then we should be using NUMA
optimisations by default.

If the machine is not a NUMA machine (e.g. single socket, sub-numa
clustering off), then the NUMA subsystem will be initialised with
nr_online_nodes = 1 (i.e. a single active node) and numa aware
algorithms should just behave as if there is a single global node.
If CONFIG_NUMA=n, then nr_online_nodes is hard coded to 1.

Hence subsystems only need to implement a single algorithm that is
NUMA aware. The bios/system config will tell the kernel how many
nodes there are, and the NUMA algorithms will just do the right
thing because nr_online_nodes=1 in those situations..

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: knfsd performance

[NeilBrown]

On Wed, 19 Jun 2024, Dave Chinner wrote:
> On Tue, Jun 18, 2024 at 07:54:43PM +0000, Chuck Lever III wrote  > On Jun 18, 2024, at 3:50 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> > > 
> > > On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
> > >> 
> > >> 
> > >>> On Jun 18, 2024, at 3:29 PM, Trond Myklebust
> > >>> <trondmy@hammerspace.com> wrote:
> > >>> 
> > >>> On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
> > >>>> 
> > >>>> 
> > >>>>> On Jun 18, 2024, at 2:32 PM, Trond Myklebust
> > >>>>> <trondmy@hammerspace.com> wrote:
> > >>>>> 
> > >>>>> I recently back ported Neil's lwq code and sunrpc server
> > >>>>> changes to
> > >>>>> our
> > >>>>> 5.15.130 based kernel in the hope of improving the performance
> > >>>>> for
> > >>>>> our
> > >>>>> data servers.
> > >>>>> 
> > >>>>> Our performance team recently ran a fio workload on a client
> > >>>>> that
> > >>>>> was
> > >>>>> doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
> > >>>>> (infiniband) against that resulting server. I've attached the
> > >>>>> resulting
> > >>>>> flame graph from a perf profile run on the server side.
> > >>>>> 
> > >>>>> Is anyone else seeing this massive contention for the spin lock
> > >>>>> in
> > >>>>> __lwq_dequeue? As you can see, it appears to be dwarfing all
> > >>>>> the
> > >>>>> other
> > >>>>> nfsd activity on the system in question here, being responsible
> > >>>>> for
> > >>>>> 45%
> > >>>>> of all the perf hits.
> 
> Ouch. __lwq_dequeue() runs llist_reverse_order() under a spinlock.
> 
> llist_reverse_order() is an O(n) algorithm involving full length
> linked list traversal. IOWs, it's a worst case cache miss algorithm
> running under a spin lock. And then consider what happens when
> enqueue processing is faster than dequeue processing.

My expectation was that if enqueue processing (incoming packets) was
faster than dequeue processing (handling NFS requests) then there was a
bottleneck elsewhere, and this one wouldn't be relevant.

It might be useful to measure how long the queue gets.

> 
> This means the depth of the queue grows, and ultimate length of the
> queue is unbound. Because fo the batch processing nature of lwq -
> it takes ->new, reverses it and places it in ->ready - the length of
> the list that needs reversing ends up growing every batch that
> we queue faster than we dequeue. Unbound processing queues are bad
> even when they have O(1) behaviour. lwq has O(n) worst case
> behaviour, and that makes this even worse...
> 
> Regardless, The current lwq could be slightly improved - the
> lockless enqueue competes for the same cacheline as the dequeue
> serialisation lock.
> 
> struct lwq {
>         spinlock_t              lock;
>         struct llist_node       *ready;         /* entries to be dequeued */
>         struct llist_head       new;            /* entries being enqueued */
> };
> 
> Adding __cacheline_aligned_in_smp to ->new (the enqueue side) might
> help reduce this enqueue/dequeue cacheline contention a bit by
> separating them onto different cachelines. That will push the point
> of catastrophic breakdown out a little bit, not solve the issue of
> queue depth based batch processing on the dequeue side.
> 

Yes, that might be beneficial - thanks.


> I suspect a lockless ring buffer might be a more scalable solution
> for the nfsd...

We would need to size the buffer to the maximum number of connections
(if there is one - I don't recall) or resize the buffer as the number of
connections grows.  Certainly doable.

Thanks,
NeilBrown

Re: knfsd performance

[Dave Chinner]

On Tue, Jun 18, 2024 at 11:26:22PM +0000, Chuck Lever III wrote:
> > On Jun 18, 2024, at 7:17 PM, NeilBrown <neilb@suse.de> wrote:
> >> Eventually we'd like to make the thread poos dynamic, at which point
> >> making that the default becomes much simpler from an administrative
> >> standpoint.
> > 
> > I agree that dynamic thread pools will make numa management simpler.
> > Greg Banks did the numa work for SGI - I wonder where he is now.  He was
> > at fastmail 10 years ago..
>
> Dave (cc'd) designed it with Greg, Greg implemented it.

[ I'll dump a bit of history about the NUMA nfsd architecture at the
end. ]

> > The idea was to bind network interfaces to numa nodes with interrupt
> > routing.  There was no expectation that work would be distributed evenly
> > across all nodes. Some might be dedicated to non-nfs work.  So there was
> > expected to be non-trivial configuration for both IRQ routing and
> > threads-per-node.  If we can make threads-per-node demand-based, then
> > half the problem goes away.

Right.

For the dynamic thread pool stuff, the grow side was a simple
heuristic: when we dequeued a request, we checked if the request
queue was empty, if there were idle nfsd threads and whether we were
under the max thread count.  i.e. If we had more work to do and no
idle workers to do it, we forked another nfsd thread to do the work.

I don't recall exactly what Greg implemented on Linux for the shrink
side. On Irix, the nfsd would record the time at which it completed
it's last request processing, we fired a timer every 30s or
so to walk the nfsd status array. If we found an nfsd with a
completion time older than 30s, the nfsd got reaped. 30s was long
enough to handle bursty loads, but short enough that people didn't
complain about having hundreds of nfsds sitting around....

This is basically a very simple version of what workqueues do for us
now.

That is, if we just make the nfsd request work be based on per-node, node
affine, unbound work queues, then thread scaling comes along for
free. I think that workqueues support this per-node thread pool
affinity natively now:

enum wq_affn_scope {
        WQ_AFFN_DFL,                    /* use system default */
        WQ_AFFN_CPU,                    /* one pod per CPU */
        WQ_AFFN_SMT,                    /* one pod poer SMT */
        WQ_AFFN_CACHE,                  /* one pod per LLC */
>>>>>   WQ_AFFN_NUMA,                   /* one pod per NUMA node */
        WQ_AFFN_SYSTEM,                 /* one pod across the whole system */

        WQ_AFFN_NR_TYPES,
};

I'm not sure that the NFS server needs to reinvent the wheel here...

> Network devices (and storage devices) are affined to one
> NUMA node.

NVMe storage devices don't need to be affine to the node. They just
need to have a hardware queue assigned to each node so that
node-local IO always hits the same hardware queue and gets
completion interrupts returned to that same node.

And, yes, this is something that still has to be configured
manually, too.

> If the nfsd threads are not on the same node
> as the network device, there is a significant penalty.
>
> I have a two-node system here, and it performs consistently
> well when I put it in pool-mode=numa and affine the network
> device's IRQs to one node.
>
> It even works with two network devices (one per node) --
> each device gets its own set of nfsd threads.

Right. But this is all orthogonal to solving the problem of demand
based thread pool scaling.

> I don't think the pool_mode needs to be demand based. If
> the system is a NUMA system, it makes sense to split up
> the thread pools and put our pencils down. The only other
> step that is needed is proper IRQ affinity settings for
> the network devices.

I think it's better for everyone if the system automatically scales
with demand, regardless of whether it's a NUMA system or not, and
regardless of whether the proper NUMA affinity has been configured
or not.

> > We could even default to one-thread-pool-per-CPU if there are more than
> > X cpus....
> 
> I've never seen a performance improvement in the per-cpu
> pool mode, fwiw.

We're not doing anything inherently per-cpu in processing an NFS
request, so I can only see downsides to trying to restrict incoming
processing to per-cpu queues. i.e. if the CPU can't handle all the
incoming requests, what processes the per-cpu request backlog? At
least with per-node queues, we have many cpus to through at the one
incoming request queue and we are much less likely to get backlogged
and starve the request queue of processing resources...

Cheers,

Dave.

-----

History....

I did the original NUMA NFS server architecture and implementation
work on Irix Origin NUMA machines. Greg took that architecture and
made it work on Linux Altix NUMA machines. The differences in OS
implementation and the general lack of NUMA and hardware topology
support in Linux meant that things had to be done differently on
Linux. The Linux project was a much bigger and complex undertaking,
and Greg did most of that work.

The original Irix architecture was per-node NICs and FC HBAs.  The
per-node HBAs were attached to shared storage via a multi-path FC
SAN, and the NICS were bonded to the local switch and load balanced.
Every node had the same network and storage bandwdith - I think it
was 4x1GbE and 2x2Gb FC ports per 8p/32GB node.

IOWs, roughly 400MB/s per numa node to/from network, to/from disk.

On Irix, the NICs and storage hardware were always configured at
startup to be affine to the nearest CPU node. The OS knew the
physical topology of every piece of hardware in the system, and knew
what NUMA node they should be bound to. Nothing needed manual
configuration to be node affine.

Because we were dealing with networks of thousands of NFS clients
(think 2000s era renderwalls) we used ip/port hash based load
balancing across all the NIC ports in the machine. This effectively
always drove the requests from a single client on the network to a
specific NIC on the server. With the NICs being bound to a specific
node, we essentially sharded per-client information into per-node
structures (e.g. duplicate request detection). Hence most requests
rarely need cross-node structure access. Incoming requests were
processed on the local node, and any IO that the local NFSd needed
to do through XFS was sent directly out the HBAs on the local node
and completions were signalled back to that node.

Essentially, all the requests, the processing and IO for a specific
client ended up being bound to a specific NUMA node. The largest
Irix machines we shipped as dedicated NAS boxes were 4 node O300
machines. They had 32 600Mhz r18k MIPS CPUs, 192GB RAM, 16 GbE
ports, 8xFC HBA ports and they scaled out to a bit over 1.2GB/s
to/from the network to/from storage. That doesn't seem like much
these days, but this was back in 2005....

There was relatively little that needed to change in Irix to do
this. It largely had all the functionality it already needed, and so
it was largely a simple matter of connecting existing dots once the
overall architecture was worked out.

OTOH, at the time Linux didn't really have a concept of physical
hardware topology. All interrupt vectoring had to be set up manually
in userspace to point them at the right node. The network drivers
didn't use node affine memory to fill receive rings. The bonding
driver didn't support the whacky arp games the Irix driver
utilised to do it's thing. The network stack performance was a long
way behind Irix. The nfsd didn't support dynamic worker thread
instantiation. The scheduler didn't really understand numa topology
or node affinity very well. The list went on.

These were some of the problems that Greg and other SGI people
addressed - that was *much* more work that what I had to do
originally to get this all to work on Irix, even though the overall
architecture was largely the same.

-- 
Dave Chinner
david@fromorbit.com

Re: knfsd performance

[Christoph Hellwig]

On Wed, Jun 19, 2024 at 12:56:44PM +1000, Dave Chinner wrote:
> I'm not sure that the NFS server needs to reinvent the wheel here...
> 
> > Network devices (and storage devices) are affined to one
> > NUMA node.
> 
> NVMe storage devices don't need to be affine to the node. They just
> need to have a hardware queue assigned to each node so that
> node-local IO always hits the same hardware queue and gets
> completion interrupts returned to that same node.
> 
> And, yes, this is something that still has to be configured
> manually, too.

For NVMe no manual configuration is required.  It uses the
blk_mq_pci_map_queues helper to spread the queues around
optimally.  If enough queues are available every core gets
one, else they are spread so that close cores share queues.

Re: knfsd performance

[Chuck Lever III]

> On Jun 18, 2024, at 10:56 PM, Dave Chinner <david@fromorbit.com> wrote:
> 
> On Tue, Jun 18, 2024 at 11:26:22PM +0000, Chuck Lever III wrote:
>>> On Jun 18, 2024, at 7:17 PM, NeilBrown <neilb@suse.de> wrote:
>>>> Eventually we'd like to make the thread poos dynamic, at which point
>>>> making that the default becomes much simpler from an administrative
>>>> standpoint.
>>> 
>>> I agree that dynamic thread pools will make numa management simpler.
>>> Greg Banks did the numa work for SGI - I wonder where he is now.  He was
>>> at fastmail 10 years ago..
>> 
>> Dave (cc'd) designed it with Greg, Greg implemented it.
> 
> [ I'll dump a bit of history about the NUMA nfsd architecture at the
> end. ]
> 
>>> The idea was to bind network interfaces to numa nodes with interrupt
>>> routing.  There was no expectation that work would be distributed evenly
>>> across all nodes. Some might be dedicated to non-nfs work.  So there was
>>> expected to be non-trivial configuration for both IRQ routing and
>>> threads-per-node.  If we can make threads-per-node demand-based, then
>>> half the problem goes away.
> 
> Right.
> 
> For the dynamic thread pool stuff, the grow side was a simple
> heuristic: when we dequeued a request, we checked if the request
> queue was empty, if there were idle nfsd threads and whether we were
> under the max thread count.  i.e. If we had more work to do and no
> idle workers to do it, we forked another nfsd thread to do the work.
> 
> I don't recall exactly what Greg implemented on Linux for the shrink
> side. On Irix, the nfsd would record the time at which it completed
> it's last request processing, we fired a timer every 30s or
> so to walk the nfsd status array. If we found an nfsd with a
> completion time older than 30s, the nfsd got reaped. 30s was long
> enough to handle bursty loads, but short enough that people didn't
> complain about having hundreds of nfsds sitting around....
> 
> This is basically a very simple version of what workqueues do for us
> now.
> 
> That is, if we just make the nfsd request work be based on per-node, node
> affine, unbound work queues, then thread scaling comes along for
> free. I think that workqueues support this per-node thread pool
> affinity natively now:
> 
> enum wq_affn_scope {
>        WQ_AFFN_DFL,                    /* use system default */
>        WQ_AFFN_CPU,                    /* one pod per CPU */
>        WQ_AFFN_SMT,                    /* one pod poer SMT */
>        WQ_AFFN_CACHE,                  /* one pod per LLC */
>>>>>>  WQ_AFFN_NUMA,                   /* one pod per NUMA node */
>        WQ_AFFN_SYSTEM,                 /* one pod across the whole system */
> 
>        WQ_AFFN_NR_TYPES,
> };

Note that wq_affn_scope appeared only in the last one
or two kernel releases. Previously there was no way to
control workqueue cache affinity.

I have observed significant improvement in throughput
scalability on the Linux NFS client with the new default
workqueue cache affinity behavior (with NFS/RDMA, of
course). The issue is, interestingly enough, the same
as it was for lwq: spinlock contention is reduced once
the set of threads being scheduled all belong to the
same hardware cache.


> I'm not sure that the NFS server needs to reinvent the wheel here...

Jeff implemented a workqueue-based svc thread scheduler
about 10 years ago. We could never make it perform as
well as the existing kthreads-based scheduler. I'm not
sure we need a work-conserving thread scheduler for
NFSD/SunRPC.

That's why we took the route of keeping the bespoke
thread scheduler in SunRPC this time.


>> Network devices (and storage devices) are affined to one
>> NUMA node.
> 
> NVMe storage devices don't need to be affine to the node. They just
> need to have a hardware queue assigned to each node so that
> node-local IO always hits the same hardware queue and gets
> completion interrupts returned to that same node.
> 
> And, yes, this is something that still has to be configured
> manually, too.
> 
>> If the nfsd threads are not on the same node
>> as the network device, there is a significant penalty.
>> 
>> I have a two-node system here, and it performs consistently
>> well when I put it in pool-mode=numa and affine the network
>> device's IRQs to one node.
>> 
>> It even works with two network devices (one per node) --
>> each device gets its own set of nfsd threads.
> 
> Right. But this is all orthogonal to solving the problem of demand
> based thread pool scaling.

True, but it /is/ related to Trond's initial observation.
The network devices need to be affined properly before
pool_mode=numa is completely effective.


>> I don't think the pool_mode needs to be demand based. If
>> the system is a NUMA system, it makes sense to split up
>> the thread pools and put our pencils down. The only other
>> step that is needed is proper IRQ affinity settings for
>> the network devices.
> 
> I think it's better for everyone if the system automatically scales
> with demand, regardless of whether it's a NUMA system or not, and
> regardless of whether the proper NUMA affinity has been configured
> or not.

Read the mistakes that I wrote here ;-) I wasn't referring
to thread count. As I responded later, I think automatically
managing the pool thread count would be a good improvement.
We've been discussing this off and on for a while.


>>> We could even default to one-thread-pool-per-CPU if there are more than
>>> X cpus....
>> 
>> I've never seen a performance improvement in the per-cpu
>> pool mode, fwiw.
> 
> We're not doing anything inherently per-cpu in processing an NFS
> request, so I can only see downsides to trying to restrict incoming
> processing to per-cpu queues. i.e. if the CPU can't handle all the
> incoming requests, what processes the per-cpu request backlog? At
> least with per-node queues, we have many cpus to through at the one
> incoming request queue and we are much less likely to get backlogged
> and starve the request queue of processing resources...

I've proposed getting rid of pool_mode=cpu, because I can't
find a winning use case for it. We erred on the conservative
side and left it in place for now.

In terms of making pool_mode=numa the default setting, that
will work in many cases, but in some cases it will result in
unpleasant surprises.

The default out-of-the-shrink-wrap thread count is currently
fixed at 8.

In pool_mode=global, all 8 threads get used.

In pool_mode=numa, the total thread count is split across
the nodes. On a single node system, there will be no
change. All is well.

On a two-node system, though, each pool will get 4 threads,
or only 2 threads each on a four-node system. That can have
a surprising and undesirable performance impact.

I don't know what will happen on a system with more nodes
than nfsd threads.

Thus I think there needs to be a solution (like dynamic
thread count adjustment) in place to address thread count
splitting before we can change the default pool mode.


--
Chuck Lever

Re: knfsd performance

[NeilBrown]

On Wed, 19 Jun 2024, Dave Chinner wrote:
> 
> I'm not sure that the NFS server needs to reinvent the wheel here...
> 

Workqueues provide threads, but we still have to provide the 'struct
work_struct'.
For nfsd that is essentially 'struct svc_rqst' which is not small by
itself, and includes pre-allocated buffer space for the largest possible
request+reply - typically a little over 1MB.

If we allocate a new svc_rqst for each incoming request and hand it to
workqueue, then we are probably doing a lot more allocating and freeing
than at present.

If we keep a pool of svc_rqst and re-use them - and if we actually have
several pools, one for each NUMA node - then it seems very little extra
effort to permanently associate a thread with each svc_rqst.  It's not
clear that workqueue would buy us anything.

Certainly we could usefully learn from workqueue and make use of the
same primitives that it uses, but I don't see there would be a win from
using it directly.

Thanks,
NeilBrown

Re: knfsd performance

[NeilBrown]

On Wed, 19 Jun 2024, NeilBrown wrote:
> On Wed, 19 Jun 2024, Dave Chinner wrote:
> > On Tue, Jun 18, 2024 at 07:54:43PM +0000, Chuck Lever III wrote  > On Jun 18, 2024, at 3:50 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> > > > 
> > > > On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
> > > >> 
> > > >> 
> > > >>> On Jun 18, 2024, at 3:29 PM, Trond Myklebust
> > > >>> <trondmy@hammerspace.com> wrote:
> > > >>> 
> > > >>> On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
> > > >>>> 
> > > >>>> 
> > > >>>>> On Jun 18, 2024, at 2:32 PM, Trond Myklebust
> > > >>>>> <trondmy@hammerspace.com> wrote:
> > > >>>>> 
> > > >>>>> I recently back ported Neil's lwq code and sunrpc server
> > > >>>>> changes to
> > > >>>>> our
> > > >>>>> 5.15.130 based kernel in the hope of improving the performance
> > > >>>>> for
> > > >>>>> our
> > > >>>>> data servers.
> > > >>>>> 
> > > >>>>> Our performance team recently ran a fio workload on a client
> > > >>>>> that
> > > >>>>> was
> > > >>>>> doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
> > > >>>>> (infiniband) against that resulting server. I've attached the
> > > >>>>> resulting
> > > >>>>> flame graph from a perf profile run on the server side.
> > > >>>>> 
> > > >>>>> Is anyone else seeing this massive contention for the spin lock
> > > >>>>> in
> > > >>>>> __lwq_dequeue? As you can see, it appears to be dwarfing all
> > > >>>>> the
> > > >>>>> other
> > > >>>>> nfsd activity on the system in question here, being responsible
> > > >>>>> for
> > > >>>>> 45%
> > > >>>>> of all the perf hits.
> > 
> > Ouch. __lwq_dequeue() runs llist_reverse_order() under a spinlock.
> > 
> > llist_reverse_order() is an O(n) algorithm involving full length
> > linked list traversal. IOWs, it's a worst case cache miss algorithm
> > running under a spin lock. And then consider what happens when
> > enqueue processing is faster than dequeue processing.
> 
> My expectation was that if enqueue processing (incoming packets) was
> faster than dequeue processing (handling NFS requests) then there was a
> bottleneck elsewhere, and this one wouldn't be relevant.
> 
> It might be useful to measure how long the queue gets.

Thinking about this some more ....  if it did turn out that the queue
gets long, and maybe even if it didn't, we could reimplement lwq as a
simple linked list with head and tail pointers.

enqueue would be something like:

  new->next = NULL;
  old_tail = xchg(&q->tail, new);
  if (old_tail)
       /* dequeue of old_tail cannot succeed until this assignment completes */
       old_tail->next = new
  else
       q->head = new

dequeue would be

  spinlock()
  ret = q->head;
  if (ret) {
        while (ret->next == NULL && cmp_xchg(&q->tail, ret, NULL) != ret)
            /* wait for enqueue of q->tail to complete */
            cpu_relax();
  }
  cmp_xchg(&q->head, ret, ret->next);
  spin_unlock();

plus some barriers and a bit more careful thought.  That would put a
stronger limit on the time that the spinlock was held, though it could
still have to wait for another CPU occasionally.

NeilBrown

Re: knfsd performance

[Dave Chinner]

On Thu, Jun 20, 2024 at 07:25:15AM +1000, NeilBrown wrote:
> On Wed, 19 Jun 2024, NeilBrown wrote:
> > On Wed, 19 Jun 2024, Dave Chinner wrote:
> > > On Tue, Jun 18, 2024 at 07:54:43PM +0000, Chuck Lever III wrote  > On Jun 18, 2024, at 3:50 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> > > > > 
> > > > > On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
> > > > >> 
> > > > >> 
> > > > >>> On Jun 18, 2024, at 3:29 PM, Trond Myklebust
> > > > >>> <trondmy@hammerspace.com> wrote:
> > > > >>> 
> > > > >>> On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
> > > > >>>> 
> > > > >>>> 
> > > > >>>>> On Jun 18, 2024, at 2:32 PM, Trond Myklebust
> > > > >>>>> <trondmy@hammerspace.com> wrote:
> > > > >>>>> 
> > > > >>>>> I recently back ported Neil's lwq code and sunrpc server
> > > > >>>>> changes to
> > > > >>>>> our
> > > > >>>>> 5.15.130 based kernel in the hope of improving the performance
> > > > >>>>> for
> > > > >>>>> our
> > > > >>>>> data servers.
> > > > >>>>> 
> > > > >>>>> Our performance team recently ran a fio workload on a client
> > > > >>>>> that
> > > > >>>>> was
> > > > >>>>> doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
> > > > >>>>> (infiniband) against that resulting server. I've attached the
> > > > >>>>> resulting
> > > > >>>>> flame graph from a perf profile run on the server side.
> > > > >>>>> 
> > > > >>>>> Is anyone else seeing this massive contention for the spin lock
> > > > >>>>> in
> > > > >>>>> __lwq_dequeue? As you can see, it appears to be dwarfing all
> > > > >>>>> the
> > > > >>>>> other
> > > > >>>>> nfsd activity on the system in question here, being responsible
> > > > >>>>> for
> > > > >>>>> 45%
> > > > >>>>> of all the perf hits.
> > > 
> > > Ouch. __lwq_dequeue() runs llist_reverse_order() under a spinlock.
> > > 
> > > llist_reverse_order() is an O(n) algorithm involving full length
> > > linked list traversal. IOWs, it's a worst case cache miss algorithm
> > > running under a spin lock. And then consider what happens when
> > > enqueue processing is faster than dequeue processing.
> > 
> > My expectation was that if enqueue processing (incoming packets) was
> > faster than dequeue processing (handling NFS requests) then there was a
> > bottleneck elsewhere, and this one wouldn't be relevant.
> > 
> > It might be useful to measure how long the queue gets.
> 
> Thinking about this some more ....  if it did turn out that the queue
> gets long, and maybe even if it didn't, we could reimplement lwq as a
> simple linked list with head and tail pointers.
> 
> enqueue would be something like:
> 
>   new->next = NULL;
>   old_tail = xchg(&q->tail, new);
>   if (old_tail)
>        /* dequeue of old_tail cannot succeed until this assignment completes */
>        old_tail->next = new
>   else
>        q->head = new
>
> dequeue would be
> 
>   spinlock()
>   ret = q->head;
>   if (ret) {
>         while (ret->next == NULL && cmp_xchg(&q->tail, ret, NULL) != ret)
>             /* wait for enqueue of q->tail to complete */
>             cpu_relax();
>   }
>   cmp_xchg(&q->head, ret, ret->next);
>   spin_unlock();

That might work, but I suspect that it's still only putting off the
inevitable.

Doing the dequeue purely with atomic operations might be possible,
but it's not immediately obvious to me how to solve both head/tail
race conditions with atomic operations. I can work out an algorithm
that makes enqueue safe against dequeue races (or vice versa), but I
can't also get the logic on the opposite side to also be safe.

I'll let it bounce around my head a bit more...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: knfsd performance

[Jeff Layton]

On Thu, 2024-06-20 at 12:29 +1000, Dave Chinner wrote:
> On Thu, Jun 20, 2024 at 07:25:15AM +1000, NeilBrown wrote:
> > On Wed, 19 Jun 2024, NeilBrown wrote:
> > > On Wed, 19 Jun 2024, Dave Chinner wrote:
> > > > On Tue, Jun 18, 2024 at 07:54:43PM +0000, Chuck Lever III wrote  > On Jun 18, 2024, at 3:50 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> > > > > > 
> > > > > > On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > > On Jun 18, 2024, at 3:29 PM, Trond Myklebust
> > > > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > > > 
> > > > > > > > On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > On Jun 18, 2024, at 2:32 PM, Trond Myklebust
> > > > > > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > > > > > 
> > > > > > > > > > I recently back ported Neil's lwq code and sunrpc server
> > > > > > > > > > changes to
> > > > > > > > > > our
> > > > > > > > > > 5.15.130 based kernel in the hope of improving the performance
> > > > > > > > > > for
> > > > > > > > > > our
> > > > > > > > > > data servers.
> > > > > > > > > > 
> > > > > > > > > > Our performance team recently ran a fio workload on a client
> > > > > > > > > > that
> > > > > > > > > > was
> > > > > > > > > > doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
> > > > > > > > > > (infiniband) against that resulting server. I've attached the
> > > > > > > > > > resulting
> > > > > > > > > > flame graph from a perf profile run on the server side.
> > > > > > > > > > 
> > > > > > > > > > Is anyone else seeing this massive contention for the spin lock
> > > > > > > > > > in
> > > > > > > > > > __lwq_dequeue? As you can see, it appears to be dwarfing all
> > > > > > > > > > the
> > > > > > > > > > other
> > > > > > > > > > nfsd activity on the system in question here, being responsible
> > > > > > > > > > for
> > > > > > > > > > 45%
> > > > > > > > > > of all the perf hits.
> > > > 
> > > > Ouch. __lwq_dequeue() runs llist_reverse_order() under a spinlock.
> > > > 
> > > > llist_reverse_order() is an O(n) algorithm involving full length
> > > > linked list traversal. IOWs, it's a worst case cache miss algorithm
> > > > running under a spin lock. And then consider what happens when
> > > > enqueue processing is faster than dequeue processing.
> > > 
> > > My expectation was that if enqueue processing (incoming packets) was
> > > faster than dequeue processing (handling NFS requests) then there was a
> > > bottleneck elsewhere, and this one wouldn't be relevant.
> > > 
> > > It might be useful to measure how long the queue gets.
> > 
> > Thinking about this some more ....  if it did turn out that the queue
> > gets long, and maybe even if it didn't, we could reimplement lwq as a
> > simple linked list with head and tail pointers.
> > 
> > enqueue would be something like:
> > 
> >   new->next = NULL;
> >   old_tail = xchg(&q->tail, new);
> >   if (old_tail)
> >        /* dequeue of old_tail cannot succeed until this assignment completes */
> >        old_tail->next = new
> >   else
> >        q->head = new
> > 
> > dequeue would be
> > 
> >   spinlock()
> >   ret = q->head;
> >   if (ret) {
> >         while (ret->next == NULL && cmp_xchg(&q->tail, ret, NULL) != ret)
> >             /* wait for enqueue of q->tail to complete */
> >             cpu_relax();
> >   }
> >   cmp_xchg(&q->head, ret, ret->next);
> >   spin_unlock();
> 
> That might work, but I suspect that it's still only putting off the
> inevitable.
> 
> Doing the dequeue purely with atomic operations might be possible,
> but it's not immediately obvious to me how to solve both head/tail
> race conditions with atomic operations. I can work out an algorithm
> that makes enqueue safe against dequeue races (or vice versa), but I
> can't also get the logic on the opposite side to also be safe.
> 
> I'll let it bounce around my head a bit more...
> 

The latest version of the multigrain timestamp patches doesn't use it,
but Jan Kara pointed out to me that there is a cmpxchg128 function in
the kernel. It's only defined for some arches (x86, s390, and aarch64,
I think), but maybe that could be used here.
-- 
Jeff Layton <jlayton@kernel.org>

Re: knfsd performance

[Chuck Lever III]

> On Jun 19, 2024, at 10:29 PM, Dave Chinner <david@fromorbit.com> wrote:
> 
> On Thu, Jun 20, 2024 at 07:25:15AM +1000, NeilBrown wrote:
>> On Wed, 19 Jun 2024, NeilBrown wrote:
>>> On Wed, 19 Jun 2024, Dave Chinner wrote:
>>>> On Tue, Jun 18, 2024 at 07:54:43PM +0000, Chuck Lever III wrote  > On Jun 18, 2024, at 3:50 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
>>>>>> 
>>>>>> On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>> On Jun 18, 2024, at 3:29 PM, Trond Myklebust
>>>>>>>> <trondmy@hammerspace.com> wrote:
>>>>>>>> 
>>>>>>>> On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> On Jun 18, 2024, at 2:32 PM, Trond Myklebust
>>>>>>>>>> <trondmy@hammerspace.com> wrote:
>>>>>>>>>> 
>>>>>>>>>> I recently back ported Neil's lwq code and sunrpc server
>>>>>>>>>> changes to
>>>>>>>>>> our
>>>>>>>>>> 5.15.130 based kernel in the hope of improving the performance
>>>>>>>>>> for
>>>>>>>>>> our
>>>>>>>>>> data servers.
>>>>>>>>>> 
>>>>>>>>>> Our performance team recently ran a fio workload on a client
>>>>>>>>>> that
>>>>>>>>>> was
>>>>>>>>>> doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
>>>>>>>>>> (infiniband) against that resulting server. I've attached the
>>>>>>>>>> resulting
>>>>>>>>>> flame graph from a perf profile run on the server side.
>>>>>>>>>> 
>>>>>>>>>> Is anyone else seeing this massive contention for the spin lock
>>>>>>>>>> in
>>>>>>>>>> __lwq_dequeue? As you can see, it appears to be dwarfing all
>>>>>>>>>> the
>>>>>>>>>> other
>>>>>>>>>> nfsd activity on the system in question here, being responsible
>>>>>>>>>> for
>>>>>>>>>> 45%
>>>>>>>>>> of all the perf hits.
>>>> 
>>>> Ouch. __lwq_dequeue() runs llist_reverse_order() under a spinlock.
>>>> 
>>>> llist_reverse_order() is an O(n) algorithm involving full length
>>>> linked list traversal. IOWs, it's a worst case cache miss algorithm
>>>> running under a spin lock. And then consider what happens when
>>>> enqueue processing is faster than dequeue processing.
>>> 
>>> My expectation was that if enqueue processing (incoming packets) was
>>> faster than dequeue processing (handling NFS requests) then there was a
>>> bottleneck elsewhere, and this one wouldn't be relevant.
>>> 
>>> It might be useful to measure how long the queue gets.
>> 
>> Thinking about this some more ....  if it did turn out that the queue
>> gets long, and maybe even if it didn't, we could reimplement lwq as a
>> simple linked list with head and tail pointers.
>> 
>> enqueue would be something like:
>> 
>>  new->next = NULL;
>>  old_tail = xchg(&q->tail, new);
>>  if (old_tail)
>>       /* dequeue of old_tail cannot succeed until this assignment completes */
>>       old_tail->next = new
>>  else
>>       q->head = new
>> 
>> dequeue would be
>> 
>>  spinlock()
>>  ret = q->head;
>>  if (ret) {
>>        while (ret->next == NULL && cmp_xchg(&q->tail, ret, NULL) != ret)
>>            /* wait for enqueue of q->tail to complete */
>>            cpu_relax();
>>  }
>>  cmp_xchg(&q->head, ret, ret->next);
>>  spin_unlock();
> 
> That might work, but I suspect that it's still only putting off the
> inevitable.
> 
> Doing the dequeue purely with atomic operations might be possible,
> but it's not immediately obvious to me how to solve both head/tail
> race conditions with atomic operations. I can work out an algorithm
> that makes enqueue safe against dequeue races (or vice versa), but I
> can't also get the logic on the opposite side to also be safe.
> 
> I'll let it bounce around my head a bit more...

I agree that O(n) dequeuing is potentially alarming.

Before we go too far down this path, I'd like to see
reproducible numbers that show there is a problem
when a recent upstream NFS server is properly set up
with a sensible number of threads and running a real
workload.

Otherwise there is a risk of introducing code in a
fundamental part of SunRPC that is optimized to the
point of brittleness, and for no good reason.

This is what keeps me from sleeping at night: [1]
See, it even has my name on it. :-)


--
Chuck Lever

[1] - https://www.linusakesson.net/programming/kernighans-lever/index.php

Re: knfsd performance

[NeilBrown]

On Thu, 20 Jun 2024, Jeff Layton wrote:
> On Thu, 2024-06-20 at 12:29 +1000, Dave Chinner wrote:
> > On Thu, Jun 20, 2024 at 07:25:15AM +1000, NeilBrown wrote:
> > > On Wed, 19 Jun 2024, NeilBrown wrote:
> > > > On Wed, 19 Jun 2024, Dave Chinner wrote:
> > > > > On Tue, Jun 18, 2024 at 07:54:43PM +0000, Chuck Lever III wrote  > On Jun 18, 2024, at 3:50 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> > > > > > > 
> > > > > > > On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > On Jun 18, 2024, at 3:29 PM, Trond Myklebust
> > > > > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > > > > 
> > > > > > > > > On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > > On Jun 18, 2024, at 2:32 PM, Trond Myklebust
> > > > > > > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > > > > > > 
> > > > > > > > > > > I recently back ported Neil's lwq code and sunrpc server
> > > > > > > > > > > changes to
> > > > > > > > > > > our
> > > > > > > > > > > 5.15.130 based kernel in the hope of improving the performance
> > > > > > > > > > > for
> > > > > > > > > > > our
> > > > > > > > > > > data servers.
> > > > > > > > > > > 
> > > > > > > > > > > Our performance team recently ran a fio workload on a client
> > > > > > > > > > > that
> > > > > > > > > > > was
> > > > > > > > > > > doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
> > > > > > > > > > > (infiniband) against that resulting server. I've attached the
> > > > > > > > > > > resulting
> > > > > > > > > > > flame graph from a perf profile run on the server side.
> > > > > > > > > > > 
> > > > > > > > > > > Is anyone else seeing this massive contention for the spin lock
> > > > > > > > > > > in
> > > > > > > > > > > __lwq_dequeue? As you can see, it appears to be dwarfing all
> > > > > > > > > > > the
> > > > > > > > > > > other
> > > > > > > > > > > nfsd activity on the system in question here, being responsible
> > > > > > > > > > > for
> > > > > > > > > > > 45%
> > > > > > > > > > > of all the perf hits.
> > > > > 
> > > > > Ouch. __lwq_dequeue() runs llist_reverse_order() under a spinlock.
> > > > > 
> > > > > llist_reverse_order() is an O(n) algorithm involving full length
> > > > > linked list traversal. IOWs, it's a worst case cache miss algorithm
> > > > > running under a spin lock. And then consider what happens when
> > > > > enqueue processing is faster than dequeue processing.
> > > > 
> > > > My expectation was that if enqueue processing (incoming packets) was
> > > > faster than dequeue processing (handling NFS requests) then there was a
> > > > bottleneck elsewhere, and this one wouldn't be relevant.
> > > > 
> > > > It might be useful to measure how long the queue gets.
> > > 
> > > Thinking about this some more ....  if it did turn out that the queue
> > > gets long, and maybe even if it didn't, we could reimplement lwq as a
> > > simple linked list with head and tail pointers.
> > > 
> > > enqueue would be something like:
> > > 
> > >   new->next = NULL;
> > >   old_tail = xchg(&q->tail, new);
> > >   if (old_tail)
> > >        /* dequeue of old_tail cannot succeed until this assignment completes */
> > >        old_tail->next = new
> > >   else
> > >        q->head = new
> > > 
> > > dequeue would be
> > > 
> > >   spinlock()
> > >   ret = q->head;
> > >   if (ret) {
> > >         while (ret->next == NULL && cmp_xchg(&q->tail, ret, NULL) != ret)
> > >             /* wait for enqueue of q->tail to complete */
> > >             cpu_relax();
> > >   }
> > >   cmp_xchg(&q->head, ret, ret->next);
> > >   spin_unlock();
> > 
> > That might work, but I suspect that it's still only putting off the
> > inevitable.
> > 
> > Doing the dequeue purely with atomic operations might be possible,
> > but it's not immediately obvious to me how to solve both head/tail
> > race conditions with atomic operations. I can work out an algorithm
> > that makes enqueue safe against dequeue races (or vice versa), but I
> > can't also get the logic on the opposite side to also be safe.
> > 
> > I'll let it bounce around my head a bit more...
> > 
> 
> The latest version of the multigrain timestamp patches doesn't use it,
> but Jan Kara pointed out to me that there is a cmpxchg128 function in
> the kernel. It's only defined for some arches (x86, s390, and aarch64,
> I think), but maybe that could be used here.

One of the particularly pernicious scenarios that a non-locking approach
needs to deal with is if two threads try to dequeue at the same time and
just before the atomic ops which commits the dequeue, one of the threads
is suspends - e.g. by an interrupt or VM pauses or whatever.
While that thread is paused the other thread completes the dequeue,
consumes the event, recycles the object, and it ends up back at the
start of the queue, though this time with a different ->next pointer.
The paused thread then wakes up and the cmpxchg succeeds but does the
wrong thing.

This can be addressed with ll/sc because the sc will always fail after a
long pause like that.  With cmpxchg we could fix it by including a large
sequence counter with the address in the atomic exchange.  That is where
cmpxchg128 comes in.

So:
   head = q->head
   ctr = q->ctr
   new = head->next
   if (cmpxchg128(q, [head,ctr] [new,ctr+1]) fails)
      goto So;

would avoid that problem as the ctr would be different - and a 64 bit
counter never cycles.

This only solves that particular issue.  There are other more obvious
races but I think it could be made to work.  We'd need a spinlock
fallback of other archs of course.

Thanks,
NeilBrown

Re: knfsd performance

[NeilBrown]

On Fri, 21 Jun 2024, Chuck Lever III wrote:
> 
> 
> > On Jun 19, 2024, at 10:29 PM, Dave Chinner <david@fromorbit.com> wrote:
> > 
> > On Thu, Jun 20, 2024 at 07:25:15AM +1000, NeilBrown wrote:
> >> On Wed, 19 Jun 2024, NeilBrown wrote:
> >>> On Wed, 19 Jun 2024, Dave Chinner wrote:
> >>>> On Tue, Jun 18, 2024 at 07:54:43PM +0000, Chuck Lever III wrote  > On Jun 18, 2024, at 3:50 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> >>>>>> 
> >>>>>> On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III wrote:
> >>>>>>> 
> >>>>>>> 
> >>>>>>>> On Jun 18, 2024, at 3:29 PM, Trond Myklebust
> >>>>>>>> <trondmy@hammerspace.com> wrote:
> >>>>>>>> 
> >>>>>>>> On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III wrote:
> >>>>>>>>> 
> >>>>>>>>> 
> >>>>>>>>>> On Jun 18, 2024, at 2:32 PM, Trond Myklebust
> >>>>>>>>>> <trondmy@hammerspace.com> wrote:
> >>>>>>>>>> 
> >>>>>>>>>> I recently back ported Neil's lwq code and sunrpc server
> >>>>>>>>>> changes to
> >>>>>>>>>> our
> >>>>>>>>>> 5.15.130 based kernel in the hope of improving the performance
> >>>>>>>>>> for
> >>>>>>>>>> our
> >>>>>>>>>> data servers.
> >>>>>>>>>> 
> >>>>>>>>>> Our performance team recently ran a fio workload on a client
> >>>>>>>>>> that
> >>>>>>>>>> was
> >>>>>>>>>> doing 100% NFSv3 reads in O_DIRECT mode over an RDMA connection
> >>>>>>>>>> (infiniband) against that resulting server. I've attached the
> >>>>>>>>>> resulting
> >>>>>>>>>> flame graph from a perf profile run on the server side.
> >>>>>>>>>> 
> >>>>>>>>>> Is anyone else seeing this massive contention for the spin lock
> >>>>>>>>>> in
> >>>>>>>>>> __lwq_dequeue? As you can see, it appears to be dwarfing all
> >>>>>>>>>> the
> >>>>>>>>>> other
> >>>>>>>>>> nfsd activity on the system in question here, being responsible
> >>>>>>>>>> for
> >>>>>>>>>> 45%
> >>>>>>>>>> of all the perf hits.
> >>>> 
> >>>> Ouch. __lwq_dequeue() runs llist_reverse_order() under a spinlock.
> >>>> 
> >>>> llist_reverse_order() is an O(n) algorithm involving full length
> >>>> linked list traversal. IOWs, it's a worst case cache miss algorithm
> >>>> running under a spin lock. And then consider what happens when
> >>>> enqueue processing is faster than dequeue processing.
> >>> 
> >>> My expectation was that if enqueue processing (incoming packets) was
> >>> faster than dequeue processing (handling NFS requests) then there was a
> >>> bottleneck elsewhere, and this one wouldn't be relevant.
> >>> 
> >>> It might be useful to measure how long the queue gets.
> >> 
> >> Thinking about this some more ....  if it did turn out that the queue
> >> gets long, and maybe even if it didn't, we could reimplement lwq as a
> >> simple linked list with head and tail pointers.
> >> 
> >> enqueue would be something like:
> >> 
> >>  new->next = NULL;
> >>  old_tail = xchg(&q->tail, new);
> >>  if (old_tail)
> >>       /* dequeue of old_tail cannot succeed until this assignment completes */
> >>       old_tail->next = new
> >>  else
> >>       q->head = new
> >> 
> >> dequeue would be
> >> 
> >>  spinlock()
> >>  ret = q->head;
> >>  if (ret) {
> >>        while (ret->next == NULL && cmp_xchg(&q->tail, ret, NULL) != ret)
> >>            /* wait for enqueue of q->tail to complete */
> >>            cpu_relax();
> >>  }
> >>  cmp_xchg(&q->head, ret, ret->next);
> >>  spin_unlock();
> > 
> > That might work, but I suspect that it's still only putting off the
> > inevitable.
> > 
> > Doing the dequeue purely with atomic operations might be possible,
> > but it's not immediately obvious to me how to solve both head/tail
> > race conditions with atomic operations. I can work out an algorithm
> > that makes enqueue safe against dequeue races (or vice versa), but I
> > can't also get the logic on the opposite side to also be safe.
> > 
> > I'll let it bounce around my head a bit more...
> 
> I agree that O(n) dequeuing is potentially alarming.

Only O(n) 1/n of the time.  On average it is still constant time.

> 
> Before we go too far down this path, I'd like to see
> reproducible numbers that show there is a problem
> when a recent upstream NFS server is properly set up
> with a sensible number of threads and running a real
> workload.

Question for Trond: was nconnect configured, or was there only a single
connection?

With a single connection there is only ever zero or one xprt in the
queue to be dequeued, and if there are zero we don't take the lock.

With 16 connections they might always be busy so as soon as a request is
read from the connection it is requeued.  This means 1/16 of dequeue
operations would be slowish and the other 15/16 would be fast.

Maybe the 1/16 slow case could swamp the others but I'd be surprised.


> 
> Otherwise there is a risk of introducing code in a
> fundamental part of SunRPC that is optimized to the
> point of brittleness, and for no good reason.
> 
> This is what keeps me from sleeping at night: [1]
> See, it even has my name on it. :-)
> 
> 
> --
> Chuck Lever
> 
> [1] - https://www.linusakesson.net/programming/kernighans-lever/index.php

The conclusion of that article is that we SHOULD try to write clever
code because the effort invested in writing it and then debugging it
makes us cleverer so that the next time we can do even better.  That
thought would help me sleep at night!

NeilBrown

Re: knfsd performance

[Trond Myklebust]

On Fri, 2024-06-21 at 08:04 +1000, NeilBrown wrote:
> On Fri, 21 Jun 2024, Chuck Lever III wrote:
> > 
> > 
> > > On Jun 19, 2024, at 10:29 PM, Dave Chinner <david@fromorbit.com>
> > > wrote:
> > > 
> > > On Thu, Jun 20, 2024 at 07:25:15AM +1000, NeilBrown wrote:
> > > > On Wed, 19 Jun 2024, NeilBrown wrote:
> > > > > On Wed, 19 Jun 2024, Dave Chinner wrote:
> > > > > > On Tue, Jun 18, 2024 at 07:54:43PM +0000, Chuck Lever III
> > > > > > wrote  > On Jun 18, 2024, at 3:50 PM, Trond Myklebust
> > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > > > 
> > > > > > > > On Tue, 2024-06-18 at 19:39 +0000, Chuck Lever III
> > > > > > > > wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > On Jun 18, 2024, at 3:29 PM, Trond Myklebust
> > > > > > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > > > > > 
> > > > > > > > > > On Tue, 2024-06-18 at 18:40 +0000, Chuck Lever III
> > > > > > > > > > wrote:
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > > On Jun 18, 2024, at 2:32 PM, Trond Myklebust
> > > > > > > > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > > > > > > > 
> > > > > > > > > > > > I recently back ported Neil's lwq code and
> > > > > > > > > > > > sunrpc server
> > > > > > > > > > > > changes to
> > > > > > > > > > > > our
> > > > > > > > > > > > 5.15.130 based kernel in the hope of improving
> > > > > > > > > > > > the performance
> > > > > > > > > > > > for
> > > > > > > > > > > > our
> > > > > > > > > > > > data servers.
> > > > > > > > > > > > 
> > > > > > > > > > > > Our performance team recently ran a fio
> > > > > > > > > > > > workload on a client
> > > > > > > > > > > > that
> > > > > > > > > > > > was
> > > > > > > > > > > > doing 100% NFSv3 reads in O_DIRECT mode over an
> > > > > > > > > > > > RDMA connection
> > > > > > > > > > > > (infiniband) against that resulting server.
> > > > > > > > > > > > I've attached the
> > > > > > > > > > > > resulting
> > > > > > > > > > > > flame graph from a perf profile run on the
> > > > > > > > > > > > server side.
> > > > > > > > > > > > 
> > > > > > > > > > > > Is anyone else seeing this massive contention
> > > > > > > > > > > > for the spin lock
> > > > > > > > > > > > in
> > > > > > > > > > > > __lwq_dequeue? As you can see, it appears to be
> > > > > > > > > > > > dwarfing all
> > > > > > > > > > > > the
> > > > > > > > > > > > other
> > > > > > > > > > > > nfsd activity on the system in question here,
> > > > > > > > > > > > being responsible
> > > > > > > > > > > > for
> > > > > > > > > > > > 45%
> > > > > > > > > > > > of all the perf hits.
> > > > > > 
> > > > > > Ouch. __lwq_dequeue() runs llist_reverse_order() under a
> > > > > > spinlock.
> > > > > > 
> > > > > > llist_reverse_order() is an O(n) algorithm involving full
> > > > > > length
> > > > > > linked list traversal. IOWs, it's a worst case cache miss
> > > > > > algorithm
> > > > > > running under a spin lock. And then consider what happens
> > > > > > when
> > > > > > enqueue processing is faster than dequeue processing.
> > > > > 
> > > > > My expectation was that if enqueue processing (incoming
> > > > > packets) was
> > > > > faster than dequeue processing (handling NFS requests) then
> > > > > there was a
> > > > > bottleneck elsewhere, and this one wouldn't be relevant.
> > > > > 
> > > > > It might be useful to measure how long the queue gets.
> > > > 
> > > > Thinking about this some more ....  if it did turn out that the
> > > > queue
> > > > gets long, and maybe even if it didn't, we could reimplement
> > > > lwq as a
> > > > simple linked list with head and tail pointers.
> > > > 
> > > > enqueue would be something like:
> > > > 
> > > >  new->next = NULL;
> > > >  old_tail = xchg(&q->tail, new);
> > > >  if (old_tail)
> > > >       /* dequeue of old_tail cannot succeed until this
> > > > assignment completes */
> > > >       old_tail->next = new
> > > >  else
> > > >       q->head = new
> > > > 
> > > > dequeue would be
> > > > 
> > > >  spinlock()
> > > >  ret = q->head;
> > > >  if (ret) {
> > > >        while (ret->next == NULL && cmp_xchg(&q->tail, ret,
> > > > NULL) != ret)
> > > >            /* wait for enqueue of q->tail to complete */
> > > >            cpu_relax();
> > > >  }
> > > >  cmp_xchg(&q->head, ret, ret->next);
> > > >  spin_unlock();
> > > 
> > > That might work, but I suspect that it's still only putting off
> > > the
> > > inevitable.
> > > 
> > > Doing the dequeue purely with atomic operations might be
> > > possible,
> > > but it's not immediately obvious to me how to solve both
> > > head/tail
> > > race conditions with atomic operations. I can work out an
> > > algorithm
> > > that makes enqueue safe against dequeue races (or vice versa),
> > > but I
> > > can't also get the logic on the opposite side to also be safe.
> > > 
> > > I'll let it bounce around my head a bit more...
> > 
> > I agree that O(n) dequeuing is potentially alarming.
> 
> Only O(n) 1/n of the time.  On average it is still constant time.
> 
> > 
> > Before we go too far down this path, I'd like to see
> > reproducible numbers that show there is a problem
> > when a recent upstream NFS server is properly set up
> > with a sensible number of threads and running a real
> > workload.

The flame graph is showing contention on a spin lock, so it is
affecting running threads only. It will be 100% insensitive to sleeping
threads.

Yes, there is the issue of NUMA and the resulting potential for cache
bouncing between CPUs, and I do agree that particular detail warrants a
re-test. However I don't see how the overall number of nfsd threads is
relevant. Nor do I see how the use of a random read workload in any way
invalidates the test.

> Question for Trond: was nconnect configured, or was there only a
> single
> connection?
> 
> With a single connection there is only ever zero or one xprt in the
> queue to be dequeued, and if there are zero we don't take the lock.
> 
> With 16 connections they might always be busy so as soon as a request
> is
> read from the connection it is requeued.  This means 1/16 of dequeue
> operations would be slowish and the other 15/16 would be fast.
> 
> Maybe the 1/16 slow case could swamp the others but I'd be surprised.
> 

We deliberately coded up commit b326df4a8ec6 ("NFS: enable nconnect for
RDMA") because of the poor performance of RDMA when compared to
nconnect enabled TCP on these tests. So yes, this particular test was
running with both RDMA and nconnect enabled.

However given that the contended spinlock is not per connection, but
rather per pool, I don't see how nconnect matters much either since all
that does is to increase the number of connections. The other obvious
way to do that is to increase the number of NFS clients.

> > 
> > Otherwise there is a risk of introducing code in a
> > fundamental part of SunRPC that is optimized to the
> > point of brittleness, and for no good reason.
> > 
> > This is what keeps me from sleeping at night: [1]
> > See, it even has my name on it. :-)
> > 
> > 
> > --
> > Chuck Lever
> > 
> > [1] -
> > https://www.linusakesson.net/programming/kernighans-lever/index.php
> 
> The conclusion of that article is that we SHOULD try to write clever
> code because the effort invested in writing it and then debugging it
> makes us cleverer so that the next time we can do even better.  That
> thought would help me sleep at night!
> 

I mostly sleep well.

However that doesn't stop me from thinking that we have a good reason
to address bottlenecks when they are as obvious as this.

Again, I'm happy to see if I can get hold of some more hardware to test
the NUMA hypothesis. However, if that retest shows the contention is
still present, then we should consider the proposals that have been
made in this thread.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com