[PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Jason Wang]

A userspace triggerable infinite loop could happen in
mlx5_cvq_kick_handler() if userspace keeps sending a huge amount of
cvq requests.

Fixing this by introducing a quota and re-queue the work if we're out
of the budget. While at it, using a per device workqueue to avoid on
demand memory allocation for cvq.

Fixes: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index d0f91078600e..d5a6fb3f9c41 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -163,6 +163,7 @@ struct mlx5_vdpa_net {
 	u32 cur_num_vqs;
 	struct notifier_block nb;
 	struct vdpa_callback config_cb;
+	struct mlx5_vdpa_wq_ent cvq_ent;
 };
 
 static void free_resources(struct mlx5_vdpa_net *ndev);
@@ -1600,6 +1601,8 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
 	return status;
 }
 
+#define MLX5_CVQ_BUDGET 16
+
 static void mlx5_cvq_kick_handler(struct work_struct *work)
 {
 	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
@@ -1609,17 +1612,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 	struct mlx5_control_vq *cvq;
 	struct mlx5_vdpa_net *ndev;
 	size_t read, write;
-	int err;
+	int err, n = 0;
 
 	wqent = container_of(work, struct mlx5_vdpa_wq_ent, work);
 	mvdev = wqent->mvdev;
 	ndev = to_mlx5_vdpa_ndev(mvdev);
 	cvq = &mvdev->cvq;
 	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
-		goto out;
+		return;
 
 	if (!cvq->ready)
-		goto out;
+		return;
 
 	while (true) {
 		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
@@ -1653,9 +1656,13 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 
 		if (vringh_need_notify_iotlb(&cvq->vring))
 			vringh_notify(&cvq->vring);
+
+		n++;
+		if (n > MLX5_CVQ_BUDGET) {
+			queue_work(mvdev->wq, &wqent->work);
+			break;
+		}
 	}
-out:
-	kfree(wqent);
 }
 
 static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
@@ -1663,7 +1670,6 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
 	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
 	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
 	struct mlx5_vdpa_virtqueue *mvq;
-	struct mlx5_vdpa_wq_ent *wqent;
 
 	if (!is_index_valid(mvdev, idx))
 		return;
@@ -1672,13 +1678,7 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
 		if (!mvdev->cvq.ready)
 			return;
 
-		wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
-		if (!wqent)
-			return;
-
-		wqent->mvdev = mvdev;
-		INIT_WORK(&wqent->work, mlx5_cvq_kick_handler);
-		queue_work(mvdev->wq, &wqent->work);
+		queue_work(mvdev->wq, &ndev->cvq_ent.work);
 		return;
 	}
 
@@ -2668,6 +2668,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 	if (err)
 		goto err_mr;
 
+	ndev->cvq_ent.mvdev = mvdev;
+	INIT_WORK(&ndev->cvq_ent.work, mlx5_cvq_kick_handler);
 	mvdev->wq = create_singlethread_workqueue("mlx5_vdpa_wq");
 	if (!mvdev->wq) {
 		err = -ENOMEM;
-- 
2.18.1

[PATCH 2/2] vdpa: mlx5: synchronize driver status with CVQ

[Jason Wang]

Currently, CVQ doesn't have any synchronization with the driver
status. Then CVQ emulation code run in the middle of:

1) device reset
2) device status changed
3) map updating

The will lead several unexpected issue like trying to execute CVQ
command after the driver has been teared down.

Fixing this by using reslock to synchronize CVQ emulation code with
the driver status changing:

- protect the whole device reset, status changing and map updating
  with reslock
- protect the CVQ handler with the reslock and check
  VIRTIO_CONFIG_S_DRIVER_OK in the CVQ handler

This will guarantee that:

1) CVQ handler won't work if VIRTIO_CONFIG_S_DRIVER_OK is not set
2) CVQ handler will see a consistent state of the driver instead of
   the partial one when it is running in the middle of the
   teardown_driver() or setup_driver().

Cc: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 42 +++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index d5a6fb3f9c41..524240f55c1c 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1618,11 +1618,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 	mvdev = wqent->mvdev;
 	ndev = to_mlx5_vdpa_ndev(mvdev);
 	cvq = &mvdev->cvq;
+
+	mutex_lock(&ndev->reslock);
+
+	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
+		goto done;
+
 	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
-		return;
+		goto done;
 
 	if (!cvq->ready)
-		return;
+		goto done;
 
 	while (true) {
 		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
@@ -1663,6 +1669,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 			break;
 		}
 	}
+
+done:
+	mutex_unlock(&ndev->reslock);
 }
 
 static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
@@ -2125,6 +2134,8 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
 	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
 	int err;
 
+	mutex_lock(&ndev->reslock);
+
 	suspend_vqs(ndev);
 	err = save_channels_info(ndev);
 	if (err)
@@ -2137,18 +2148,20 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
 		goto err_mr;
 
 	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
-		return 0;
+		goto err_mr;
 
 	restore_channels_info(ndev);
 	err = setup_driver(mvdev);
 	if (err)
 		goto err_setup;
 
+	mutex_unlock(&ndev->reslock);
 	return 0;
 
 err_setup:
 	mlx5_vdpa_destroy_mr(mvdev);
 err_mr:
+	mutex_unlock(&ndev->reslock);
 	return err;
 }
 
@@ -2157,7 +2170,8 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
 	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
 	int err;
 
-	mutex_lock(&ndev->reslock);
+	WARN_ON(!mutex_is_locked(&ndev->reslock));
+
 	if (ndev->setup) {
 		mlx5_vdpa_warn(mvdev, "setup driver called for already setup driver\n");
 		err = 0;
@@ -2187,7 +2201,6 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
 		goto err_fwd;
 	}
 	ndev->setup = true;
-	mutex_unlock(&ndev->reslock);
 
 	return 0;
 
@@ -2198,23 +2211,22 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
 err_rqt:
 	teardown_virtqueues(ndev);
 out:
-	mutex_unlock(&ndev->reslock);
 	return err;
 }
 
 static void teardown_driver(struct mlx5_vdpa_net *ndev)
 {
-	mutex_lock(&ndev->reslock);
+
+	WARN_ON(!mutex_is_locked(&ndev->reslock));
+
 	if (!ndev->setup)
-		goto out;
+		return;
 
 	remove_fwd_to_tir(ndev);
 	destroy_tir(ndev);
 	destroy_rqt(ndev);
 	teardown_virtqueues(ndev);
 	ndev->setup = false;
-out:
-	mutex_unlock(&ndev->reslock);
 }
 
 static void clear_vqs_ready(struct mlx5_vdpa_net *ndev)
@@ -2235,6 +2247,8 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
 
 	print_status(mvdev, status, true);
 
+	mutex_lock(&ndev->reslock);
+
 	if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) {
 		if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
 			err = setup_driver(mvdev);
@@ -2244,16 +2258,19 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
 			}
 		} else {
 			mlx5_vdpa_warn(mvdev, "did not expect DRIVER_OK to be cleared\n");
-			return;
+			goto err_clear;
 		}
 	}
 
 	ndev->mvdev.status = status;
+	mutex_unlock(&ndev->reslock);
 	return;
 
 err_setup:
 	mlx5_vdpa_destroy_mr(&ndev->mvdev);
 	ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
+err_clear:
+	mutex_unlock(&ndev->reslock);
 }
 
 static int mlx5_vdpa_reset(struct vdpa_device *vdev)
@@ -2263,6 +2280,8 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
 
 	print_status(mvdev, 0, true);
 	mlx5_vdpa_info(mvdev, "performing device reset\n");
+
+	mutex_lock(&ndev->reslock);
 	teardown_driver(ndev);
 	clear_vqs_ready(ndev);
 	mlx5_vdpa_destroy_mr(&ndev->mvdev);
@@ -2275,6 +2294,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
 		if (mlx5_vdpa_create_mr(mvdev, NULL))
 			mlx5_vdpa_warn(mvdev, "create MR failed\n");
 	}
+	mutex_unlock(&ndev->reslock);
 
 	return 0;
 }
-- 
2.18.1

