RE: [PATCH] vdpa/mlx5: Fix MQ to support non power of two num queues

[Eli Cohen]

> From: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
> Sent: Wednesday, 14 September 2022 14:15
> To: Eli Cohen <elic@xxxxxxxxxx>; mst@xxxxxxxxxx; jasowang@xxxxxxxxxx;
> eperezma@xxxxxxxxxx; virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] vdpa/mlx5: Fix MQ to support non power of two num
> queues
> 
> 
> 
> On 9/12/2022 1:50 PM, Eli Cohen wrote:
> > RQT objects require that a power of two value be configured for both
> > rqt_max_size and rqt_actual size.
> >
> > For create_rqt, make sure to round up to the power of two the value of
> > given by the user who created the vdpa device and given by
> > ndev->rqt_size. The actual size is also rounded up to the power of two
> > using the current number of VQs given by ndev->cur_num_vqs.
> >
> > Same goes with modify_rqt where we need to make sure act size is power
> > of two based on the new number of QPs.
> >
> > Without this patch, attempt to create a device with non power of two QPs
> > would result in error from firmware.
> 
> What kind of error would it end up with, is there explicit warning in
> dmesg or it's just implicit?
The error would look something like this:
[Sep14 15:20] mlx5_core.sf mlx5_core.sf.6: mlx5_cmd_out_err:778:(pid 3466): CREATE_RQT(0x916) op_mod(0x0) failed, status bad parameter(0x3), syndrome (0x538fab), err(-22)

> And is there performance impact? It'd be
> nice to add such description that we can easily match the symptom just
> in case.
> 
I don't think there would be performance impact but I haven't checked performance with this fix.

> >
> > Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> > Signed-off-by: Eli Cohen <elic@xxxxxxxxxx>
> Acked-by: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
> 
> Thanks,
> -Siwei
> > ---
> >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++++++++-------
> >   1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index ed100a35e596..90913365def4 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -1320,6 +1320,8 @@ static void teardown_vq(struct mlx5_vdpa_net
> *ndev, struct mlx5_vdpa_virtqueue *
> >
> >   static int create_rqt(struct mlx5_vdpa_net *ndev)
> >   {
> > +	int rqt_table_size = roundup_pow_of_two(ndev->rqt_size);
> > +	int act_sz = roundup_pow_of_two(ndev->cur_num_vqs / 2);
> >   	__be32 *list;
> >   	void *rqtc;
> >   	int inlen;
> > @@ -1327,7 +1329,7 @@ static int create_rqt(struct mlx5_vdpa_net
> *ndev)
> >   	int i, j;
> >   	int err;
> >
> > -	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size *
> MLX5_ST_SZ_BYTES(rq_num);
> > +	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + rqt_table_size *
> MLX5_ST_SZ_BYTES(rq_num);
> >   	in = kzalloc(inlen, GFP_KERNEL);
> >   	if (!in)
> >   		return -ENOMEM;
> > @@ -1336,12 +1338,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, ndev->rqt_size);
> > +	MLX5_SET(rqtc, rqtc, rqt_max_size, rqt_table_size);
> >   	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> > -	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> > +	for (i = 0, j = 0; i < act_sz; i++, j += 2)
> >   		list[i] = cpu_to_be32(ndev->vqs[j % ndev-
> >cur_num_vqs].virtq_id);
> >
> > -	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
> > +	MLX5_SET(rqtc, rqtc, rqt_actual_size, act_sz);
> >   	err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev-
> >res.rqtn);
> >   	kfree(in);
> >   	if (err)
> > @@ -1354,6 +1356,7 @@ static int create_rqt(struct mlx5_vdpa_net
> *ndev)
> >
> >   static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
> >   {
> > +	int act_sz = roundup_pow_of_two(num / 2);
> >   	__be32 *list;
> >   	void *rqtc;
> >   	int inlen;
> > @@ -1361,7 +1364,7 @@ static int modify_rqt(struct mlx5_vdpa_net
> *ndev, int num)
> >   	int i, j;
> >   	int err;
> >
> > -	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size *
> MLX5_ST_SZ_BYTES(rq_num);
> > +	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + act_sz *
> MLX5_ST_SZ_BYTES(rq_num);
> >   	in = kzalloc(inlen, GFP_KERNEL);
> >   	if (!in)
> >   		return -ENOMEM;
> > @@ -1372,10 +1375,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 < ndev->rqt_size; i++, j += 2)
> > +	for (i = 0, j = 0; i < act_sz; i++, j = j + 2)
> >   		list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
> >
> > -	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
> > +	MLX5_SET(rqtc, rqtc, rqt_actual_size, act_sz);
> >   	err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, ndev-
> >res.rqtn);
> >   	kfree(in);
> >   	if (err)