Re: [PATCH 1/2] vdpa: Allow to configure max data virtqueues

Re: [PATCH 1/2] vdpa: Allow to configure max data virtqueues

[Si-Wei Liu]

On 11/30/2021 1:48 AM, Eli Cohen wrote:
> Allow to configure the max virtqueues for a device.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vdpa/vdpa.c  | 16 +++++++++++++++-
>   include/linux/vdpa.h |  1 +
>   2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 7332a74a4b00..e185ec2ee851 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -480,7 +480,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
>   }
>   
>   #define VDPA_DEV_NET_ATTRS_MASK ((1 << VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
> -				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU))
> +				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
> +				 (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
It seems VDPA_ATTR_DEV_MAX_VQS (u32) is what you want (# of data 
virtqueues instead of # of data virtqueue pairs)? Not sure what's 
possible use of VDPA_ATTR_DEV_NET_CFG_MAX_VQP, was it to dump/display 
the config space max_virtqueue_pairs value (u16, 1-32768) for 
virtio-net? Why there's such quasi-duplicate attribute introduced in the 
first place?

Not even sure VDPA_ATTR_DEV_MAX_VQS by definition should include other 
virtqueues as well: such as control virtqueue or event virtqueue. Hence 
the name will be more applicable to vdpa devices of other virtio type 
than just virtio-net. Otherwise I would think this attribute is slightly 
misnamed (max_data_vqs might be a proper name).

>   
>   static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *info)
>   {
> @@ -506,6 +507,19 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
>   			nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
>   		config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MTU);
>   	}
> +	if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]) {
> +		config.max_virtqueues = nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]);
> +		if (config.max_virtqueues < 2) {
> +			NL_SET_ERR_MSG_MOD(info->extack, "At least two virtqueues are required");
> +			return -EINVAL;
> +		}
> +		if ((config.max_virtqueues - 1) & config.max_virtqueues) {
> +			NL_SET_ERR_MSG_MOD(info->extack,
> +					   "Must provide power of two number of virtqueues");
Why there's such limitation for the number of vDPA virtqueues? I thought 
the software virtio doesn't have this limitation (power of two).

-Siwei

> +			return -EINVAL;
> +		}
> +		config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
> +	}
>   
>   	/* Skip checking capability if user didn't prefer to configure any
>   	 * device networking attributes. It is likely that user might have used
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index c3011ccda430..2f0b09c6d1ae 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -101,6 +101,7 @@ struct vdpa_dev_set_config {
>   		u16 mtu;
>   	} net;
>   	u64 mask;
> +	u16 max_virtqueues;
>   };
>   
>   /**

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] vdpa/mlx5: Support configuring max data virtqueues

[Si-Wei Liu]

On 11/30/2021 1:48 AM, Eli Cohen wrote:
> Check if the required number of data virtqueues was provided when a
> adding a new device and verify the new value does not exceed device
> capabilities.
>
> In addition, change the arrays holding virtqueue and callback contexts
> to be dynamically allocated.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 32 +++++++++++++++++++------------
>   1 file changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 63813fbb5f62..54e276e0df18 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
>   	struct mlx5_vq_restore_info ri;
>   };
>   
> -/* We will remove this limitation once mlx5_vdpa_alloc_resources()
> - * provides for driver space allocation
> - */
> -#define MLX5_MAX_SUPPORTED_VQS 16
> -
>   static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
>   {
>   	if (unlikely(idx > mvdev->max_idx))
> @@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
>   	struct mlx5_vdpa_dev mvdev;
>   	struct mlx5_vdpa_net_resources res;
>   	struct virtio_net_config config;
> -	struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
> -	struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
> +	struct mlx5_vdpa_virtqueue *vqs;
> +	struct vdpa_callback *event_cbs;
>   
>   	/* Serialize vq resources creation and destruction. This is required
>   	 * since memory map might change and we need to destroy and create
> @@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net *ndev)
>   {
>   	int i;
>   
> -	for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
> +	for (i = 0; i < ndev->mvdev.max_vqs; i++)
>   		suspend_vq(ndev, &ndev->vqs[i]);
>   }
>   
> @@ -1245,7 +1240,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>   	int i, j;
>   	int err;
>   
> -	max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
> +	max_rqt = min_t(int, ndev->mvdev.max_vqs  / 2,
>   			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
Here the user configured number of data vqs may get silently capped to 
what the device can support (up to 2^log_max_rqt_size). Do you consider 
expose this device specific capacity to vdpa users so it doesn't get 
blindly misconfigured, and/or return appropriate error message to 
indicate the failure cause?

If mlx5 vdpa really can't support power of 2 number of data vqs due to 
hardware limitation (?), an appropriate message should be returned here.

-Siwei
>   	if (max_rqt < 1)
>   		return -EOPNOTSUPP;
> @@ -2235,7 +2230,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>   	clear_vqs_ready(ndev);
>   	mlx5_vdpa_destroy_mr(&ndev->mvdev);
>   	ndev->mvdev.status = 0;
> -	memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
> +	memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
>   	ndev->mvdev.actual_features = 0;
>   	++mvdev->generation;
>   	if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
> @@ -2308,6 +2303,8 @@ static void mlx5_vdpa_free(struct vdpa_device *vdev)
>   	}
>   	mlx5_vdpa_free_resources(&ndev->mvdev);
>   	mutex_destroy(&ndev->reslock);
> +	kfree(ndev->event_cbs);
> +	kfree(ndev->vqs);
>   }
>   
>   static struct vdpa_notification_area mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)
> @@ -2547,13 +2544,23 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>   
>   	/* we save one virtqueue for control virtqueue should we require it */
>   	max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
> -	max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS);
> +	if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) {
> +		if (add_config->max_virtqueues > max_vqs)
> +			return -EINVAL;
> +		max_vqs = min_t(u32, max_vqs, add_config->max_virtqueues);
> +	}
>   
>   	ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev, mdev->device, &mlx5_vdpa_ops,
>   				 name, false);
>   	if (IS_ERR(ndev))
>   		return PTR_ERR(ndev);
>   
> +	ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);
> +	ndev->event_cbs = kcalloc(max_vqs + 1, sizeof(*ndev->event_cbs), GFP_KERNEL);
> +	if (!ndev->vqs || !ndev->event_cbs) {
> +		err = -ENOMEM;
> +		goto err_mtu;
> +	}
>   	ndev->mvdev.max_vqs = max_vqs;
>   	mvdev = &ndev->mvdev;
>   	mvdev->mdev = mdev;
> @@ -2676,7 +2683,8 @@ static int mlx5v_probe(struct auxiliary_device *adev,
>   	mgtdev->mgtdev.ops = &mdev_ops;
>   	mgtdev->mgtdev.device = mdev->device;
>   	mgtdev->mgtdev.id_table = id_table;
> -	mgtdev->mgtdev.config_attr_mask = (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR);
> +	mgtdev->mgtdev.config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
> +					  BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
>   	mgtdev->madev = madev;
>   
>   	err = vdpa_mgmtdev_register(&mgtdev->mgtdev);

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH 1/2] vdpa: Allow to configure max data virtqueues

