* [RFC PATCH] vhost: cache avail index in vhost_enable_notify()
@ 2022-01-13 14:56 Stefano Garzarella
2022-01-13 15:19 ` Michael S. Tsirkin
2022-01-14 6:18 ` Jason Wang
0 siblings, 2 replies; 6+ messages in thread
From: Stefano Garzarella @ 2022-01-13 14:56 UTC (permalink / raw)
To: virtualization; +Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, stefanha
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 <sgarzare@redhat.com>
---
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] vhost: cache avail index in vhost_enable_notify()
2022-01-13 14:56 [RFC PATCH] vhost: cache avail index in vhost_enable_notify() Stefano Garzarella
@ 2022-01-13 15:19 ` Michael S. Tsirkin
2022-01-13 15:44 ` Stefano Garzarella
2022-01-14 6:18 ` Jason Wang
1 sibling, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2022-01-13 15:19 UTC (permalink / raw)
To: Stefano Garzarella; +Cc: kvm, netdev, linux-kernel, virtualization, stefanha
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 <sgarzare@redhat.com>
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] vhost: cache avail index in vhost_enable_notify()
2022-01-13 15:19 ` Michael S. Tsirkin
@ 2022-01-13 15:44 ` Stefano Garzarella
2022-01-13 16:05 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Stefano Garzarella @ 2022-01-13 15:44 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, netdev, linux-kernel, virtualization, stefanha
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 <sgarzare@redhat.com>
>
>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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] vhost: cache avail index in vhost_enable_notify()
2022-01-13 15:44 ` Stefano Garzarella
@ 2022-01-13 16:05 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2022-01-13 16:05 UTC (permalink / raw)
To: Stefano Garzarella; +Cc: kvm, netdev, linux-kernel, virtualization, stefanha
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 <sgarzare@redhat.com>
> >
> > 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] vhost: cache avail index in vhost_enable_notify()
2022-01-13 14:56 [RFC PATCH] vhost: cache avail index in vhost_enable_notify() Stefano Garzarella
2022-01-13 15:19 ` Michael S. Tsirkin
@ 2022-01-14 6:18 ` Jason Wang
2022-01-14 8:02 ` Stefano Garzarella
1 sibling, 1 reply; 6+ messages in thread
From: Jason Wang @ 2022-01-14 6:18 UTC (permalink / raw)
To: Stefano Garzarella
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Stefan Hajnoczi
On Thu, Jan 13, 2022 at 10:57 PM Stefano Garzarella <sgarzare@redhat.com> 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 <sgarzare@redhat.com>
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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] vhost: cache avail index in vhost_enable_notify()
2022-01-14 6:18 ` Jason Wang
@ 2022-01-14 8:02 ` Stefano Garzarella
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2022-01-14 8:02 UTC (permalink / raw)
To: Jason Wang
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Stefan Hajnoczi
On Fri, Jan 14, 2022 at 02:18:01PM +0800, Jason Wang wrote:
>On Thu, Jan 13, 2022 at 10:57 PM Stefano Garzarella <sgarzare@redhat.com> 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 <sgarzare@redhat.com>
>
>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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-14 8:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-01-13 14:56 [RFC PATCH] vhost: cache avail index in vhost_enable_notify() Stefano Garzarella
2022-01-13 15:19 ` Michael S. Tsirkin
2022-01-13 15:44 ` Stefano Garzarella
2022-01-13 16:05 ` Michael S. Tsirkin
2022-01-14 6:18 ` Jason Wang
2022-01-14 8:02 ` Stefano Garzarella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).