On Thu, Mar 14, 2024 at 10:50:15PM +1000, Gavin Shan wrote: > On 3/14/24 21:50, Michael S. Tsirkin wrote: > > On Thu, Mar 14, 2024 at 08:15:22PM +1000, Gavin Shan wrote: > > > On 3/14/24 18:05, Michael S. Tsirkin wrote: > > > > On Thu, Mar 14, 2024 at 05:49:23PM +1000, Gavin Shan wrote: > > > > > The issue is reported by Yihuang Yu who have 'netperf' test on > > > > > NVidia's grace-grace and grace-hopper machines. The 'netperf' > > > > > client is started in the VM hosted by grace-hopper machine, > > > > > while the 'netperf' server is running on grace-grace machine. > > > > > > > > > > The VM is started with virtio-net and vhost has been enabled. > > > > > We observe a error message spew from VM and then soft-lockup > > > > > report. The error message indicates the data associated with > > > > > the descriptor (index: 135) has been released, and the queue > > > > > is marked as broken. It eventually leads to the endless effort > > > > > to fetch free buffer (skb) in drivers/net/virtio_net.c::start_xmit() > > > > > and soft-lockup. The stale index 135 is fetched from the available > > > > > ring and published to the used ring by vhost, meaning we have > > > > > disordred write to the available ring element and available index. > > > > > > > > > > /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 > > > > > \ > > > > > -accel kvm -machine virt,gic-version=host > > > > > \ > > > > > : > > > > > \ > > > > > -netdev tap,id=vnet0,vhost=on > > > > > \ > > > > > -device > > > > > virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0 \ > > > > > > > > > > [ 19.993158] virtio_net virtio1: output.0:id 135 is not a head! > > > > > > > > > > Fix the issue by replacing virtio_wmb(vq->weak_barriers) with stronger > > > > > virtio_mb(false), equivalent to replaced 'dmb' by 'dsb' instruction on > > > > > ARM64. It should work for other architectures, but performance loss is > > > > > expected. > > > > > > > > > > Cc: sta...@vger.kernel.org > > > > > Reported-by: Yihuang Yu <yi...@redhat.com> > > > > > Signed-off-by: Gavin Shan <gs...@redhat.com> > > > > > --- > > > > > drivers/virtio/virtio_ring.c | 12 +++++++++--- > > > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c > > > > > b/drivers/virtio/virtio_ring.c > > > > > index 49299b1f9ec7..7d852811c912 100644 > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > @@ -687,9 +687,15 @@ static inline int virtqueue_add_split(struct > > > > > virtqueue *_vq, > > > > > avail = vq->split.avail_idx_shadow & (vq->split.vring.num - 1); > > > > > vq->split.vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, > > > > > head); > > > > > - /* Descriptors and available array need to be set before we > > > > > expose the > > > > > - * new available array entries. */ > > > > > - virtio_wmb(vq->weak_barriers); > > > > > + /* > > > > > + * Descriptors and available array need to be set before we > > > > > expose > > > > > + * the new available array entries. virtio_wmb() should be > > > > > enough > > > > > + * to ensuere the order theoretically. However, a stronger > > > > > barrier > > > > > + * is needed by ARM64. Otherwise, the stale data can be observed > > > > > + * by the host (vhost). A stronger barrier should work for other > > > > > + * architectures, but performance loss is expected. > > > > > + */ > > > > > + virtio_mb(false); > > > > > > > > > > > > I don't get what is going on here. Any explanation why virtio_wmb is not > > > > enough besides "it does not work"? > > > > > > > > > > The change is replacing instruction "dmb" with "dsb". "dsb" is stronger > > > barrier > > > than "dmb" because "dsb" ensures that all memory accesses raised before > > > this > > > instruction is completed when the 'dsb' instruction completes. However, > > > "dmb" > > > doesn't guarantee the order of completion of the memory accesses. > > > > > > So 'vq->split.vring.avail->idx = cpu_to_virtio(_vq->vdev, > > > vq->split.avail_idx_shadow)' > > > can be completed before 'vq->split.vring.avail->ring[avail] = > > > cpu_to_virtio16(_vq->vdev, head)'. > > > > Completed as observed by which CPU? > > We have 2 writes that we want observed by another CPU in order. > > So if CPU observes a new value of idx we want it to see > > new value in ring. > > This is standard use of smp_wmb() > > How are these 2 writes different? > > > > What DMB does, is that is seems to ensure that effects > > of 'vq->split.vring.avail->idx = cpu_to_virtio(_vq->vdev, > > vq->split.avail_idx_shadow)' > > are observed after effects of > > 'vq->split.vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head)'. > > > > > > Completed as observed by the CPU where vhost worker is running. I don't think > DMB > does the work here. If I'm understanding correctly, DMB ensures the order of > these > two writes from the local CPU's standpoint.
No this makes no sense at all. All memory accesses are in order from local CPU standpoint. > The written data can be stored in local > CPU's cache, not flushed to DRAM and propogated to the cache of the far CPU > where > vhost worker is running. So DMB isn't ensuring the write data is observed > from the > far CPU. No but it has to ensure *ordering*. So if index is oberved then ring is observed too because there is a MB there when reading. > DSB ensures that the written data is observable from the far CPU immediately. > > > > > > > The stronger barrier 'dsb' ensures the completion order as we expected. > > > > > > virtio_wmb(true) virt_mb(false) > > > virt_wmb mb > > > __smp_wmb __mb > > > dmb(ishst) dsb(sy) > > > > First, why would you want a non smp barrier when you are > > synchronizing with another CPU? This is what smp_ means ... > > > > > > > Extraced from ARMv9 specificaton > > > ================================ > > > The DMB instruction is a memory barrier instruction that ensures the > > > relative > > > order of memory accesses before the barrier with memory accesses after the > > > barrier. The DMB instruction _does not_ ensure the completion of any of > > > the > > > memory accesses for which it ensures relative order. > > > > Isn't this exactly what we need? > > > > I don't think so. DMB gurantees the order of EXECUTION, but DSB gurantees the > order of COMPLETION. After the data store is executed, the written data can > be stored in local CPU's cache, far from completion. Only the instruction is > completed, the written data is synchronized to the far CPU where vhost worker > is running. > > Here is another paper talking about the difference between DMB and DSB. It's > explaining the difference between DMB/DSB better than ARMv9 specification. > However, it's hard for us to understand DMB/DSB work from the hardware level > based on the specification and paper. > > https://ipads.se.sjtu.edu.cn/_media/publications/liuppopp20.pdf > (section 2.2 Order-preserving Options) > (I'm extracting the relevant part as below) > > Data Memory Barrier (DMB) prevents reordering of memory accesses across the > barrier. > Instead of waiting for previous accesses to become observable in the > specified domain, > DMB only maintains the relative order between memory accesses. Meanwhile, DMB > does > not block any non-memory access operations. > > Data Synchronization Barrier (DSB) prevents reordering of any instructions > across > the barrier. DSB will make sure that all masters in the specified domain can > observe > the previous operations before issuing any subsequent instructions. > What you are describing is that smp_wmb is buggy on this platform. Poossible but are you sure? Write a small test and check. Leave virtio alone. > > > A DSB instruction is a memory barrier that ensures that memory accesses > > > that > > > occur before the DSB instruction have __completed__ before the completion > > > of > > > the DSB instruction. > > > > This seems appropriate for when you want to order things more > > strongly. I do not get why it's necessary here. > > > > Please refer to above explanations. > > > > In doing this, it acts as a stronger barrier than a DMB > > > and all ordering that is created by a DMB with specific options is also > > > generated > > > by a DSB with the same options. > > > > > > > > vq->split.avail_idx_shadow++; > > > > > vq->split.vring.avail->idx = cpu_to_virtio16(_vq->vdev, > > > > > > > > > > vq->split.avail_idx_shadow); > > > > > -- > > > > > 2.44.0 > > > > > > Thanks, > Gavin
