Virtualization - Jun 2022 - [PATCH V2 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: Monday, June 13, 2022 10:33 PM
> 
> 
> On 6/14/2022 4:36 AM, Parav Pandit wrote:
> >
> >> From: Zhu Lingshan <lingshan.zhu at intel.com>
> >> Sent: Monday, June 13, 2022 6:17 AM
> >> To: jasowang at redhat.com; mst at redhat.com
> >> Cc: virtualization at lists.linux-foundation.org;
> >> netdev at vger.kernel.org; Parav Pandit <parav at
nvidia.com>;
> >> xieyongji at bytedance.com; gautam.dawar at amd.com; Zhu Lingshan
> >> <lingshan.zhu at intel.com>
> >> Subject: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to
> >> userspace when VIRTIO_NET_F_MQ == 0
> >>
> >> 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
> >>
> > Max_vq_pairs should not be printed when _MQ feature is not negotiated.
> > Existing code that returns 0 is correct following this line of the
spec.
> >
> > " The following driver-read-only field, max_virtqueue_pairs only
> > exists if VIRTIO_NET_F_MQ or VIRTIO_- NET_F_RSS is set."
> > The field doesn't exist when _MQ is not there. Hence, it should
not be
> printed.
> > Is _RSS offered and is that why you see it?
> >
> > If not a fix in the iproute2/vdpa should be done.
> IMHO, The spec says:
> The following driver-read-only field, max_virtqueue_pairs only exists if
> VIRTIO_NET_F_MQ is *set*
> 
> The spec doesn't say:
> The following driver-read-only field, max_virtqueue_pairs only exists if
> VIRTIO_NET_F_MQ is *negotiated*
> 
> If a device is capable of of multi-queues, this capability does not depend
on
> the driver. We are querying the device, not the driver.
> 
> If there is MQ, we print the onboard implemented total number of the
> queue pairs.
> If MQ is not set, we will not read the onboard mq number, because it is not
> there as the spec says.
> But there should be at least one queue pair to be a functional virtio-net,
so 1
> is printed.
The commit [1] is supposed to show the device configuration layout as what
device _offers_ as mentioned in the subject line of the commit [1] very clearly.

The commit [2] changed the original intent of commit [1] even though commit [2]
mentioned "fixes" in the patch without any fixes tag. :(
commit [2] was bug.

The right fix to restore [1] is to report the device features offered using
get_device_features() callback instead of get_drivers_features().

Once above fix is applied,
when _MQ is not offered, max_queue_pairs should be treated as 1 by the driver.
We do not have a concept of management driver yet in the specification.
So when _MQ is not offered by the device, either kernel driver can return
max_queue_pairs = 1, or the user space caller can see that _MQ is not offered by
the device hence treat max_vq_pairs = 1.
(Like how it is described in the spec as - " Identify and initialize the
receive and transmission virtqueues, up to N of each kind. If VIRTIO_NET_F_MQ
feature bit is negotiated, N=max_virtqueue_pairs, otherwise identify N=1."

So let orchestration layer can certainly derive this N when _MQ feature is not
offered, instead of coming from the vdpa management layer.
I agree that this extra if() condition in the user space can be avoided if
kernel always provides it.
But better to avoid such assumption because we have more such config space
attributes. i.e., they exist only when features are offered.
So to keep it uniform, I prefer we avoid the exception for max_virtqueue_pairs.

Please submit the fix for [2] to call get_device_features() for purpose of
returning config space.
Continue to use get_driver_features() to show VDPA_ATTR_DEV_NEGOTIATED_FEATURES.

I didn?t review these patches and it slipped through the cracks. :(

[1] a64917bc2e9b ("vdpa: Introduce query of device config layout")
[2] a64917bc2e9 ("vdpa: Provide interface to read driver features")