Linux Virtualization - Oct 2019 - [PATCH] virtio_ring: fix packed ring event may missing

On Tue, Oct 22, 2019 at 01:10:04AM +0800, Marvin Liu
wrote:
> When callback is delayed, virtio expect that vhost will kick when
> rolling over event offset. Recheck should be taken as used index may
> exceed event offset between status check and driver event update.
> 
> However, it is possible that flags was not modified if descriptors are
> chained or in_order feature was negotiated. So flags at event offset
> may not be valid for descriptor's status checking. Fix it by using last
> used index as replacement. Tx queue will be stopped if there's not
> enough freed buffers after recheck.
> 
> Signed-off-by: Marvin Liu <yong.liu at intel.com>
So as I wrote separately, I don't think the patch is correct.  And it
doesn't look like it was tested properly.  However, I do think
virtqueue_enable_cb_delayed_packed is implemented incorrectly in case of
chained s/g and ring that is close to being full.  It's just that
chained only happens on OOM at the moment, so it's hard to trigger.
virtio_ring.c can be hacked to force chained to make it trigger more.
And with napi_tx the default this function is barely used at all.

So it's hard to test, and I think in this area we really should include
info on how patch was tested before applying.


But just theoretically speaking I agree: the trick
virtqueue_enable_cb_delayed_packed is using to check that 3/4 of the
ring has been used just by looking at a single s/g entry can't work
reliably.

We generally really need to scan the ring.  How much to scan?  If we
want to implement this efficiently, I propose that we track the number
of outstanding bufs.  Then # of s/g - # of bufs is the max number of
entries we need to scan. For the common case of a single s/g this will
give exactly 0.


Alternatively, we can just scan the whole ring up to the index using the
standard logic. virtqueue_enable_cb_delayed_packed is rare enough.


> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index bdc08244a648..a8041e451e9e 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1499,9 +1499,6 @@ static bool virtqueue_enable_cb_delayed_packed(struct
virtqueue *_vq)
>  		 * counter first before updating event flags.
>  		 */
>  		virtio_wmb(vq->weak_barriers);
> -	} else {
> -		used_idx = vq->last_used_idx;
> -		wrap_counter = vq->packed.used_wrap_counter;
>  	}
>  
>  	if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DISABLE)
{
> @@ -1518,7 +1515,9 @@ static bool virtqueue_enable_cb_delayed_packed(struct
virtqueue *_vq)
>  	 */
>  	virtio_mb(vq->weak_barriers);
>  
> -	if (is_used_desc_packed(vq, used_idx, wrap_counter)) {
> +	if (is_used_desc_packed(vq,
> +				vq->last_used_idx,
> +				vq->packed.used_wrap_counter)) {
>  		END_USE(vq);
>  		return false;
>  	}
> -- 
> 2.17.1

On Fri, Oct 25, 2019 at 06:53:55AM -0400, Michael S. Tsirkin
wrote:
> On Tue, Oct 22, 2019 at 01:10:04AM +0800, Marvin Liu wrote:
> > When callback is delayed, virtio expect that vhost will kick when
> > rolling over event offset. Recheck should be taken as used index may
> > exceed event offset between status check and driver event update.
> > 
> > However, it is possible that flags was not modified if descriptors are
> > chained or in_order feature was negotiated. So flags at event offset
> > may not be valid for descriptor's status checking. Fix it by using
last
> > used index as replacement. Tx queue will be stopped if there's not
> > enough freed buffers after recheck.
> > 
> > Signed-off-by: Marvin Liu <yong.liu at intel.com>
> 
> So as I wrote separately, I don't think the patch is correct.  And it
> doesn't look like it was tested properly.
I was wrong to say this: the patch is correct.

Thanks for working on this, and sorry about creating the confusion.

It might not be the
optimal thing to do but it fixes a real bug, so I think it's
a good idea to merge it for now, and implement optimizations on top.
>  However, I do think
> virtqueue_enable_cb_delayed_packed is implemented incorrectly in case of
> chained s/g and ring that is close to being full.  It's just that
> chained only happens on OOM at the moment, so it's hard to trigger.
> virtio_ring.c can be hacked to force chained to make it trigger more.
> And with napi_tx the default this function is barely used at all.
> 
> So it's hard to test, and I think in this area we really should include
> info on how patch was tested before applying.
> 
> 
> But just theoretically speaking I agree: the trick
> virtqueue_enable_cb_delayed_packed is using to check that 3/4 of the
> ring has been used just by looking at a single s/g entry can't work
> reliably.
> 
> We generally really need to scan the ring.  How much to scan?  If we
> want to implement this efficiently, I propose that we track the number
> of outstanding bufs.  Then # of s/g - # of bufs is the max number of
> entries we need to scan. For the common case of a single s/g this will
> give exactly 0.
> 
> 
> Alternatively, we can just scan the whole ring up to the index using the
> standard logic. virtqueue_enable_cb_delayed_packed is rare enough.
> 
> 
> 
> > 
> > diff --git a/drivers/virtio/virtio_ring.c
b/drivers/virtio/virtio_ring.c
> > index bdc08244a648..a8041e451e9e 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -1499,9 +1499,6 @@ static bool
virtqueue_enable_cb_delayed_packed(struct virtqueue *_vq)
> >  		 * counter first before updating event flags.
> >  		 */
> >  		virtio_wmb(vq->weak_barriers);
> > -	} else {
> > -		used_idx = vq->last_used_idx;
> > -		wrap_counter = vq->packed.used_wrap_counter;
> >  	}
> >  
> >  	if (vq->packed.event_flags_shadow ==
VRING_PACKED_EVENT_FLAG_DISABLE) {
> > @@ -1518,7 +1515,9 @@ static bool
virtqueue_enable_cb_delayed_packed(struct virtqueue *_vq)
> >  	 */
> >  	virtio_mb(vq->weak_barriers);
> >  
> > -	if (is_used_desc_packed(vq, used_idx, wrap_counter)) {
> > +	if (is_used_desc_packed(vq,
> > +				vq->last_used_idx,
> > +				vq->packed.used_wrap_counter)) {
> >  		END_USE(vq);
> >  		return false;
> >  	}
> > -- 
> > 2.17.1