Linux Virtualization - May 2018 - [RFC v3 4/5] virtio_ring: add event idx support in packed ring

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).

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.

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
>

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

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`.

Best regards,
Tiwei Bie
> 
> ??? 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
> > > 
>