On 7/27/2022 4:15 PM, Si-Wei Liu wrote:

On 7/5/2022 4:56 AM, Parav Pandit via Virtualization wrote:

quoted

quoted

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.

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.

quoted

quoted

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

quoted

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