RE: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Eli Cohen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Monday, March 21, 2022 8:04 AM
> To: mst@redhat.com; jasowang@redhat.com
> Cc: Eli Cohen <elic@nvidia.com>; virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU
> 
> A userspace triggerable infinite loop could happen in
> mlx5_cvq_kick_handler() if userspace keeps sending a huge amount of
> cvq requests.
> 
> Fixing this by introducing a quota and re-queue the work if we're out
not requeuing the work ...
> of the budget. While at it, using a per device workqueue to avoid on
using per device work. The workqueue is already per device.
> demand memory allocation for cvq.
> 
> Fixes: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Reviewed-by: Eli Cohen <elic@nvidia.com>

> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index d0f91078600e..d5a6fb3f9c41 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -163,6 +163,7 @@ struct mlx5_vdpa_net {
>  	u32 cur_num_vqs;
>  	struct notifier_block nb;
>  	struct vdpa_callback config_cb;
> +	struct mlx5_vdpa_wq_ent cvq_ent;
>  };
> 
>  static void free_resources(struct mlx5_vdpa_net *ndev);
> @@ -1600,6 +1601,8 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>  	return status;
>  }
> 
> +#define MLX5_CVQ_BUDGET 16
> +
>  static void mlx5_cvq_kick_handler(struct work_struct *work)
>  {
>  	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> @@ -1609,17 +1612,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>  	struct mlx5_control_vq *cvq;
>  	struct mlx5_vdpa_net *ndev;
>  	size_t read, write;
> -	int err;
> +	int err, n = 0;
> 
>  	wqent = container_of(work, struct mlx5_vdpa_wq_ent, work);
>  	mvdev = wqent->mvdev;
>  	ndev = to_mlx5_vdpa_ndev(mvdev);
>  	cvq = &mvdev->cvq;
>  	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> -		goto out;
> +		return;
> 
>  	if (!cvq->ready)
> -		goto out;
> +		return;
> 
>  	while (true) {
>  		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
> @@ -1653,9 +1656,13 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
> 
>  		if (vringh_need_notify_iotlb(&cvq->vring))
>  			vringh_notify(&cvq->vring);
> +
> +		n++;
> +		if (n > MLX5_CVQ_BUDGET) {
> +			queue_work(mvdev->wq, &wqent->work);
> +			break;
> +		}
>  	}
> -out:
> -	kfree(wqent);
>  }
> 
>  static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> @@ -1663,7 +1670,6 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
>  	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>  	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>  	struct mlx5_vdpa_virtqueue *mvq;
> -	struct mlx5_vdpa_wq_ent *wqent;
> 
>  	if (!is_index_valid(mvdev, idx))
>  		return;
> @@ -1672,13 +1678,7 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
>  		if (!mvdev->cvq.ready)
>  			return;
> 
> -		wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> -		if (!wqent)
> -			return;
> -
> -		wqent->mvdev = mvdev;
> -		INIT_WORK(&wqent->work, mlx5_cvq_kick_handler);
> -		queue_work(mvdev->wq, &wqent->work);
> +		queue_work(mvdev->wq, &ndev->cvq_ent.work);
>  		return;
>  	}
> 
> @@ -2668,6 +2668,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>  	if (err)
>  		goto err_mr;
> 
> +	ndev->cvq_ent.mvdev = mvdev;
> +	INIT_WORK(&ndev->cvq_ent.work, mlx5_cvq_kick_handler);
>  	mvdev->wq = create_singlethread_workqueue("mlx5_vdpa_wq");
>  	if (!mvdev->wq) {
>  		err = -ENOMEM;
> --
> 2.18.1

RE: [PATCH 2/2] vdpa: mlx5: synchronize driver status with CVQ

[Eli Cohen]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Monday, March 21, 2022 8:04 AM
> To: mst@redhat.com; jasowang@redhat.com
> Cc: Eli Cohen <elic@nvidia.com>; virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 2/2] vdpa: mlx5: synchronize driver status with CVQ
> 
> Currently, CVQ doesn't have any synchronization with the driver
> status. Then CVQ emulation code run in the middle of:
> 
> 1) device reset
> 2) device status changed
> 3) map updating
> 
> The will lead several unexpected issue like trying to execute CVQ
> command after the driver has been teared down.
> 
> Fixing this by using reslock to synchronize CVQ emulation code with
> the driver status changing:
> 
> - protect the whole device reset, status changing and map updating
>   with reslock
> - protect the CVQ handler with the reslock and check
>   VIRTIO_CONFIG_S_DRIVER_OK in the CVQ handler
> 
> This will guarantee that:
> 
> 1) CVQ handler won't work if VIRTIO_CONFIG_S_DRIVER_OK is not set
> 2) CVQ handler will see a consistent state of the driver instead of
>    the partial one when it is running in the middle of the
>    teardown_driver() or setup_driver().
> 
> Cc: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---

Reviewed-by: Eli Cohen <elic@nvidia.com>

>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 42 +++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index d5a6fb3f9c41..524240f55c1c 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1618,11 +1618,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>  	mvdev = wqent->mvdev;
>  	ndev = to_mlx5_vdpa_ndev(mvdev);
>  	cvq = &mvdev->cvq;
> +
> +	mutex_lock(&ndev->reslock);
> +
> +	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> +		goto done;
> +
>  	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> -		return;
> +		goto done;
> 
>  	if (!cvq->ready)
> -		return;
> +		goto done;
> 
>  	while (true) {
>  		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
> @@ -1663,6 +1669,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>  			break;
>  		}
>  	}
> +
> +done:
> +	mutex_unlock(&ndev->reslock);
>  }
> 
>  static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> @@ -2125,6 +2134,8 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
>  	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>  	int err;
> 
> +	mutex_lock(&ndev->reslock);
> +
>  	suspend_vqs(ndev);
>  	err = save_channels_info(ndev);
>  	if (err)
> @@ -2137,18 +2148,20 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
>  		goto err_mr;
> 
>  	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> -		return 0;
> +		goto err_mr;
> 
>  	restore_channels_info(ndev);
>  	err = setup_driver(mvdev);
>  	if (err)
>  		goto err_setup;
> 
> +	mutex_unlock(&ndev->reslock);
>  	return 0;
> 
>  err_setup:
>  	mlx5_vdpa_destroy_mr(mvdev);
>  err_mr:
> +	mutex_unlock(&ndev->reslock);
>  	return err;
>  }
> 
> @@ -2157,7 +2170,8 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
>  	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>  	int err;
> 
> -	mutex_lock(&ndev->reslock);
> +	WARN_ON(!mutex_is_locked(&ndev->reslock));
> +
>  	if (ndev->setup) {
>  		mlx5_vdpa_warn(mvdev, "setup driver called for already setup driver\n");
>  		err = 0;
> @@ -2187,7 +2201,6 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
>  		goto err_fwd;
>  	}
>  	ndev->setup = true;
> -	mutex_unlock(&ndev->reslock);
> 
>  	return 0;
> 
> @@ -2198,23 +2211,22 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
>  err_rqt:
>  	teardown_virtqueues(ndev);
>  out:
> -	mutex_unlock(&ndev->reslock);
>  	return err;
>  }
> 
>  static void teardown_driver(struct mlx5_vdpa_net *ndev)
>  {
> -	mutex_lock(&ndev->reslock);
> +
> +	WARN_ON(!mutex_is_locked(&ndev->reslock));
> +
>  	if (!ndev->setup)
> -		goto out;
> +		return;
> 
>  	remove_fwd_to_tir(ndev);
>  	destroy_tir(ndev);
>  	destroy_rqt(ndev);
>  	teardown_virtqueues(ndev);
>  	ndev->setup = false;
> -out:
> -	mutex_unlock(&ndev->reslock);
>  }
> 
>  static void clear_vqs_ready(struct mlx5_vdpa_net *ndev)
> @@ -2235,6 +2247,8 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> 
>  	print_status(mvdev, status, true);
> 
> +	mutex_lock(&ndev->reslock);
> +
>  	if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) {
>  		if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>  			err = setup_driver(mvdev);
> @@ -2244,16 +2258,19 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
>  			}
>  		} else {
>  			mlx5_vdpa_warn(mvdev, "did not expect DRIVER_OK to be cleared\n");
> -			return;
> +			goto err_clear;
>  		}
>  	}
> 
>  	ndev->mvdev.status = status;
> +	mutex_unlock(&ndev->reslock);
>  	return;
> 
>  err_setup:
>  	mlx5_vdpa_destroy_mr(&ndev->mvdev);
>  	ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
> +err_clear:
> +	mutex_unlock(&ndev->reslock);
>  }
> 
>  static int mlx5_vdpa_reset(struct vdpa_device *vdev)
> @@ -2263,6 +2280,8 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
> 
>  	print_status(mvdev, 0, true);
>  	mlx5_vdpa_info(mvdev, "performing device reset\n");
> +
> +	mutex_lock(&ndev->reslock);
>  	teardown_driver(ndev);
>  	clear_vqs_ready(ndev);
>  	mlx5_vdpa_destroy_mr(&ndev->mvdev);
> @@ -2275,6 +2294,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>  		if (mlx5_vdpa_create_mr(mvdev, NULL))
>  			mlx5_vdpa_warn(mvdev, "create MR failed\n");
>  	}
> +	mutex_unlock(&ndev->reslock);
> 
>  	return 0;
>  }
> --
> 2.18.1

Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Michael S. Tsirkin]

