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

[Jason Wang]

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;
ÂÂÂ 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;

ÂÂÂ return vring_need_event(off, new, old);
}


> Best regards,
> Tiwei Bie
>
> >  ÂÂÂ else
> >  ÂÂÂ ÂÂÂ d1 = new - off - 1;
> >
> >  ÂÂÂ if (new > old)
> >  ÂÂÂ ÂÂÂ d2 = new - old;
> >  ÂÂÂ else
> >  ÂÂÂ ÂÂÂ d2 = new + vq->num - old;
> >
> >  ÂÂÂ return d1 < d2;
> > }
> >
> > Thanks