Si-Wei Liu
2022-Jan-12 07:46 UTC
[PATCH 4/4] vdpa/mlx5: Fix tracking of current number of VQs
On 1/11/2022 10:37 PM, Eli Cohen wrote:> On Tue, Jan 11, 2022 at 02:14:47PM -0800, Si-Wei Liu 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)? > Not sure I follow. It's valid to have only CTRL_VQ feature but not MQ. > In that case we should have only two data VQs but then cur_num_vqs would > be set to whatever was configured by the user which could more than 2.In that case (CTRL_VQ && !MQ), it will end up with just two data VQs (plus one control queue which is not accounted in 'ethtool -l') negotiated. There should be some validation in handle_ctrl_mq to disallow user from configuring more than 2 data VQs for that case. You don't need to worry too much for that at the moment, I'll post a patch? following your series to fix it. Let's keep it simple for just checking both BIT_ULL(VIRTIO_NET_F_MQ) | BIT_ULL(VIRTIO_NET_F_CTRL_VQ) together. Thanks, -Siwei> >> 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;