On Mon, Mar 21, 2022 at 02:04:28PM +0800, Jason Wang wrote:
> A userspace triggerable infinite loop could happen in
> mlx5_cvq_kick_handler() if userspace keeps sending a huge amount of
> cvq requests.
> 
> Fixing this by introducing a quota and re-queue the work if we're out
> of the budget. While at it, using a per device workqueue to avoid on
> demand memory allocation for cvq.
> 
> Fixes: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index d0f91078600e..d5a6fb3f9c41 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -163,6 +163,7 @@ struct mlx5_vdpa_net {
>  	u32 cur_num_vqs;
>  	struct notifier_block nb;
>  	struct vdpa_callback config_cb;
> +	struct mlx5_vdpa_wq_ent cvq_ent;
>  };
>  
>  static void free_resources(struct mlx5_vdpa_net *ndev);
> @@ -1600,6 +1601,8 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>  	return status;
>  }
>  
> +#define MLX5_CVQ_BUDGET 16
> +
>  static void mlx5_cvq_kick_handler(struct work_struct *work)
>  {
>  	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> @@ -1609,17 +1612,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>  	struct mlx5_control_vq *cvq;
>  	struct mlx5_vdpa_net *ndev;
>  	size_t read, write;
> -	int err;
> +	int err, n = 0;
>  
>  	wqent = container_of(work, struct mlx5_vdpa_wq_ent, work);
>  	mvdev = wqent->mvdev;
>  	ndev = to_mlx5_vdpa_ndev(mvdev);
>  	cvq = &mvdev->cvq;
>  	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> -		goto out;
> +		return;
>  
>  	if (!cvq->ready)
> -		goto out;
> +		return;
>  
>  	while (true) {
>  		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
> @@ -1653,9 +1656,13 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>  
>  		if (vringh_need_notify_iotlb(&cvq->vring))
>  			vringh_notify(&cvq->vring);
> +
> +		n++;
> +		if (n > MLX5_CVQ_BUDGET) {
> +			queue_work(mvdev->wq, &wqent->work);
> +			break;
> +		}
>  	}
> -out:
> -	kfree(wqent);
>  }
>  
>  static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> @@ -1663,7 +1670,6 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
>  	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>  	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>  	struct mlx5_vdpa_virtqueue *mvq;
> -	struct mlx5_vdpa_wq_ent *wqent;
>  
>  	if (!is_index_valid(mvdev, idx))
>  		return;
> @@ -1672,13 +1678,7 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
>  		if (!mvdev->cvq.ready)
>  			return;
>  
> -		wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> -		if (!wqent)
> -			return;
> -
> -		wqent->mvdev = mvdev;
> -		INIT_WORK(&wqent->work, mlx5_cvq_kick_handler);
> -		queue_work(mvdev->wq, &wqent->work);
> +		queue_work(mvdev->wq, &ndev->cvq_ent.work);
>  		return;
>  	}
>  
> @@ -2668,6 +2668,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>  	if (err)
>  		goto err_mr;
>  
> +	ndev->cvq_ent.mvdev = mvdev;
> +	INIT_WORK(&ndev->cvq_ent.work, mlx5_cvq_kick_handler);
>  	mvdev->wq = create_singlethread_workqueue("mlx5_vdpa_wq");
>  	if (!mvdev->wq) {
>  		err = -ENOMEM;

Shouldn't there be a flush during cleanup somewhere?

> -- 
> 2.18.1

Re: [PATCH 2/2] vdpa: mlx5: synchronize driver status with CVQ

[Michael S. Tsirkin]

On Mon, Mar 21, 2022 at 02:04:29PM +0800, Jason Wang wrote:
> Currently, CVQ doesn't have any synchronization with the driver
> status. Then CVQ emulation code run in the middle of:
> 
> 1) device reset
> 2) device status changed
> 3) map updating
> 
> The will lead several unexpected issue like trying to execute CVQ
> command after the driver has been teared down.
> 
> Fixing this by using reslock to synchronize CVQ emulation code with
> the driver status changing:
> 
> - protect the whole device reset, status changing and map updating
>   with reslock
> - protect the CVQ handler with the reslock and check
>   VIRTIO_CONFIG_S_DRIVER_OK in the CVQ handler
> 
> This will guarantee that:
> 
> 1) CVQ handler won't work if VIRTIO_CONFIG_S_DRIVER_OK is not set
> 2) CVQ handler will see a consistent state of the driver instead of
>    the partial one when it is running in the middle of the
>    teardown_driver() or setup_driver().
> 
> Cc: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 42 +++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index d5a6fb3f9c41..524240f55c1c 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1618,11 +1618,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>  	mvdev = wqent->mvdev;
>  	ndev = to_mlx5_vdpa_ndev(mvdev);
>  	cvq = &mvdev->cvq;
> +
> +	mutex_lock(&ndev->reslock);
> +
> +	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> +		goto done;
> +
>  	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> -		return;
> +		goto done;
>  
>  	if (!cvq->ready)
> -		return;
> +		goto done;
>  
>  	while (true) {
>  		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
> @@ -1663,6 +1669,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>  			break;
>  		}
>  	}
> +
> +done:
> +	mutex_unlock(&ndev->reslock);
>  }
>  
>  static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> @@ -2125,6 +2134,8 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
>  	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>  	int err;
>  
> +	mutex_lock(&ndev->reslock);
> +
>  	suspend_vqs(ndev);
>  	err = save_channels_info(ndev);
>  	if (err)
> @@ -2137,18 +2148,20 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
>  		goto err_mr;
>  
>  	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> -		return 0;
> +		goto err_mr;
>  
>  	restore_channels_info(ndev);
>  	err = setup_driver(mvdev);
>  	if (err)
>  		goto err_setup;
>  
> +	mutex_unlock(&ndev->reslock);
>  	return 0;
>  
>  err_setup:
>  	mlx5_vdpa_destroy_mr(mvdev);
>  err_mr:
> +	mutex_unlock(&ndev->reslock);
>  	return err;
>  }
>  
> @@ -2157,7 +2170,8 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
>  	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>  	int err;
>  
> -	mutex_lock(&ndev->reslock);
> +	WARN_ON(!mutex_is_locked(&ndev->reslock));
> +
>  	if (ndev->setup) {
>  		mlx5_vdpa_warn(mvdev, "setup driver called for already setup driver\n");
>  		err = 0;


Maybe also add a comment near function header explaining this must be
called with lock held.

> @@ -2187,7 +2201,6 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
>  		goto err_fwd;
>  	}
>  	ndev->setup = true;
> -	mutex_unlock(&ndev->reslock);
>  
>  	return 0;
>  
> @@ -2198,23 +2211,22 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
>  err_rqt:
>  	teardown_virtqueues(ndev);
>  out:
> -	mutex_unlock(&ndev->reslock);
>  	return err;
>  }
>  
>  static void teardown_driver(struct mlx5_vdpa_net *ndev)
>  {
> -	mutex_lock(&ndev->reslock);
> +
> +	WARN_ON(!mutex_is_locked(&ndev->reslock));
> +
>  	if (!ndev->setup)
> -		goto out;
> +		return;
>  
>  	remove_fwd_to_tir(ndev);
>  	destroy_tir(ndev);
>  	destroy_rqt(ndev);
>  	teardown_virtqueues(ndev);
>  	ndev->setup = false;
> -out:
> -	mutex_unlock(&ndev->reslock);
>  }
>  
>  static void clear_vqs_ready(struct mlx5_vdpa_net *ndev)
> @@ -2235,6 +2247,8 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
>  
>  	print_status(mvdev, status, true);
>  
> +	mutex_lock(&ndev->reslock);
> +
>  	if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) {
>  		if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>  			err = setup_driver(mvdev);
> @@ -2244,16 +2258,19 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
>  			}
>  		} else {
>  			mlx5_vdpa_warn(mvdev, "did not expect DRIVER_OK to be cleared\n");
> -			return;
> +			goto err_clear;
>  		}
>  	}
>  
>  	ndev->mvdev.status = status;
> +	mutex_unlock(&ndev->reslock);
>  	return;
>  
>  err_setup:
>  	mlx5_vdpa_destroy_mr(&ndev->mvdev);
>  	ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
> +err_clear:
> +	mutex_unlock(&ndev->reslock);
>  }
>  
>  static int mlx5_vdpa_reset(struct vdpa_device *vdev)
> @@ -2263,6 +2280,8 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>  
>  	print_status(mvdev, 0, true);
>  	mlx5_vdpa_info(mvdev, "performing device reset\n");
> +
> +	mutex_lock(&ndev->reslock);
>  	teardown_driver(ndev);
>  	clear_vqs_ready(ndev);
>  	mlx5_vdpa_destroy_mr(&ndev->mvdev);
> @@ -2275,6 +2294,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>  		if (mlx5_vdpa_create_mr(mvdev, NULL))
>  			mlx5_vdpa_warn(mvdev, "create MR failed\n");
>  	}
> +	mutex_unlock(&ndev->reslock);
>  
>  	return 0;
>  }
> -- 
> 2.18.1

Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Jason Wang]

On Mon, Mar 21, 2022 at 3:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Mar 21, 2022 at 02:04:28PM +0800, Jason Wang wrote:
> > A userspace triggerable infinite loop could happen in
> > mlx5_cvq_kick_handler() if userspace keeps sending a huge amount of
> > cvq requests.
> >
> > Fixing this by introducing a quota and re-queue the work if we're out
> > of the budget. While at it, using a per device workqueue to avoid on
> > demand memory allocation for cvq.
> >
> > Fixes: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 28 +++++++++++++++-------------
> >  1 file changed, 15 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index d0f91078600e..d5a6fb3f9c41 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -163,6 +163,7 @@ struct mlx5_vdpa_net {
> >       u32 cur_num_vqs;
> >       struct notifier_block nb;
> >       struct vdpa_callback config_cb;
> > +     struct mlx5_vdpa_wq_ent cvq_ent;
> >  };
> >
> >  static void free_resources(struct mlx5_vdpa_net *ndev);
> > @@ -1600,6 +1601,8 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> >       return status;
> >  }
> >
> > +#define MLX5_CVQ_BUDGET 16
> > +
> >  static void mlx5_cvq_kick_handler(struct work_struct *work)
> >  {
> >       virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> > @@ -1609,17 +1612,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
> >       struct mlx5_control_vq *cvq;
> >       struct mlx5_vdpa_net *ndev;
> >       size_t read, write;
> > -     int err;
> > +     int err, n = 0;
> >
> >       wqent = container_of(work, struct mlx5_vdpa_wq_ent, work);
> >       mvdev = wqent->mvdev;
> >       ndev = to_mlx5_vdpa_ndev(mvdev);
> >       cvq = &mvdev->cvq;
> >       if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > -             goto out;
> > +             return;
> >
> >       if (!cvq->ready)
> > -             goto out;
> > +             return;
> >
> >       while (true) {
> >               err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
> > @@ -1653,9 +1656,13 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
> >
> >               if (vringh_need_notify_iotlb(&cvq->vring))
> >                       vringh_notify(&cvq->vring);
> > +
> > +             n++;
> > +             if (n > MLX5_CVQ_BUDGET) {
> > +                     queue_work(mvdev->wq, &wqent->work);
> > +                     break;
> > +             }
> >       }
> > -out:
> > -     kfree(wqent);
> >  }
> >
> >  static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> > @@ -1663,7 +1670,6 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> >       struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> >       struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> >       struct mlx5_vdpa_virtqueue *mvq;
> > -     struct mlx5_vdpa_wq_ent *wqent;
> >
> >       if (!is_index_valid(mvdev, idx))
> >               return;
> > @@ -1672,13 +1678,7 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> >               if (!mvdev->cvq.ready)
> >                       return;
> >
> > -             wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> > -             if (!wqent)
> > -                     return;
> > -
> > -             wqent->mvdev = mvdev;
> > -             INIT_WORK(&wqent->work, mlx5_cvq_kick_handler);
> > -             queue_work(mvdev->wq, &wqent->work);
> > +             queue_work(mvdev->wq, &ndev->cvq_ent.work);
> >               return;
> >       }
> >
> > @@ -2668,6 +2668,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> >       if (err)
> >               goto err_mr;
> >
> > +     ndev->cvq_ent.mvdev = mvdev;
> > +     INIT_WORK(&ndev->cvq_ent.work, mlx5_cvq_kick_handler);
> >       mvdev->wq = create_singlethread_workqueue("mlx5_vdpa_wq");
> >       if (!mvdev->wq) {
> >               err = -ENOMEM;
>
> Shouldn't there be a flush during cleanup somewhere?

I think the destory_workqueue() in mlx5_vdpa_dev_del() will do the flush/drain.

Thanks

>
> > --
> > 2.18.1
>

Re: [PATCH 2/2] vdpa: mlx5: synchronize driver status with CVQ

[Jason Wang]

On Mon, Mar 21, 2022 at 3:24 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Mar 21, 2022 at 02:04:29PM +0800, Jason Wang wrote:
> > Currently, CVQ doesn't have any synchronization with the driver
> > status. Then CVQ emulation code run in the middle of:
> >
> > 1) device reset
> > 2) device status changed
> > 3) map updating
> >
> > The will lead several unexpected issue like trying to execute CVQ
> > command after the driver has been teared down.
> >
> > Fixing this by using reslock to synchronize CVQ emulation code with
> > the driver status changing:
> >
> > - protect the whole device reset, status changing and map updating
> >   with reslock
> > - protect the CVQ handler with the reslock and check
> >   VIRTIO_CONFIG_S_DRIVER_OK in the CVQ handler
> >
> > This will guarantee that:
> >
> > 1) CVQ handler won't work if VIRTIO_CONFIG_S_DRIVER_OK is not set
> > 2) CVQ handler will see a consistent state of the driver instead of
> >    the partial one when it is running in the middle of the
> >    teardown_driver() or setup_driver().
> >
> > Cc: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 42 +++++++++++++++++++++++--------
> >  1 file changed, 31 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index d5a6fb3f9c41..524240f55c1c 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -1618,11 +1618,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
> >       mvdev = wqent->mvdev;
> >       ndev = to_mlx5_vdpa_ndev(mvdev);
> >       cvq = &mvdev->cvq;
> > +
> > +     mutex_lock(&ndev->reslock);
> > +
> > +     if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> > +             goto done;
> > +
> >       if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > -             return;
> > +             goto done;
> >
> >       if (!cvq->ready)
> > -             return;
> > +             goto done;
> >
> >       while (true) {
> >               err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
> > @@ -1663,6 +1669,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
> >                       break;
> >               }
> >       }
> > +
> > +done:
> > +     mutex_unlock(&ndev->reslock);
> >  }
> >
> >  static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> > @@ -2125,6 +2134,8 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> >       struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> >       int err;
> >
> > +     mutex_lock(&ndev->reslock);
> > +
> >       suspend_vqs(ndev);
> >       err = save_channels_info(ndev);
> >       if (err)
> > @@ -2137,18 +2148,20 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> >               goto err_mr;
> >
> >       if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> > -             return 0;
> > +             goto err_mr;
> >
> >       restore_channels_info(ndev);
> >       err = setup_driver(mvdev);
> >       if (err)
> >               goto err_setup;
> >
> > +     mutex_unlock(&ndev->reslock);
> >       return 0;
> >
> >  err_setup:
> >       mlx5_vdpa_destroy_mr(mvdev);
> >  err_mr:
> > +     mutex_unlock(&ndev->reslock);
> >       return err;
> >  }
> >
> > @@ -2157,7 +2170,8 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
> >       struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> >       int err;
> >
> > -     mutex_lock(&ndev->reslock);
> > +     WARN_ON(!mutex_is_locked(&ndev->reslock));
> > +
> >       if (ndev->setup) {
> >               mlx5_vdpa_warn(mvdev, "setup driver called for already setup driver\n");
> >               err = 0;
>
>
> Maybe also add a comment near function header explaining this must be
> called with lock held.

Will do.

Thanks

>

Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Jason Wang]

