Michael S. Tsirkin
2019-Oct-25 09:32 UTC
[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 offsetThis 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 at intel.com> > > 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.> @@ -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
Michael S. Tsirkin
2019-Oct-27 09:09 UTC
[PATCH] virtio_ring: fix packed ring event may missing
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 at intel.com> > > > > 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 at lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization