From: Si-Wei Liu <si-wei.liu@oracle.com> To: "Michael S. Tsirkin" <mst@redhat.com>, Parav Pandit <parav@nvidia.com> Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "virtualization@lists.linux-foundation.org" <virtualization@lists.linux-foundation.org>, "xieyongji@bytedance.com" <xieyongji@bytedance.com>, "gautam.dawar@amd.com" <gautam.dawar@amd.com>, Zhu Lingshan <lingshan.zhu@intel.com> Subject: Re: [PATCH V4 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0 Date: Tue, 9 Aug 2022 17:54:21 -0700 [thread overview] Message-ID: <42e6d45b-4fba-1c8e-1726-3f082dd7a629@oracle.com> (raw) In-Reply-To: <20220809152457-mutt-send-email-mst@kernel.org> On 8/9/2022 12:36 PM, Michael S. Tsirkin wrote: > On Fri, Jul 22, 2022 at 01:14:42PM +0000, Parav Pandit wrote: >> >>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>> Sent: Friday, July 22, 2022 7:53 AM >>> >>> 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 >>> >> No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions. >> Please drop this patch. >> Nack on this patch. > Wrt this patch as far as I'm concerned you guys are bikeshedding. > > Parav generally don't send nacks please they are not helpful. > > Zhu Lingshan please always address comments in some way. E.g. refer to > them in the commit log explaining the motivation and the alternatives. > I know you don't agree with Parav but ghosting isn't nice. > > I still feel what we should have done is > not return a value, as long as we do return it we might > as well return something reasonable, not 0. Maybe I missed something but I don't get this, when VIRTIO_NET_F_MQ is not negotiated, the VDPA_ATTR_DEV_NET_CFG_MAX_VQP attribute is simply not there, but userspace (iproute) mistakenly puts a zero value there. This is a pattern every tool in iproute follows consistently by large. I don't get why kernel has to return something without seeing a very convincing use case? Not to mention spec doesn't give us explicit definition for when the field in config space becomes valid and/or the default value at first sights as part of feature negotiation. If we want to stick to the model Lingshan proposed, maybe fix the spec first then get back on the details? -Siwei > > And I like it that this fixes sparse warning actually: > max_virtqueue_pairs it tagged as __virtio, not __le. > > However, I am worried there is another bug here: > this is checking driver features. But really max vqs > should not depend on that, it depends on device > features, no? > > > >>> 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 > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
WARNING: multiple messages have this Message-ID (diff)
From: Si-Wei Liu <si-wei.liu@oracle.com> To: "Michael S. Tsirkin" <mst@redhat.com>, Parav Pandit <parav@nvidia.com> Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>, Zhu Lingshan <lingshan.zhu@intel.com>, "xieyongji@bytedance.com" <xieyongji@bytedance.com>, "gautam.dawar@amd.com" <gautam.dawar@amd.com>, "virtualization@lists.linux-foundation.org" <virtualization@lists.linux-foundation.org> Subject: Re: [PATCH V4 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0 Date: Tue, 9 Aug 2022 17:54:21 -0700 [thread overview] Message-ID: <42e6d45b-4fba-1c8e-1726-3f082dd7a629@oracle.com> (raw) In-Reply-To: <20220809152457-mutt-send-email-mst@kernel.org> On 8/9/2022 12:36 PM, Michael S. Tsirkin wrote: > On Fri, Jul 22, 2022 at 01:14:42PM +0000, Parav Pandit wrote: >> >>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>> Sent: Friday, July 22, 2022 7:53 AM >>> >>> 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 >>> >> No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions. >> Please drop this patch. >> Nack on this patch. > Wrt this patch as far as I'm concerned you guys are bikeshedding. > > Parav generally don't send nacks please they are not helpful. > > Zhu Lingshan please always address comments in some way. E.g. refer to > them in the commit log explaining the motivation and the alternatives. > I know you don't agree with Parav but ghosting isn't nice. > > I still feel what we should have done is > not return a value, as long as we do return it we might > as well return something reasonable, not 0. Maybe I missed something but I don't get this, when VIRTIO_NET_F_MQ is not negotiated, the VDPA_ATTR_DEV_NET_CFG_MAX_VQP attribute is simply not there, but userspace (iproute) mistakenly puts a zero value there. This is a pattern every tool in iproute follows consistently by large. I don't get why kernel has to return something without seeing a very convincing use case? Not to mention spec doesn't give us explicit definition for when the field in config space becomes valid and/or the default value at first sights as part of feature negotiation. If we want to stick to the model Lingshan proposed, maybe fix the spec first then get back on the details? -Siwei > > And I like it that this fixes sparse warning actually: > max_virtqueue_pairs it tagged as __virtio, not __le. > > However, I am worried there is another bug here: > this is checking driver features. But really max vqs > should not depend on that, it depends on device > features, no? > > > >>> 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 > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2022-08-10 0:54 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-22 11:53 [PATCH V4 0/6] ifcvf/vDPA: support query device config space through netlink Zhu Lingshan 2022-07-22 11:53 ` [PATCH V4 1/6] vDPA/ifcvf: get_config_size should return a value no greater than dev implementation Zhu Lingshan 2022-07-22 11:53 ` [PATCH V4 2/6] vDPA/ifcvf: support userspace to query features and MQ of a management device Zhu Lingshan 2022-07-22 11:53 ` [PATCH V4 3/6] vDPA: allow userspace to query features of a vDPA device Zhu Lingshan 2022-07-22 13:12 ` Parav Pandit 2022-07-22 13:12 ` Parav Pandit via Virtualization 2022-07-23 11:23 ` Zhu, Lingshan 2022-07-24 15:21 ` Parav Pandit 2022-07-24 15:21 ` Parav Pandit via Virtualization 2022-07-26 11:02 ` Zhu, Lingshan 2022-07-26 11:06 ` Parav Pandit 2022-07-26 11:06 ` Parav Pandit via Virtualization 2022-07-26 11:15 ` Zhu Lingshan 2022-07-27 6:02 ` Zhu, Lingshan 2022-08-09 19:27 ` Parav Pandit 2022-08-09 19:27 ` Parav Pandit via Virtualization 2022-08-09 19:24 ` Michael S. Tsirkin 2022-08-09 19:24 ` Michael S. Tsirkin 2022-08-09 19:28 ` Parav Pandit 2022-08-09 19:28 ` Parav Pandit via Virtualization 2022-08-10 2:51 ` Zhu, Lingshan 2022-07-22 11:53 ` [PATCH V4 4/6] vDPA: !FEATURES_OK should not block querying device config space Zhu Lingshan 2022-07-22 11:53 ` [PATCH V4 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0 Zhu Lingshan 2022-07-22 13:14 ` Parav Pandit 2022-07-22 13:14 ` Parav Pandit via Virtualization 2022-07-23 11:24 ` Zhu, Lingshan 2022-08-09 19:36 ` Michael S. Tsirkin 2022-08-09 19:36 ` Michael S. Tsirkin 2022-08-09 19:48 ` Parav Pandit via Virtualization 2022-08-09 19:48 ` Parav Pandit 2022-08-10 0:54 ` Si-Wei Liu [this message] 2022-08-10 0:54 ` Si-Wei Liu 2022-08-10 1:09 ` Jason Wang 2022-08-10 1:09 ` Jason Wang 2022-08-11 0:58 ` Si-Wei Liu 2022-08-11 0:58 ` Si-Wei Liu 2022-08-10 2:40 ` Zhu, Lingshan 2022-07-22 11:53 ` [PATCH V4 6/6] vDPA: fix 'cast to restricted le16' warnings in vdpa.c Zhu Lingshan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=42e6d45b-4fba-1c8e-1726-3f082dd7a629@oracle.com \ --to=si-wei.liu@oracle.com \ --cc=gautam.dawar@amd.com \ --cc=lingshan.zhu@intel.com \ --cc=mst@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=parav@nvidia.com \ --cc=virtualization@lists.linux-foundation.org \ --cc=xieyongji@bytedance.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.