Re: [PATCH V3 3/6] vDPA: allow userspace to query features of a vDPA device

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

On 7/27/2022 4:15 PM, Si-Wei Liu wrote:
>
>
> On 7/5/2022 4:56 AM, Parav Pandit via Virtualization wrote:
>>> From: Zhu, Lingshan <lingshan.zhu@intel.com>
>>> Sent: Tuesday, July 5, 2022 3:59 AM
>>>
>>>
>>> On 7/4/2022 8:53 PM, Parav Pandit wrote:
>>>>> From: Jason Wang <jasowang@redhat.com>
>>>>> Sent: Monday, July 4, 2022 12:47 AM
>>>>>
>>>>>
>>>>> 在 2022/7/2 06:02, Parav Pandit 写道:
>>>>>>> From: Zhu Lingshan <lingshan.zhu@intel.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.
oh yes, thanks for pointing this out, they are on different sockets 
belonging to different userspace programs.
>>
>>> 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.
yes there is a buggy commit, but we can not change it now, because we 
are not expected to break current uapi, so I think it is better to add a 
new attr, no benefits to reuse another attr.
>
> -Siwei
>> _______________________________________________
>> Virtualization mailing list
>> Virtualization@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>