The issue was reported by Yihuang Yu on NVidia's grace-hopper (ARM64)
platform. The wrong head (available ring entry) is seen by the guest
when running 'netperf' on the guest and running 'netserver' on another
NVidia's grace-grace machine.
/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
-accel kvm -machine virt,gic-version=host -cpu host \
-smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
-m 4096M,slots=16,maxmem=64G \
-object memory-backend-ram,id=mem0,size=4096M \
: \
-netdev tap,id=tap0,vhost=true \
-device virtio-net-pci,bus=pcie.8,netdev=tap0,mac=52:54:00:f1:26:b0
:
guest# ifconfig eth0 | grep 'inet addr'
inet addr:10.26.1.220
guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
virtio_net virtio0: output.0:id 100 is not a head!
There is missed smp_rmb() in vhost_vq_avail_empty() and vhost_enable_notify().
Without smp_rmb(), vq->avail_idx is increased but the available ring
entries aren't arriving to vhost side yet. So a stale available ring
entry can be fetched in vhost_get_vq_desc().
Fix it by adding smp_rmb() in those two functions. Note that I need
two patches so that they can be easily picked up by the stable kernel.
With the changes, I'm unable to hit the issue again.
Gavin Shan (2):
vhost: Add smp_rmb() in vhost_vq_avail_empty()
vhost: Add smp_rmb() in vhost_enable_notify()
drivers/vhost/vhost.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
--
2.44.0
A smp_rmb() has been missed in vhost_vq_avail_empty(), spotted by
Will Deacon <[email protected]>. Otherwise, it's not ensured the
available ring entries pushed by guest can be observed by vhost
in time, leading to stale available ring entries fetched by vhost
in vhost_get_vq_desc(), as reported by Yihuang Yu on NVidia's
grace-hopper (ARM64) platform.
/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
-accel kvm -machine virt,gic-version=host -cpu host \
-smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
-m 4096M,slots=16,maxmem=64G \
-object memory-backend-ram,id=mem0,size=4096M \
: \
-netdev tap,id=vnet0,vhost=true \
-device virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0
:
guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
virtio_net virtio0: output.0:id 100 is not a head!
Add the missed smp_rmb() in vhost_vq_avail_empty(). Note that it
should be safe until vq->avail_idx is changed by commit 275bf960ac697
("vhost: better detection of available buffers").
Fixes: 275bf960ac697 ("vhost: better detection of available buffers")
Cc: <[email protected]> # v4.11+
Reported-by: Yihuang Yu <[email protected]>
Signed-off-by: Gavin Shan <[email protected]>
---
drivers/vhost/vhost.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 045f666b4f12..00445ab172b3 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2799,9 +2799,18 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
r = vhost_get_avail_idx(vq, &avail_idx);
if (unlikely(r))
return false;
+
vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
+ if (vq->avail_idx != vq->last_avail_idx) {
+ /* Similar to what's done in vhost_get_vq_desc(), we need
+ * to ensure the available ring entries have been exposed
+ * by guest.
+ */
+ smp_rmb();
+ return false;
+ }
- return vq->avail_idx == vq->last_avail_idx;
+ return true;
}
EXPORT_SYMBOL_GPL(vhost_vq_avail_empty);
--
2.44.0
A smp_rmb() has been missed in vhost_enable_notify(), inspired by
Will Deacon <[email protected]>. Otherwise, it's not ensured the
available ring entries pushed by guest can be observed by vhost
in time, leading to stale available ring entries fetched by vhost
in vhost_get_vq_desc(), as reported by Yihuang Yu on NVidia's
grace-hopper (ARM64) platform.
/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
-accel kvm -machine virt,gic-version=host -cpu host \
-smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
-m 4096M,slots=16,maxmem=64G \
-object memory-backend-ram,id=mem0,size=4096M \
: \
-netdev tap,id=vnet0,vhost=true \
-device virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0
:
guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
virtio_net virtio0: output.0:id 100 is not a head!
Add the missed smp_rmb() in vhost_enable_notify(). Note that it
should be safe until vq->avail_idx is changed by commit d3bb267bbdcb
("vhost: cache avail index in vhost_enable_notify()").
Fixes: d3bb267bbdcb ("vhost: cache avail index in vhost_enable_notify()")
Cc: <[email protected]> # v5.18+
Reported-by: Yihuang Yu <[email protected]>
Signed-off-by: Gavin Shan <[email protected]>
---
drivers/vhost/vhost.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 00445ab172b3..58f9d6a435f0 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2847,9 +2847,18 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
&vq->avail->idx, r);
return false;
}
+
vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
+ if (vq->avail_idx != vq->last_avail_idx) {
+ /* Similar to what's done in vhost_get_vq_desc(), we need
+ * to ensure the available ring entries have been exposed
+ * by guest.
+ */
+ smp_rmb();
+ return true;
+ }
- return vq->avail_idx != vq->last_avail_idx;
+ return false;
}
EXPORT_SYMBOL_GPL(vhost_enable_notify);
--
2.44.0
On 3/27/24 09:38, Gavin Shan wrote:
> The issue was reported by Yihuang Yu on NVidia's grace-hopper (ARM64)
> platform. The wrong head (available ring entry) is seen by the guest
> when running 'netperf' on the guest and running 'netserver' on another
> NVidia's grace-grace machine.
>
> /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> -accel kvm -machine virt,gic-version=host -cpu host \
> -smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
> -m 4096M,slots=16,maxmem=64G \
> -object memory-backend-ram,id=mem0,size=4096M \
> : \
> -netdev tap,id=tap0,vhost=true \
> -device virtio-net-pci,bus=pcie.8,netdev=tap0,mac=52:54:00:f1:26:b0
> :
> guest# ifconfig eth0 | grep 'inet addr'
> inet addr:10.26.1.220
> guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
> virtio_net virtio0: output.0:id 100 is not a head!
>
> There is missed smp_rmb() in vhost_vq_avail_empty() and vhost_enable_notify().
> Without smp_rmb(), vq->avail_idx is increased but the available ring
> entries aren't arriving to vhost side yet. So a stale available ring
> entry can be fetched in vhost_get_vq_desc().
>
> Fix it by adding smp_rmb() in those two functions. Note that I need
> two patches so that they can be easily picked up by the stable kernel.
> With the changes, I'm unable to hit the issue again.
>
> Gavin Shan (2):
> vhost: Add smp_rmb() in vhost_vq_avail_empty()
> vhost: Add smp_rmb() in vhost_enable_notify()
>
> drivers/vhost/vhost.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
Sorry, I was supposed to copy Will. Amending for it.
Thanks,
Gavin
On Wed, Mar 27, 2024 at 7:39 AM Gavin Shan <[email protected]> wrote:
>
> A smp_rmb() has been missed in vhost_vq_avail_empty(), spotted by
> Will Deacon <[email protected]>. Otherwise, it's not ensured the
> available ring entries pushed by guest can be observed by vhost
> in time, leading to stale available ring entries fetched by vhost
> in vhost_get_vq_desc(), as reported by Yihuang Yu on NVidia's
> grace-hopper (ARM64) platform.
>
> /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> -accel kvm -machine virt,gic-version=host -cpu host \
> -smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
> -m 4096M,slots=16,maxmem=64G \
> -object memory-backend-ram,id=mem0,size=4096M \
> : \
> -netdev tap,id=vnet0,vhost=true \
> -device virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0
> :
> guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
> virtio_net virtio0: output.0:id 100 is not a head!
>
> Add the missed smp_rmb() in vhost_vq_avail_empty(). Note that it
> should be safe until vq->avail_idx is changed by commit 275bf960ac697
> ("vhost: better detection of available buffers").
>
> Fixes: 275bf960ac697 ("vhost: better detection of available buffers")
> Cc: <[email protected]> # v4.11+
> Reported-by: Yihuang Yu <[email protected]>
> Signed-off-by: Gavin Shan <[email protected]>
> ---
> drivers/vhost/vhost.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 045f666b4f12..00445ab172b3 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2799,9 +2799,18 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> r = vhost_get_avail_idx(vq, &avail_idx);
> if (unlikely(r))
> return false;
> +
> vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
> + if (vq->avail_idx != vq->last_avail_idx) {
> + /* Similar to what's done in vhost_get_vq_desc(), we need
> + * to ensure the available ring entries have been exposed
> + * by guest.
> + */
We need to be more verbose here. For example, which load needs to be
ordered with which load.
The rmb in vhost_get_vq_desc() is used to order the load of avail idx
and the load of head. It is paired with e.g virtio_wmb() in
virtqueue_add_split().
vhost_vq_avail_empty() are mostly used as a hint in
vhost_net_busy_poll() which is under the protection of the vq mutex.
An exception is the tx_can_batch(), but in that case it doesn't even
want to read the head.
Thanks
> + smp_rmb();
> + return false;
> + }
>
> - return vq->avail_idx == vq->last_avail_idx;
> + return true;
> }
> EXPORT_SYMBOL_GPL(vhost_vq_avail_empty);
>
> --
> 2.44.0
>
On Wed, Mar 27, 2024 at 7:39 AM Gavin Shan <[email protected]> wrote:
>
> A smp_rmb() has been missed in vhost_enable_notify(), inspired by
> Will Deacon <[email protected]>. Otherwise, it's not ensured the
> available ring entries pushed by guest can be observed by vhost
> in time, leading to stale available ring entries fetched by vhost
> in vhost_get_vq_desc(), as reported by Yihuang Yu on NVidia's
> grace-hopper (ARM64) platform.
>
> /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> -accel kvm -machine virt,gic-version=host -cpu host \
> -smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
> -m 4096M,slots=16,maxmem=64G \
> -object memory-backend-ram,id=mem0,size=4096M \
> : \
> -netdev tap,id=vnet0,vhost=true \
> -device virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0
> :
> guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
> virtio_net virtio0: output.0:id 100 is not a head!
>
> Add the missed smp_rmb() in vhost_enable_notify(). Note that it
> should be safe until vq->avail_idx is changed by commit d3bb267bbdcb
> ("vhost: cache avail index in vhost_enable_notify()").
>
> Fixes: d3bb267bbdcb ("vhost: cache avail index in vhost_enable_notify()")
> Cc: <[email protected]> # v5.18+
> Reported-by: Yihuang Yu <[email protected]>
> Signed-off-by: Gavin Shan <[email protected]>
> ---
> drivers/vhost/vhost.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 00445ab172b3..58f9d6a435f0 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2847,9 +2847,18 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> &vq->avail->idx, r);
> return false;
> }
> +
> vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
> + if (vq->avail_idx != vq->last_avail_idx) {
> + /* Similar to what's done in vhost_get_vq_desc(), we need
> + * to ensure the available ring entries have been exposed
> + * by guest.
> + */
> + smp_rmb();
> + return true;
> + }
>
> - return vq->avail_idx != vq->last_avail_idx;
> + return false;
So we only care about the case when vhost_enable_notify() returns true.
In that case, I think you want to order with vhost_get_vq_desc():
last_avail_idx = vq->last_avail_idx;
if (vq->avail_idx == vq->last_avail_idx) { /* false */
}
vhost_get_avail_head(vq, &ring_head, last_avail_idx)
Assuming I understand the patch correctly.
Acked-by: Jason Wang <[email protected]>
Thanks
> }
> EXPORT_SYMBOL_GPL(vhost_enable_notify);
>
> --
> 2.44.0
>
On Wed, Mar 27, 2024 at 10:34 AM Jason Wang <[email protected]> wrote:
>
> On Wed, Mar 27, 2024 at 7:39 AM Gavin Shan <[email protected]> wrote:
> >
> > A smp_rmb() has been missed in vhost_vq_avail_empty(), spotted by
> > Will Deacon <[email protected]>. Otherwise, it's not ensured the
> > available ring entries pushed by guest can be observed by vhost
> > in time, leading to stale available ring entries fetched by vhost
> > in vhost_get_vq_desc(), as reported by Yihuang Yu on NVidia's
> > grace-hopper (ARM64) platform.
> >
> > /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> > -accel kvm -machine virt,gic-version=host -cpu host \
> > -smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
> > -m 4096M,slots=16,maxmem=64G \
> > -object memory-backend-ram,id=mem0,size=4096M \
> > : \
> > -netdev tap,id=vnet0,vhost=true \
> > -device virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0
> > :
> > guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
> > virtio_net virtio0: output.0:id 100 is not a head!
> >
> > Add the missed smp_rmb() in vhost_vq_avail_empty(). Note that it
> > should be safe until vq->avail_idx is changed by commit 275bf960ac697
> > ("vhost: better detection of available buffers").
> >
> > Fixes: 275bf960ac697 ("vhost: better detection of available buffers")
> > Cc: <[email protected]> # v4.11+
> > Reported-by: Yihuang Yu <[email protected]>
> > Signed-off-by: Gavin Shan <[email protected]>
> > ---
> > drivers/vhost/vhost.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 045f666b4f12..00445ab172b3 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2799,9 +2799,18 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> > r = vhost_get_avail_idx(vq, &avail_idx);
> > if (unlikely(r))
> > return false;
> > +
> > vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
> > + if (vq->avail_idx != vq->last_avail_idx) {
> > + /* Similar to what's done in vhost_get_vq_desc(), we need
> > + * to ensure the available ring entries have been exposed
> > + * by guest.
> > + */
>
> We need to be more verbose here. For example, which load needs to be
> ordered with which load.
>
> The rmb in vhost_get_vq_desc() is used to order the load of avail idx
> and the load of head. It is paired with e.g virtio_wmb() in
> virtqueue_add_split().
>
> vhost_vq_avail_empty() are mostly used as a hint in
> vhost_net_busy_poll() which is under the protection of the vq mutex.
>
> An exception is the tx_can_batch(), but in that case it doesn't even
> want to read the head.
Ok, if it is needed only in that path, maybe we can move the barriers there.
Thanks
>
> Thanks
>
>
> > + smp_rmb();
> > + return false;
> > + }
> >
> > - return vq->avail_idx == vq->last_avail_idx;
> > + return true;
> > }
> > EXPORT_SYMBOL_GPL(vhost_vq_avail_empty);
> >
> > --
> > 2.44.0
> >
On 3/27/24 12:44, Jason Wang wrote:
> On Wed, Mar 27, 2024 at 10:34 AM Jason Wang <[email protected]> wrote:
>> On Wed, Mar 27, 2024 at 7:39 AM Gavin Shan <[email protected]> wrote:
>>>
>>> A smp_rmb() has been missed in vhost_vq_avail_empty(), spotted by
>>> Will Deacon <[email protected]>. Otherwise, it's not ensured the
>>> available ring entries pushed by guest can be observed by vhost
>>> in time, leading to stale available ring entries fetched by vhost
>>> in vhost_get_vq_desc(), as reported by Yihuang Yu on NVidia's
>>> grace-hopper (ARM64) platform.
>>>
>>> /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>>> -accel kvm -machine virt,gic-version=host -cpu host \
>>> -smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
>>> -m 4096M,slots=16,maxmem=64G \
>>> -object memory-backend-ram,id=mem0,size=4096M \
>>> : \
>>> -netdev tap,id=vnet0,vhost=true \
>>> -device virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0
>>> :
>>> guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
>>> virtio_net virtio0: output.0:id 100 is not a head!
>>>
>>> Add the missed smp_rmb() in vhost_vq_avail_empty(). Note that it
>>> should be safe until vq->avail_idx is changed by commit 275bf960ac697
>>> ("vhost: better detection of available buffers").
>>>
>>> Fixes: 275bf960ac697 ("vhost: better detection of available buffers")
>>> Cc: <[email protected]> # v4.11+
>>> Reported-by: Yihuang Yu <[email protected]>
>>> Signed-off-by: Gavin Shan <[email protected]>
>>> ---
>>> drivers/vhost/vhost.c | 11 ++++++++++-
>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>>> index 045f666b4f12..00445ab172b3 100644
>>> --- a/drivers/vhost/vhost.c
>>> +++ b/drivers/vhost/vhost.c
>>> @@ -2799,9 +2799,18 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>>> r = vhost_get_avail_idx(vq, &avail_idx);
>>> if (unlikely(r))
>>> return false;
>>> +
>>> vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
>>> + if (vq->avail_idx != vq->last_avail_idx) {
>>> + /* Similar to what's done in vhost_get_vq_desc(), we need
>>> + * to ensure the available ring entries have been exposed
>>> + * by guest.
>>> + */
>>
>> We need to be more verbose here. For example, which load needs to be
>> ordered with which load.
>>
>> The rmb in vhost_get_vq_desc() is used to order the load of avail idx
>> and the load of head. It is paired with e.g virtio_wmb() in
>> virtqueue_add_split().
>>
>> vhost_vq_avail_empty() are mostly used as a hint in
>> vhost_net_busy_poll() which is under the protection of the vq mutex.
>>
>> An exception is the tx_can_batch(), but in that case it doesn't even
>> want to read the head.
>
> Ok, if it is needed only in that path, maybe we can move the barriers there.
>
[cc Will Deacon]
Jason, appreciate for your review and comments. I think PATCH[1/2] is
the fix for the hypothesis, meaning PATCH[2/2] is the real fix. However,
it would be nice to fix all of them in one shoot. I will try with PATCH[2/2]
only to see if our issue will disappear or not. However, the issue still
exists if PATCH[2/2] is missed.
Firstly, We were failing on the transmit queue and {tvq, rvq}->busyloop_timeout
== false if I remember correctly. So the added smp_rmb() in vhost_vq_avail_empty()
is only a concern to tx_can_batch(). A mutex isn't enough to ensure the order
for the available index and available ring entry (head). For example, vhost_vq_avail_empty()
called by tx_can_batch() can see next available index, but its corresponding
available ring entry (head) may not be seen by vhost yet if smp_rmb() is missed.
The next call to get_tx_bufs(), where the available ring entry (head) doesn't
arrived yet, leading to stale available ring entry (head) being fetched.
handle_tx_copy
get_tx_bufs // smp_rmb() won't be executed when vq->avail_idx != vq->last_avail_idx
tx_can_batch
vhost_vq_avail_empty // vq->avail_idx is updated from vq->avail->idx
The reason why I added smp_rmb() to vhost_vq_avail_empty() is because the function
is a exposed API, even it's only used by drivers/vhost/net.c at present. It means
the API has been broken internally. So it seems more appropriate to fix it up in
vhost_vq_avail_empty() so that the API's users needn't worry about the memory access
order.
>>
>>
>>> + smp_rmb();
>>> + return false;
>>> + }
>>>
>>> - return vq->avail_idx == vq->last_avail_idx;
>>> + return true;
>>> }
>>> EXPORT_SYMBOL_GPL(vhost_vq_avail_empty);
Thanks,
Gavin
On 3/27/24 12:41, Jason Wang wrote:
> On Wed, Mar 27, 2024 at 7:39 AM Gavin Shan <[email protected]> wrote:
>>
>> A smp_rmb() has been missed in vhost_enable_notify(), inspired by
>> Will Deacon <[email protected]>. Otherwise, it's not ensured the
>> available ring entries pushed by guest can be observed by vhost
>> in time, leading to stale available ring entries fetched by vhost
>> in vhost_get_vq_desc(), as reported by Yihuang Yu on NVidia's
>> grace-hopper (ARM64) platform.
>>
>> /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>> -accel kvm -machine virt,gic-version=host -cpu host \
>> -smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
>> -m 4096M,slots=16,maxmem=64G \
>> -object memory-backend-ram,id=mem0,size=4096M \
>> : \
>> -netdev tap,id=vnet0,vhost=true \
>> -device virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0
>> :
>> guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
>> virtio_net virtio0: output.0:id 100 is not a head!
>>
>> Add the missed smp_rmb() in vhost_enable_notify(). Note that it
>> should be safe until vq->avail_idx is changed by commit d3bb267bbdcb
>> ("vhost: cache avail index in vhost_enable_notify()").
>>
>> Fixes: d3bb267bbdcb ("vhost: cache avail index in vhost_enable_notify()")
>> Cc: <[email protected]> # v5.18+
>> Reported-by: Yihuang Yu <[email protected]>
>> Signed-off-by: Gavin Shan <[email protected]>
>> ---
>> drivers/vhost/vhost.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index 00445ab172b3..58f9d6a435f0 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -2847,9 +2847,18 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>> &vq->avail->idx, r);
>> return false;
>> }
>> +
>> vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
>> + if (vq->avail_idx != vq->last_avail_idx) {
>> + /* Similar to what's done in vhost_get_vq_desc(), we need
>> + * to ensure the available ring entries have been exposed
>> + * by guest.
>> + */
>> + smp_rmb();
>> + return true;
>> + }
>>
>> - return vq->avail_idx != vq->last_avail_idx;
>> + return false;
>
> So we only care about the case when vhost_enable_notify() returns true.
>
> In that case, I think you want to order with vhost_get_vq_desc():
>
> last_avail_idx = vq->last_avail_idx;
>
> if (vq->avail_idx == vq->last_avail_idx) { /* false */
> }
>
> vhost_get_avail_head(vq, &ring_head, last_avail_idx)
>
> Assuming I understand the patch correctly.
>
> Acked-by: Jason Wang <[email protected]>
>
Jason, thanks for your review and comments. Your understanding is exactly
what I understood.
>
>> }
>> EXPORT_SYMBOL_GPL(vhost_enable_notify);
>>
Thanks,
Gavin
On 3/27/24 14:08, Gavin Shan wrote:
> On 3/27/24 12:44, Jason Wang wrote:
>> On Wed, Mar 27, 2024 at 10:34 AM Jason Wang <[email protected]> wrote:
>>> On Wed, Mar 27, 2024 at 7:39 AM Gavin Shan <[email protected]> wrote:
>>>>
>>>> A smp_rmb() has been missed in vhost_vq_avail_empty(), spotted by
>>>> Will Deacon <[email protected]>. Otherwise, it's not ensured the
>>>> available ring entries pushed by guest can be observed by vhost
>>>> in time, leading to stale available ring entries fetched by vhost
>>>> in vhost_get_vq_desc(), as reported by Yihuang Yu on NVidia's
>>>> grace-hopper (ARM64) platform.
>>>>
>>>> /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>>>> -accel kvm -machine virt,gic-version=host -cpu host \
>>>> -smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
>>>> -m 4096M,slots=16,maxmem=64G \
>>>> -object memory-backend-ram,id=mem0,size=4096M \
>>>> : \
>>>> -netdev tap,id=vnet0,vhost=true \
>>>> -device virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0
>>>> :
>>>> guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
>>>> virtio_net virtio0: output.0:id 100 is not a head!
>>>>
>>>> Add the missed smp_rmb() in vhost_vq_avail_empty(). Note that it
>>>> should be safe until vq->avail_idx is changed by commit 275bf960ac697
>>>> ("vhost: better detection of available buffers").
>>>>
>>>> Fixes: 275bf960ac697 ("vhost: better detection of available buffers")
>>>> Cc: <[email protected]> # v4.11+
>>>> Reported-by: Yihuang Yu <[email protected]>
>>>> Signed-off-by: Gavin Shan <[email protected]>
>>>> ---
>>>> drivers/vhost/vhost.c | 11 ++++++++++-
>>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>>>> index 045f666b4f12..00445ab172b3 100644
>>>> --- a/drivers/vhost/vhost.c
>>>> +++ b/drivers/vhost/vhost.c
>>>> @@ -2799,9 +2799,18 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>>>> r = vhost_get_avail_idx(vq, &avail_idx);
>>>> if (unlikely(r))
>>>> return false;
>>>> +
>>>> vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
>>>> + if (vq->avail_idx != vq->last_avail_idx) {
>>>> + /* Similar to what's done in vhost_get_vq_desc(), we need
>>>> + * to ensure the available ring entries have been exposed
>>>> + * by guest.
>>>> + */
>>>
>>> We need to be more verbose here. For example, which load needs to be
>>> ordered with which load.
>>>
>>> The rmb in vhost_get_vq_desc() is used to order the load of avail idx
>>> and the load of head. It is paired with e.g virtio_wmb() in
>>> virtqueue_add_split().
>>>
>>> vhost_vq_avail_empty() are mostly used as a hint in
>>> vhost_net_busy_poll() which is under the protection of the vq mutex.
>>>
>>> An exception is the tx_can_batch(), but in that case it doesn't even
>>> want to read the head.
>>
>> Ok, if it is needed only in that path, maybe we can move the barriers there.
>>
>
> [cc Will Deacon]
>
> Jason, appreciate for your review and comments. I think PATCH[1/2] is
> the fix for the hypothesis, meaning PATCH[2/2] is the real fix. However,
> it would be nice to fix all of them in one shoot. I will try with PATCH[2/2]
> only to see if our issue will disappear or not. However, the issue still
> exists if PATCH[2/2] is missed.
>
Jason, PATCH[2/2] is sufficient to fix our current issue. I tried with PATCH[2/2]
only and unable to hit the issue. However, PATCH[1/2] may be needed by other scenarios.
So it would be nice to fix them in one shoot.
> Firstly, We were failing on the transmit queue and {tvq, rvq}->busyloop_timeout
> == false if I remember correctly. So the added smp_rmb() in vhost_vq_avail_empty()
> is only a concern to tx_can_batch(). A mutex isn't enough to ensure the order
> for the available index and available ring entry (head). For example, vhost_vq_avail_empty()
> called by tx_can_batch() can see next available index, but its corresponding
> available ring entry (head) may not be seen by vhost yet if smp_rmb() is missed.
> The next call to get_tx_bufs(), where the available ring entry (head) doesn't
> arrived yet, leading to stale available ring entry (head) being fetched.
>
> handle_tx_copy
> get_tx_bufs // smp_rmb() won't be executed when vq->avail_idx != vq->last_avail_idx
> tx_can_batch
> vhost_vq_avail_empty // vq->avail_idx is updated from vq->avail->idx
>
> The reason why I added smp_rmb() to vhost_vq_avail_empty() is because the function
> is a exposed API, even it's only used by drivers/vhost/net.c at present. It means
> the API has been broken internally. So it seems more appropriate to fix it up in
> vhost_vq_avail_empty() so that the API's users needn't worry about the memory access
> order.
>
>>>
>>>
>>>> + smp_rmb();
>>>> + return false;
>>>> + }
>>>>
>>>> - return vq->avail_idx == vq->last_avail_idx;
>>>> + return true;
>>>> }
>>>> EXPORT_SYMBOL_GPL(vhost_vq_avail_empty);
Thanks,
Gavin
On Wed, Mar 27, 2024 at 3:35 PM Gavin Shan <[email protected]> wrote:
>
> On 3/27/24 14:08, Gavin Shan wrote:
> > On 3/27/24 12:44, Jason Wang wrote:
> >> On Wed, Mar 27, 2024 at 10:34 AM Jason Wang <[email protected]> wrote:
> >>> On Wed, Mar 27, 2024 at 7:39 AM Gavin Shan <[email protected]> wrote:
> >>>>
> >>>> A smp_rmb() has been missed in vhost_vq_avail_empty(), spotted by
> >>>> Will Deacon <[email protected]>. Otherwise, it's not ensured the
> >>>> available ring entries pushed by guest can be observed by vhost
> >>>> in time, leading to stale available ring entries fetched by vhost
> >>>> in vhost_get_vq_desc(), as reported by Yihuang Yu on NVidia's
> >>>> grace-hopper (ARM64) platform.
> >>>>
> >>>> /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> >>>> -accel kvm -machine virt,gic-version=host -cpu host \
> >>>> -smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
> >>>> -m 4096M,slots=16,maxmem=64G \
> >>>> -object memory-backend-ram,id=mem0,size=4096M \
> >>>> : \
> >>>> -netdev tap,id=vnet0,vhost=true \
> >>>> -device virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0
> >>>> :
> >>>> guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
> >>>> virtio_net virtio0: output.0:id 100 is not a head!
> >>>>
> >>>> Add the missed smp_rmb() in vhost_vq_avail_empty(). Note that it
> >>>> should be safe until vq->avail_idx is changed by commit 275bf960ac697
> >>>> ("vhost: better detection of available buffers").
> >>>>
> >>>> Fixes: 275bf960ac697 ("vhost: better detection of available buffers")
> >>>> Cc: <[email protected]> # v4.11+
> >>>> Reported-by: Yihuang Yu <[email protected]>
> >>>> Signed-off-by: Gavin Shan <[email protected]>
> >>>> ---
> >>>> drivers/vhost/vhost.c | 11 ++++++++++-
> >>>> 1 file changed, 10 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >>>> index 045f666b4f12..00445ab172b3 100644
> >>>> --- a/drivers/vhost/vhost.c
> >>>> +++ b/drivers/vhost/vhost.c
> >>>> @@ -2799,9 +2799,18 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> >>>> r = vhost_get_avail_idx(vq, &avail_idx);
> >>>> if (unlikely(r))
> >>>> return false;
> >>>> +
> >>>> vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
> >>>> + if (vq->avail_idx != vq->last_avail_idx) {
> >>>> + /* Similar to what's done in vhost_get_vq_desc(), we need
> >>>> + * to ensure the available ring entries have been exposed
> >>>> + * by guest.
> >>>> + */
> >>>
> >>> We need to be more verbose here. For example, which load needs to be
> >>> ordered with which load.
> >>>
> >>> The rmb in vhost_get_vq_desc() is used to order the load of avail idx
> >>> and the load of head. It is paired with e.g virtio_wmb() in
> >>> virtqueue_add_split().
> >>>
> >>> vhost_vq_avail_empty() are mostly used as a hint in
> >>> vhost_net_busy_poll() which is under the protection of the vq mutex.
> >>>
> >>> An exception is the tx_can_batch(), but in that case it doesn't even
> >>> want to read the head.
> >>
> >> Ok, if it is needed only in that path, maybe we can move the barriers there.
> >>
> >
> > [cc Will Deacon]
> >
> > Jason, appreciate for your review and comments. I think PATCH[1/2] is
> > the fix for the hypothesis, meaning PATCH[2/2] is the real fix. However,
> > it would be nice to fix all of them in one shoot. I will try with PATCH[2/2]
> > only to see if our issue will disappear or not. However, the issue still
> > exists if PATCH[2/2] is missed.
> >
>
> Jason, PATCH[2/2] is sufficient to fix our current issue. I tried with PATCH[2/2]
> only and unable to hit the issue. However, PATCH[1/2] may be needed by other scenarios.
> So it would be nice to fix them in one shoot.
Yes, see below.
>
>
> > Firstly, We were failing on the transmit queue and {tvq, rvq}->busyloop_timeout
> > == false if I remember correctly. So the added smp_rmb() in vhost_vq_avail_empty()
> > is only a concern to tx_can_batch(). A mutex isn't enough to ensure the order
> > for the available index and available ring entry (head). For example, vhost_vq_avail_empty()
> > called by tx_can_batch() can see next available index, but its corresponding
> > available ring entry (head) may not be seen by vhost yet if smp_rmb() is missed.
> > The next call to get_tx_bufs(), where the available ring entry (head) doesn't
> > arrived yet, leading to stale available ring entry (head) being fetched.
> >
> > handle_tx_copy
> > get_tx_bufs // smp_rmb() won't be executed when vq->avail_idx != vq->last_avail_idx
> > tx_can_batch
> > vhost_vq_avail_empty // vq->avail_idx is updated from vq->avail->idx
> >
> > The reason why I added smp_rmb() to vhost_vq_avail_empty() is because the function
> > is a exposed API, even it's only used by drivers/vhost/net.c at present It means
> > the API has been broken internally. So it seems more appropriate to fix it up in
> > vhost_vq_avail_empty() so that the API's users needn't worry about the memory access
> > order.
When tx_can_batch returns true it means there's still pending tx
buffers. Since it might read indices so it still can bypass the
smp_rmb() in the vhost_get_vq_desc().
I'd suggest adding those above to change log.
With this,
Acked-by: Jason Wang <[email protected]>
Thanks
> >
> >>>
> >>>
> >>>> + smp_rmb();
> >>>> + return false;
> >>>> + }
> >>>>
> >>>> - return vq->avail_idx == vq->last_avail_idx;
> >>>> + return true;
> >>>> }
> >>>> EXPORT_SYMBOL_GPL(vhost_vq_avail_empty);
>
> Thanks,
> Gavin
>
On Wed, Mar 27, 2024 at 09:38:45AM +1000, Gavin Shan wrote:
> A smp_rmb() has been missed in vhost_vq_avail_empty(), spotted by
> Will Deacon <[email protected]>. Otherwise, it's not ensured the
> available ring entries pushed by guest can be observed by vhost
> in time, leading to stale available ring entries fetched by vhost
> in vhost_get_vq_desc(), as reported by Yihuang Yu on NVidia's
> grace-hopper (ARM64) platform.
>
> /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> -accel kvm -machine virt,gic-version=host -cpu host \
> -smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
> -m 4096M,slots=16,maxmem=64G \
> -object memory-backend-ram,id=mem0,size=4096M \
> : \
> -netdev tap,id=vnet0,vhost=true \
> -device virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0
> :
> guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
> virtio_net virtio0: output.0:id 100 is not a head!
>
> Add the missed smp_rmb() in vhost_vq_avail_empty(). Note that it
> should be safe until vq->avail_idx is changed by commit 275bf960ac697
> ("vhost: better detection of available buffers").
>
> Fixes: 275bf960ac697 ("vhost: better detection of available buffers")
> Cc: <[email protected]> # v4.11+
> Reported-by: Yihuang Yu <[email protected]>
> Signed-off-by: Gavin Shan <[email protected]>
> ---
> drivers/vhost/vhost.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 045f666b4f12..00445ab172b3 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2799,9 +2799,18 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> r = vhost_get_avail_idx(vq, &avail_idx);
> if (unlikely(r))
> return false;
> +
> vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
> + if (vq->avail_idx != vq->last_avail_idx) {
> + /* Similar to what's done in vhost_get_vq_desc(), we need
> + * to ensure the available ring entries have been exposed
> + * by guest.
> + */
A slightly clearer comment:
/* Since we have updated avail_idx, the following call to
* vhost_get_vq_desc will read available ring entries.
* Make sure that read happens after the avail_idx read.
*/
Pls repost with that, and I will apply.
Also add suggested-by for will.
> + smp_rmb();
> + return false;
> + }
>
> - return vq->avail_idx == vq->last_avail_idx;
> + return true;
> }
> EXPORT_SYMBOL_GPL(vhost_vq_avail_empty);
As a follow-up patch, we should clean out code duplication that
accumulated with 3 places reading avail idx in essentially
the same way - this duplication is what causes the mess in
the 1st place.
> --
> 2.44.0
On 3/27/24 22:07, Michael S. Tsirkin wrote:
> On Wed, Mar 27, 2024 at 09:38:45AM +1000, Gavin Shan wrote:
>> A smp_rmb() has been missed in vhost_vq_avail_empty(), spotted by
>> Will Deacon <[email protected]>. Otherwise, it's not ensured the
>> available ring entries pushed by guest can be observed by vhost
>> in time, leading to stale available ring entries fetched by vhost
>> in vhost_get_vq_desc(), as reported by Yihuang Yu on NVidia's
>> grace-hopper (ARM64) platform.
>>
>> /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>> -accel kvm -machine virt,gic-version=host -cpu host \
>> -smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
>> -m 4096M,slots=16,maxmem=64G \
>> -object memory-backend-ram,id=mem0,size=4096M \
>> : \
>> -netdev tap,id=vnet0,vhost=true \
>> -device virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0
>> :
>> guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
>> virtio_net virtio0: output.0:id 100 is not a head!
>>
>> Add the missed smp_rmb() in vhost_vq_avail_empty(). Note that it
>> should be safe until vq->avail_idx is changed by commit 275bf960ac697
>> ("vhost: better detection of available buffers").
>>
>> Fixes: 275bf960ac697 ("vhost: better detection of available buffers")
>> Cc: <[email protected]> # v4.11+
>> Reported-by: Yihuang Yu <[email protected]>
>> Signed-off-by: Gavin Shan <[email protected]>
>> ---
>> drivers/vhost/vhost.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index 045f666b4f12..00445ab172b3 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -2799,9 +2799,18 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>> r = vhost_get_avail_idx(vq, &avail_idx);
>> if (unlikely(r))
>> return false;
>> +
>> vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
>> + if (vq->avail_idx != vq->last_avail_idx) {
>> + /* Similar to what's done in vhost_get_vq_desc(), we need
>> + * to ensure the available ring entries have been exposed
>> + * by guest.
>> + */
>
> A slightly clearer comment:
>
> /* Since we have updated avail_idx, the following call to
> * vhost_get_vq_desc will read available ring entries.
> * Make sure that read happens after the avail_idx read.
> */
>
> Pls repost with that, and I will apply.
>
> Also add suggested-by for will.
>
Sure, the suggested comments have been included to v3.
>
>> + smp_rmb();
>> + return false;
>> + }
>>
>> - return vq->avail_idx == vq->last_avail_idx;
>> + return true;
>> }
>> EXPORT_SYMBOL_GPL(vhost_vq_avail_empty);
>
> As a follow-up patch, we should clean out code duplication that
> accumulated with 3 places reading avail idx in essentially
> the same way - this duplication is what causes the mess in
> the 1st place.
>
Yes, nice idea. I've added PATCH[v3 3/3] to improve vhost_get_avail_idx()
to handle the memory barrier since all the callers have the concern.
v3: https://lore.kernel.org/virtualization/[email protected]/
Thanks,
Gavin
On 3/27/24 17:42, Jason Wang wrote:
> On Wed, Mar 27, 2024 at 3:35 PM Gavin Shan <[email protected]> wrote:
>>
>> On 3/27/24 14:08, Gavin Shan wrote:
>>> On 3/27/24 12:44, Jason Wang wrote:
>>>> On Wed, Mar 27, 2024 at 10:34 AM Jason Wang <[email protected]> wrote:
>>>>> On Wed, Mar 27, 2024 at 7:39 AM Gavin Shan <[email protected]> wrote:
>>>>>>
>>>>>> A smp_rmb() has been missed in vhost_vq_avail_empty(), spotted by
>>>>>> Will Deacon <[email protected]>. Otherwise, it's not ensured the
>>>>>> available ring entries pushed by guest can be observed by vhost
>>>>>> in time, leading to stale available ring entries fetched by vhost
>>>>>> in vhost_get_vq_desc(), as reported by Yihuang Yu on NVidia's
>>>>>> grace-hopper (ARM64) platform.
>>>>>>
>>>>>> /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>>>>>> -accel kvm -machine virt,gic-version=host -cpu host \
>>>>>> -smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
>>>>>> -m 4096M,slots=16,maxmem=64G \
>>>>>> -object memory-backend-ram,id=mem0,size=4096M \
>>>>>> : \
>>>>>> -netdev tap,id=vnet0,vhost=true \
>>>>>> -device virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0
>>>>>> :
>>>>>> guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
>>>>>> virtio_net virtio0: output.0:id 100 is not a head!
>>>>>>
>>>>>> Add the missed smp_rmb() in vhost_vq_avail_empty(). Note that it
>>>>>> should be safe until vq->avail_idx is changed by commit 275bf960ac697
>>>>>> ("vhost: better detection of available buffers").
>>>>>>
>>>>>> Fixes: 275bf960ac697 ("vhost: better detection of available buffers")
>>>>>> Cc: <[email protected]> # v4.11+
>>>>>> Reported-by: Yihuang Yu <[email protected]>
>>>>>> Signed-off-by: Gavin Shan <[email protected]>
>>>>>> ---
>>>>>> drivers/vhost/vhost.c | 11 ++++++++++-
>>>>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>>>>>> index 045f666b4f12..00445ab172b3 100644
>>>>>> --- a/drivers/vhost/vhost.c
>>>>>> +++ b/drivers/vhost/vhost.c
>>>>>> @@ -2799,9 +2799,18 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>>>>>> r = vhost_get_avail_idx(vq, &avail_idx);
>>>>>> if (unlikely(r))
>>>>>> return false;
>>>>>> +
>>>>>> vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
>>>>>> + if (vq->avail_idx != vq->last_avail_idx) {
>>>>>> + /* Similar to what's done in vhost_get_vq_desc(), we need
>>>>>> + * to ensure the available ring entries have been exposed
>>>>>> + * by guest.
>>>>>> + */
>>>>>
>>>>> We need to be more verbose here. For example, which load needs to be
>>>>> ordered with which load.
>>>>>
>>>>> The rmb in vhost_get_vq_desc() is used to order the load of avail idx
>>>>> and the load of head. It is paired with e.g virtio_wmb() in
>>>>> virtqueue_add_split().
>>>>>
>>>>> vhost_vq_avail_empty() are mostly used as a hint in
>>>>> vhost_net_busy_poll() which is under the protection of the vq mutex.
>>>>>
>>>>> An exception is the tx_can_batch(), but in that case it doesn't even
>>>>> want to read the head.
>>>>
>>>> Ok, if it is needed only in that path, maybe we can move the barriers there.
>>>>
>>>
>>> [cc Will Deacon]
>>>
>>> Jason, appreciate for your review and comments. I think PATCH[1/2] is
>>> the fix for the hypothesis, meaning PATCH[2/2] is the real fix. However,
>>> it would be nice to fix all of them in one shoot. I will try with PATCH[2/2]
>>> only to see if our issue will disappear or not. However, the issue still
>>> exists if PATCH[2/2] is missed.
>>>
>>
>> Jason, PATCH[2/2] is sufficient to fix our current issue. I tried with PATCH[2/2]
>> only and unable to hit the issue. However, PATCH[1/2] may be needed by other scenarios.
>> So it would be nice to fix them in one shoot.
>
> Yes, see below.
>
>>
>>
>>> Firstly, We were failing on the transmit queue and {tvq, rvq}->busyloop_timeout
>>> == false if I remember correctly. So the added smp_rmb() in vhost_vq_avail_empty()
>>> is only a concern to tx_can_batch(). A mutex isn't enough to ensure the order
>>> for the available index and available ring entry (head). For example, vhost_vq_avail_empty()
>>> called by tx_can_batch() can see next available index, but its corresponding
>>> available ring entry (head) may not be seen by vhost yet if smp_rmb() is missed.
>>> The next call to get_tx_bufs(), where the available ring entry (head) doesn't
>>> arrived yet, leading to stale available ring entry (head) being fetched.
>>>
>>> handle_tx_copy
>>> get_tx_bufs // smp_rmb() won't be executed when vq->avail_idx != vq->last_avail_idx
>>> tx_can_batch
>>> vhost_vq_avail_empty // vq->avail_idx is updated from vq->avail->idx
>>>
>>> The reason why I added smp_rmb() to vhost_vq_avail_empty() is because the function
>>> is a exposed API, even it's only used by drivers/vhost/net.c at present. It means
>>> the API has been broken internally. So it seems more appropriate to fix it up in
>>> vhost_vq_avail_empty() so that the API's users needn't worry about the memory access
>>> order.
>
> When tx_can_batch returns true it means there's still pending tx
> buffers. Since it might read indices so it still can bypass the
> smp_rmb() in the vhost_get_vq_desc().
>
> I'd suggest adding those above to change log.
>
> With this,
>
> Acked-by: Jason Wang <[email protected]>
>
Thanks, Jason. The change log has been improved as you suggested in
v3, which was posted for further review.
Thanks,
Gavin
>>>
>>>>>
>>>>>
>>>>>> + smp_rmb();
>>>>>> + return false;
>>>>>> + }
>>>>>>
>>>>>> - return vq->avail_idx == vq->last_avail_idx;
>>>>>> + return true;
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(vhost_vq_avail_empty);