Si-Wei Liu
2022-Jul-28 01:41 UTC
[PATCH V3 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0
On 7/27/2022 4:54 AM, Zhu, Lingshan wrote:> > > On 7/27/2022 6:09 PM, Si-Wei Liu wrote: >> >> >> On 7/27/2022 2:01 AM, Michael S. Tsirkin wrote: >>> On Wed, Jul 27, 2022 at 12:50:33AM -0700, Si-Wei Liu wrote: >>>> >>>> On 7/26/2022 11:01 PM, Michael S. Tsirkin wrote: >>>>> On Wed, Jul 27, 2022 at 03:47:35AM +0000, Parav Pandit wrote: >>>>>>> From: Zhu, Lingshan <lingshan.zhu at intel.com> >>>>>>> Sent: Tuesday, July 26, 2022 10:53 PM >>>>>>> >>>>>>> On 7/27/2022 10:17 AM, Parav Pandit wrote: >>>>>>>>> From: Zhu, Lingshan <lingshan.zhu at intel.com> >>>>>>>>> Sent: Tuesday, July 26, 2022 10:15 PM >>>>>>>>> >>>>>>>>> On 7/26/2022 11:56 PM, Parav Pandit wrote: >>>>>>>>>>> From: Zhu, Lingshan <lingshan.zhu at intel.com> >>>>>>>>>>> Sent: Tuesday, July 12, 2022 11:46 PM >>>>>>>>>>>> When the user space which invokes netlink commands, detects >>>>>>>>>>>> that >>>>>>>>> _MQ >>>>>>>>>>> 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. >>>>>>>>>> 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. >>>>>>>>>> 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. >>>>>>>>>> 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: >>>>> >>>>> >>>>> ????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 >>>>> >>>>> >>>>> ????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 >>>>> >>>>> ????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 >>>>> >>>>> >>>>> What's going on here? >>>>> >>>>> >>>> I guess this is spec thing (historical debt), I vaguely recall >>>> these fields >>>> are always present in config space regardless the existence of >>>> corresponding >>>> feature bit. >>>> >>>> -Siwei >>> Nope: >>> >>> 2.5.1? Driver Requirements: Device Configuration Space >>> >>> ... >>> >>> For optional configuration space fields, the driver MUST check that >>> the corresponding feature is offered >>> before accessing that part of the configuration space. >> Well, this is driver side of requirement. As this interface is for >> host admin tool to query or configure vdpa device, we don't have to >> wait until feature negotiation is done on guest driver to extract >> vdpa attributes/parameters, say if we want to replicate another vdpa >> device with the same config on migration destination. I think what >> may need to be fix is to move off from using >> .vdpa_get_config_unlocked() which depends on feature negotiation. >> And/or expose config space register values through another set of >> attributes. > Yes, we don't have to wait for FEATURES_OK. In another patch in this > series, I have added a new netlink attr to report the device features, > and removed the blocker. So the LM orchestration SW can query the > device features of the devices at the destination cluster, and pick a > proper one, even mask out some features to meet the LM requirements.For that end, you'd need to move off from using vdpa_get_config_unlocked() which depends on feature negotiation. Since this would slightly change the original semantics of each field that "vdpa dev config" shows, it probably need another netlink command and new uAPI. -Siwei> > Thanks, > Zhu Lingshan >> -Siwei >> >> >> >> >