Re: [PATCH V3 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

On 7/9/2022 12:23 AM, Parav Pandit wrote:
>> From: Zhu, Lingshan <lingshan.zhu@intel.com>
>> Sent: Friday, July 8, 2022 2:21 AM
>>
>>
>> On 7/2/2022 6:07 AM, Parav Pandit wrote:
>>>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> Sent: Friday, July 1, 2022 9:28 AM
>>>> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue
>>>> pair, so when userspace querying queue pair numbers, it should return
>>>> mq=1 than zero.
>>>>
>>>> Function vdpa_dev_net_config_fill() fills the attributions of the
>>>> vDPA devices, so that it should call vdpa_dev_net_mq_config_fill() so
>>>> the parameter in vdpa_dev_net_mq_config_fill() should be
>>>> feature_device than feature_driver for the vDPA devices themselves
>>>>
>>>> Before this change, when MQ = 0, iproute2 output:
>>>> $vdpa dev config show vdpa0
>>>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs
>>>> 0 mtu 1500
>>>>
>>> The fix belongs to user space.
>>> When a feature bit _MQ is not negotiated, vdpa kernel space will not add
>> attribute VDPA_ATTR_DEV_NET_CFG_MAX_VQP.
>>> When such attribute is not returned by kernel, max_vq_pairs should not
>> be shown by the iproute2.
>> I think userspace tool does not need to care whether MQ is offered or
>> negotiated, it just needs to read the number of queues there, so if no MQ, it
>> is not "not any queues", there are still 1 queue pair to be a virtio-net device,
>> means two queues.
>>
>> If not, how can you tell the user there are only 2 queues? The end users may
>> don't know this is default. They may misunderstand this as an error or
>> defects.
> When max_vq_pairs is not shown, it means that device didn’t expose MAX_VQ_PAIRS attribute to its guest users.
> (Because _MQ was not negotiated).
> It is not error or defect.
> It precisely shows what is exposed.
>
> User space will care when it wants to turn off/on _MQ feature bits and MAX_QP values.
>
> Showing max_vq_pairs of 1 even when _MQ is not negotiated, incorrectly says that max_vq_pairs is exposed to the guest, but it is not offered.
>
> So, please fix the iproute2 to not print max_vq_pairs when it is not returned by the kernel.
iproute2 can report whether there is MQ feature in the device / driver 
feature bits.
I think iproute2 only queries the number of max queues here.

max_vq_pairs shows how many queue pairs there, this attribute's existence does not depend on MQ,
if no MQ, there are still one queue pair, so just show one.

>
>>> We have many config space fields that depend on the feature bits and
>> some of them do not have any defaults.
>>> To keep consistency of existence of config space fields among all, we don't
>> want to show default like below.
>>> Please fix the iproute2 to not print max_vq_pairs when it is not returned
>> by the kernel.
>>>> After applying this commit, when MQ = 0, iproute2 output:
>>>> $vdpa dev config show vdpa0
>>>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs
>>>> 1 mtu 1500
>>>>
>>>> Fixes: a64917bc2e9b (vdpa: Provide interface to read driver features)
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>    drivers/vdpa/vdpa.c | 7 ++++---
>>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
>>>> d76b22b2f7ae..846dd37f3549 100644
>>>> --- a/drivers/vdpa/vdpa.c
>>>> +++ b/drivers/vdpa/vdpa.c
>>>> @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct
>>>> vdpa_device *vdev,
>>>>    	u16 val_u16;
>>>>
>>>>    	if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0)
>>>> -		return 0;
>>>> +		val_u16 = 1;
>>>> +	else
>>>> +		val_u16 = __virtio16_to_cpu(true, config-
>>>>> max_virtqueue_pairs);
>>>> -	val_u16 = le16_to_cpu(config->max_virtqueue_pairs);
>>>>    	return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
>> val_u16);
>>>> }
>>>>
>>>> @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct
>>>> vdpa_device *vdev, struct sk_buff *ms
>>>>    			      VDPA_ATTR_PAD))
>>>>    		return -EMSGSIZE;
>>>>
>>>> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
>>>> &config);
>>>> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_device,
>>>> +&config);
>>>>    }
>>>>
>>>>    static int
>>>> --
>>>> 2.31.1