[Parav Pandit via Virtualization]

> From: Eli Cohen <elic@nvidia.com>
> Sent: Wednesday, December 1, 2021 3:33 PM
> 
> On Tue, Nov 30, 2021 at 05:24:03PM -0800, Si-Wei Liu wrote:
> >
> >
> > On 11/30/2021 1:48 AM, Eli Cohen wrote:
> > > Allow to configure the max virtqueues for a device.
> > >
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > ---
> > >   drivers/vdpa/vdpa.c  | 16 +++++++++++++++-
> > >   include/linux/vdpa.h |  1 +
> > >   2 files changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> > > 7332a74a4b00..e185ec2ee851 100644
> > > --- a/drivers/vdpa/vdpa.c
> > > +++ b/drivers/vdpa/vdpa.c
> > > @@ -480,7 +480,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff
> *msg, struct netlink_callback *cb)
> > >   }
> > >   #define VDPA_DEV_NET_ATTRS_MASK ((1 <<
> VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
> > > -				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU))
> > > +				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
> > > +				 (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
> > It seems VDPA_ATTR_DEV_MAX_VQS (u32) is what you want (# of data
> > virtqueues instead of # of data virtqueue pairs)? Not sure what's
> > possible use of VDPA_ATTR_DEV_NET_CFG_MAX_VQP, was it to
> dump/display
> > the config space max_virtqueue_pairs value (u16, 1-32768) for
> > virtio-net? Why there's such quasi-duplicate attribute introduced in the first
> place?
> >
> 
> VDPA_ATTR_DEV_MAX_VQS currently returns vdev->nvqs which equals
> whatever passed to _vdpa_register_device(). The latter depends on the value
> provided by (struct vdpa_dev_set_config).max_virtqueues. 
> 
Max VQs configuration should reuse VDPA_ATTR_DEV_MAX_VQS.
it indicates what is the max vqs a given vdpa device is using. Until now it was driver's choice, now its users choice if provided.
So no need for additional attribute.

> Maybe we should add attributes to add aditional virtqueues like control
> virtqueue and their index. They could be returned by
> vdpa_dev_net_config_fill().
Yes. 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH 1/2] vdpa: Allow to configure max data virtqueues

[Parav Pandit via Virtualization]

> From: Eli Cohen <elic@nvidia.com>
> Sent: Wednesday, December 1, 2021 5:29 PM
> 
> On Wed, Dec 01, 2021 at 12:09:17PM +0200, Parav Pandit wrote:
> >
> >
> > > From: Eli Cohen <elic@nvidia.com>
> > > Sent: Wednesday, December 1, 2021 3:33 PM
> > >
> > > On Tue, Nov 30, 2021 at 05:24:03PM -0800, Si-Wei Liu wrote:
> > > >
> > > >
> > > > On 11/30/2021 1:48 AM, Eli Cohen wrote:
> > > > > Allow to configure the max virtqueues for a device.
> > > > >
> > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > ---
> > > > >   drivers/vdpa/vdpa.c  | 16 +++++++++++++++-
> > > > >   include/linux/vdpa.h |  1 +
> > > > >   2 files changed, 16 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> > > > > 7332a74a4b00..e185ec2ee851 100644
> > > > > --- a/drivers/vdpa/vdpa.c
> > > > > +++ b/drivers/vdpa/vdpa.c
> > > > > @@ -480,7 +480,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct
> > > > > sk_buff
> > > *msg, struct netlink_callback *cb)
> > > > >   }
> > > > >   #define VDPA_DEV_NET_ATTRS_MASK ((1 <<
> > > VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
> > > > > -				 (1 <<
> VDPA_ATTR_DEV_NET_CFG_MTU))
> > > > > +				 (1 <<
> VDPA_ATTR_DEV_NET_CFG_MTU) | \
> > > > > +				 (1 <<
> VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
> > > > It seems VDPA_ATTR_DEV_MAX_VQS (u32) is what you want (# of data
> > > > virtqueues instead of # of data virtqueue pairs)? Not sure what's
> > > > possible use of VDPA_ATTR_DEV_NET_CFG_MAX_VQP, was it to
> > > dump/display
> > > > the config space max_virtqueue_pairs value (u16, 1-32768) for
> > > > virtio-net? Why there's such quasi-duplicate attribute introduced
> > > > in the first
> > > place?
> > > >
> > >
> > > VDPA_ATTR_DEV_MAX_VQS currently returns vdev->nvqs which equals
> > > whatever passed to _vdpa_register_device(). The latter depends on
> > > the value provided by (struct vdpa_dev_set_config).max_virtqueues.
> > >
> 
> But currently VDPA_ATTR_DEV_MAX_VQS returns the number of used queues,
No. it doesn't.
It reports nvqs passed in _vdpa_register_device().
And this API has clear definition of nvqs = number of virtqueues supported by this device
nvqs != number of used vqs.
So current MAX_VQS is doing its job right.
For example nvqs can be be 8 and guest VM driver may just use two VQs (1 tx, 1 rx, and used vqs = 2).
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] vdpa: Allow to configure max data virtqueues

[Si-Wei Liu]

On 12/1/2021 2:03 AM, Eli Cohen wrote:
> On Tue, Nov 30, 2021 at 05:24:03PM -0800, Si-Wei Liu wrote:
>>
>> On 11/30/2021 1:48 AM, Eli Cohen wrote:
>>> Allow to configure the max virtqueues for a device.
>>>
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> ---
>>>    drivers/vdpa/vdpa.c  | 16 +++++++++++++++-
>>>    include/linux/vdpa.h |  1 +
>>>    2 files changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 7332a74a4b00..e185ec2ee851 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -480,7 +480,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
>>>    }
>>>    #define VDPA_DEV_NET_ATTRS_MASK ((1 << VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
>>> -				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU))
>>> +				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
>>> +				 (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
>> It seems VDPA_ATTR_DEV_MAX_VQS (u32) is what you want (# of data virtqueues
>> instead of # of data virtqueue pairs)? Not sure what's possible use of
>> VDPA_ATTR_DEV_NET_CFG_MAX_VQP, was it to dump/display the config space
>> max_virtqueue_pairs value (u16, 1-32768) for virtio-net? Why there's such
>> quasi-duplicate attribute introduced in the first place?
>>
> VDPA_ATTR_DEV_MAX_VQS currently returns vdev->nvqs which equals whatever
> passed to _vdpa_register_device(). The latter depends on the value
> provided by (struct vdpa_dev_set_config).max_virtqueues. It's the only
> way to let the user know if there are other virtqueues besides the data
> virtqueues. For example, if we support control virtqueue and configured
> 2 data QPs, we will return 5.
That's right. Then let VDPA_ATTR_DEV_MAX_VQS continue to be read-only as is.
>
> Maybe we should add attributes to add aditional virtqueues like control
> virtqueue and their index. They could be returned by
> vdpa_dev_net_config_fill().
Probably this info would need to return to QEMU for proper virtq 
modeling via vDPA ioctls rather than just netlink API. Agreed it's 
virtio-net specific config.
> VDPA_ATTR_DEV_NET_CFG_MAX_VQP seems the right choice since it is used to
> provid the requested number of data virtqueues when creating the device.
> Maybe we need to change the name to VDPA_ATTR_DEV_NET_CFG_MAX_VQ and
> then this patch makes more sense.
I would just keep the name for VDPA_ATTR_DEV_NET_CFG_MAX_VQP and have 
user configure the number of data queue pairs instead. Very few virtio 
types come with virtqueue in pairs, so this has to be vdpa net specific 
config.

Thanks,
-Siwei

>
>
> I will post another series to address all these issues.
>
>> Not even sure VDPA_ATTR_DEV_MAX_VQS by definition should include other
>> virtqueues as well: such as control virtqueue or event virtqueue. Hence the
>> name will be more applicable to vdpa devices of other virtio type than just
>> virtio-net. Otherwise I would think this attribute is slightly misnamed
>> (max_data_vqs might be a proper name).
>>
>>>    static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *info)
>>>    {
>>> @@ -506,6 +507,19 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
>>>    			nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
>>>    		config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MTU);
>>>    	}
>>> +	if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]) {
>>> +		config.max_virtqueues = nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]);
>>> +		if (config.max_virtqueues < 2) {
>>> +			NL_SET_ERR_MSG_MOD(info->extack, "At least two virtqueues are required");
>>> +			return -EINVAL;
>>> +		}
>>> +		if ((config.max_virtqueues - 1) & config.max_virtqueues) {
>>> +			NL_SET_ERR_MSG_MOD(info->extack,
>>> +					   "Must provide power of two number of virtqueues");
>> Why there's such limitation for the number of vDPA virtqueues? I thought the
>> software virtio doesn't have this limitation (power of two).
> Right, the limitation comes from mlx5_vdpa. I will post a patch later to
> remove that limitation.
>
>> -Siwei
>>
>>> +			return -EINVAL;
>>> +		}
>>> +		config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
>>> +	}
>>>    	/* Skip checking capability if user didn't prefer to configure any
>>>    	 * device networking attributes. It is likely that user might have used
>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>> index c3011ccda430..2f0b09c6d1ae 100644
>>> --- a/include/linux/vdpa.h
>>> +++ b/include/linux/vdpa.h
>>> @@ -101,6 +101,7 @@ struct vdpa_dev_set_config {
>>>    		u16 mtu;
>>>    	} net;
>>>    	u64 mask;
>>> +	u16 max_virtqueues;
>>>    };
>>>    /**

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] vdpa: Allow to configure max data virtqueues

[Si-Wei Liu]

[-- Attachment #1.1: Type: text/plain, Size: 568 bytes --]



On 12/1/2021 2:03 AM, Eli Cohen wrote:
>>> +		if ((config.max_virtqueues - 1) & config.max_virtqueues) {
>>> +			NL_SET_ERR_MSG_MOD(info->extack,
>>> +					   "Must provide power of two number of virtqueues");
>> Why there's such limitation for the number of vDPA virtqueues? I thought the
>> software virtio doesn't have this limitation (power of two).
> Right, the limitation comes from mlx5_vdpa. I will post a patch later to
> remove that limitation.
>
I mean the check should not live here. Other vdpa vendor device/driver 
may not have this limitation.

-Siwei

[-- Attachment #1.2: Type: text/html, Size: 1183 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] vdpa: Allow to configure max data virtqueues

[Jason Wang]

在 2021/12/1 下午6:09, Parav Pandit 写道:
>
>> From: Eli Cohen <elic@nvidia.com>
>> Sent: Wednesday, December 1, 2021 3:33 PM
>>
>> On Tue, Nov 30, 2021 at 05:24:03PM -0800, Si-Wei Liu wrote:
>>>
>>> On 11/30/2021 1:48 AM, Eli Cohen wrote:
>>>> Allow to configure the max virtqueues for a device.
>>>>
>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>> ---
>>>>    drivers/vdpa/vdpa.c  | 16 +++++++++++++++-
>>>>    include/linux/vdpa.h |  1 +
>>>>    2 files changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
>>>> 7332a74a4b00..e185ec2ee851 100644
>>>> --- a/drivers/vdpa/vdpa.c
>>>> +++ b/drivers/vdpa/vdpa.c
>>>> @@ -480,7 +480,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff
>> *msg, struct netlink_callback *cb)
>>>>    }
>>>>    #define VDPA_DEV_NET_ATTRS_MASK ((1 <<
>> VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
>>>> -				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU))
>>>> +				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
>>>> +				 (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
>>> It seems VDPA_ATTR_DEV_MAX_VQS (u32) is what you want (# of data
>>> virtqueues instead of # of data virtqueue pairs)? Not sure what's
>>> possible use of VDPA_ATTR_DEV_NET_CFG_MAX_VQP, was it to
>> dump/display
>>> the config space max_virtqueue_pairs value (u16, 1-32768) for
>>> virtio-net? Why there's such quasi-duplicate attribute introduced in the first
>> place?
>> VDPA_ATTR_DEV_MAX_VQS currently returns vdev->nvqs which equals
>> whatever passed to _vdpa_register_device(). The latter depends on the value
>> provided by (struct vdpa_dev_set_config).max_virtqueues.
>>
> Max VQs configuration should reuse VDPA_ATTR_DEV_MAX_VQS.
> it indicates what is the max vqs a given vdpa device is using. Until now it was driver's choice, now its users choice if provided.
> So no need for additional attribute.


I think a device specific attribute might be better:

1) max_virt_queue_pairs is part of config space so it should work as mtu 
and mac

2) it's more convenient for the user to specifc qps instead of doing 
2*N+1 calculation

Thanks


>
>> Maybe we should add attributes to add aditional virtqueues like control
>> virtqueue and their index. They could be returned by
>> vdpa_dev_net_config_fill().
> Yes.
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH 1/2] vdpa: Allow to configure max data virtqueues

[Parav Pandit via Virtualization]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Thursday, December 2, 2021 9:11 AM
> 
> 在 2021/12/1 下午6:09, Parav Pandit 写道:
> >
> >> From: Eli Cohen <elic@nvidia.com>
> >> Sent: Wednesday, December 1, 2021 3:33 PM
> >>
> >> On Tue, Nov 30, 2021 at 05:24:03PM -0800, Si-Wei Liu wrote:
> >>>
> >>> On 11/30/2021 1:48 AM, Eli Cohen wrote:
> >>>> Allow to configure the max virtqueues for a device.
> >>>>
> >>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
> >>>> ---
> >>>>    drivers/vdpa/vdpa.c  | 16 +++++++++++++++-
> >>>>    include/linux/vdpa.h |  1 +
> >>>>    2 files changed, 16 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> >>>> 7332a74a4b00..e185ec2ee851 100644
> >>>> --- a/drivers/vdpa/vdpa.c
> >>>> +++ b/drivers/vdpa/vdpa.c
> >>>> @@ -480,7 +480,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct
> sk_buff
> >> *msg, struct netlink_callback *cb)
> >>>>    }
> >>>>    #define VDPA_DEV_NET_ATTRS_MASK ((1 <<
> >> VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
> >>>> -				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU))
> >>>> +				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
> >>>> +				 (1 <<
> VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
> >>> It seems VDPA_ATTR_DEV_MAX_VQS (u32) is what you want (# of data
> >>> virtqueues instead of # of data virtqueue pairs)? Not sure what's
> >>> possible use of VDPA_ATTR_DEV_NET_CFG_MAX_VQP, was it to
> >> dump/display
> >>> the config space max_virtqueue_pairs value (u16, 1-32768) for
> >>> virtio-net? Why there's such quasi-duplicate attribute introduced in
> >>> the first
> >> place?
> >> VDPA_ATTR_DEV_MAX_VQS currently returns vdev->nvqs which equals
> >> whatever passed to _vdpa_register_device(). The latter depends on the
> >> value provided by (struct vdpa_dev_set_config).max_virtqueues.
> >>
> > Max VQs configuration should reuse VDPA_ATTR_DEV_MAX_VQS.
> > it indicates what is the max vqs a given vdpa device is using. Until now it was
> driver's choice, now its users choice if provided.
> > So no need for additional attribute.
> 
> 
> I think a device specific attribute might be better:
> 
> 1) max_virt_queue_pairs is part of config space so it should work as mtu and
> mac
> 
> 2) it's more convenient for the user to specifc qps instead of doing
> 2*N+1 calculation
> 
Yes. Eli's new series already took care of this by reusing existing attribute.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization