Messages in this thread | | | Subject | Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring | From | Jason Wang <> | Date | Wed, 9 May 2018 11:43:15 +0800 |
| |
On 2018年05月08日 17:44, Tiwei Bie wrote: > On Tue, May 08, 2018 at 05:34:40PM +0800, Jason Wang wrote: >> On 2018年05月08日 17:16, Tiwei Bie wrote: >>> On Tue, May 08, 2018 at 03:16:53PM +0800, Jason Wang wrote: >>>> On 2018年05月08日 14:44, Tiwei Bie wrote: >>>>> On Tue, May 08, 2018 at 01:40:40PM +0800, Jason Wang wrote: >>>>>> On 2018年05月08日 11:05, Jason Wang wrote: >>>>>>>> Because in virtqueue_enable_cb_delayed(), we may set an >>>>>>>> event_off which is bigger than new and both of them have >>>>>>>> wrapped. And in this case, although new is smaller than >>>>>>>> event_off (i.e. the third param -- old), new shouldn't >>>>>>>> add vq->num, and actually we are expecting a very big >>>>>>>> idx diff. >>>>>>> Yes, so to calculate distance correctly between event and new, we just >>>>>>> need to compare the warp counter and return false if it doesn't match >>>>>>> without the need to try to add vq.num here. >>>>>>> >>>>>>> Thanks >>>>>> Sorry, looks like the following should work, we need add vq.num if >>>>>> used_wrap_counter does not match: >>>>>> >>>>>> static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, >>>>>> __u16 off_wrap, __u16 new, >>>>>> __u16 old) >>>>>> { >>>>>> bool wrap = off_wrap >> 15; >>>>>> int off = off_wrap & ~(1 << 15); >>>>>> __u16 d1, d2; >>>>>> >>>>>> if (wrap != vq->used_wrap_counter) >>>>>> d1 = new + vq->num - off - 1; >>>>> Just to draw your attention (maybe you have already >>>>> noticed this). >>>> I miss this, thanks! >>>> >>>>> In this case (i.e. wrap != vq->used_wrap_counter), >>>>> it's also possible that (off < new) is true. Because, >>>>> >>>>> when virtqueue_enable_cb_delayed_packed() is used, >>>>> `off` is calculated in driver in a way like this: >>>>> >>>>> off = vq->last_used_idx + bufs; >>>>> if (off >= vq->vring_packed.num) { >>>>> off -= vq->vring_packed.num; >>>>> wrap_counter ^= 1; >>>>> } >>>>> >>>>> And when `new` (in vhost) is close to vq->num. The >>>>> vq->last_used_idx + bufs (in driver) can be bigger >>>>> than vq->vring_packed.num, and: >>>>> >>>>> 1. `off` will wrap; >>>>> 2. wrap counters won't match; >>>>> 3. off < new; >>>>> >>>>> And d1 (i.e. new + vq->num - off - 1) will be a value >>>>> bigger than vq->num. I'm okay with this, although it's >>>>> a bit weird. >>>> So I'm considering something more compact by reusing vring_need_event() by >>>> pretending a larger queue size and adding vq->num back when necessary: >>>> >>>> static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, >>>> __u16 off_wrap, __u16 new, >>>> __u16 old) >>>> { >>>> bool wrap = vq->used_wrap_counter; >>> If the wrap counter is obtained from the vq, >>> I think `new` should also be obtained from >>> the vq. Or the wrap counter should be carried >>> in `new`. >>> >>>> int off = off_wrap & ~(1 << 15); >>>> __u16 d1, d2; >>>> >>>> if (new < old) { >>>> new += vq->num; >>>> wrap ^= 1; >>>> } >>>> >>>> if (wrap != off_wrap >> 15) >>>> off += vq->num; >>> When `new` and `old` wraps, and `off` doesn't wrap, >>> wrap != (off_wrap >> 15) will be true. In this case, >>> `off` is bigger than `new`, and what we should do >>> is `off -= vq->num` instead of `off += vq->num`. >> If I understand this correctly, if we track old correctly, it won't happen >> if guest driver behave correctly. That means it should only happen for a >> buggy driver (e.g trying to move off_wrap back). > If vhost is faster than virtio driver, I guess above > case may happen. The `old` and `new` will be updated > each time we want to notify the driver. If the driver > is slower, `old` and `new` in vhost may wrap before > the `off` which is set by driver wraps. > > Best regards, > Tiwei Bie >
Oh, right.
But the code still work (in this case new - event_idx - 1 will underflow). (And I admit it still looks ugly).
Thanks
| |