Si-Wei Liu
2022-Oct-26 01:10 UTC
[PATCH v3 3/4] vdpa: show dev config as-is in "vdpa dev show" output
On 10/24/2022 7:24 PM, Jason Wang wrote:> On Tue, Oct 25, 2022 at 3:14 AM Si-Wei Liu<si-wei.liu at oracle.com> wrote: >> >> >> On 10/24/2022 1:40 AM, Jason Wang wrote: >>> On Sat, Oct 22, 2022 at 7:49 AM Si-Wei Liu<si-wei.liu at oracle.com> wrote: >>>> Live migration of vdpa would typically require re-instate vdpa >>>> device with an idential set of configs on the destination node, >>>> same way as how source node created the device in the first >>>> place. In order to save orchestration software from memorizing >>>> and keeping track of vdpa config, it will be helpful if the vdpa >>>> tool provides the aids for exporting the initial configs as-is, >>>> the way how vdpa device was created. The "vdpa dev show" command >>>> seems to be the right vehicle for that. It is unlike the "vdpa dev >>>> config show" command output which usually goes with the live value >>>> in the device config space, and is not quite reliable subject to >>>> the dynamics of feature negotiation or possible change by the >>>> driver to the config space. >>>> >>>> Examples: >>>> >>>> 1) Create vDPA by default without any config attribute >>>> >>>> $ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0 >>>> $ vdpa dev show vdpa0 >>>> vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256 >>>> $ vdpa dev -jp show vdpa0 >>>> { >>>> "dev": { >>>> "vdpa0": { >>>> "type": "network", >>>> "mgmtdev": "pci/0000:41:04.2", >>>> "vendor_id": 5555, >>>> "max_vqs": 9, >>>> "max_vq_size": 256, >>>> } >>>> } >>>> } >>>> >>>> 2) Create vDPA with config attribute(s) specified >>>> >>>> $ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0 \ >>>> mac e4:11:c6:d3:45:f0 max_vq_pairs 4 >>>> $ vdpa dev show >>>> vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256 >>>> initial_config: mac e4:11:c6:d3:45:f0 max_vq_pairs 4 >>>> $ vdpa dev -jp show >>>> { >>>> "dev": { >>>> "vdpa0": { >>>> "type": "network", >>>> "mgmtdev": "pci/0000:41:04.2", >>>> "vendor_id": 5555, >>>> "max_vqs": 9, >>>> "max_vq_size": 256, >>>> "initial_config": { >>>> "mac": "e4:11:c6:d3:45:f0", >>>> "max_vq_pairs": 4 >>>> } >>>> } >>>> } >>>> } >>>> >>>> Signed-off-by: Si-Wei Liu<si-wei.liu at oracle.com> >>>> --- >>>> drivers/vdpa/vdpa.c | 39 +++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 39 insertions(+) >>>> >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >>>> index bebded6..bfb8f54 100644 >>>> --- a/drivers/vdpa/vdpa.c >>>> +++ b/drivers/vdpa/vdpa.c >>>> @@ -677,6 +677,41 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i >>>> } >>>> >>>> static int >>>> +vdpa_dev_initcfg_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 device_id) >>>> +{ >>>> + struct vdpa_dev_set_config *cfg = &vdev->init_cfg; >>>> + int err = -EMSGSIZE; >>>> + >>>> + if (!cfg->mask) >>>> + return 0; >>>> + >>>> + switch (device_id) { >>>> + case VIRTIO_ID_NET: >>>> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR)) != 0 && >>>> + nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, >>>> + sizeof(cfg->net.mac), cfg->net.mac)) >>>> + return err; >>>> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) != 0 && >>>> + nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, cfg->net.mtu)) >>>> + return err; >>>> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) != 0 && >>>> + nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, >>>> + cfg->net.max_vq_pairs)) >>>> + return err; >>>> + break; >>>> + default: >>>> + break; >>>> + } >>>> + >>>> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) != 0 && >>>> + nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES, >>>> + cfg->device_features, VDPA_ATTR_PAD)) >>>> + return err; >>> A question: If any of those above attributes were not provisioned, >>> should we show the ones that are inherited from the parent? >> A simple answer would be yes, but the long answer is that I am not sure >> if there's any for the moment - there's no default value for mtu, mac, >> and max_vqp that can be inherited from the parent (max_vqp by default >> being 1 is spec defined, not something inherited from the parent). > Note that it is by default from driver level that if _F_MQ is not > negotiated. But I think we are talking about something different that > is out of the spec here, what if: > > vDPA inherit _F_MQ but mqx_vqp is not provisioned via netlink. > > Or is it not allowed?My understanding is that this is not allowed any more since the introduction of max_vqp attribute. Noted, currently we don't have a way for vendor driver to report the default value for mqx_vqp, if not otherwise specified in the CLI. Without getting the default value reported in 'vdpa mgmtdev show' level, it'd just confuse mgmt software even more.> At least some time in the past, mlx5 were > enabled with MQ with 8 queue pairs by default.That was the situation when there's no max_vqp attribute support from vdpa netlink API level. I think now every driver honors the vdpa core disposition to get a single queue pair if max_vqp config is missing.? And the mlx5_vdpa driver with 8 queue pairs in the wild days is just irrelevant to be manageable by mgmt software, regardless of live migration.> >> And >> the device_features if inherited is displayed at 'vdpa dev config show' >> output. Can you remind me of a good example for inherited value that we >> may want to show here? > Some other cases: > > 1) MTU: there should be something that the device needs to report if > _F_MTU is negotiated even if it is not provisioned from netlink.I am not sure I understand the ask here. Noted the QEMU argument has to offer host_mtu=X with the maximum MTU value for guest to use (and applied as the initial MTU config during virtio-net probing for Linux driver), and the way to get the parent device MTU and whether that's relevant to vdpa device's MTU is very vendor specific. I think we would need new attribute(s) in the mgmtdev level to support what you want here?> 2) device_features: if device_features is not provisioned, we should > still report it via netlink hereNot the way I expected it, but with Lingshan's series to expose fields out of FEATURES_OK, the device_features is now reported through 'vdpa dev config show' regardless being specified or not, if I am not mistaken? Currently we export the config attributes upon vdpa creation under the "initial_config" key. If we want to expose more default values inherited from mgmtdev, I think we can wrap up these default values under another key "inherited_config" to display in 'vdpa dev show' output. Does it fit what you have in mind?> or do you mean the mgmt can assume it > should be the same as mgmtdev. Anyhow if we don't show device_features > if it is not provisioned, it will complicate the mgmt software.Yes, as I said earlier, since the device_features attr getting added to the 'vdpa dev config show' command, this divergence started to complicate mgmt software already. Thanks, -Siwei> > Thanks > >> >> Thanks, >> -Siwei >> >> >>> Thanks >>> >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int >>>> vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq, >>>> int flags, struct netlink_ext_ack *extack) >>>> { >>>> @@ -715,6 +750,10 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i >>>> if (nla_put_u16(msg, VDPA_ATTR_DEV_MIN_VQ_SIZE, min_vq_size)) >>>> goto msg_err; >>>> >>>> + err = vdpa_dev_initcfg_fill(vdev, msg, device_id); >>>> + if (err) >>>> + goto msg_err; >>>> + >>>> genlmsg_end(msg, hdr); >>>> return 0; >>>> >>>> -- >>>> 1.8.3.1 >>>>-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20221025/d66c5f2f/attachment-0001.html>
Jason Wang
2022-Oct-26 04:44 UTC
[PATCH v3 3/4] vdpa: show dev config as-is in "vdpa dev show" output
? 2022/10/26 09:10, Si-Wei Liu ??:> > > On 10/24/2022 7:24 PM, Jason Wang wrote: >> On Tue, Oct 25, 2022 at 3:14 AM Si-Wei Liu<si-wei.liu at oracle.com> wrote: >>> On 10/24/2022 1:40 AM, Jason Wang wrote: >>>> On Sat, Oct 22, 2022 at 7:49 AM Si-Wei Liu<si-wei.liu at oracle.com> wrote: >>>>> Live migration of vdpa would typically require re-instate vdpa >>>>> device with an idential set of configs on the destination node, >>>>> same way as how source node created the device in the first >>>>> place. In order to save orchestration software from memorizing >>>>> and keeping track of vdpa config, it will be helpful if the vdpa >>>>> tool provides the aids for exporting the initial configs as-is, >>>>> the way how vdpa device was created. The "vdpa dev show" command >>>>> seems to be the right vehicle for that. It is unlike the "vdpa dev >>>>> config show" command output which usually goes with the live value >>>>> in the device config space, and is not quite reliable subject to >>>>> the dynamics of feature negotiation or possible change by the >>>>> driver to the config space. >>>>> >>>>> Examples: >>>>> >>>>> 1) Create vDPA by default without any config attribute >>>>> >>>>> $ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0 >>>>> $ vdpa dev show vdpa0 >>>>> vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256 >>>>> $ vdpa dev -jp show vdpa0 >>>>> { >>>>> "dev": { >>>>> "vdpa0": { >>>>> "type": "network", >>>>> "mgmtdev": "pci/0000:41:04.2", >>>>> "vendor_id": 5555, >>>>> "max_vqs": 9, >>>>> "max_vq_size": 256, >>>>> } >>>>> } >>>>> } >>>>> >>>>> 2) Create vDPA with config attribute(s) specified >>>>> >>>>> $ vdpa dev add mgmtdev pci/0000:41:04.2 name vdpa0 \ >>>>> mac e4:11:c6:d3:45:f0 max_vq_pairs 4 >>>>> $ vdpa dev show >>>>> vdpa0: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs 9 max_vq_size 256 >>>>> initial_config: mac e4:11:c6:d3:45:f0 max_vq_pairs 4 >>>>> $ vdpa dev -jp show >>>>> { >>>>> "dev": { >>>>> "vdpa0": { >>>>> "type": "network", >>>>> "mgmtdev": "pci/0000:41:04.2", >>>>> "vendor_id": 5555, >>>>> "max_vqs": 9, >>>>> "max_vq_size": 256, >>>>> "initial_config": { >>>>> "mac": "e4:11:c6:d3:45:f0", >>>>> "max_vq_pairs": 4 >>>>> } >>>>> } >>>>> } >>>>> } >>>>> >>>>> Signed-off-by: Si-Wei Liu<si-wei.liu at oracle.com> >>>>> --- >>>>> drivers/vdpa/vdpa.c | 39 +++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 39 insertions(+) >>>>> >>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >>>>> index bebded6..bfb8f54 100644 >>>>> --- a/drivers/vdpa/vdpa.c >>>>> +++ b/drivers/vdpa/vdpa.c >>>>> @@ -677,6 +677,41 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i >>>>> } >>>>> >>>>> static int >>>>> +vdpa_dev_initcfg_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 device_id) >>>>> +{ >>>>> + struct vdpa_dev_set_config *cfg = &vdev->init_cfg; >>>>> + int err = -EMSGSIZE; >>>>> + >>>>> + if (!cfg->mask) >>>>> + return 0; >>>>> + >>>>> + switch (device_id) { >>>>> + case VIRTIO_ID_NET: >>>>> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR)) != 0 && >>>>> + nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, >>>>> + sizeof(cfg->net.mac), cfg->net.mac)) >>>>> + return err; >>>>> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) != 0 && >>>>> + nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, cfg->net.mtu)) >>>>> + return err; >>>>> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) != 0 && >>>>> + nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, >>>>> + cfg->net.max_vq_pairs)) >>>>> + return err; >>>>> + break; >>>>> + default: >>>>> + break; >>>>> + } >>>>> + >>>>> + if ((cfg->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) != 0 && >>>>> + nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES, >>>>> + cfg->device_features, VDPA_ATTR_PAD)) >>>>> + return err; >>>> A question: If any of those above attributes were not provisioned, >>>> should we show the ones that are inherited from the parent? >>> A simple answer would be yes, but the long answer is that I am not sure >>> if there's any for the moment - there's no default value for mtu, mac, >>> and max_vqp that can be inherited from the parent (max_vqp by default >>> being 1 is spec defined, not something inherited from the parent). >> Note that it is by default from driver level that if _F_MQ is not >> negotiated. But I think we are talking about something different that >> is out of the spec here, what if: >> >> vDPA inherit _F_MQ but mqx_vqp is not provisioned via netlink. >> >> Or is it not allowed? > My understanding is that this is not allowed any more since the > introduction of max_vqp attribute. Noted, currently we don't have a > way for vendor driver to report the default value for mqx_vqp,I think it can be reported in this patch?> if not otherwise specified in the CLI. Without getting the default > value reported in 'vdpa mgmtdev show' level, it'd just confuse mgmt > software even more.Yes, this is something that we need to fix. And what's more in order to support dynamic provisioning, we need a way to report the number of available instances that could be used for vDPA device provisioning.> >> At least some time in the past, mlx5 were >> enabled with MQ with 8 queue pairs by default. > That was the situation when there's no max_vqp attribute support from > vdpa netlink API level. I think now every driver honors the vdpa core > disposition to get a single queue pair if max_vqp config is missing.So we have: int vdpa_register_device(struct vdpa_device *vdev, int nvqs) This means technically, parent can allocate a multiqueue devices with _F_MQ features if max_vqp and device_features is not provisioned. And what's more, what happens if _F_MQ is provisioned by max_vqp is not specified? The question is: When a attribute is not specificed/provisioned via net link, what's the default value? The answer should be consistent: if device_features is determined by the parent, we should do the same for mqx_vqp. And it looks to me all of those belongs to the initial config (self-contained)> And the mlx5_vdpa driver with 8 queue pairs in the wild days is just > irrelevant to be manageable by mgmt software, regardless of live > migration. >>> And >>> the device_features if inherited is displayed at 'vdpa dev config show' >>> output. Can you remind me of a good example for inherited value that we >>> may want to show here? >> Some other cases: >> >> 1) MTU: there should be something that the device needs to report if >> _F_MTU is negotiated even if it is not provisioned from netlink. > I am not sure I understand the ask here. Noted the QEMU argument has > to offer host_mtu=X with the maximum MTU value for guest to use (and > applied as the initial MTU config during virtio-net probing for Linux > driver),Adding Cindy. I think it's a known issue that we need to do sanity check to make sure cli parameters matches what is provisioned from netlink.> and the way to get the parent device MTU and whether that's relevant > to vdpa device's MTU is very vendor specific.So I think the max MTU of parent should be equal to the max MTU of the vDPA.> I think we would need new attribute(s) in the mgmtdev level to support > what you want here?Not sure, but what I want to ask is consider we provision MTU feature but without max MTU value, do we need to report the initial max MTU here?> >> 2) device_features: if device_features is not provisioned, we should >> still report it via netlink here > Not the way I expected it, but with Lingshan's series to expose fields > out of FEATURES_OK, the device_features is now reported through 'vdpa > dev config show' regardless being specified or not, if I am not mistaken?Yes.> > Currently we export the config attributes upon vdpa creation under the > "initial_config" key. If we want to expose more default values > inherited from mgmtdev, I think we can wrap up these default values > under another key "inherited_config" to display in 'vdpa dev show' > output. Does it fit what you have in mind?I wonder if it's better to merge those two, or is there any advantages of splitting them?> >> or do you mean the mgmt can assume it >> should be the same as mgmtdev. Anyhow if we don't show device_features >> if it is not provisioned, it will complicate the mgmt software. > Yes, as I said earlier, since the device_features attr getting added > to the 'vdpa dev config show' command, this divergence started to > complicate mgmt software already. > > Thanks,Thanks> -Siwei >> Thanks >> >>> Thanks, >>> -Siwei >>> >>> >>>> Thanks >>>> >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int >>>>> vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq, >>>>> int flags, struct netlink_ext_ack *extack) >>>>> { >>>>> @@ -715,6 +750,10 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i >>>>> if (nla_put_u16(msg, VDPA_ATTR_DEV_MIN_VQ_SIZE, min_vq_size)) >>>>> goto msg_err; >>>>> >>>>> + err = vdpa_dev_initcfg_fill(vdev, msg, device_id); >>>>> + if (err) >>>>> + goto msg_err; >>>>> + >>>>> genlmsg_end(msg, hdr); >>>>> return 0; >>>>> >>>>> -- >>>>> 1.8.3.1 >>>>> >