On Mon, Mar 21, 2022 at 4:53 PM Hillf Danton <hdanton@sina.com> wrote:
>
> On Mon, 21 Mar 2022 14:04:28 +0800 Jason Wang wrote:
> > A userspace triggerable infinite loop could happen in
> > mlx5_cvq_kick_handler() if userspace keeps sending a huge amount of
> > cvq requests.
> >
> > Fixing this by introducing a quota and re-queue the work if we're out
> > of the budget. While at it, using a per device workqueue to avoid on
> > demand memory allocation for cvq.
> >
> > Fixes: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 28 +++++++++++++++-------------
> >  1 file changed, 15 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index d0f91078600e..d5a6fb3f9c41 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -163,6 +163,7 @@ struct mlx5_vdpa_net {
> >       u32 cur_num_vqs;
> >       struct notifier_block nb;
> >       struct vdpa_callback config_cb;
> > +     struct mlx5_vdpa_wq_ent cvq_ent;
> >  };
> >
> >  static void free_resources(struct mlx5_vdpa_net *ndev);
> > @@ -1600,6 +1601,8 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> >       return status;
> >  }
> >
> > +#define MLX5_CVQ_BUDGET 16
> > +
>
> This is not needed as given a single thread workqueue, a cond_resched()
> can do the job in the worker context instead of requeue of work.
>
> Hillf

I'm not sure I get this, but there's a loop in the work fn:

while(true) {
}

Where there could be no chance for the cond_resched() to run?

Thanks

>
> >  static void mlx5_cvq_kick_handler(struct work_struct *work)
> >  {
> >       virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> > @@ -1609,17 +1612,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
> >       struct mlx5_control_vq *cvq;
> >       struct mlx5_vdpa_net *ndev;
> >       size_t read, write;
> > -     int err;
> > +     int err, n = 0;
> >
> >       wqent = container_of(work, struct mlx5_vdpa_wq_ent, work);
> >       mvdev = wqent->mvdev;
> >       ndev = to_mlx5_vdpa_ndev(mvdev);
> >       cvq = &mvdev->cvq;
> >       if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > -             goto out;
> > +             return;
> >
> >       if (!cvq->ready)
> > -             goto out;
> > +             return;
> >
> >       while (true) {
> >               err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
> > @@ -1653,9 +1656,13 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
> >
> >               if (vringh_need_notify_iotlb(&cvq->vring))
> >                       vringh_notify(&cvq->vring);
> > +
> > +             n++;
> > +             if (n > MLX5_CVQ_BUDGET) {
> > +                     queue_work(mvdev->wq, &wqent->work);
> > +                     break;
> > +             }
> >       }
> > -out:
> > -     kfree(wqent);
> >  }
> >
> >  static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> > @@ -1663,7 +1670,6 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> >       struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> >       struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> >       struct mlx5_vdpa_virtqueue *mvq;
> > -     struct mlx5_vdpa_wq_ent *wqent;
> >
> >       if (!is_index_valid(mvdev, idx))
> >               return;
> > @@ -1672,13 +1678,7 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> >               if (!mvdev->cvq.ready)
> >                       return;
> >
> > -             wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> > -             if (!wqent)
> > -                     return;
> > -
> > -             wqent->mvdev = mvdev;
> > -             INIT_WORK(&wqent->work, mlx5_cvq_kick_handler);
> > -             queue_work(mvdev->wq, &wqent->work);
> > +             queue_work(mvdev->wq, &ndev->cvq_ent.work);
> >               return;
> >       }
> >
> > @@ -2668,6 +2668,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> >       if (err)
> >               goto err_mr;
> >
> > +     ndev->cvq_ent.mvdev = mvdev;
> > +     INIT_WORK(&ndev->cvq_ent.work, mlx5_cvq_kick_handler);
> >       mvdev->wq = create_singlethread_workqueue("mlx5_vdpa_wq");
> >       if (!mvdev->wq) {
> >               err = -ENOMEM;
> > --
> > 2.18.1
>

Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Jason Wang]

On Mon, Mar 21, 2022 at 4:59 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Mar 21, 2022 at 4:53 PM Hillf Danton <hdanton@sina.com> wrote:
> >
> > On Mon, 21 Mar 2022 14:04:28 +0800 Jason Wang wrote:
> > > A userspace triggerable infinite loop could happen in
> > > mlx5_cvq_kick_handler() if userspace keeps sending a huge amount of
> > > cvq requests.
> > >
> > > Fixing this by introducing a quota and re-queue the work if we're out
> > > of the budget. While at it, using a per device workqueue to avoid on
> > > demand memory allocation for cvq.
> > >
> > > Fixes: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 28 +++++++++++++++-------------
> > >  1 file changed, 15 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index d0f91078600e..d5a6fb3f9c41 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -163,6 +163,7 @@ struct mlx5_vdpa_net {
> > >       u32 cur_num_vqs;
> > >       struct notifier_block nb;
> > >       struct vdpa_callback config_cb;
> > > +     struct mlx5_vdpa_wq_ent cvq_ent;
> > >  };
> > >
> > >  static void free_resources(struct mlx5_vdpa_net *ndev);
> > > @@ -1600,6 +1601,8 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> > >       return status;
> > >  }
> > >
> > > +#define MLX5_CVQ_BUDGET 16
> > > +
> >
> > This is not needed as given a single thread workqueue, a cond_resched()
> > can do the job in the worker context instead of requeue of work.
> >
> > Hillf
>
> I'm not sure I get this, but there's a loop in the work fn:
>
> while(true) {
> }
>
> Where there could be no chance for the cond_resched() to run?

Ok, speak too fast. So you meant to add a cond_resched() in the loop?

Thanks

>
> Thanks
>
> >
> > >  static void mlx5_cvq_kick_handler(struct work_struct *work)
> > >  {
> > >       virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> > > @@ -1609,17 +1612,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
> > >       struct mlx5_control_vq *cvq;
> > >       struct mlx5_vdpa_net *ndev;
> > >       size_t read, write;
> > > -     int err;
> > > +     int err, n = 0;
> > >
> > >       wqent = container_of(work, struct mlx5_vdpa_wq_ent, work);
> > >       mvdev = wqent->mvdev;
> > >       ndev = to_mlx5_vdpa_ndev(mvdev);
> > >       cvq = &mvdev->cvq;
> > >       if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> > > -             goto out;
> > > +             return;
> > >
> > >       if (!cvq->ready)
> > > -             goto out;
> > > +             return;
> > >
> > >       while (true) {
> > >               err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
> > > @@ -1653,9 +1656,13 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
> > >
> > >               if (vringh_need_notify_iotlb(&cvq->vring))
> > >                       vringh_notify(&cvq->vring);
> > > +
> > > +             n++;
> > > +             if (n > MLX5_CVQ_BUDGET) {
> > > +                     queue_work(mvdev->wq, &wqent->work);
> > > +                     break;
> > > +             }
> > >       }
> > > -out:
> > > -     kfree(wqent);
> > >  }
> > >
> > >  static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> > > @@ -1663,7 +1670,6 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> > >       struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > >       struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > >       struct mlx5_vdpa_virtqueue *mvq;
> > > -     struct mlx5_vdpa_wq_ent *wqent;
> > >
> > >       if (!is_index_valid(mvdev, idx))
> > >               return;
> > > @@ -1672,13 +1678,7 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> > >               if (!mvdev->cvq.ready)
> > >                       return;
> > >
> > > -             wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> > > -             if (!wqent)
> > > -                     return;
> > > -
> > > -             wqent->mvdev = mvdev;
> > > -             INIT_WORK(&wqent->work, mlx5_cvq_kick_handler);
> > > -             queue_work(mvdev->wq, &wqent->work);
> > > +             queue_work(mvdev->wq, &ndev->cvq_ent.work);
> > >               return;
> > >       }
> > >
> > > @@ -2668,6 +2668,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> > >       if (err)
> > >               goto err_mr;
> > >
> > > +     ndev->cvq_ent.mvdev = mvdev;
> > > +     INIT_WORK(&ndev->cvq_ent.work, mlx5_cvq_kick_handler);
> > >       mvdev->wq = create_singlethread_workqueue("mlx5_vdpa_wq");
> > >       if (!mvdev->wq) {
> > >               err = -ENOMEM;
> > > --
> > > 2.18.1
> >

Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Jason Wang]

On Mon, Mar 21, 2022 at 8:34 PM Hillf Danton <hdanton@sina.com> wrote:
>
> On Mon, 21 Mar 2022 17:00:09 +0800 Jason Wang wrote:
> >
> > Ok, speak too fast.
>
> Frankly I have fun running faster five days a week.

:)

>
> > So you meant to add a cond_resched() in the loop?
>
> Yes, it is one liner.

Yes, there will be no "infinite" loop, but since the loop is triggered
by userspace. It looks to me it will delay the flush/drain of the
workqueue forever which is still suboptimal.

Thanks

>
> Hillf
>

Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Jason Wang]

On Thu, Mar 24, 2022 at 8:54 AM Hillf Danton <hdanton@sina.com> wrote:
>
> On Tue, 22 Mar 2022 09:59:14 +0800 Jason Wang wrote:
> >
> > Yes, there will be no "infinite" loop, but since the loop is triggered
> > by userspace. It looks to me it will delay the flush/drain of the
> > workqueue forever which is still suboptimal.
>
> Usually it is barely possible to shoot two birds using a stone.
>
> Given the "forever", I am inclined to not running faster, hehe, though
> another cobble is to add another line in the loop checking if mvdev is
> unregistered, and for example make mvdev->cvq unready before destroying
> workqueue.
>
> static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev)
> {
>         struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
>         struct mlx5_vdpa_dev *mvdev = to_mvdev(dev);
>         struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>
>         mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
>         destroy_workqueue(mvdev->wq);
>         _vdpa_unregister_device(dev);
>         mgtdev->ndev = NULL;
> }
>

Yes, so we had

1) using a quota for re-requeue
2) using something like

while (READ_ONCE(cvq->ready)) {
        ...
        cond_resched();
}

There should not be too much difference except we need to use
cancel_work_sync() instead of flush_work for 1).

I would keep the code as is but if you stick I can change.

Thanks

Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Michael S. Tsirkin]

On Thu, Mar 24, 2022 at 02:04:19PM +0800, Hillf Danton wrote:
> On Thu, 24 Mar 2022 10:34:09 +0800 Jason Wang wrote:
> > On Thu, Mar 24, 2022 at 8:54 AM Hillf Danton <hdanton@sina.com> wrote:
> > >
> > > On Tue, 22 Mar 2022 09:59:14 +0800 Jason Wang wrote:
> > > >
> > > > Yes, there will be no "infinite" loop, but since the loop is triggered
> > > > by userspace. It looks to me it will delay the flush/drain of the
> > > > workqueue forever which is still suboptimal.
> > >
> > > Usually it is barely possible to shoot two birds using a stone.
> > >
> > > Given the "forever", I am inclined to not running faster, hehe, though
> > > another cobble is to add another line in the loop checking if mvdev is
> > > unregistered, and for example make mvdev->cvq unready before destroying
> > > workqueue.
> > >
> > > static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev)
> > > {
> > >         struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
> > >         struct mlx5_vdpa_dev *mvdev = to_mvdev(dev);
> > >         struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > >
> > >         mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> > >         destroy_workqueue(mvdev->wq);
> > >         _vdpa_unregister_device(dev);
> > >         mgtdev->ndev = NULL;
> > > }
> > >
> > 
> > Yes, so we had
> > 
> > 1) using a quota for re-requeue
> > 2) using something like
> > 
> > while (READ_ONCE(cvq->ready)) {
> >         ...
> >         cond_resched();
> > }
> > 
> > There should not be too much difference except we need to use
> > cancel_work_sync() instead of flush_work for 1).
> > 
> > I would keep the code as is but if you stick I can change.
> 
> No Sir I would not - I am simply not a fan of work requeue.
> 
> Hillf

I think I agree - requeue adds latency spikes under heavy load -
unfortunately, not measured by netperf but still important
for latency sensitive workloads. Checking a flag is cheaper.

-- 
MST

Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Jason Wang]

On Thu, Mar 24, 2022 at 2:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Mar 24, 2022 at 02:04:19PM +0800, Hillf Danton wrote:
> > On Thu, 24 Mar 2022 10:34:09 +0800 Jason Wang wrote:
> > > On Thu, Mar 24, 2022 at 8:54 AM Hillf Danton <hdanton@sina.com> wrote:
> > > >
> > > > On Tue, 22 Mar 2022 09:59:14 +0800 Jason Wang wrote:
> > > > >
> > > > > Yes, there will be no "infinite" loop, but since the loop is triggered
> > > > > by userspace. It looks to me it will delay the flush/drain of the
> > > > > workqueue forever which is still suboptimal.
> > > >
> > > > Usually it is barely possible to shoot two birds using a stone.
> > > >
> > > > Given the "forever", I am inclined to not running faster, hehe, though
> > > > another cobble is to add another line in the loop checking if mvdev is
> > > > unregistered, and for example make mvdev->cvq unready before destroying
> > > > workqueue.
> > > >
> > > > static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev)
> > > > {
> > > >         struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
> > > >         struct mlx5_vdpa_dev *mvdev = to_mvdev(dev);
> > > >         struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > >
> > > >         mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> > > >         destroy_workqueue(mvdev->wq);
> > > >         _vdpa_unregister_device(dev);
> > > >         mgtdev->ndev = NULL;
> > > > }
> > > >
> > >
> > > Yes, so we had
> > >
> > > 1) using a quota for re-requeue
> > > 2) using something like
> > >
> > > while (READ_ONCE(cvq->ready)) {
> > >         ...
> > >         cond_resched();
> > > }
> > >
> > > There should not be too much difference except we need to use
> > > cancel_work_sync() instead of flush_work for 1).
> > >
> > > I would keep the code as is but if you stick I can change.
> >
> > No Sir I would not - I am simply not a fan of work requeue.
> >
> > Hillf
>
> I think I agree - requeue adds latency spikes under heavy load -
> unfortunately, not measured by netperf but still important
> for latency sensitive workloads. Checking a flag is cheaper.

Just spot another possible issue.

The workqueue will be used by another work to update the carrier
(event_handler()). Using cond_resched() may still have unfair issue
which blocks the carrier update for infinite time,

Thanks

>
> --
> MST
>

RE: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Eli Cohen]

