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. In this case, the device would go to re-read avail
index to access the descriptor.
As we already do in other place, we can cache the value in `avail_idx`
and compare it with `last_avail_idx` to check if there are new
buffers available.
Signed-off-by: Stefano Garzarella <[email protected]>
---
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;
}
EXPORT_SYMBOL_GPL(vhost_enable_notify);
--
2.31.1
On Thu, Jan 13, 2022 at 03:56:42PM +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. In this case, the device would go to re-read avail
> index to access the descriptor.
>
> As we already do in other place, we can cache the value in `avail_idx`
> and compare it with `last_avail_idx` to check if there are new
> buffers available.
>
> Signed-off-by: Stefano Garzarella <[email protected]>
I guess we can ... but what's the point?
> ---
> 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;
> }
> EXPORT_SYMBOL_GPL(vhost_enable_notify);
>
> --
> 2.31.1
On Thu, Jan 13, 2022 at 10:19:46AM -0500, Michael S. Tsirkin wrote:
>On Thu, Jan 13, 2022 at 03:56:42PM +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. In this case, the device would go to re-read avail
>> index to access the descriptor.
>>
>> As we already do in other place, we can cache the value in `avail_idx`
>> and compare it with `last_avail_idx` to check if there are new
>> buffers available.
>>
>> Signed-off-by: Stefano Garzarella <[email protected]>
>
>I guess we can ... but what's the point?
>
That without this patch if avail index is new, then device when will
call vhost_get_vq_desc() will find old value in cache and will read it
again.
With this patch we also do the same path and update the cache every time
we read avail index.
I marked it RFC because I don't know if it's worth it :-)
Stefano
On Thu, Jan 13, 2022 at 04:44:47PM +0100, Stefano Garzarella wrote:
> On Thu, Jan 13, 2022 at 10:19:46AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Jan 13, 2022 at 03:56:42PM +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. In this case, the device would go to re-read avail
> > > index to access the descriptor.
> > >
> > > As we already do in other place, we can cache the value in `avail_idx`
> > > and compare it with `last_avail_idx` to check if there are new
> > > buffers available.
> > >
> > > Signed-off-by: Stefano Garzarella <[email protected]>
> >
> > I guess we can ... but what's the point?
> >
>
> That without this patch if avail index is new, then device when will call
> vhost_get_vq_desc() will find old value in cache and will read it again.
>
> With this patch we also do the same path and update the cache every time we
> read avail index.
>
> I marked it RFC because I don't know if it's worth it :-)
>
> Stefano
Pls include info like this in commit log. Thanks!
--
MST
On Thu, Jan 13, 2022 at 10:57 PM Stefano Garzarella <[email protected]> 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. In this case, the device would go to re-read avail
> index to access the descriptor.
>
> As we already do in other place, we can cache the value in `avail_idx`
> and compare it with `last_avail_idx` to check if there are new
> buffers available.
>
> Signed-off-by: Stefano Garzarella <[email protected]>
Patch looks fine but I guess we won't get performance improvement
since it doesn't save any userspace/VM memory access?
Thanks
> ---
> 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;
> }
> EXPORT_SYMBOL_GPL(vhost_enable_notify);
>
> --
> 2.31.1
>
On Fri, Jan 14, 2022 at 02:18:01PM +0800, Jason Wang wrote:
>On Thu, Jan 13, 2022 at 10:57 PM Stefano Garzarella <[email protected]> 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. In this case, the device would go to re-read avail
>> index to access the descriptor.
>>
>> As we already do in other place, we can cache the value in `avail_idx`
>> and compare it with `last_avail_idx` to check if there are new
>> buffers available.
>>
>> Signed-off-by: Stefano Garzarella <[email protected]>
>
>Patch looks fine but I guess we won't get performance improvement
>since it doesn't save any userspace/VM memory access?
It should save the memory access when vhost_enable_notify() find
something new in the VQ, so in this path:
vhost_enable_notify() <- VM memory access for avail index
== true
vhost_disable_notify()
...
vhost_get_vq_desc() <- VM memory access for avail index
with the patch applied, this access is
avoided since avail index is cached
In any case, I don't expect this to be a very common path, indeed we
usually use unlikely() for this path:
if (unlikely(vhost_enable_notify(dev, vq))) {
vhost_disable_notify(dev, vq);
continue;
}
So I don't expect a significant performance increase.
v1 coming with a better commit description.
Thanks,
Stefano