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 <[email protected]>
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
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
