lkml.org 
[lkml]   [2018]   [May]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
    From
    Date


    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

    \
     
     \ /
      Last update: 2018-05-09 05:44    [W:3.373 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site