| Message ID | 20220613101652.195216-6-lingshan.zhu@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ifcvf/vDPA: support query device config space through netlink | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
> From: Zhu Lingshan <lingshan.zhu@intel.com> > Sent: Monday, June 13, 2022 6:17 AM > To: jasowang@redhat.com; mst@redhat.com > Cc: virtualization@lists.linux-foundation.org; netdev@vger.kernel.org; Parav > Pandit <parav@nvidia.com>; xieyongji@bytedance.com; > gautam.dawar@amd.com; Zhu Lingshan <lingshan.zhu@intel.com> > Subject: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace > when VIRTIO_NET_F_MQ == 0 > > If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so > when userspace querying queue pair numbers, it should return mq=1 than > zero. > > Function vdpa_dev_net_config_fill() fills the attributions of the vDPA > devices, so that it should call vdpa_dev_net_mq_config_fill() so the > parameter in vdpa_dev_net_mq_config_fill() should be feature_device than > feature_driver for the vDPA devices themselves > > Before this change, when MQ = 0, iproute2 output: > $vdpa dev config show vdpa0 > vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0 > mtu 1500 > Max_vq_pairs should not be printed when _MQ feature is not negotiated. Existing code that returns 0 is correct following this line of the spec. " The following driver-read-only field, max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ or VIRTIO_- NET_F_RSS is set." The field doesn't exist when _MQ is not there. Hence, it should not be printed. Is _RSS offered and is that why you see it? If not a fix in the iproute2/vdpa should be done. > After applying this commit, when MQ = 0, iproute2 output: > $vdpa dev config show vdpa0 > vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1 > mtu 1500 > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > --- > drivers/vdpa/vdpa.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index > d76b22b2f7ae..846dd37f3549 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct > vdpa_device *vdev, > u16 val_u16; > > if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0) > - return 0; > + val_u16 = 1; > + else > + val_u16 = __virtio16_to_cpu(true, config- > >max_virtqueue_pairs); > > - val_u16 = le16_to_cpu(config->max_virtqueue_pairs); > return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, > val_u16); } > > @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct > vdpa_device *vdev, struct sk_buff *ms > VDPA_ATTR_PAD)) > return -EMSGSIZE; > > - return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, > &config); > + return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, > +&config); > } > > static int > -- > 2.31.1
On 6/14/2022 4:36 AM, Parav Pandit wrote: > >> From: Zhu Lingshan <lingshan.zhu@intel.com> >> Sent: Monday, June 13, 2022 6:17 AM >> To: jasowang@redhat.com; mst@redhat.com >> Cc: virtualization@lists.linux-foundation.org; netdev@vger.kernel.org; Parav >> Pandit <parav@nvidia.com>; xieyongji@bytedance.com; >> gautam.dawar@amd.com; Zhu Lingshan <lingshan.zhu@intel.com> >> Subject: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace >> when VIRTIO_NET_F_MQ == 0 >> >> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so >> when userspace querying queue pair numbers, it should return mq=1 than >> zero. >> >> Function vdpa_dev_net_config_fill() fills the attributions of the vDPA >> devices, so that it should call vdpa_dev_net_mq_config_fill() so the >> parameter in vdpa_dev_net_mq_config_fill() should be feature_device than >> feature_driver for the vDPA devices themselves >> >> Before this change, when MQ = 0, iproute2 output: >> $vdpa dev config show vdpa0 >> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0 >> mtu 1500 >> > Max_vq_pairs should not be printed when _MQ feature is not negotiated. > Existing code that returns 0 is correct following this line of the spec. > > " The following driver-read-only field, max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ or VIRTIO_- > NET_F_RSS is set." > The field doesn't exist when _MQ is not there. Hence, it should not be printed. > Is _RSS offered and is that why you see it? > > If not a fix in the iproute2/vdpa should be done. IMHO, The spec says: The following driver-read-only field, max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ is *set* The spec doesn't say: The following driver-read-only field, max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ is *negotiated* If a device is capable of of multi-queues, this capability does not depend on the driver. We are querying the device, not the driver. If there is MQ, we print the onboard implemented total number of the queue pairs. If MQ is not set, we will not read the onboard mq number, because it is not there as the spec says. But there should be at least one queue pair to be a functional virtio-net, so 1 is printed. Thanks, Zhu Lingshan > > >> After applying this commit, when MQ = 0, iproute2 output: >> $vdpa dev config show vdpa0 >> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1 >> mtu 1500 >> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >> --- >> drivers/vdpa/vdpa.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index >> d76b22b2f7ae..846dd37f3549 100644 >> --- a/drivers/vdpa/vdpa.c >> +++ b/drivers/vdpa/vdpa.c >> @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct >> vdpa_device *vdev, >> u16 val_u16; >> >> if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0) >> - return 0; >> + val_u16 = 1; >> + else >> + val_u16 = __virtio16_to_cpu(true, config- >>> max_virtqueue_pairs); >> - val_u16 = le16_to_cpu(config->max_virtqueue_pairs); >> return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, >> val_u16); } >> >> @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct >> vdpa_device *vdev, struct sk_buff *ms >> VDPA_ATTR_PAD)) >> return -EMSGSIZE; >> >> - return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, >> &config); >> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, >> +&config); >> } >> >> static int >> -- >> 2.31.1
> From: Zhu, Lingshan <lingshan.zhu@intel.com> > Sent: Monday, June 13, 2022 10:33 PM > > > On 6/14/2022 4:36 AM, Parav Pandit wrote: > > > >> From: Zhu Lingshan <lingshan.zhu@intel.com> > >> Sent: Monday, June 13, 2022 6:17 AM > >> To: jasowang@redhat.com; mst@redhat.com > >> Cc: virtualization@lists.linux-foundation.org; > >> netdev@vger.kernel.org; Parav Pandit <parav@nvidia.com>; > >> xieyongji@bytedance.com; gautam.dawar@amd.com; Zhu Lingshan > >> <lingshan.zhu@intel.com> > >> Subject: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to > >> userspace when VIRTIO_NET_F_MQ == 0 > >> > >> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue > >> pair, so when userspace querying queue pair numbers, it should return > >> mq=1 than zero. > >> > >> Function vdpa_dev_net_config_fill() fills the attributions of the > >> vDPA devices, so that it should call vdpa_dev_net_mq_config_fill() so > >> the parameter in vdpa_dev_net_mq_config_fill() should be > >> feature_device than feature_driver for the vDPA devices themselves > >> > >> Before this change, when MQ = 0, iproute2 output: > >> $vdpa dev config show vdpa0 > >> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs > >> 0 mtu 1500 > >> > > Max_vq_pairs should not be printed when _MQ feature is not negotiated. > > Existing code that returns 0 is correct following this line of the spec. > > > > " The following driver-read-only field, max_virtqueue_pairs only > > exists if VIRTIO_NET_F_MQ or VIRTIO_- NET_F_RSS is set." > > The field doesn't exist when _MQ is not there. Hence, it should not be > printed. > > Is _RSS offered and is that why you see it? > > > > If not a fix in the iproute2/vdpa should be done. > IMHO, The spec says: > The following driver-read-only field, max_virtqueue_pairs only exists if > VIRTIO_NET_F_MQ is *set* > > The spec doesn't say: > The following driver-read-only field, max_virtqueue_pairs only exists if > VIRTIO_NET_F_MQ is *negotiated* > > If a device is capable of of multi-queues, this capability does not depend on > the driver. We are querying the device, not the driver. > > If there is MQ, we print the onboard implemented total number of the > queue pairs. > If MQ is not set, we will not read the onboard mq number, because it is not > there as the spec says. > But there should be at least one queue pair to be a functional virtio-net, so 1 > is printed. The commit [1] is supposed to show the device configuration layout as what device _offers_ as mentioned in the subject line of the commit [1] very clearly. The commit [2] changed the original intent of commit [1] even though commit [2] mentioned "fixes" in the patch without any fixes tag. :( commit [2] was bug. The right fix to restore [1] is to report the device features offered using get_device_features() callback instead of get_drivers_features(). Once above fix is applied, when _MQ is not offered, max_queue_pairs should be treated as 1 by the driver. We do not have a concept of management driver yet in the specification. So when _MQ is not offered by the device, either kernel driver can return max_queue_pairs = 1, or the user space caller can see that _MQ is not offered by the device hence treat max_vq_pairs = 1. (Like how it is described in the spec as - " Identify and initialize the receive and transmission virtqueues, up to N of each kind. If VIRTIO_NET_F_MQ feature bit is negotiated, N=max_virtqueue_pairs, otherwise identify N=1." So let orchestration layer can certainly derive this N when _MQ feature is not offered, instead of coming from the vdpa management layer. I agree that this extra if() condition in the user space can be avoided if kernel always provides it. But better to avoid such assumption because we have more such config space attributes. i.e., they exist only when features are offered. So to keep it uniform, I prefer we avoid the exception for max_virtqueue_pairs. Please submit the fix for [2] to call get_device_features() for purpose of returning config space. Continue to use get_driver_features() to show VDPA_ATTR_DEV_NEGOTIATED_FEATURES. I didn’t review these patches and it slipped through the cracks. :( [1] a64917bc2e9b ("vdpa: Introduce query of device config layout") [2] a64917bc2e9 ("vdpa: Provide interface to read driver features")
On 6/15/2022 3:26 AM, Parav Pandit wrote: > >> From: Zhu, Lingshan <lingshan.zhu@intel.com> >> Sent: Monday, June 13, 2022 10:33 PM >> >> >> On 6/14/2022 4:36 AM, Parav Pandit wrote: >>>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>>> Sent: Monday, June 13, 2022 6:17 AM >>>> To: jasowang@redhat.com; mst@redhat.com >>>> Cc: virtualization@lists.linux-foundation.org; >>>> netdev@vger.kernel.org; Parav Pandit <parav@nvidia.com>; >>>> xieyongji@bytedance.com; gautam.dawar@amd.com; Zhu Lingshan >>>> <lingshan.zhu@intel.com> >>>> Subject: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to >>>> userspace when VIRTIO_NET_F_MQ == 0 >>>> >>>> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue >>>> pair, so when userspace querying queue pair numbers, it should return >>>> mq=1 than zero. >>>> >>>> Function vdpa_dev_net_config_fill() fills the attributions of the >>>> vDPA devices, so that it should call vdpa_dev_net_mq_config_fill() so >>>> the parameter in vdpa_dev_net_mq_config_fill() should be >>>> feature_device than feature_driver for the vDPA devices themselves >>>> >>>> Before this change, when MQ = 0, iproute2 output: >>>> $vdpa dev config show vdpa0 >>>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs >>>> 0 mtu 1500 >>>> >>> Max_vq_pairs should not be printed when _MQ feature is not negotiated. >>> Existing code that returns 0 is correct following this line of the spec. >>> >>> " The following driver-read-only field, max_virtqueue_pairs only >>> exists if VIRTIO_NET_F_MQ or VIRTIO_- NET_F_RSS is set." >>> The field doesn't exist when _MQ is not there. Hence, it should not be >> printed. >>> Is _RSS offered and is that why you see it? >>> >>> If not a fix in the iproute2/vdpa should be done. >> IMHO, The spec says: >> The following driver-read-only field, max_virtqueue_pairs only exists if >> VIRTIO_NET_F_MQ is *set* >> >> The spec doesn't say: >> The following driver-read-only field, max_virtqueue_pairs only exists if >> VIRTIO_NET_F_MQ is *negotiated* >> >> If a device is capable of of multi-queues, this capability does not depend on >> the driver. We are querying the device, not the driver. >> >> If there is MQ, we print the onboard implemented total number of the >> queue pairs. >> If MQ is not set, we will not read the onboard mq number, because it is not >> there as the spec says. >> But there should be at least one queue pair to be a functional virtio-net, so 1 >> is printed. > The commit [1] is supposed to show the device configuration layout as what device _offers_ as mentioned in the subject line of the commit [1] very clearly. > > The commit [2] changed the original intent of commit [1] even though commit [2] mentioned "fixes" in the patch without any fixes tag. :( > commit [2] was bug. > > The right fix to restore [1] is to report the device features offered using get_device_features() callback instead of get_drivers_features(). already done in 3/6 patch in this series, see: static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg) { struct virtio_net_config config = {}; - u64 features; + u64 features_device, features_driver; u16 val_u16; and - features = vdev->config->get_driver_features(vdev); - if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features, + features_driver = vdev->config->get_driver_features(vdev); + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, + VDPA_ATTR_PAD)) + return -EMSGSIZE; + + features_device = vdev->config->get_device_features(vdev); + if (nla_put_u64_64bit(msg, VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, features_device, VDPA_ATTR_PAD)) return -EMSGSIZE; Features of the device and the driver are reported separately. > > Once above fix is applied, > when _MQ is not offered, max_queue_pairs should be treated as 1 by the driver. Yes, it is done by this patch. > We do not have a concept of management driver yet in the specification. > So when _MQ is not offered by the device, either kernel driver can return max_queue_pairs = 1, or the user space caller can see that _MQ is not offered by the device hence treat max_vq_pairs = 1. > (Like how it is described in the spec as - " Identify and initialize the receive and transmission virtqueues, up to N of each kind. If VIRTIO_NET_F_MQ feature bit is negotiated, N=max_virtqueue_pairs, otherwise identify N=1." Yes, that's why I set max_queue_pairs = 1 if MQ = 0 in this patch. > > So let orchestration layer can certainly derive this N when _MQ feature is not offered, instead of coming from the vdpa management layer. > I agree that this extra if() condition in the user space can be avoided if kernel always provides it. > But better to avoid such assumption because we have more such config space attributes. i.e., they exist only when features are offered. > So to keep it uniform, I prefer we avoid the exception for max_virtqueue_pairs. > > Please submit the fix for [2] to call get_device_features() for purpose of returning config space. > Continue to use get_driver_features() to show VDPA_ATTR_DEV_NEGOTIATED_FEATURES. This is already there in 3/6 patch, we have these code to report driver features: features_driver = vdev->config->get_driver_features(vdev); if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, VDPA_ATTR_PAD)) return -EMSGSIZE; And these code to report device features: features_device = vdev->config->get_device_features(vdev); if (nla_put_u64_64bit(msg, VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, features_device, VDPA_ATTR_PAD)) return -EMSGSIZE; > > I didn’t review these patches and it slipped through the cracks. :( > > [1] a64917bc2e9b ("vdpa: Introduce query of device config layout") > [2] a64917bc2e9 ("vdpa: Provide interface to read driver features")
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index d76b22b2f7ae..846dd37f3549 100644 --- a/drivers/vdpa/vdpa.c +++ b/drivers/vdpa/vdpa.c @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev, u16 val_u16; if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0) - return 0; + val_u16 = 1; + else + val_u16 = __virtio16_to_cpu(true, config->max_virtqueue_pairs); - val_u16 = le16_to_cpu(config->max_virtqueue_pairs); return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, val_u16); } @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms VDPA_ATTR_PAD)) return -EMSGSIZE; - return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, &config); + return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, &config); } static int
If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so when userspace querying queue pair numbers, it should return mq=1 than zero. Function vdpa_dev_net_config_fill() fills the attributions of the vDPA devices, so that it should call vdpa_dev_net_mq_config_fill() so the parameter in vdpa_dev_net_mq_config_fill() should be feature_device than feature_driver for the vDPA devices themselves Before this change, when MQ = 0, iproute2 output: $vdpa dev config show vdpa0 vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0 mtu 1500 After applying this commit, when MQ = 0, iproute2 output: $vdpa dev config show vdpa0 vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1 mtu 1500 Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> --- drivers/vdpa/vdpa.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)