Date:   Wed, 27 Jul 2022 01:15:58 -0700
From:   Si-Wei Liu <si-wei.liu@...cle.com>
To:     Parav Pandit <parav@...dia.com>,
        "Zhu, Lingshan" <lingshan.zhu@...el.com>,
        Jason Wang <jasowang@...hat.com>,
        "mst@...hat.com" <mst@...hat.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "xieyongji@...edance.com" <xieyongji@...edance.com>,
        "gautam.dawar@....com" <gautam.dawar@....com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>
Subject: Re: [PATCH V3 3/6] vDPA: allow userspace to query features of a vDPA
 device



On 7/5/2022 4:56 AM, Parav Pandit via Virtualization wrote:
>> From: Zhu, Lingshan <lingshan.zhu@...el.com>
>> Sent: Tuesday, July 5, 2022 3:59 AM
>>
>>
>> On 7/4/2022 8:53 PM, Parav Pandit wrote:
>>>> From: Jason Wang <jasowang@...hat.com>
>>>> Sent: Monday, July 4, 2022 12:47 AM
>>>>
>>>>
>>>> 在 2022/7/2 06:02, Parav Pandit 写道:
>>>>>> From: Zhu Lingshan <lingshan.zhu@...el.com>
>>>>>> Sent: Friday, July 1, 2022 9:28 AM
>>>>>>
>>>>>> This commit adds a new vDPA netlink attribution
>>>>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
>>>> features
>>>>>> of vDPA devices through this new attr.
>>>>>>
>>>>>> Fixes: a64917bc2e9b vdpa: (Provide interface to read driver
>>>>>> feature)
>>>>> Missing the "" in the line.
>>>>> I reviewed the patches again.
>>>>>
>>>>> However, this is not the fix.
>>>>> A fix cannot add a new UAPI.
>>>>>
>>>>> Code is already considering negotiated driver features to return the
>>>>> device
>>>> config space.
>>>>> Hence it is fine.
>>>>>
>>>>> This patch intents to provide device features to user space.
>>>>> First what vdpa device are capable of, are already returned by
>>>>> features
>>>> attribute on the management device.
>>>>> This is done in commit [1].
>>>>>
>>>>> The only reason to have it is, when one management device indicates
>>>>> that
>>>> feature is supported, but device may end up not supporting this
>>>> feature if such feature is shared with other devices on same physical device.
>>>>> For example all VFs may not be symmetric after large number of them
>>>>> are
>>>> in use. In such case features bit of management device can differ
>>>> (more
>>>> features) than the vdpa device of this VF.
>>>>> Hence, showing on the device is useful.
>>>>>
>>>>> As mentioned before in V2, commit [1] has wrongly named the
>>>>> attribute to
>>>> VDPA_ATTR_DEV_SUPPORTED_FEATURES.
>>>>> It should have been,
>>>> VDPA_ATTR_DEV_MGMTDEV_SUPPORTED_FEATURES.
>>>>> Because it is in UAPI, and since we don't want to break compilation
>>>>> of iproute2, It cannot be renamed anymore.
>>>>>
>>>>> Given that, we do not want to start trend of naming device
>>>>> attributes with
>>>> additional _VDPA_ to it as done in this patch.
>>>>> Error in commit [1] was exception.
>>>>>
>>>>> Hence, please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to return
>>>> for device features too.
>>>>
>>>>
>>>> This will probably break or confuse the existing userspace?
>>>>
>>> It shouldn't break, because its new attribute on the device.
>>> All attributes are per command, so old one will not be confused either.
>> A netlink attr should has its own and unique purpose, that's why we don't need
>> locks for the attrs, only one consumer and only one producer.
>> I am afraid re-using (for both management device and the vDPA device) the attr
>> VDPA_ATTR_DEV_SUPPORTED_FEATURES would lead to new race condition.
>> E.g., There are possibilities of querying FEATURES of a management device and
>> a vDPA device simultaneously, or can there be a syncing issue in a tick?
> Both can be queried simultaneously. Each will return their own feature bits using same attribute.
> It wont lead to the race.
Agreed. Multiple userspace callers would do recv() calls on different 
netlink sockets. Looks to me shouldn't involve any race.
>
>> IMHO, I don't see any advantages of re-using this attr.
> We don’t want to continue this mess of VDPA_DEV prefix for new attributes due to previous wrong naming.
Well, you can say it's a mess but since the attr name can be reused for 
different command,  I didn't care that much while reviewing this. 
Actually, it was initially named this way to show the device features in 
"vdpa dev config ..." output, but later on it had been moved to mgmtdev 
to show parent's capability.

-Siwei
> _______________________________________________
> Virtualization mailing list
> Virtualization@...ts.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Powered by blists - more mailing lists