On Fri, Oct 25, 2019 at 05:32:49AM -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
>
> This mention of event offset I don't understand: your patch
> only affects code that runs when !event. So how can it
> affect 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@xxxxxxxxx>
> >
> > 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;
> > }
>
>
> Is all this theorectical? Or did you actually see a problem
> and then fixed it?
> Because as far as I could see after this patch and with
> event index off, used_idx and wrap_counter will be used
> without being initialized.
>
> OTOH the behaviour with event index on is completely unaffected.
>
>
> >
> > if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DISABLE) {
>
> OK so trying to unpack the scenario.
>
> First you patch only affects code running when EVENT_IDX is off, so
> legal values for flags are enable and disable.
>
>
> Next point, this calculates the index at which we are going
> to look for the flags to change, in other words
> it affects the line
> if (is_used_desc_packed(vq, used_idx, wrap_counter)) {
> below.
>
> Without your patch, we simply look at the next descriptor.
> This is exactly what the spec says we should do:
>
> Writes of device and driver descriptors can generally be
> reordered, but each side (driver and device) are only required to
> poll (or test) a single location in memory: the next device descriptor after
> the one they processed previously, in circular order.
>
OK please ignore all this, I misunderstood the patch. Sorry about the
noise.
>
>
>
>
> > @@ -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
> Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
