Stefano Garzarella
2022-Jan-14 09:05 UTC
[PATCH v1] vhost: cache avail index in vhost_enable_notify()
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. 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 at redhat.com> --- 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; } EXPORT_SYMBOL_GPL(vhost_enable_notify); -- 2.31.1
Michael S. Tsirkin
2022-Jan-14 12:45 UTC
[PATCH v1] vhost: cache avail index in vhost_enable_notify()
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. > > 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 at redhat.com>... and can in theory even hurt due to an extra memory write. So ... performance test restults pls?> --- > 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; > } > EXPORT_SYMBOL_GPL(vhost_enable_notify); > > -- > 2.31.1
Stefan Hajnoczi
2022-Jan-24 11:31 UTC
[PATCH v1] vhost: cache avail index in vhost_enable_notify()
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/> > 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 at redhat.com> > --- > 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; -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20220124/266dc37d/attachment.sig>