| Message ID | 20220722115309.82746-4-lingshan.zhu@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| 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: Friday, July 22, 2022 7:53 AM > > This commit adds a new vDPA netlink attribution > VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query > features of vDPA devices through this new attr. > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > --- > drivers/vdpa/vdpa.c | 13 +++++++++---- > include/uapi/linux/vdpa.h | 1 + > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index > ebf2f363fbe7..9b0e39b2f022 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct > vdpa_device *vdev, 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; > > vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ - > 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device > *vdev, struct sk_buff *ms > if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) > return -EMSGSIZE; > > - 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; > > - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config); > + return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, > +&config); > } > > static int > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h index > 25c55cab3d7c..39f1c3d7c112 100644 > --- a/include/uapi/linux/vdpa.h > +++ b/include/uapi/linux/vdpa.h > @@ -47,6 +47,7 @@ enum vdpa_attr { > VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ > VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ > VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ > + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ > I have answered in previous emails. I disagree with the change. Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES. MST, I nack this patch. As mentioned in the previous versions, also it is missing the example output in the commit log. Please include example output. > VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ > VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ > -- > 2.31.1
On 7/22/2022 9:12 PM, Parav Pandit wrote: >> From: Zhu Lingshan <lingshan.zhu@intel.com> >> Sent: Friday, July 22, 2022 7:53 AM >> >> This commit adds a new vDPA netlink attribution >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query >> features of vDPA devices through this new attr. >> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >> --- >> drivers/vdpa/vdpa.c | 13 +++++++++---- >> include/uapi/linux/vdpa.h | 1 + >> 2 files changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index >> ebf2f363fbe7..9b0e39b2f022 100644 >> --- a/drivers/vdpa/vdpa.c >> +++ b/drivers/vdpa/vdpa.c >> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct >> vdpa_device *vdev, 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; >> >> vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ - >> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device >> *vdev, struct sk_buff *ms >> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) >> return -EMSGSIZE; >> >> - 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; >> >> - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config); >> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, >> +&config); >> } >> >> static int >> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h index >> 25c55cab3d7c..39f1c3d7c112 100644 >> --- a/include/uapi/linux/vdpa.h >> +++ b/include/uapi/linux/vdpa.h >> @@ -47,6 +47,7 @@ enum vdpa_attr { >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ >> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ >> VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ >> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ >> > I have answered in previous emails. > I disagree with the change. > Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES. I believe we have already discussed this before in the V3 thread. I have told you that reusing this attr will lead to a new race condition. Pleas refer to the previous thread, and you did not answer my questions in that thread. Thanks, Zhu Lingshan > > MST, > I nack this patch. > As mentioned in the previous versions, also it is missing the example output in the commit log. > Please include example output. > >> VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ >> VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ >> -- >> 2.31.1
> From: Zhu, Lingshan <lingshan.zhu@intel.com> > Sent: Saturday, July 23, 2022 7:24 AM > > > On 7/22/2022 9:12 PM, Parav Pandit wrote: > >> From: Zhu Lingshan <lingshan.zhu@intel.com> > >> Sent: Friday, July 22, 2022 7:53 AM > >> > >> This commit adds a new vDPA netlink attribution > >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query > features > >> of vDPA devices through this new attr. > >> > >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > >> --- > >> drivers/vdpa/vdpa.c | 13 +++++++++---- > >> include/uapi/linux/vdpa.h | 1 + > >> 2 files changed, 10 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index > >> ebf2f363fbe7..9b0e39b2f022 100644 > >> --- a/drivers/vdpa/vdpa.c > >> +++ b/drivers/vdpa/vdpa.c > >> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct > >> vdpa_device *vdev, 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; > >> > >> vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ - > >> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct > >> vdpa_device *vdev, struct sk_buff *ms > >> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) > >> return -EMSGSIZE; > >> > >> - 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; > >> > >> - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config); > >> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, > >> +&config); > >> } > >> > >> static int > >> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h > >> index > >> 25c55cab3d7c..39f1c3d7c112 100644 > >> --- a/include/uapi/linux/vdpa.h > >> +++ b/include/uapi/linux/vdpa.h > >> @@ -47,6 +47,7 @@ enum vdpa_attr { > >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ > >> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ > >> VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ > >> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ > >> > > I have answered in previous emails. > > I disagree with the change. > > Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES. > I believe we have already discussed this before in the V3 thread. > I have told you that reusing this attr will lead to a new race condition. > Returning attribute cannot lead to any race condition. > Pleas refer to the previous thread, and you did not answer my questions in > that thread. > > Thanks, > Zhu Lingshan > > > > MST, > > I nack this patch. > > As mentioned in the previous versions, also it is missing the example > output in the commit log. > > Please include example output. > > > >> VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ > >> VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ > >> -- > >> 2.31.1
On 7/24/2022 11:21 PM, Parav Pandit wrote: > >> From: Zhu, Lingshan <lingshan.zhu@intel.com> >> Sent: Saturday, July 23, 2022 7:24 AM >> >> >> On 7/22/2022 9:12 PM, Parav Pandit wrote: >>>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>>> Sent: Friday, July 22, 2022 7:53 AM >>>> >>>> This commit adds a new vDPA netlink attribution >>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query >> features >>>> of vDPA devices through this new attr. >>>> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>> --- >>>> drivers/vdpa/vdpa.c | 13 +++++++++---- >>>> include/uapi/linux/vdpa.h | 1 + >>>> 2 files changed, 10 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index >>>> ebf2f363fbe7..9b0e39b2f022 100644 >>>> --- a/drivers/vdpa/vdpa.c >>>> +++ b/drivers/vdpa/vdpa.c >>>> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct >>>> vdpa_device *vdev, 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; >>>> >>>> vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ - >>>> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct >>>> vdpa_device *vdev, struct sk_buff *ms >>>> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) >>>> return -EMSGSIZE; >>>> >>>> - 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; >>>> >>>> - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config); >>>> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, >>>> +&config); >>>> } >>>> >>>> static int >>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h >>>> index >>>> 25c55cab3d7c..39f1c3d7c112 100644 >>>> --- a/include/uapi/linux/vdpa.h >>>> +++ b/include/uapi/linux/vdpa.h >>>> @@ -47,6 +47,7 @@ enum vdpa_attr { >>>> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ >>>> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ >>>> VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ >>>> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ >>>> >>> I have answered in previous emails. >>> I disagree with the change. >>> Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES. >> I believe we have already discussed this before in the V3 thread. >> I have told you that reusing this attr will lead to a new race condition. >> > Returning attribute cannot lead to any race condition. Please refer to our discussion in the V3 series, I have explained if re-use this attr, it will be a multiple consumers and multiple produces model, it is a typical racing condition. > > >> Pleas refer to the previous thread, and you did not answer my questions in >> that thread. >> >> Thanks, >> Zhu Lingshan >>> MST, >>> I nack this patch. >>> As mentioned in the previous versions, also it is missing the example >> output in the commit log. >>> Please include example output. >>> >>>> VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ >>>> VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ >>>> -- >>>> 2.31.1
> From: Zhu, Lingshan <lingshan.zhu@intel.com> > Sent: Tuesday, July 26, 2022 7:03 AM > > On 7/24/2022 11:21 PM, Parav Pandit wrote: > > > >> From: Zhu, Lingshan <lingshan.zhu@intel.com> > >> Sent: Saturday, July 23, 2022 7:24 AM > >> > >> > >> On 7/22/2022 9:12 PM, Parav Pandit wrote: > >>>> From: Zhu Lingshan <lingshan.zhu@intel.com> > >>>> Sent: Friday, July 22, 2022 7:53 AM > >>>> > >>>> This commit adds a new vDPA netlink attribution > >>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can > query > >> features > >>>> of vDPA devices through this new attr. > >>>> > >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > >>>> --- > >>>> drivers/vdpa/vdpa.c | 13 +++++++++---- > >>>> include/uapi/linux/vdpa.h | 1 + > >>>> 2 files changed, 10 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index > >>>> ebf2f363fbe7..9b0e39b2f022 100644 > >>>> --- a/drivers/vdpa/vdpa.c > >>>> +++ b/drivers/vdpa/vdpa.c > >>>> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct > >>>> vdpa_device *vdev, 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; > >>>> > >>>> vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ > >>>> - > >>>> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct > >>>> vdpa_device *vdev, struct sk_buff *ms > >>>> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) > >>>> return -EMSGSIZE; > >>>> > >>>> - 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; > >>>> > >>>> - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config); > >>>> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, > >>>> +&config); > >>>> } > >>>> > >>>> static int > >>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h > >>>> index > >>>> 25c55cab3d7c..39f1c3d7c112 100644 > >>>> --- a/include/uapi/linux/vdpa.h > >>>> +++ b/include/uapi/linux/vdpa.h > >>>> @@ -47,6 +47,7 @@ enum vdpa_attr { > >>>> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ > >>>> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ > >>>> VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ > >>>> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ > >>>> > >>> I have answered in previous emails. > >>> I disagree with the change. > >>> Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES. > >> I believe we have already discussed this before in the V3 thread. > >> I have told you that reusing this attr will lead to a new race condition. > >> > > Returning attribute cannot lead to any race condition. > Please refer to our discussion in the V3 series, I have explained if re-use this > attr, it will be a multiple consumers and multiple produces model, it is a > typical racing condition. I read the emails with subject = " Re: [PATCH V3 3/6] vDPA: allow userspace to query features of a vDPA device" I couldn’t find multiple consumers multiple producers working on same nla message.
On 7/26/2022 7:06 PM, Parav Pandit wrote: >> From: Zhu, Lingshan <lingshan.zhu@intel.com> >> Sent: Tuesday, July 26, 2022 7:03 AM >> >> On 7/24/2022 11:21 PM, Parav Pandit wrote: >>>> From: Zhu, Lingshan <lingshan.zhu@intel.com> >>>> Sent: Saturday, July 23, 2022 7:24 AM >>>> >>>> >>>> On 7/22/2022 9:12 PM, Parav Pandit wrote: >>>>>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>>>>> Sent: Friday, July 22, 2022 7:53 AM >>>>>> >>>>>> This commit adds a new vDPA netlink attribution >>>>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can >> query >>>> features >>>>>> of vDPA devices through this new attr. >>>>>> >>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>>>> --- >>>>>> drivers/vdpa/vdpa.c | 13 +++++++++---- >>>>>> include/uapi/linux/vdpa.h | 1 + >>>>>> 2 files changed, 10 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index >>>>>> ebf2f363fbe7..9b0e39b2f022 100644 >>>>>> --- a/drivers/vdpa/vdpa.c >>>>>> +++ b/drivers/vdpa/vdpa.c >>>>>> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct >>>>>> vdpa_device *vdev, 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; >>>>>> >>>>>> vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ >>>>>> - >>>>>> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct >>>>>> vdpa_device *vdev, struct sk_buff *ms >>>>>> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) >>>>>> return -EMSGSIZE; >>>>>> >>>>>> - 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; >>>>>> >>>>>> - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config); >>>>>> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, >>>>>> +&config); >>>>>> } >>>>>> >>>>>> static int >>>>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h >>>>>> index >>>>>> 25c55cab3d7c..39f1c3d7c112 100644 >>>>>> --- a/include/uapi/linux/vdpa.h >>>>>> +++ b/include/uapi/linux/vdpa.h >>>>>> @@ -47,6 +47,7 @@ enum vdpa_attr { >>>>>> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ >>>>>> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ >>>>>> VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ >>>>>> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ >>>>>> >>>>> I have answered in previous emails. >>>>> I disagree with the change. >>>>> Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES. >>>> I believe we have already discussed this before in the V3 thread. >>>> I have told you that reusing this attr will lead to a new race condition. >>>> >>> Returning attribute cannot lead to any race condition. >> Please refer to our discussion in the V3 series, I have explained if re-use this >> attr, it will be a multiple consumers and multiple produces model, it is a >> typical racing condition. > I read the emails with subject = " Re: [PATCH V3 3/6] vDPA: allow userspace to query features of a vDPA device" > I couldn’t find multiple consumers multiple producers working on same nla message. If this attr is reused, then there can be multiple iproute2 instances or other applications querying feature bits of the management device and the vDPA device simultaneously, and both kernel side management feature bits filler function and vDPA device feature bits filler function can write the NLA message at the same time. That's the multiple consumers and producers, and no locks
On Fri, Jul 22, 2022 at 07:53:06PM +0800, Zhu Lingshan wrote: > This commit adds a new vDPA netlink attribution > VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query > features of vDPA devices through this new attr. > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> I think at least some discussion and documentation of this attribute versus VDPA_ATTR_DEV_SUPPORTED_FEATURES is called for. Otherwise how do people know which one to use? We can't send everyone to go read the lkml thread. > --- > drivers/vdpa/vdpa.c | 13 +++++++++---- > include/uapi/linux/vdpa.h | 1 + > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > index ebf2f363fbe7..9b0e39b2f022 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev, > 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; > > vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); > @@ -832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms > if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) > return -EMSGSIZE; > > - 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; > > - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config); > + return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, &config); > } > > static int > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h > index 25c55cab3d7c..39f1c3d7c112 100644 > --- a/include/uapi/linux/vdpa.h > +++ b/include/uapi/linux/vdpa.h > @@ -47,6 +47,7 @@ enum vdpa_attr { > VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ > VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ > VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ > + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ > > VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ > VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ > -- > 2.31.1
> From: Zhu, Lingshan <lingshan.zhu@intel.com> > Sent: Wednesday, July 27, 2022 2:02 AM > > > On 7/26/2022 7:06 PM, Parav Pandit wrote: > >> From: Zhu, Lingshan <lingshan.zhu@intel.com> > >> Sent: Tuesday, July 26, 2022 7:03 AM > >> > >> On 7/24/2022 11:21 PM, Parav Pandit wrote: > >>>> From: Zhu, Lingshan <lingshan.zhu@intel.com> > >>>> Sent: Saturday, July 23, 2022 7:24 AM > >>>> > >>>> > >>>> On 7/22/2022 9:12 PM, Parav Pandit wrote: > >>>>>> From: Zhu Lingshan <lingshan.zhu@intel.com> > >>>>>> Sent: Friday, July 22, 2022 7:53 AM > >>>>>> > >>>>>> This commit adds a new vDPA netlink attribution > >>>>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can > >> query > >>>> features > >>>>>> of vDPA devices through this new attr. > >>>>>> > >>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > >>>>>> --- > >>>>>> drivers/vdpa/vdpa.c | 13 +++++++++---- > >>>>>> include/uapi/linux/vdpa.h | 1 + > >>>>>> 2 files changed, 10 insertions(+), 4 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index > >>>>>> ebf2f363fbe7..9b0e39b2f022 100644 > >>>>>> --- a/drivers/vdpa/vdpa.c > >>>>>> +++ b/drivers/vdpa/vdpa.c > >>>>>> @@ -815,7 +815,7 @@ static int > vdpa_dev_net_mq_config_fill(struct > >>>>>> vdpa_device *vdev, 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; > >>>>>> > >>>>>> vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); > >>>>>> @@ > >>>>>> - > >>>>>> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct > >>>>>> vdpa_device *vdev, struct sk_buff *ms > >>>>>> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, > val_u16)) > >>>>>> return -EMSGSIZE; > >>>>>> > >>>>>> - 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; > >>>>>> > >>>>>> - return vdpa_dev_net_mq_config_fill(vdev, msg, features, > &config); > >>>>>> + return vdpa_dev_net_mq_config_fill(vdev, msg, > features_driver, > >>>>>> +&config); > >>>>>> } > >>>>>> > >>>>>> static int > >>>>>> diff --git a/include/uapi/linux/vdpa.h > >>>>>> b/include/uapi/linux/vdpa.h index > >>>>>> 25c55cab3d7c..39f1c3d7c112 100644 > >>>>>> --- a/include/uapi/linux/vdpa.h > >>>>>> +++ b/include/uapi/linux/vdpa.h > >>>>>> @@ -47,6 +47,7 @@ enum vdpa_attr { > >>>>>> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ > >>>>>> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* > u32 */ > >>>>>> VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ > >>>>>> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* > u64 */ > >>>>>> > >>>>> I have answered in previous emails. > >>>>> I disagree with the change. > >>>>> Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES. > >>>> I believe we have already discussed this before in the V3 thread. > >>>> I have told you that reusing this attr will lead to a new race condition. > >>>> > >>> Returning attribute cannot lead to any race condition. > >> Please refer to our discussion in the V3 series, I have explained if > >> re-use this attr, it will be a multiple consumers and multiple > >> produces model, it is a typical racing condition. > > I read the emails with subject = " Re: [PATCH V3 3/6] vDPA: allow userspace > to query features of a vDPA device" > > I couldn’t find multiple consumers multiple producers working on same nla > message. > If this attr is reused, then there can be multiple iproute2 instances or other > applications querying feature bits of the management device and the vDPA > device simultaneously, and both kernel side management feature bits filler > function and vDPA device feature bits filler function can write the NLA > message at the same time. That's the multiple consumers and producers, > and no locks No. Each filling up happens in each process context. There is no race here.
> From: Michael S. Tsirkin <mst@redhat.com> > Sent: Tuesday, August 9, 2022 3:24 PM > > On Fri, Jul 22, 2022 at 07:53:06PM +0800, Zhu Lingshan wrote: > > This commit adds a new vDPA netlink attribution > > VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query > features of > > vDPA devices through this new attr. > > > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > > > I think at least some discussion and documentation of this attribute versus > VDPA_ATTR_DEV_SUPPORTED_FEATURES is called for. > > Otherwise how do people know which one to use? > > We can't send everyone to go read the lkml thread. There is no race here. Commit log is missing example anyway. I explained multiple times that this patch and/or its previous version cannot proceed in same state.
On 8/10/2022 3:24 AM, Michael S. Tsirkin wrote: > On Fri, Jul 22, 2022 at 07:53:06PM +0800, Zhu Lingshan wrote: >> This commit adds a new vDPA netlink attribution >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query >> features of vDPA devices through this new attr. >> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > > I think at least some discussion and documentation > of this attribute versus VDPA_ATTR_DEV_SUPPORTED_FEATURES > is called for. > > Otherwise how do people know which one to use? > > We can't send everyone to go read the lkml thread. I will add comments in both this vdpa_dev_net_config_fill() and the header file. Thanks, Zhu Lingshan > >> --- >> drivers/vdpa/vdpa.c | 13 +++++++++---- >> include/uapi/linux/vdpa.h | 1 + >> 2 files changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >> index ebf2f363fbe7..9b0e39b2f022 100644 >> --- a/drivers/vdpa/vdpa.c >> +++ b/drivers/vdpa/vdpa.c >> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev, >> 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; >> >> vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); >> @@ -832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms >> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) >> return -EMSGSIZE; >> >> - 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; >> >> - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config); >> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, &config); >> } >> >> static int >> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h >> index 25c55cab3d7c..39f1c3d7c112 100644 >> --- a/include/uapi/linux/vdpa.h >> +++ b/include/uapi/linux/vdpa.h >> @@ -47,6 +47,7 @@ enum vdpa_attr { >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ >> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ >> VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ >> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ >> >> VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ >> VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ >> -- >> 2.31.1
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index ebf2f363fbe7..9b0e39b2f022 100644 --- a/drivers/vdpa/vdpa.c +++ b/drivers/vdpa/vdpa.c @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev, 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; vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ -832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) return -EMSGSIZE; - 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; - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config); + return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, &config); } static int diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h index 25c55cab3d7c..39f1c3d7c112 100644 --- a/include/uapi/linux/vdpa.h +++ b/include/uapi/linux/vdpa.h @@ -47,6 +47,7 @@ enum vdpa_attr { VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */
This commit adds a new vDPA netlink attribution VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query features of vDPA devices through this new attr. Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> --- drivers/vdpa/vdpa.c | 13 +++++++++---- include/uapi/linux/vdpa.h | 1 + 2 files changed, 10 insertions(+), 4 deletions(-)