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; > > >
Si-Wei Liu
2022-Jan-12 03:12 UTC
[PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs
On 1/11/2022 6:29 PM, Jason Wang wrote:> 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()That'll be fine. I thought the upper layer can be made device type aware and consolidate it to common library routines avoiding duplicated code in every individual driver of the same type. If this is against the goal of making vdpa core device type agnostic, then it's perhaps not needed. -Siwei> 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; >>>