On 2019/10/27 ??6:08, Michael S. Tsirkin wrote:> From: Marvin Liu <yong.liu at intel.com>
>
> When VIRTIO_F_RING_EVENT_IDX is negotiated, virtio devices can
> use virtqueue_enable_cb_delayed_packed to reduce the number of device
> interrupts. At the moment, this is the case for virtio-net when the
> napi_tx module parameter is set to false.
>
> In this case, the virtio driver selects an event offset and expects that
> the device will send a notification when rolling over the event offset
> in the ring. However, if this roll-over happens before the event
> suppression structure update, the notification won't be sent. To
address
> this race condition the driver needs to check wether the device rolled
> over the offset after updating the event suppression structure.
>
> With VIRTIO_F_RING_PACKED, the virtio driver did this by reading the
> flags field of the descriptor at the specified offset.
>
> Unfortunately, checking at the event offset isn't reliable: if
> descriptors are chained (e.g. when INDIRECT is off) not all descriptors
> are overwritten by the device, so it's possible that the device skipped
> the specific descriptor driver is checking when writing out used
> descriptors. If this happens, the driver won't detect the race
condition
> and will incorrectly expect the device to send a notification.
>
> For virtio-net, the result will be a TX queue stall, with the
> transmission getting blocked forever.
>
> With the packed ring, it isn't easy to find a location which is
> guaranteed to change upon the roll-over, except the next device
> descriptor, as described in the spec:
>
> 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.
>
> while this might be sub-optimal, let's do exactly this for now.
>
> Cc: stable at vger.kernel.org
Fixes: f51f982682e2a ("virtio_ring: leverage event idx in packed
ring")> Cc: Jason Wang <jasowang at redhat.com>
> Signed-off-by: Marvin Liu <yong.liu at intel.com>
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>
> So this is what I have in my tree now - this is just Marvin's patch
> with a tweaked description.
>
>
> drivers/virtio/virtio_ring.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> 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;
> }