Si-Wei Liu
2021-Dec-22 19:25 UTC
[PATCH v5 10/13] vdpa: Support reporting max device virtqueues
On 12/21/2021 11:54 PM, Eli Cohen wrote:> On Tue, Dec 21, 2021 at 11:29:36PM -0800, Si-Wei Liu wrote: >> >> On 12/21/2021 11:10 PM, Eli Cohen wrote: >>> On Wed, Dec 22, 2021 at 09:03:37AM +0200, Parav Pandit wrote: >>>>> From: Eli Cohen <elic at nvidia.com> >>>>> Sent: Wednesday, December 22, 2021 12:17 PM >>>>> >>>>>>>> --- a/drivers/vdpa/vdpa.c >>>>>>>> +++ b/drivers/vdpa/vdpa.c >>>>>>>> @@ -507,6 +507,9 @@ static int vdpa_mgmtdev_fill(const struct >>>>>>> vdpa_mgmt_dev *mdev, struct sk_buff *m >>>>>>>> err = -EMSGSIZE; >>>>>>>> goto msg_err; >>>>>>>> } >>>>>>>> + if (nla_put_u16(msg, VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, >>>>>>>> + mdev->max_supported_vqs)) >>>>>>> It still needs a default value when the field is not explicitly >>>>>>> filled in by the driver. >>>>>>> >>>>>> Unlikely. This can be optional field to help user decide device max limit. >>>>>> When max_supported_vqs is set to zero. Vdpa should omit exposing it to user >>>>> space. >>>>> This is not about what you expose to userspace. It's about the number of VQs >>>>> you want to create for a specific instance of vdpa. >>>> This value on mgmtdev indicates that a given mgmt device supports creating a vdpa device who can have maximum VQs of N. >>>> User will choose to create VQ with VQs <= N depending on its vcpu and other factors. >>> You're right. >>> So each vendor needs to put there their value. >> If I understand Parav correctly, he was suggesting not to expose >> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS to userspace if seeing (max_supported_vqs =>> 0) from the driver. > I can see the reasoning, but maybe we should leave it as zero which > means it was not reported. The user will then need to guess. I believe > other vendors will follow with an update so this to a real value.Unless you place a check in the vdpa core to enforce it on vdpa creation, otherwise it's very likely to get ignored by other vendors.> >> But meanwhile, I do wonder how users tell apart multiqueue supporting parent >> from the single queue mgmtdev without getting the aid from this field. I >> hope the answer won't be to create a vdpa instance to try. >> > Do you see a scenario that an admin decides to not instantiate vdpa just > because it does not support MQ?Yes, there is. If the hardware doesn't support MQ, the provisioning tool in the mgmt software will need to fallback to software vhost backend with mq=on. At the time the tool is checking out, it doesn't run with root privilege.> > And it the management device reports it does support, there's still no > guarantee you'll end up with a MQ net device.I'm not sure I follow. Do you mean it may be up to the guest feature negotiation? But the device itself is still MQ capable, isn't it? Thanks, -Siwei> > >> -Siwei >> >>>> This is what is exposed to the user to decide the upper bound. >>>>>> There has been some talk/patches of rdma virtio device. >>>>>> I anticipate such device to support more than 64K queues by nature of rdma. >>>>>> It is better to keep max_supported_vqs as u32. >>>>> Why not add it when we have it? >>>> Sure, with that approach we will end up adding two fields (current u16 and later another u32) due to smaller bit width of current one. >>>> Either way is fine. Michael was suggesting similar higher bit-width in other patches, so bringing up here for this field on how he sees it. >>> I can use u32 then.
Jason Wang
2021-Dec-23 02:27 UTC
[PATCH v5 10/13] vdpa: Support reporting max device virtqueues
On Thu, Dec 23, 2021 at 3:25 AM Si-Wei Liu <si-wei.liu at oracle.com> wrote:> > > > On 12/21/2021 11:54 PM, Eli Cohen wrote: > > On Tue, Dec 21, 2021 at 11:29:36PM -0800, Si-Wei Liu wrote: > >> > >> On 12/21/2021 11:10 PM, Eli Cohen wrote: > >>> On Wed, Dec 22, 2021 at 09:03:37AM +0200, Parav Pandit wrote: > >>>>> From: Eli Cohen <elic at nvidia.com> > >>>>> Sent: Wednesday, December 22, 2021 12:17 PM > >>>>> > >>>>>>>> --- a/drivers/vdpa/vdpa.c > >>>>>>>> +++ b/drivers/vdpa/vdpa.c > >>>>>>>> @@ -507,6 +507,9 @@ static int vdpa_mgmtdev_fill(const struct > >>>>>>> vdpa_mgmt_dev *mdev, struct sk_buff *m > >>>>>>>> err = -EMSGSIZE; > >>>>>>>> goto msg_err; > >>>>>>>> } > >>>>>>>> + if (nla_put_u16(msg, VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, > >>>>>>>> + mdev->max_supported_vqs)) > >>>>>>> It still needs a default value when the field is not explicitly > >>>>>>> filled in by the driver. > >>>>>>> > >>>>>> Unlikely. This can be optional field to help user decide device max limit. > >>>>>> When max_supported_vqs is set to zero. Vdpa should omit exposing it to user > >>>>> space. > >>>>> This is not about what you expose to userspace. It's about the number of VQs > >>>>> you want to create for a specific instance of vdpa. > >>>> This value on mgmtdev indicates that a given mgmt device supports creating a vdpa device who can have maximum VQs of N. > >>>> User will choose to create VQ with VQs <= N depending on its vcpu and other factors. > >>> You're right. > >>> So each vendor needs to put there their value. > >> If I understand Parav correctly, he was suggesting not to expose > >> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS to userspace if seeing (max_supported_vqs => >> 0) from the driver. > > I can see the reasoning, but maybe we should leave it as zero which > > means it was not reported. The user will then need to guess. I believe > > other vendors will follow with an update so this to a real value. > Unless you place a check in the vdpa core to enforce it on vdpa > creation, otherwise it's very likely to get ignored by other vendors. > > > > >> But meanwhile, I do wonder how users tell apart multiqueue supporting parent > >> from the single queue mgmtdev without getting the aid from this field. I > >> hope the answer won't be to create a vdpa instance to try. > >> > > Do you see a scenario that an admin decides to not instantiate vdpa just > > because it does not support MQ? > Yes, there is. If the hardware doesn't support MQ, the provisioning tool > in the mgmt software will need to fallback to software vhost backend > with mq=on. At the time the tool is checking out, it doesn't run with > root privilege. > > > > > And it the management device reports it does support, there's still no > > guarantee you'll end up with a MQ net device. > I'm not sure I follow. Do you mean it may be up to the guest feature > negotiation? But the device itself is still MQ capable, isn't it?I think we need to clarify the "device" here. For compatibility reasons, there could be a case that mgmt doesn't expect a mq capable vdpa device. So in this case, even if the parent is MQ capable, the vdpa isn't. Thanks> > Thanks, > -Siwei > > > > > > >> -Siwei > >> > >>>> This is what is exposed to the user to decide the upper bound. > >>>>>> There has been some talk/patches of rdma virtio device. > >>>>>> I anticipate such device to support more than 64K queues by nature of rdma. > >>>>>> It is better to keep max_supported_vqs as u32. > >>>>> Why not add it when we have it? > >>>> Sure, with that approach we will end up adding two fields (current u16 and later another u32) due to smaller bit width of current one. > >>>> Either way is fine. Michael was suggesting similar higher bit-width in other patches, so bringing up here for this field on how he sees it. > >>> I can use u32 then. >