From: Zhu, Lingshan <redacted>
Sent: Tuesday, July 5, 2022 3:59 AM


On 7/4/2022 8:53 PM, Parav Pandit wrote:

quoted

quoted

From: Jason Wang <jasowang@redhat.com>
Sent: Monday, July 4, 2022 12:47 AM


在 2022/7/2 06:02, Parav Pandit 写道:

quoted

quoted

From: Zhu Lingshan <redacted>
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

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

It should have been,

VDPA_ATTR_DEV_MGMTDEV_SUPPORTED_FEATURES.

quoted

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.

quoted

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.

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.