Date:   Wed, 15 Jun 2022 14:34:01 +0800
From:   "Zhu, Lingshan" <lingshan.zhu@...el.com>
To:     Parav Pandit <parav@...dia.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        "mst@...hat.com" <mst@...hat.com>
Cc:     "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "xieyongji@...edance.com" <xieyongji@...edance.com>,
        "gautam.dawar@....com" <gautam.dawar@....com>
Subject: Re: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace
 when VIRTIO_NET_F_MQ == 0



On 6/15/2022 3:26 AM, Parav Pandit wrote:
>
>> From: Zhu, Lingshan <lingshan.zhu@...el.com>
>> Sent: Monday, June 13, 2022 10:33 PM
>>
>>
>> On 6/14/2022 4:36 AM, Parav Pandit wrote:
>>>> From: Zhu Lingshan <lingshan.zhu@...el.com>
>>>> Sent: Monday, June 13, 2022 6:17 AM
>>>> To: jasowang@...hat.com; mst@...hat.com
>>>> Cc: virtualization@...ts.linux-foundation.org;
>>>> netdev@...r.kernel.org; Parav Pandit <parav@...dia.com>;
>>>> xieyongji@...edance.com; gautam.dawar@....com; Zhu Lingshan
>>>> <lingshan.zhu@...el.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().
already done in 3/6 patch in this series, see:
  static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct 
sk_buff *msg)
  {
         struct virtio_net_config config = {};
-       u64 features;
+       u64 features_device, features_driver;
         u16 val_u16;

and
-       features = vdev->config->get_driver_features(vdev);
-       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, 
features,
+       features_driver = vdev->config->get_driver_features(vdev);
+       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, 
features_driver,
+                             VDPA_ATTR_PAD))
+               return -EMSGSIZE;
+
+       features_device = vdev->config->get_device_features(vdev);
+       if (nla_put_u64_64bit(msg, 
VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, features_device,
                               VDPA_ATTR_PAD))
                 return -EMSGSIZE;

Features of the device and the driver are reported separately.

>
> Once above fix is applied,
> when _MQ is not offered, max_queue_pairs should be treated as 1 by the driver.
Yes, it is done by this patch.
> 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."
Yes, that's why I set max_queue_pairs = 1 if MQ = 0 in this patch.
>
> 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.
This is already there in 3/6 patch, we have these code to report driver 
features:
         features_driver = vdev->config->get_driver_features(vdev);
         if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, 
features_driver,
                               VDPA_ATTR_PAD))
                 return -EMSGSIZE;

And these code to report device features:
         features_device = vdev->config->get_device_features(vdev);
         if (nla_put_u64_64bit(msg, 
VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, features_device,
                               VDPA_ATTR_PAD))
                 return -EMSGSIZE;



>
> 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")

Powered by blists - more mailing lists