On Wed, Jul 27, 2022 at 2:26 PM Zhu, Lingshan [off-list ref] wrote:

On 7/27/2022 2:01 PM, Michael S. Tsirkin wrote:

quoted

On Wed, Jul 27, 2022 at 03:47:35AM +0000, Parav Pandit wrote:

quoted

quoted

From: Zhu, Lingshan <redacted>
Sent: Tuesday, July 26, 2022 10:53 PM

On 7/27/2022 10:17 AM, Parav Pandit wrote:

quoted

quoted

From: Zhu, Lingshan <redacted>
Sent: Tuesday, July 26, 2022 10:15 PM

On 7/26/2022 11:56 PM, Parav Pandit wrote:

quoted

quoted

From: Zhu, Lingshan <redacted>
Sent: Tuesday, July 12, 2022 11:46 PM

quoted

When the user space which invokes netlink commands, detects that

_MQ

quoted

quoted

is not supported, hence it takes max_queue_pair = 1 by itself.
I think the kernel module have all necessary information and it is
the only one which have precise information of a device, so it
should answer precisely than let the user space guess. The kernel
module should be reliable than stay silent, leave the question to
the user space

tool.

quoted

Kernel is reliable. It doesn’t expose a config space field if the
field doesn’t

exist regardless of field should have default or no default.
so when you know it is one queue pair, you should answer one, not try
to guess.

quoted

User space should not guess either. User space gets to see if _MQ

present/not present. If _MQ present than get reliable data from kernel.

quoted

If _MQ not present, it means this device has one VQ pair.

it is still a guess, right? And all user space tools implemented this
feature need to guess

No. it is not a guess.
It is explicitly checking the _MQ feature and deriving the value.
The code you proposed will be present in the user space.
It will be uniform for _MQ and 10 other features that are present now and

in the future.
MQ and other features like RSS are different. If there is no _RSS_XX, there
are no attributes like max_rss_key_size, and there is not a default value.
But for MQ, we know it has to be 1 wihtout _MQ.

"we" = user space.
To keep the consistency among all the config space fields.

Actually I looked and the code some more and I'm puzzled:

I can submit a fix in my next version patch for these issue.

quoted

      struct virtio_net_config config = {};
      u64 features;
      u16 val_u16;

      vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config));

      if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac),
                  config.mac))
              return -EMSGSIZE;


Mac returned even without VIRTIO_NET_F_MAC

if no VIRTIO_NET_F_MAC, we should not nla_put
VDPA_ATTR_DEV_NET_CFG_MAC_ADDR, the spec says the driver should generate
a random mac.

It's probably too late to do this. Most of the parents have this
feature support, so probably not a real issue.

quoted

      val_u16 = le16_to_cpu(config.status);
      if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
              return -EMSGSIZE;


status returned even without VIRTIO_NET_F_STATUS

if no VIRTIO_NET_F_STATUS, we should not nla_put
VDPA_ATTR_DEV_NET_STATUS, the spec says the driver should assume the
link is active.

Somehow similar to F_MAC. But we can report if F_MAC is not negotiated.

quoted

      val_u16 = le16_to_cpu(config.mtu);
      if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
              return -EMSGSIZE;


MTU returned even without VIRTIO_NET_F_MTU

same as above, the spec says config.mtu depends on VIRTIO_NET_F_MTU, so
without this feature bit, we should not return MTU to the userspace.

Not a big issue, we just need to make sure the parent can report a
correct MTU here.

Thanks

Does these fix look good to you?

And I think we may need your adjudication for the two issues:
(1) Shall we answer max_vq_paris = 1 when _MQ not exist, I know you have
agreed on this in a previous thread, its nice to clarify
(2) I think we should not re-use the netlink attr to report feature bits
of both the management device and the vDPA device,
this can lead to a new race condition, there are no locks(especially
distributed locks for kernel_space and user_space) in the nla_put
functions. Re-using the attr is some kind of breaking the netlink
lockless design.

Thanks,
Zhu Lingshan

quoted

What's going on here?