Message-ID: <e546e6c0-37bd-ee3e-76e5-def63a33f432@oracle.com>
Date:   Wed, 27 Jul 2022 18:41:28 -0700
From:   Si-Wei Liu <si-wei.liu@...cle.com>
To:     "Zhu, Lingshan" <lingshan.zhu@...el.com>,
        "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Parav Pandit <parav@...dia.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "xieyongji@...edance.com" <xieyongji@...edance.com>,
        "gautam.dawar@....com" <gautam.dawar@....com>
Subject: Re: [PATCH V3 5/6] vDPA: answer num of queue pairs = 1 to userspace
 when VIRTIO_NET_F_MQ == 0



On 7/27/2022 4:54 AM, Zhu, Lingshan wrote:
>
>
> On 7/27/2022 6:09 PM, Si-Wei Liu wrote:
>>
>>
>> On 7/27/2022 2:01 AM, Michael S. Tsirkin wrote:
>>> On Wed, Jul 27, 2022 at 12:50:33AM -0700, Si-Wei Liu wrote:
>>>>
>>>> On 7/26/2022 11:01 PM, Michael S. Tsirkin wrote:
>>>>> On Wed, Jul 27, 2022 at 03:47:35AM +0000, Parav Pandit wrote:
>>>>>>> From: Zhu, Lingshan <lingshan.zhu@...el.com>
>>>>>>> Sent: Tuesday, July 26, 2022 10:53 PM
>>>>>>>
>>>>>>> On 7/27/2022 10:17 AM, Parav Pandit wrote:
>>>>>>>>> From: Zhu, Lingshan <lingshan.zhu@...el.com>
>>>>>>>>> Sent: Tuesday, July 26, 2022 10:15 PM
>>>>>>>>>
>>>>>>>>> On 7/26/2022 11:56 PM, Parav Pandit wrote:
>>>>>>>>>>> From: Zhu, Lingshan <lingshan.zhu@...el.com>
>>>>>>>>>>> Sent: Tuesday, July 12, 2022 11:46 PM
>>>>>>>>>>>> When the user space which invokes netlink commands, detects 
>>>>>>>>>>>> that
>>>>>>>>> _MQ
>>>>>>>>>>> is not supported, hence it takes max_queue_pair = 1 by itself.
>>>>>>>>>>> I think the kernel module have all necessary information and 
>>>>>>>>>>> it is
>>>>>>>>>>> the only one which have precise information of a device, so it
>>>>>>>>>>> should answer precisely than let the user space guess. The 
>>>>>>>>>>> kernel
>>>>>>>>>>> module should be reliable than stay silent, leave the 
>>>>>>>>>>> question to
>>>>>>>>>>> the user space
>>>>>>>>> tool.
>>>>>>>>>> Kernel is reliable. It doesn’t expose a config space field if 
>>>>>>>>>> the
>>>>>>>>>> field doesn’t
>>>>>>>>> exist regardless of field should have default or no default.
>>>>>>>>> so when you know it is one queue pair, you should answer one, 
>>>>>>>>> not try
>>>>>>>>> to guess.
>>>>>>>>>> User space should not guess either. User space gets to see if 
>>>>>>>>>> _MQ
>>>>>>>>> present/not present. If _MQ present than get reliable data 
>>>>>>>>> from kernel.
>>>>>>>>>> If _MQ not present, it means this device has one VQ pair.
>>>>>>>>> it is still a guess, right? And all user space tools 
>>>>>>>>> implemented this
>>>>>>>>> feature need to guess
>>>>>>>> No. it is not a guess.
>>>>>>>> It is explicitly checking the _MQ feature and deriving the value.
>>>>>>>> The code you proposed will be present in the user space.
>>>>>>>> It will be uniform for _MQ and 10 other features that are 
>>>>>>>> present now and
>>>>>>> in the future.
>>>>>>> MQ and other features like RSS are different. If there is no 
>>>>>>> _RSS_XX, there
>>>>>>> are no attributes like max_rss_key_size, and there is not a 
>>>>>>> default value.
>>>>>>> But for MQ, we know it has to be 1 wihtout _MQ.
>>>>>> "we" = user space.
>>>>>> To keep the consistency among all the config space fields.
>>>>> Actually I looked and the code some more and I'm puzzled:
>>>>>
>>>>>
>>>>>     struct virtio_net_config config = {};
>>>>>     u64 features;
>>>>>     u16 val_u16;
>>>>>
>>>>>     vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config));
>>>>>
>>>>>     if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, 
>>>>> sizeof(config.mac),
>>>>>             config.mac))
>>>>>         return -EMSGSIZE;
>>>>>
>>>>>
>>>>> Mac returned even without VIRTIO_NET_F_MAC
>>>>>
>>>>>
>>>>>     val_u16 = le16_to_cpu(config.status);
>>>>>     if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
>>>>>         return -EMSGSIZE;
>>>>>
>>>>>
>>>>> status returned even without VIRTIO_NET_F_STATUS
>>>>>
>>>>>     val_u16 = le16_to_cpu(config.mtu);
>>>>>     if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>>>>>         return -EMSGSIZE;
>>>>>
>>>>>
>>>>> MTU returned even without VIRTIO_NET_F_MTU
>>>>>
>>>>>
>>>>> What's going on here?
>>>>>
>>>>>
>>>> I guess this is spec thing (historical debt), I vaguely recall 
>>>> these fields
>>>> are always present in config space regardless the existence of 
>>>> corresponding
>>>> feature bit.
>>>>
>>>> -Siwei
>>> Nope:
>>>
>>> 2.5.1  Driver Requirements: Device Configuration Space
>>>
>>> ...
>>>
>>> For optional configuration space fields, the driver MUST check that 
>>> the corresponding feature is offered
>>> before accessing that part of the configuration space.
>> Well, this is driver side of requirement. As this interface is for 
>> host admin tool to query or configure vdpa device, we don't have to 
>> wait until feature negotiation is done on guest driver to extract 
>> vdpa attributes/parameters, say if we want to replicate another vdpa 
>> device with the same config on migration destination. I think what 
>> may need to be fix is to move off from using 
>> .vdpa_get_config_unlocked() which depends on feature negotiation. 
>> And/or expose config space register values through another set of 
>> attributes.
> Yes, we don't have to wait for FEATURES_OK. In another patch in this 
> series, I have added a new netlink attr to report the device features, 
> and removed the blocker. So the LM orchestration SW can query the 
> device features of the devices at the destination cluster, and pick a 
> proper one, even mask out some features to meet the LM requirements.
For that end, you'd need to move off from using 
vdpa_get_config_unlocked() which depends on feature negotiation. Since 
this would slightly change the original semantics of each field that 
"vdpa dev config" shows, it probably need another netlink command and 
new uAPI.

-Siwei


>
> Thanks,
> Zhu Lingshan
>> -Siwei
>>
>>
>>
>>
>

Powered by blists - more mailing lists