On Mon, Jan 24, 2022 at 11:31:49AM +0000, Stefan Hajnoczi wrote:
On Fri, Jan 14, 2022 at 10:05:08AM +0100, Stefano Garzarella wrote:
In vhost_enable_notify() we enable the notifications and we read
the avail index to check if new buffers have become available in
the meantime.
We are not caching the avail index, so when the device will call
vhost_get_vq_desc(), it will find the old value in the cache and
it will read the avail index again.
I think this wording is clearer because we do keep a cached the avail
index value, but the issue is we don't update it:
s/We are not caching the avail index/We do not update the cached avail
index value/
I'll fix in v3.
It seems I forgot to CC you on v2:
https://lore.kernel.org/virtualization/20220121153108.187291-1-sgarzare@xxxxxxxxxx/
It would be better to refresh the cache every time we read avail
index, so let's change vhost_enable_notify() caching the value in
`avail_idx` and compare it with `last_avail_idx` to check if there
are new buffers available.
Anyway, we don't expect a significant performance boost because
the above path is not very common, indeed vhost_enable_notify()
is often called with unlikely(), expecting that avail index has
not been updated.
Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
---
v1:
- improved the commit description [MST, Jason]
---
drivers/vhost/vhost.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 59edb5a1ffe2..07363dff559e 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2543,8 +2543,9 @@ 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);
- return vhost16_to_cpu(vq, avail_idx) != vq->avail_idx;
+ return vq->avail_idx != vq->last_avail_idx;
vhost_vq_avail_empty() has a fast path that's missing in
vhost_enable_notify():
if (vq->avail_idx != vq->last_avail_idx)
return false;
Yep, I thought about that, but devices usually call
vhost_enable_notify() right when vq->avail_idx == vq->last_avail_idx, so
I don't know if it's an extra check for a branch that will never be
taken.
Do you think it is better to add that check? (maybe with unlikely())
Thanks,
Stefano
