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

> From: Zhu Lingshan <lingshan.zhu at 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.

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 at 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

On Fri, Jul 01, 2022 at 10:07:59PM +0000, Parav Pandit
wrote:
> 
> 
> > From: Zhu Lingshan <lingshan.zhu at 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.
> 
> 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.
Parav I read the discussion and don't get your argument. From driver's
POV
_MQ with 1 VQ pair and !_MQ are exactly functionally equivalent.

It's true that iproute probably needs to be fixed too, to handle old
kernels. But iproute is not the only userspace, why not make it's life
easier by fixing the kernel?

-- 
MST