Si-Wei Liu
2022-Jan-11 22:14 UTC
[PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs
On 1/11/2022 10:34 AM, Eli Cohen wrote:> Modify the code such that ndev->cur_num_vqs better reflects the actual > number of data virtqueues. The value can be accurately realized after > features have been negotiated. > > This is to prevent possible failures when modifying the RQT object if > the cur_num_vqs bears invalid value. > > No issue was actually encountered but this also makes the code more > readable. > > Fixes: c5a5cd3d3217 ("vdpa/mlx5: Support configuring max data virtqueue") > Signed-off-by: Eli Cohen <elic at nvidia.com> > --- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index 9eacfdb48434..b53603d94082 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -1246,8 +1246,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev) > if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))) > num = 1; > else > - num = mlx5vdpa16_to_cpu(&ndev->mvdev, > - ndev->config.max_virtqueue_pairs); > + num = ndev->cur_num_vqs / 2;Nit: the if branch can be consolidated> > max_rqt = min_t(int, roundup_pow_of_two(num), > 1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size)); > @@ -1983,6 +1982,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features) > return err; > > ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features; > + if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)) > + ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);Hmmm, not this patch, but there should've been validation done in the upper layer to guarantee set_featuers() for VIRTIO_NET_F_MQ always comes with VIRTIO_NET_F_CTRL_VQ. Maybe checking both: BIT_ULL(VIRTIO_NET_F_MQ) |? BIT_ULL(VIRTIO_NET_F_CTRL_VQ)? otherwise it looks good to me. Reviewed-by: Si-Wei Liu<si-wei.liu at oracle.com>> + else > + ndev->cur_num_vqs = 2; > + > update_cvq_info(mvdev); > return err; > } > @@ -2233,6 +2237,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev) > clear_vqs_ready(ndev); > mlx5_vdpa_destroy_mr(&ndev->mvdev); > ndev->mvdev.status = 0; > + ndev->cur_num_vqs = 0; > memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1)); > ndev->mvdev.actual_features = 0; > ++mvdev->generation; > @@ -2641,9 +2646,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name, > > ndev->nb.notifier_call = event_handler; > mlx5_notifier_register(mdev, &ndev->nb); > - ndev->cur_num_vqs = 2 * mlx5_vdpa_max_qps(max_vqs); > mvdev->vdev.mdev = &mgtdev->mgtdev; > - err = _vdpa_register_device(&mvdev->vdev, ndev->cur_num_vqs + 1); > + err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1); > if (err) > goto err_reg; >
Jason Wang
2022-Jan-12 02:29 UTC
[PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs
On Wed, Jan 12, 2022 at 6:15 AM Si-Wei Liu <si-wei.liu at oracle.com> wrote:> > > > On 1/11/2022 10:34 AM, Eli Cohen wrote: > > Modify the code such that ndev->cur_num_vqs better reflects the actual > > number of data virtqueues. The value can be accurately realized after > > features have been negotiated. > > > > This is to prevent possible failures when modifying the RQT object if > > the cur_num_vqs bears invalid value. > > > > No issue was actually encountered but this also makes the code more > > readable. > > > > Fixes: c5a5cd3d3217 ("vdpa/mlx5: Support configuring max data virtqueue") > > Signed-off-by: Eli Cohen <elic at nvidia.com> > > --- > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > index 9eacfdb48434..b53603d94082 100644 > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > @@ -1246,8 +1246,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev) > > if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))) > > num = 1; > > else > > - num = mlx5vdpa16_to_cpu(&ndev->mvdev, > > - ndev->config.max_virtqueue_pairs); > > + num = ndev->cur_num_vqs / 2; > Nit: the if branch can be consolidated > > > > > max_rqt = min_t(int, roundup_pow_of_two(num), > > 1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size)); > > @@ -1983,6 +1982,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features) > > return err; > > > > ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features; > > + if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)) > > + ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs); > Hmmm, not this patch, but there should've been validation done in the > upper layer to guarantee set_featuers() for VIRTIO_NET_F_MQ always comes > with VIRTIO_NET_F_CTRL_VQ. Maybe checking both: BIT_ULL(VIRTIO_NET_F_MQ) > | BIT_ULL(VIRTIO_NET_F_CTRL_VQ)?So the upper layer is unaware of the device type. It's better to do that in mlx5's set_features() according to the spec: The device MUST NOT offer a feature which requires another feature which was not offered. Thanks> > otherwise it looks good to me. > > Reviewed-by: Si-Wei Liu<si-wei.liu at oracle.com> > > + else > > + ndev->cur_num_vqs = 2; > > + > > update_cvq_info(mvdev); > > return err; > > } > > @@ -2233,6 +2237,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev) > > clear_vqs_ready(ndev); > > mlx5_vdpa_destroy_mr(&ndev->mvdev); > > ndev->mvdev.status = 0; > > + ndev->cur_num_vqs = 0; > > memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1)); > > ndev->mvdev.actual_features = 0; > > ++mvdev->generation; > > @@ -2641,9 +2646,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name, > > > > ndev->nb.notifier_call = event_handler; > > mlx5_notifier_register(mdev, &ndev->nb); > > - ndev->cur_num_vqs = 2 * mlx5_vdpa_max_qps(max_vqs); > > mvdev->vdev.mdev = &mgtdev->mgtdev; > > - err = _vdpa_register_device(&mvdev->vdev, ndev->cur_num_vqs + 1); > > + err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1); > > if (err) > > goto err_reg; > > >
Michael S. Tsirkin
2022-Jan-14 20:25 UTC
[PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs
On Tue, Jan 11, 2022 at 02:14:47PM -0800, Si-Wei Liu wrote:> Reviewed-by: Si-Wei Liu<si-wei.liu at oracle.com>Note: the correct format is: Reviewed-by: Si-Wei Liu <si-wei.liu at oracle.com> (with a space before <>). Pls take care in the future. Thanks! -- MST