Messages in this thread | | | Date | Tue, 30 Apr 2024 09:18:53 +1000 | Subject | Re: [PATCH v2 1/4] vhost: Improve vhost_get_avail_idx() with smp_rmb() | From | Gavin Shan <> |
| |
On 4/30/24 04:44, Michael S. Tsirkin wrote: > On Mon, Apr 29, 2024 at 08:13:57PM +1000, Gavin Shan wrote: >> From: "Michael S. Tsirkin" <mst@redhat.com> >> >> All the callers of vhost_get_avail_idx() are concerned to the memory > > *with* the memory barrier >
Thanks, will be corrected in v3.
>> barrier, imposed by smp_rmb() to ensure the order of the available >> ring entry read and avail_idx read. >> >> Improve vhost_get_avail_idx() so that smp_rmb() is executed when >> the avail_idx is advanced. > > accessed, not advanced. guest advances it. >
smp_rmb() is executed only when vp->last_avail_idx != vp->avail_idx. I used 'advanced' to indicate the condition. 'accessed' is also correct since the 'advanced' case included to 'accessed' case.
>> With it, the callers needn't to worry >> about the memory barrier. >> >> No functional change intended. > > I'd add: > > As a side benefit, we also validate the index on all paths now, which > will hopefully help catch future errors earlier. > > Note: current code is inconsistent in how it handles errors: > some places treat it as an empty ring, others - non empty. > This patch does not attempt to change the existing behaviour. >
Ok, I will integrate this to v3's commit log.
> > >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> [gshan: repainted vhost_get_avail_idx()] > > ?repainted? >
It's just a indicator to say the changes aren't simply copied from [1]. Some follow-up changes are also applied. So it needs to be reviewed. I will drop this in v3.
>> Reviewed-by: Gavin Shan <gshan@redhat.com> >> Acked-by: Will Deacon <will@kernel.org> >> --- >> drivers/vhost/vhost.c | 106 +++++++++++++++++------------------------- >> 1 file changed, 42 insertions(+), 64 deletions(-) >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> index 8995730ce0bf..7aa623117aab 100644 >> --- a/drivers/vhost/vhost.c >> +++ b/drivers/vhost/vhost.c >> @@ -1290,10 +1290,36 @@ static void vhost_dev_unlock_vqs(struct vhost_dev *d) >> mutex_unlock(&d->vqs[i]->mutex); >> } >> >> -static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq, >> - __virtio16 *idx) >> +static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq) >> { >> - return vhost_get_avail(vq, *idx, &vq->avail->idx); >> + __virtio16 idx; >> + int r; >> + >> + r = vhost_get_avail(vq, idx, &vq->avail->idx); >> + if (unlikely(r < 0)) { >> + vq_err(vq, "Failed to access available index at %p (%d)\n", >> + &vq->avail->idx, r); >> + return r; >> + } >> + >> + /* Check it isn't doing very strange thing with available indexes */ >> + vq->avail_idx = vhost16_to_cpu(vq, idx); >> + if (unlikely((u16)(vq->avail_idx - vq->last_avail_idx) > vq->num)) { >> + vq_err(vq, "Invalid available index change from %u to %u", >> + vq->last_avail_idx, vq->avail_idx); >> + return -EINVAL; >> + } >> + >> + /* We're done if there is nothing new */ >> + if (vq->avail_idx == vq->last_avail_idx) >> + return 0; >> + >> + /* >> + * We updated vq->avail_idx so we need a memory barrier between >> + * the index read above and the caller reading avail ring entries. >> + */ >> + smp_rmb(); >> + return 1; >> } >> >> static inline int vhost_get_avail_head(struct vhost_virtqueue *vq, >> @@ -2498,38 +2524,17 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq, >> { >> struct vring_desc desc; >> unsigned int i, head, found = 0; >> - u16 last_avail_idx; >> - __virtio16 avail_idx; >> + u16 last_avail_idx = vq->last_avail_idx; >> __virtio16 ring_head; >> int ret, access; >> >> - /* Check it isn't doing very strange things with descriptor numbers. */ >> - last_avail_idx = vq->last_avail_idx; >> - >> if (vq->avail_idx == vq->last_avail_idx) { >> - if (unlikely(vhost_get_avail_idx(vq, &avail_idx))) { >> - vq_err(vq, "Failed to access avail idx at %p\n", >> - &vq->avail->idx); >> - return -EFAULT; >> - } >> - vq->avail_idx = vhost16_to_cpu(vq, avail_idx); >> - >> - if (unlikely((u16)(vq->avail_idx - last_avail_idx) > vq->num)) { >> - vq_err(vq, "Guest moved avail index from %u to %u", >> - last_avail_idx, vq->avail_idx); >> - return -EFAULT; >> - } >> + ret = vhost_get_avail_idx(vq); >> + if (unlikely(ret < 0)) >> + return ret; >> >> - /* If there's nothing new since last we looked, return >> - * invalid. >> - */ >> - if (vq->avail_idx == last_avail_idx) >> + if (!ret) >> return vq->num; >> - >> - /* Only get avail ring entries after they have been >> - * exposed by guest. >> - */ >> - smp_rmb(); >> } >> >> /* Grab the next descriptor number they're advertising, and increment >> @@ -2790,35 +2795,20 @@ EXPORT_SYMBOL_GPL(vhost_add_used_and_signal_n); >> /* return true if we're sure that avaiable ring is empty */ >> bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq) >> { >> - __virtio16 avail_idx; >> int r; >> >> if (vq->avail_idx != vq->last_avail_idx) >> return false; >> >> - 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) { >> - /* 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. >> - */ >> - smp_rmb(); >> - return false; >> - } >> - >> - return true; >> + /* Treat error as non-empty here */ > > If you write the comment like that then put it before "return": > that is where you treat an error like this. > And I feel Note: is better in that the comment does not > explain all of what is going on, just an aspect of it. >
Ok. Will do in v3.
>> + r = vhost_get_avail_idx(vq); >> + return r == 0; >> } >> EXPORT_SYMBOL_GPL(vhost_vq_avail_empty); >> >> /* OK, now we need to know about added descriptors. */ >> bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq) >> { >> - __virtio16 avail_idx; >> int r; >> >> if (!(vq->used_flags & VRING_USED_F_NO_NOTIFY)) >> @@ -2842,25 +2832,13 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq) >> /* They could have slipped one in as we were doing that: make >> * sure it's written, then check again. */ >> smp_mb(); >> - r = vhost_get_avail_idx(vq, &avail_idx); >> - if (r) { >> - vq_err(vq, "Failed to check avail idx at %p: %d\n", >> - &vq->avail->idx, r); >> - return false; >> - } >> >> - vq->avail_idx = vhost16_to_cpu(vq, avail_idx); >> - if (vq->avail_idx != vq->last_avail_idx) { >> - /* 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. >> - */ >> - smp_rmb(); >> - return true; >> - } >> + /* Treat error as empty here */ >> + r = vhost_get_avail_idx(vq); > > If you write the comment like that then put it before "return": > that is where you treat an error like this. > And I feel Note: is better in that the comment does not > explain all of what is going on, just an aspect of it. >
Sure, will do in v3.
>> + if (unlikely(r < 0)) >> + return false; >> >> - return false; >> + return r; >> } >> EXPORT_SYMBOL_GPL(vhost_enable_notify);
Thanks, Gavin
| |