> -----Original Message-----
> From: Hillf Danton <hdanton@sina.com>
> Sent: Thursday, March 24, 2022 2:02 PM
> To: Jason Wang <jasowang@redhat.com>
> Cc: Eli Cohen <elic@nvidia.com>; Michael S. Tsirkin <mst@redhat.com>; virtualization <virtualization@lists.linux-foundation.org>; linux-
> kernel <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU
> 
> On Thu, 24 Mar 2022 16:20:34 +0800 Jason Wang wrote:
> > On Thu, Mar 24, 2022 at 2:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Thu, Mar 24, 2022 at 02:04:19PM +0800, Hillf Danton wrote:
> > > > On Thu, 24 Mar 2022 10:34:09 +0800 Jason Wang wrote:
> > > > > On Thu, Mar 24, 2022 at 8:54 AM Hillf Danton <hdanton@sina.com> wrote:
> > > > > >
> > > > > > On Tue, 22 Mar 2022 09:59:14 +0800 Jason Wang wrote:
> > > > > > >
> > > > > > > Yes, there will be no "infinite" loop, but since the loop is triggered
> > > > > > > by userspace. It looks to me it will delay the flush/drain of the
> > > > > > > workqueue forever which is still suboptimal.
> > > > > >
> > > > > > Usually it is barely possible to shoot two birds using a stone.
> > > > > >
> > > > > > Given the "forever", I am inclined to not running faster, hehe, though
> > > > > > another cobble is to add another line in the loop checking if mvdev is
> > > > > > unregistered, and for example make mvdev->cvq unready before destroying
> > > > > > workqueue.
> > > > > >
> > > > > > static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev)
> > > > > > {
> > > > > >         struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
> > > > > >         struct mlx5_vdpa_dev *mvdev = to_mvdev(dev);
> > > > > >         struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > >
> > > > > >         mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> > > > > >         destroy_workqueue(mvdev->wq);
> > > > > >         _vdpa_unregister_device(dev);
> > > > > >         mgtdev->ndev = NULL;
> > > > > > }
> > > > > >
> > > > >
> > > > > Yes, so we had
> > > > >
> > > > > 1) using a quota for re-requeue
> > > > > 2) using something like
> > > > >
> > > > > while (READ_ONCE(cvq->ready)) {
> > > > >         ...
> > > > >         cond_resched();
> > > > > }
> > > > >
> > > > > There should not be too much difference except we need to use
> > > > > cancel_work_sync() instead of flush_work for 1).
> > > > >
> > > > > I would keep the code as is but if you stick I can change.
> > > >
> > > > No Sir I would not - I am simply not a fan of work requeue.
> > > >
> > > > Hillf
> > >
> > > I think I agree - requeue adds latency spikes under heavy load -
> > > unfortunately, not measured by netperf but still important
> > > for latency sensitive workloads. Checking a flag is cheaper.
> >
> > Just spot another possible issue.
> >
> > The workqueue will be used by another work to update the carrier
> > (event_handler()). Using cond_resched() may still have unfair issue
> > which blocks the carrier update for infinite time,
> 
> Then would you please specify the reason why mvdev->wq is single
> threaded? Given requeue, the serialization of the two works is not
> strong. Otherwise unbound WQ that can process works in parallel is
> a cure to the unfairness above.
> 

I think the proposed patch can still be used with quota equal to one.
That would guarantee fairness.
This is not performance critical and a single workqueue should be enough.

> Thanks
> Hillf

Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Jason Wang]

On Thu, Mar 24, 2022 at 8:24 PM Eli Cohen <elic@nvidia.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Hillf Danton <hdanton@sina.com>
> > Sent: Thursday, March 24, 2022 2:02 PM
> > To: Jason Wang <jasowang@redhat.com>
> > Cc: Eli Cohen <elic@nvidia.com>; Michael S. Tsirkin <mst@redhat.com>; virtualization <virtualization@lists.linux-foundation.org>; linux-
> > kernel <linux-kernel@vger.kernel.org>
> > Subject: Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU
> >
> > On Thu, 24 Mar 2022 16:20:34 +0800 Jason Wang wrote:
> > > On Thu, Mar 24, 2022 at 2:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Thu, Mar 24, 2022 at 02:04:19PM +0800, Hillf Danton wrote:
> > > > > On Thu, 24 Mar 2022 10:34:09 +0800 Jason Wang wrote:
> > > > > > On Thu, Mar 24, 2022 at 8:54 AM Hillf Danton <hdanton@sina.com> wrote:
> > > > > > >
> > > > > > > On Tue, 22 Mar 2022 09:59:14 +0800 Jason Wang wrote:
> > > > > > > >
> > > > > > > > Yes, there will be no "infinite" loop, but since the loop is triggered
> > > > > > > > by userspace. It looks to me it will delay the flush/drain of the
> > > > > > > > workqueue forever which is still suboptimal.
> > > > > > >
> > > > > > > Usually it is barely possible to shoot two birds using a stone.
> > > > > > >
> > > > > > > Given the "forever", I am inclined to not running faster, hehe, though
> > > > > > > another cobble is to add another line in the loop checking if mvdev is
> > > > > > > unregistered, and for example make mvdev->cvq unready before destroying
> > > > > > > workqueue.
> > > > > > >
> > > > > > > static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev)
> > > > > > > {
> > > > > > >         struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
> > > > > > >         struct mlx5_vdpa_dev *mvdev = to_mvdev(dev);
> > > > > > >         struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > > >
> > > > > > >         mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> > > > > > >         destroy_workqueue(mvdev->wq);
> > > > > > >         _vdpa_unregister_device(dev);
> > > > > > >         mgtdev->ndev = NULL;
> > > > > > > }
> > > > > > >
> > > > > >
> > > > > > Yes, so we had
> > > > > >
> > > > > > 1) using a quota for re-requeue
> > > > > > 2) using something like
> > > > > >
> > > > > > while (READ_ONCE(cvq->ready)) {
> > > > > >         ...
> > > > > >         cond_resched();
> > > > > > }
> > > > > >
> > > > > > There should not be too much difference except we need to use
> > > > > > cancel_work_sync() instead of flush_work for 1).
> > > > > >
> > > > > > I would keep the code as is but if you stick I can change.
> > > > >
> > > > > No Sir I would not - I am simply not a fan of work requeue.
> > > > >
> > > > > Hillf
> > > >
> > > > I think I agree - requeue adds latency spikes under heavy load -
> > > > unfortunately, not measured by netperf but still important
> > > > for latency sensitive workloads. Checking a flag is cheaper.
> > >
> > > Just spot another possible issue.
> > >
> > > The workqueue will be used by another work to update the carrier
> > > (event_handler()). Using cond_resched() may still have unfair issue
> > > which blocks the carrier update for infinite time,
> >
> > Then would you please specify the reason why mvdev->wq is single
> > threaded?

I didn't see a reason why it needs to be a single threaded (ordered).

> Given requeue, the serialization of the two works is not
> > strong. Otherwise unbound WQ that can process works in parallel is
> > a cure to the unfairness above.

Yes, and we probably don't want a per device workqueue but a per
module one. Or simply use the system_wq one.

> >
>
> I think the proposed patch can still be used with quota equal to one.
> That would guarantee fairness.
> This is not performance critical and a single workqueue should be enough.

Yes, but both Hillf and Michael don't like requeuing. So my plan is

1) send patch 2 first since it's a hard requirement for the next RHEL release
2) a series to fix this hogging issue by
2.1) switch to use a per module workqueue
2.2) READ_ONCE(cvq->ready) + cond_resched()

Thanks

>
> > Thanks
> > Hillf
>

Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Michael S. Tsirkin]

