Messages in this thread | | | Date | Wed, 6 Apr 2022 16:25:56 +0800 | Subject | Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size | From | Jason Wang <> |
| |
在 2022/4/6 上午10:35, Jason Wang 写道: > > 在 2022/4/4 下午7:24, Michael S. Tsirkin 写道: >> On Mon, Apr 04, 2022 at 11:07:36AM +0000, Eli Cohen wrote: >>>> From: Michael S. Tsirkin <mst@redhat.com> >>>> Sent: Monday, April 4, 2022 1:35 PM >>>> To: Jason Wang <jasowang@redhat.com> >>>> Cc: Eli Cohen <elic@nvidia.com>; hdanton@sina.com; >>>> virtualization@lists.linux-foundation.org; >>>> linux-kernel@vger.kernel.org >>>> Subject: Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size >>>> >>>> On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote: >>>>> From: Eli Cohen <elic@nvidia.com> >>>>> >>>>> The current code evaluates RQT size based on the configured number of >>>>> virtqueues. This can raise an issue in the following scenario: >>>>> >>>>> Assume MQ was negotiated. >>>>> 1. mlx5_vdpa_set_map() gets called. >>>>> 2. handle_ctrl_mq() is called setting cur_num_vqs to some value, >>>>> lower >>>>> than the configured max VQs. >>>>> 3. A second set_map gets called, but now a smaller number of VQs >>>>> is used >>>>> to evaluate the size of the RQT. >>>>> 4. handle_ctrl_mq() is called with a value larger than what the >>>>> RQT can >>>>> hold. This will emit errors and the driver state is compromised. >>>>> >>>>> To fix this, we use a new field in struct mlx5_vdpa_net to hold the >>>>> required number of entries in the RQT. This value is evaluated in >>>>> mlx5_vdpa_set_driver_features() where we have the negotiated features >>>>> all set up. >>>>> >>>>> In addtion >>>> addition? >>> Do you need me to send another version? >> It's a bit easier that way but I can handle it manually too. > > > Let me send a new version with this fixed.
Ok, it looks like if I use git-send-email when From:tag is not me. The patch will be sent as a attachment as spotted by Maxime.
Eli, would you please send a v3 with my acked-by? (Since I don't want to change the author)
Thanks
> > >> >>> If so, let's wait for Jason's reply. >> Right. >> >>>>> to that, we take into consideration the max capability of RQT >>>>> entries early when the device is added so we don't need to take >>>>> consider >>>>> it when creating the RQT. >>>>> >>>>> Last, we remove the use of mlx5_vdpa_max_qps() which just returns the >>>>> max_vas / 2 and make the code clearer. >>>>> >>>>> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support") >>>>> Signed-off-by: Eli Cohen <elic@nvidia.com> >>>> Jason I don't have your ack or S.O.B on this one. > > > My bad, for some reason, I miss that. > > Will fix. > > Thanks > > >>>> >>>> >>>>> --- >>>>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 >>>>> +++++++++++-------------------- >>>>> 1 file changed, 21 insertions(+), 40 deletions(-) >>>>> >>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c >>>>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c >>>>> index 53b8c1a68f90..61bec1ed0bc9 100644 >>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c >>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c >>>>> @@ -161,6 +161,7 @@ struct mlx5_vdpa_net { >>>>> struct mlx5_flow_handle *rx_rule_mcast; >>>>> bool setup; >>>>> u32 cur_num_vqs; >>>>> + u32 rqt_size; >>>>> struct notifier_block nb; >>>>> struct vdpa_callback config_cb; >>>>> struct mlx5_vdpa_wq_ent cvq_ent; >>>>> @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct >>>>> mlx5_vdpa_dev *mvdev, u16 val) >>>>> return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), >>>>> val); >>>>> } >>>>> >>>>> -static inline u32 mlx5_vdpa_max_qps(int max_vqs) >>>>> -{ >>>>> - return max_vqs / 2; >>>>> -} >>>>> - >>>>> static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev) >>>>> { >>>>> if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ))) >>>>> return 2; >>>>> >>>>> - return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); >>>>> + return mvdev->max_vqs; >>>>> } >>>>> >>>>> static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx) >>>>> @@ -1236,25 +1232,13 @@ static void teardown_vq(struct >>>>> mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue * >>>>> static int create_rqt(struct mlx5_vdpa_net *ndev) >>>>> { >>>>> __be32 *list; >>>>> - int max_rqt; >>>>> void *rqtc; >>>>> int inlen; >>>>> void *in; >>>>> int i, j; >>>>> int err; >>>>> - int num; >>>>> - >>>>> - if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))) >>>>> - num = 1; >>>>> - else >>>>> - num = ndev->cur_num_vqs / 2; >>>>> >>>>> - max_rqt = min_t(int, roundup_pow_of_two(num), >>>>> - 1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size)); >>>>> - if (max_rqt < 1) >>>>> - return -EOPNOTSUPP; >>>>> - >>>>> - inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt * >>>>> MLX5_ST_SZ_BYTES(rq_num); >>>>> + inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size * >>>>> MLX5_ST_SZ_BYTES(rq_num); >>>>> in = kzalloc(inlen, GFP_KERNEL); >>>>> if (!in) >>>>> return -ENOMEM; >>>>> @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net >>>>> *ndev) >>>>> rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context); >>>>> >>>>> MLX5_SET(rqtc, rqtc, list_q_type, >>>>> MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q); >>>>> - MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt); >>>>> + MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size); >>>>> list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]); >>>>> - for (i = 0, j = 0; i < max_rqt; i++, j += 2) >>>>> - list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id); >>>>> + for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2) >>>>> + list[i] = cpu_to_be32(ndev->vqs[j % >>>>> ndev->cur_num_vqs].virtq_id); >>>>> >>>>> - MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt); >>>>> + MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size); >>>>> err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, >>>>> &ndev->res.rqtn); >>>>> kfree(in); >>>>> if (err) >>>>> @@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net >>>>> *ndev) >>>>> static int modify_rqt(struct mlx5_vdpa_net *ndev, int num) >>>>> { >>>>> __be32 *list; >>>>> - int max_rqt; >>>>> void *rqtc; >>>>> int inlen; >>>>> void *in; >>>>> int i, j; >>>>> int err; >>>>> >>>>> - max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2), >>>>> - 1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size)); >>>>> - if (max_rqt < 1) >>>>> - return -EOPNOTSUPP; >>>>> - >>>>> - inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt * >>>>> MLX5_ST_SZ_BYTES(rq_num); >>>>> + inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size * >>>>> MLX5_ST_SZ_BYTES(rq_num); >>>>> in = kzalloc(inlen, GFP_KERNEL); >>>>> if (!in) >>>>> return -ENOMEM; >>>>> @@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net >>>>> *ndev, int num) >>>>> MLX5_SET(rqtc, rqtc, list_q_type, >>>>> MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q); >>>>> >>>>> list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]); >>>>> - for (i = 0, j = 0; i < max_rqt; i++, j += 2) >>>>> + for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2) >>>>> list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id); >>>>> >>>>> - MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt); >>>>> + MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size); >>>>> err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, >>>>> ndev->res.rqtn); >>>>> kfree(in); >>>>> if (err) >>>>> @@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack >>>>> handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd) >>>>> >>>>> newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs); >>>>> if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN || >>>>> - newqps > mlx5_vdpa_max_qps(mvdev->max_vqs)) >>>>> + newqps > ndev->rqt_size) >>>>> break; >>>>> >>>>> if (ndev->cur_num_vqs == 2 * newqps) { >>>>> @@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct >>>>> mlx5_vdpa_dev *mvdev) >>>>> int err; >>>>> int i; >>>>> >>>>> - for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) { >>>>> + for (i = 0; i < mvdev->max_vqs; i++) { >>>>> err = setup_vq(ndev, &ndev->vqs[i]); >>>>> if (err) >>>>> goto err_vq; >>>>> @@ -2017,9 +1995,11 @@ static int >>>>> mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features) >>>>> >>>>> 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); >>>>> + ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, >>>>> ndev->config.max_virtqueue_pairs); >>>>> else >>>>> - ndev->cur_num_vqs = 2; >>>>> + ndev->rqt_size = 1; >>>>> + >>>>> + ndev->cur_num_vqs = 2 * ndev->rqt_size; >>>>> >>>>> update_cvq_info(mvdev); >>>>> return err; >>>>> @@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net >>>>> *ndev) >>>>> struct mlx5_vdpa_virtqueue *mvq; >>>>> int i; >>>>> >>>>> - for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs); >>>>> ++i) { >>>>> + for (i = 0; i < ndev->mvdev.max_vqs; ++i) { >>>>> mvq = &ndev->vqs[i]; >>>>> memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri)); >>>>> mvq->index = i; >>>>> @@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct >>>>> vdpa_mgmt_dev *v_mdev, const char *name, >>>>> return -EOPNOTSUPP; >>>>> } >>>>> >>>>> - max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, >>>>> max_num_virtio_queues); >>>>> + max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev, >>>>> max_num_virtio_queues), >>>>> + 1 << MLX5_CAP_GEN(mdev, log_max_rqt_size)); >>>>> if (max_vqs < 2) { >>>>> dev_warn(mdev->device, >>>>> "%d virtqueues are supported. At least 2 are >>>>> required\n", >>>>> @@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct >>>>> vdpa_mgmt_dev *v_mdev, const char *name, >>>>> ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC); >>>>> } >>>>> >>>>> - config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, >>>>> mlx5_vdpa_max_qps(max_vqs)); >>>>> + config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, >>>>> max_vqs / 2); >>>>> mvdev->vdev.dma_dev = &mdev->pdev->dev; >>>>> err = mlx5_vdpa_alloc_resources(&ndev->mvdev); >>>>> if (err) >>>>> @@ -2697,7 +2678,7 @@ 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); >>>>> mvdev->vdev.mdev = &mgtdev->mgtdev; >>>>> - err = _vdpa_register_device(&mvdev->vdev, 2 * >>>>> mlx5_vdpa_max_qps(max_vqs) + 1); >>>>> + err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1); >>>>> if (err) >>>>> goto err_reg; >>>>> >>>>> -- >>>>> 2.18.1
| |