[PATCH] virtio_ring: fix stalls for packed rings

[PATCH] virtio_ring: fix stalls for packed rings

[Michael S. Tsirkin]

From: Marvin Liu <yong.liu@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@vger.kernel.org
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Marvin Liu <yong.liu@intel.com>
Signed-off-by: Michael S. Tsirkin <mst@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;
 	}
-- 
MST

Re: [PATCH] virtio_ring: fix stalls for packed rings

[Jason Wang]

On 2019/10/28 下午2:59, Sasha Levin wrote:
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
>
> The bot has tested the following trees: v5.3.7, v4.19.80, v4.14.150, v4.9.197, v4.4.197.
>
> v5.3.7: Build OK!
> v4.19.80: Failed to apply! Possible dependencies:
>      138fd25148638 ("virtio_ring: add _split suffix for split ring functions")
>      1ce9e6055fa0a ("virtio_ring: introduce packed ring support")
>      cbeedb72b97ad ("virtio_ring: allocate desc state for split ring separately")
>      d79dca75c7968 ("virtio_ring: extract split ring handling from ring creation")
>      e593bf9751566 ("virtio_ring: put split ring fields in a sub struct")
>      e6f633e5beab6 ("virtio_ring: put split ring functions together")
>      f51f982682e2a ("virtio_ring: leverage event idx in packed ring")
>      fb3fba6b162aa ("virtio_ring: cache whether we will use DMA API")
>
> v4.14.150: Failed to apply! Possible dependencies:
>      138fd25148638 ("virtio_ring: add _split suffix for split ring functions")
>      1ce9e6055fa0a ("virtio_ring: introduce packed ring support")
>      cbeedb72b97ad ("virtio_ring: allocate desc state for split ring separately")
>      d79dca75c7968 ("virtio_ring: extract split ring handling from ring creation")
>      e593bf9751566 ("virtio_ring: put split ring fields in a sub struct")
>      e6f633e5beab6 ("virtio_ring: put split ring functions together")
>      f51f982682e2a ("virtio_ring: leverage event idx in packed ring")
>      fb3fba6b162aa ("virtio_ring: cache whether we will use DMA API")
>
> v4.9.197: Failed to apply! Possible dependencies:
>      0c7eaf5930e14 ("virtio_ring: fix description of virtqueue_get_buf")
>      138fd25148638 ("virtio_ring: add _split suffix for split ring functions")
>      1ce9e6055fa0a ("virtio_ring: introduce packed ring support")
>      44ed8089e991a ("scsi: virtio: Reduce BUG if total_sg > virtqueue size to WARN.")
>      5a08b04f63792 ("virtio: allow extra context per descriptor")
>      c60923cb9cb5e ("virtio_ring: fix complaint by sparse")
>      cbeedb72b97ad ("virtio_ring: allocate desc state for split ring separately")
>      e593bf9751566 ("virtio_ring: put split ring fields in a sub struct")
>      e6f633e5beab6 ("virtio_ring: put split ring functions together")
>      f51f982682e2a ("virtio_ring: leverage event idx in packed ring")
>      fb3fba6b162aa ("virtio_ring: cache whether we will use DMA API")
>
> v4.4.197: Failed to apply! Possible dependencies:
>      0c7eaf5930e14 ("virtio_ring: fix description of virtqueue_get_buf")
>      138fd25148638 ("virtio_ring: add _split suffix for split ring functions")
>      1ce9e6055fa0a ("virtio_ring: introduce packed ring support")
>      44ed8089e991a ("scsi: virtio: Reduce BUG if total_sg > virtqueue size to WARN.")
>      5a08b04f63792 ("virtio: allow extra context per descriptor")
>      780bc7903a32e ("virtio_ring: Support DMA APIs")
>      c60923cb9cb5e ("virtio_ring: fix complaint by sparse")
>      d26c96c810254 ("vring: Introduce vring_use_dma_api()")
>      e6f633e5beab6 ("virtio_ring: put split ring functions together")
>      f51f982682e2a ("virtio_ring: leverage event idx in packed ring")
>      fb3fba6b162aa ("virtio_ring: cache whether we will use DMA API")
>
>
> NOTE: The patch will not be queued to stable trees until it is upstream.
>
> How should we proceed with this patch?


It is only needed for kernel > 4.20.

Thanks


>

Re: [PATCH] virtio_ring: fix stalls for packed rings

[Jason Wang]

On 2019/10/27 下午6:08, Michael S. Tsirkin wrote:
> From: Marvin Liu <yong.liu@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@vger.kernel.org
Fixes: f51f982682e2a ("virtio_ring: leverage event idx in packed ring")
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> Signed-off-by: Michael S. Tsirkin <mst@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;
>   	}

Re: [PATCH] virtio_ring: fix stalls for packed rings

[Jason Wang]

On 2019/10/27 下午6:08, Michael S. Tsirkin wrote:
> From: Marvin Liu <yong.liu@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@vger.kernel.org
Fixes: f51f982682e2a ("virtio_ring: leverage event idx in packed ring")
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> Signed-off-by: Michael S. Tsirkin <mst@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;
>   	}

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] virtio_ring: fix stalls for packed rings

[Michael S. Tsirkin]

From: Marvin Liu <yong.liu@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@vger.kernel.org
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Marvin Liu <yong.liu@intel.com>
Signed-off-by: Michael S. Tsirkin <mst@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;
 	}
-- 
MST