On Fri, Mar 25, 2022 at 11:22:25AM +0800, Jason Wang wrote:
> On Thu, Mar 24, 2022 at 8:24 PM Eli Cohen <elic@nvidia.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Hillf Danton <hdanton@sina.com>
> > > Sent: Thursday, March 24, 2022 2:02 PM
> > > To: Jason Wang <jasowang@redhat.com>
> > > Cc: Eli Cohen <elic@nvidia.com>; Michael S. Tsirkin <mst@redhat.com>; virtualization <virtualization@lists.linux-foundation.org>; linux-
> > > kernel <linux-kernel@vger.kernel.org>
> > > Subject: Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU
> > >
> > > On Thu, 24 Mar 2022 16:20:34 +0800 Jason Wang wrote:
> > > > On Thu, Mar 24, 2022 at 2:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Thu, Mar 24, 2022 at 02:04:19PM +0800, Hillf Danton wrote:
> > > > > > On Thu, 24 Mar 2022 10:34:09 +0800 Jason Wang wrote:
> > > > > > > On Thu, Mar 24, 2022 at 8:54 AM Hillf Danton <hdanton@sina.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, 22 Mar 2022 09:59:14 +0800 Jason Wang wrote:
> > > > > > > > >
> > > > > > > > > Yes, there will be no "infinite" loop, but since the loop is triggered
> > > > > > > > > by userspace. It looks to me it will delay the flush/drain of the
> > > > > > > > > workqueue forever which is still suboptimal.
> > > > > > > >
> > > > > > > > Usually it is barely possible to shoot two birds using a stone.
> > > > > > > >
> > > > > > > > Given the "forever", I am inclined to not running faster, hehe, though
> > > > > > > > another cobble is to add another line in the loop checking if mvdev is
> > > > > > > > unregistered, and for example make mvdev->cvq unready before destroying
> > > > > > > > workqueue.
> > > > > > > >
> > > > > > > > static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev)
> > > > > > > > {
> > > > > > > >         struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
> > > > > > > >         struct mlx5_vdpa_dev *mvdev = to_mvdev(dev);
> > > > > > > >         struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > > > >
> > > > > > > >         mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> > > > > > > >         destroy_workqueue(mvdev->wq);
> > > > > > > >         _vdpa_unregister_device(dev);
> > > > > > > >         mgtdev->ndev = NULL;
> > > > > > > > }
> > > > > > > >
> > > > > > >
> > > > > > > Yes, so we had
> > > > > > >
> > > > > > > 1) using a quota for re-requeue
> > > > > > > 2) using something like
> > > > > > >
> > > > > > > while (READ_ONCE(cvq->ready)) {
> > > > > > >         ...
> > > > > > >         cond_resched();
> > > > > > > }
> > > > > > >
> > > > > > > There should not be too much difference except we need to use
> > > > > > > cancel_work_sync() instead of flush_work for 1).
> > > > > > >
> > > > > > > I would keep the code as is but if you stick I can change.
> > > > > >
> > > > > > No Sir I would not - I am simply not a fan of work requeue.
> > > > > >
> > > > > > Hillf
> > > > >
> > > > > I think I agree - requeue adds latency spikes under heavy load -
> > > > > unfortunately, not measured by netperf but still important
> > > > > for latency sensitive workloads. Checking a flag is cheaper.
> > > >
> > > > Just spot another possible issue.
> > > >
> > > > The workqueue will be used by another work to update the carrier
> > > > (event_handler()). Using cond_resched() may still have unfair issue
> > > > which blocks the carrier update for infinite time,
> > >
> > > Then would you please specify the reason why mvdev->wq is single
> > > threaded?
> 
> I didn't see a reason why it needs to be a single threaded (ordered).
> 
> > Given requeue, the serialization of the two works is not
> > > strong. Otherwise unbound WQ that can process works in parallel is
> > > a cure to the unfairness above.
> 
> Yes, and we probably don't want a per device workqueue but a per
> module one. Or simply use the system_wq one.
> 
> > >
> >
> > I think the proposed patch can still be used with quota equal to one.
> > That would guarantee fairness.
> > This is not performance critical and a single workqueue should be enough.
> 
> Yes, but both Hillf and Michael don't like requeuing. So my plan is
> 
> 1) send patch 2 first since it's a hard requirement for the next RHEL release
> 2) a series to fix this hogging issue by
> 2.1) switch to use a per module workqueue
> 2.2) READ_ONCE(cvq->ready) + cond_resched()
> 
> Thanks

Actually if we don't care about speed here then requeing with quota of 1
is fine, in that we don't have a quota at all, we just always requeue
instead of a loop.

It's the mix of requeue and a loop that I consider confusing.


> >
> > > Thanks
> > > Hillf
> >

Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU

[Jason Wang]

On Fri, Mar 25, 2022 at 2:45 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Mar 25, 2022 at 11:22:25AM +0800, Jason Wang wrote:
> > On Thu, Mar 24, 2022 at 8:24 PM Eli Cohen <elic@nvidia.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Hillf Danton <hdanton@sina.com>
> > > > Sent: Thursday, March 24, 2022 2:02 PM
> > > > To: Jason Wang <jasowang@redhat.com>
> > > > Cc: Eli Cohen <elic@nvidia.com>; Michael S. Tsirkin <mst@redhat.com>; virtualization <virtualization@lists.linux-foundation.org>; linux-
> > > > kernel <linux-kernel@vger.kernel.org>
> > > > Subject: Re: [PATCH 1/2] vdpa: mlx5: prevent cvq work from hogging CPU
> > > >
> > > > On Thu, 24 Mar 2022 16:20:34 +0800 Jason Wang wrote:
> > > > > On Thu, Mar 24, 2022 at 2:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > On Thu, Mar 24, 2022 at 02:04:19PM +0800, Hillf Danton wrote:
> > > > > > > On Thu, 24 Mar 2022 10:34:09 +0800 Jason Wang wrote:
> > > > > > > > On Thu, Mar 24, 2022 at 8:54 AM Hillf Danton <hdanton@sina.com> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, 22 Mar 2022 09:59:14 +0800 Jason Wang wrote:
> > > > > > > > > >
> > > > > > > > > > Yes, there will be no "infinite" loop, but since the loop is triggered
> > > > > > > > > > by userspace. It looks to me it will delay the flush/drain of the
> > > > > > > > > > workqueue forever which is still suboptimal.
> > > > > > > > >
> > > > > > > > > Usually it is barely possible to shoot two birds using a stone.
> > > > > > > > >
> > > > > > > > > Given the "forever", I am inclined to not running faster, hehe, though
> > > > > > > > > another cobble is to add another line in the loop checking if mvdev is
> > > > > > > > > unregistered, and for example make mvdev->cvq unready before destroying
> > > > > > > > > workqueue.
> > > > > > > > >
> > > > > > > > > static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev)
> > > > > > > > > {
> > > > > > > > >         struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
> > > > > > > > >         struct mlx5_vdpa_dev *mvdev = to_mvdev(dev);
> > > > > > > > >         struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > > > > > > >
> > > > > > > > >         mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> > > > > > > > >         destroy_workqueue(mvdev->wq);
> > > > > > > > >         _vdpa_unregister_device(dev);
> > > > > > > > >         mgtdev->ndev = NULL;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > >
> > > > > > > > Yes, so we had
> > > > > > > >
> > > > > > > > 1) using a quota for re-requeue
> > > > > > > > 2) using something like
> > > > > > > >
> > > > > > > > while (READ_ONCE(cvq->ready)) {
> > > > > > > >         ...
> > > > > > > >         cond_resched();
> > > > > > > > }
> > > > > > > >
> > > > > > > > There should not be too much difference except we need to use
> > > > > > > > cancel_work_sync() instead of flush_work for 1).
> > > > > > > >
> > > > > > > > I would keep the code as is but if you stick I can change.
> > > > > > >
> > > > > > > No Sir I would not - I am simply not a fan of work requeue.
> > > > > > >
> > > > > > > Hillf
> > > > > >
> > > > > > I think I agree - requeue adds latency spikes under heavy load -
> > > > > > unfortunately, not measured by netperf but still important
> > > > > > for latency sensitive workloads. Checking a flag is cheaper.
> > > > >
> > > > > Just spot another possible issue.
> > > > >
> > > > > The workqueue will be used by another work to update the carrier
> > > > > (event_handler()). Using cond_resched() may still have unfair issue
> > > > > which blocks the carrier update for infinite time,
> > > >
> > > > Then would you please specify the reason why mvdev->wq is single
> > > > threaded?
> >
> > I didn't see a reason why it needs to be a single threaded (ordered).
> >
> > > Given requeue, the serialization of the two works is not
> > > > strong. Otherwise unbound WQ that can process works in parallel is
> > > > a cure to the unfairness above.
> >
> > Yes, and we probably don't want a per device workqueue but a per
> > module one. Or simply use the system_wq one.
> >
> > > >
> > >
> > > I think the proposed patch can still be used with quota equal to one.
> > > That would guarantee fairness.
> > > This is not performance critical and a single workqueue should be enough.
> >
> > Yes, but both Hillf and Michael don't like requeuing. So my plan is
> >
> > 1) send patch 2 first since it's a hard requirement for the next RHEL release
> > 2) a series to fix this hogging issue by
> > 2.1) switch to use a per module workqueue
> > 2.2) READ_ONCE(cvq->ready) + cond_resched()
> >
> > Thanks
>
> Actually if we don't care about speed here then requeing with quota of 1
> is fine, in that we don't have a quota at all, we just always requeue
> instead of a loop.
>
> It's the mix of requeue and a loop that I consider confusing.

Ok, Hillf, does this make sense for you? We want the issue to be fixed
soon, it's near to our product release.

Thanks

>
>
> > >
> > > > Thanks
> > > > Hillf
> > >
>