Sure. Seems they crept in over time. I had some plans to write a
lockless HTB implementation. But with fq+EDT with BPF it seems that
it is no longer needed, we have a more generic/better solution. So
I dropped it. Also most folks should really be using fq, fq_codel,
etc. by default anyways. Using pfifo_fast alone is not ideal IMO.
Half a year later, we still have the NOLOCK implementation
present, and pfifo_fast still does set the TCQ_F_NOLOCK flag on itself.
And we've just been bitten by this very same race which appears to be
still unfixed, with single packet being stuck in pfifo_fast qdisc
basically indefinitely due to this very race that this whole thread began
with back in 2019.
Unless there are
(a) any nice ideas how to solve this in an elegant way without
(re-)introducing extra spinlock (Cong's fix) or
(b) any objections to revert as per the argumentation above
I'll be happy to send a revert of the whole NOLOCK implementation next
week.
Jiri
Feel free to revert it as the scorch wont end without a deluge.
From: Yunsheng Lin <hidden> Date: 2021-04-06 00:55:47
On 2021/4/3 20:23, Jiri Kosina wrote:
On Sat, 3 Apr 2021, Hillf Danton wrote:
quoted
quoted
quoted
quoted
Sure. Seems they crept in over time. I had some plans to write a
lockless HTB implementation. But with fq+EDT with BPF it seems that
it is no longer needed, we have a more generic/better solution. So
I dropped it. Also most folks should really be using fq, fq_codel,
etc. by default anyways. Using pfifo_fast alone is not ideal IMO.
Half a year later, we still have the NOLOCK implementation
present, and pfifo_fast still does set the TCQ_F_NOLOCK flag on itself.
And we've just been bitten by this very same race which appears to be
still unfixed, with single packet being stuck in pfifo_fast qdisc
basically indefinitely due to this very race that this whole thread began
with back in 2019.
Unless there are
(a) any nice ideas how to solve this in an elegant way without
(re-)introducing extra spinlock (Cong's fix) or
(b) any objections to revert as per the argumentation above
I'll be happy to send a revert of the whole NOLOCK implementation next
week.
Jiri
Feel free to revert it as the scorch wont end without a deluge.
I am still planning to have Yunsheng Lin's (CCing) fix [1] tested in the
coming days. If it works, then we can consider proceeding with it,
otherwise I am all for reverting the whole NOLOCK stuff.
Hi, Jiri
Do you have a reproducer that can be shared here?
With reproducer, I can debug and test it myself too.
Thanks.
I personally prefer to just revert that bit, as it brings more troubles
than gains. Even with Yunsheng's patch, there are still some issues.
Essentially, I think the core qdisc scheduling code is not ready for
lockless, just look at those NOLOCK checks in sch_generic.c. :-/
Thanks.
I personally prefer to just revert that bit, as it brings more troubles
than gains. Even with Yunsheng's patch, there are still some issues.
Essentially, I think the core qdisc scheduling code is not ready for
lockless, just look at those NOLOCK checks in sch_generic.c. :-/
I am also awared of the NOLOCK checks too:), and I am willing to
take care of it if that is possible.
As the number of cores in a system is increasing, it is the trend
to become lockless, right? Even there is only one cpu involved, the
spinlock taking and releasing takes about 30ns on our arm64 system
when CONFIG_PREEMPT_VOLUNTARY is enable(ip forwarding testing).
Currently I has three ideas to optimize the lockless qdisc:
1. implement the qdisc bypass for lockless qdisc too, see [1].
2. implement lockless enqueuing for lockless qdisc using the idea
from Jason and Toke. And it has a noticable proformance increase with
1-4 threads running using the below prototype based on ptr_ring.
static inline int __ptr_ring_multi_produce(struct ptr_ring *r, void *ptr)
{
int producer, next_producer;
do {
producer = READ_ONCE(r->producer);
if (unlikely(!r->size) || r->queue[producer])
return -ENOSPC;
next_producer = producer + 1;
if (unlikely(next_producer >= r->size))
next_producer = 0;
} while(cmpxchg_relaxed(&r->producer, producer, next_producer) != producer);
/* Make sure the pointer we are storing points to a valid data. */
/* Pairs with the dependency ordering in __ptr_ring_consume. */
smp_wmb();
WRITE_ONCE(r->queue[producer], ptr);
return 0;
}
3. Maybe it is possible to remove the netif_tx_lock for lockless qdisc
too, because dev_hard_start_xmit is also in the protection of
qdisc_run_begin()/qdisc_run_end()(if there is only one qdisc using
a netdev queue, which is true for pfifo_fast, I believe).
[1]. https://patchwork.kernel.org/project/netdevbpf/patch/1616404156-11772-1-git-send-email-linyunsheng@huawei.com/
From: Michal Kubecek <hidden> Date: 2021-04-06 07:07:05
On Tue, Apr 06, 2021 at 08:55:41AM +0800, Yunsheng Lin wrote:
Hi, Jiri
Do you have a reproducer that can be shared here?
With reproducer, I can debug and test it myself too.
I'm afraid we are not aware of a simple reproducer. As mentioned in the
original discussion, the race window is extremely small and the other
thread has to do quite a lot in the meantime which is probably why, as
far as I know, this was never observed on real hardware, only in
virtualization environments. NFS may also be important as, IIUC, it can
often issue an RPC request from a different CPU right after a data
transfer. Perhaps you could cheat a bit and insert a random delay
between the empty queue check and releasing q->seqlock to make it more
likely to happen.
Other than that, it's rather just "run this complex software in a xen VM
and wait".
Michal
I personally prefer to just revert that bit, as it brings more troubles
than gains. Even with Yunsheng's patch, there are still some issues.
Essentially, I think the core qdisc scheduling code is not ready for
lockless, just look at those NOLOCK checks in sch_generic.c. :-/
I am also awared of the NOLOCK checks too:), and I am willing to
take care of it if that is possible.
As the number of cores in a system is increasing, it is the trend
to become lockless, right? Even there is only one cpu involved, the
spinlock taking and releasing takes about 30ns on our arm64 system
when CONFIG_PREEMPT_VOLUNTARY is enable(ip forwarding testing).
I agree with the benefits but currently the situation is that we have
a race condition affecting the default qdisc which is being hit in
production and can cause serious trouble which is made worse by commit
1f3279ae0c13 ("tcp: avoid retransmits of TCP packets hanging in host
queues") preventing the retransmits of the stuck packet being sent.
Perhaps rather than patching over current implementation which requires
more and more complicated hacks to work around the fact that we cannot
make the "queue is empty" check and leaving the critical section atomic,
it would make sense to reimplement it in a way which would allow us
making it atomic.
Michal
On Tue, Apr 06, 2021 at 08:55:41AM +0800, Yunsheng Lin wrote:
quoted
Hi, Jiri
Do you have a reproducer that can be shared here?
With reproducer, I can debug and test it myself too.
I'm afraid we are not aware of a simple reproducer. As mentioned in the
original discussion, the race window is extremely small and the other
thread has to do quite a lot in the meantime which is probably why, as
far as I know, this was never observed on real hardware, only in
virtualization environments. NFS may also be important as, IIUC, it can
often issue an RPC request from a different CPU right after a data
transfer. Perhaps you could cheat a bit and insert a random delay
between the empty queue check and releasing q->seqlock to make it more
likely to happen.
Other than that, it's rather just "run this complex software in a xen VM
and wait".
Being the one who has managed to reproduce the issue I can share my
setup, maybe you can setup something similar (we have seen the issue
with this kind of setup on two different machines).
I'm using a physical machine with 72 cpus and 48 GB of memory. It is
running Xen as virtualization platform.
Xen dom0 is limited to 40 vcpus and 32 GB of memory, the dom0 vcpus are
limited to run on the first 40 physical cpus (no idea whether that
matters, though).
In a guest with 16 vcpu and 8GB of memory I'm running 8 parallel
sysbench instances in a loop, those instances are prepared via
sysbench --file-test-mode=rndrd --test=fileio prepare
and then started in a do while loop via:
sysbench --test=fileio --file-test-mode=rndrw --rand-seed=0
--max-time=300 --max-requests=0 run
Each instance is using a dedicated NFS mount to run on. The NFS
server for the 8 mounts is running in dom0 of the same server, the
data of the NFS shares is located in a RAM disk (size is a little bit
above 16GB). The shares are mounted in the guest with:
mount -t nfs -o
rw,proto=tcp,nolock,nfsvers=3,rsize=65536,wsize=65536,nosharetransport
dom0:/ramdisk/share[1-8] /mnt[1-8]
The guests vcpus are limited to run on physical cpus 40-55, on the same
physical cpus I have 16 small guests running eating up cpu time, each of
those guests is pinned to one of the physical cpus 40-55.
That's basically it. All you need to do is to watch out for sysbench
reporting maximum latencies above one second or so (in my setup there
are latencies of several minutes at least once each hour of testing).
In case you'd like to have some more details about the setup don't
hesitate to contact me directly. I can provide you with some scripts
and config runes if you want.
Juergen
From: Yunsheng Lin <hidden> Date: 2021-04-06 12:18:00
On 2021/4/6 18:13, Juergen Gross wrote:
On 06.04.21 09:06, Michal Kubecek wrote:
quoted
On Tue, Apr 06, 2021 at 08:55:41AM +0800, Yunsheng Lin wrote:
quoted
Hi, Jiri
Do you have a reproducer that can be shared here?
With reproducer, I can debug and test it myself too.
I'm afraid we are not aware of a simple reproducer. As mentioned in the
original discussion, the race window is extremely small and the other
thread has to do quite a lot in the meantime which is probably why, as
far as I know, this was never observed on real hardware, only in
virtualization environments. NFS may also be important as, IIUC, it can
often issue an RPC request from a different CPU right after a data
transfer. Perhaps you could cheat a bit and insert a random delay
between the empty queue check and releasing q->seqlock to make it more
likely to happen.
Other than that, it's rather just "run this complex software in a xen VM
and wait".
Being the one who has managed to reproduce the issue I can share my
setup, maybe you can setup something similar (we have seen the issue
with this kind of setup on two different machines).
I'm using a physical machine with 72 cpus and 48 GB of memory. It is
running Xen as virtualization platform.
Xen dom0 is limited to 40 vcpus and 32 GB of memory, the dom0 vcpus are
limited to run on the first 40 physical cpus (no idea whether that
matters, though).
In a guest with 16 vcpu and 8GB of memory I'm running 8 parallel
sysbench instances in a loop, those instances are prepared via
sysbench --file-test-mode=rndrd --test=fileio prepare
and then started in a do while loop via:
sysbench --test=fileio --file-test-mode=rndrw --rand-seed=0 --max-time=300 --max-requests=0 run
Each instance is using a dedicated NFS mount to run on. The NFS
server for the 8 mounts is running in dom0 of the same server, the
data of the NFS shares is located in a RAM disk (size is a little bit
above 16GB). The shares are mounted in the guest with:
mount -t nfs -o rw,proto=tcp,nolock,nfsvers=3,rsize=65536,wsize=65536,nosharetransport dom0:/ramdisk/share[1-8] /mnt[1-8]
The guests vcpus are limited to run on physical cpus 40-55, on the same
physical cpus I have 16 small guests running eating up cpu time, each of
those guests is pinned to one of the physical cpus 40-55.
That's basically it. All you need to do is to watch out for sysbench
reporting maximum latencies above one second or so (in my setup there
are latencies of several minutes at least once each hour of testing).
In case you'd like to have some more details about the setup don't
hesitate to contact me directly. I can provide you with some scripts
and config runes if you want.
The setup is rather complex, I just tried Michal' suggestion using
the below patch:
I personally prefer to just revert that bit, as it brings more troubles
than gains. Even with Yunsheng's patch, there are still some issues.
Essentially, I think the core qdisc scheduling code is not ready for
lockless, just look at those NOLOCK checks in sch_generic.c. :-/
I am also awared of the NOLOCK checks too:), and I am willing to
take care of it if that is possible.
As the number of cores in a system is increasing, it is the trend
to become lockless, right? Even there is only one cpu involved, the
spinlock taking and releasing takes about 30ns on our arm64 system
when CONFIG_PREEMPT_VOLUNTARY is enable(ip forwarding testing).
I agree with the benefits but currently the situation is that we have
a race condition affecting the default qdisc which is being hit in
production and can cause serious trouble which is made worse by commit
1f3279ae0c13 ("tcp: avoid retransmits of TCP packets hanging in host
queues") preventing the retransmits of the stuck packet being sent.
Perhaps rather than patching over current implementation which requires
more and more complicated hacks to work around the fact that we cannot
make the "queue is empty" check and leaving the critical section atomic,
it would make sense to reimplement it in a way which would allow us
making it atomic.
Yes, reimplementing that is also an option.
But what if reimplemention also has the same problem if we do not find
the root cause of this problem? I think it better to find the root cause
of it first?