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

Si-Wei Liu <si-wei.liu@xxxxxxxxxx> · Wed, 27 Jul 2022 01:15:58 -0700

On 7/5/2022 4:56 AM, Parav Pandit via Virtualization wrote:
From: Zhu, Lingshan <lingshan.zhu@xxxxxxxxx>
Sent: Tuesday, July 5, 2022 3:59 AM

On 7/4/2022 8:53 PM, Parav Pandit wrote:
From: Jason Wang <jasowang@xxxxxxxxxx>
Sent: Monday, July 4, 2022 12:47 AM

在 2022/7/2 06:02, Parav Pandit 写道:
From: Zhu Lingshan <lingshan.zhu@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization