Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring

[Jason Wang <jasowang@redhat.com>]

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