Re: [PATCH v3 3/4] vdpa: show dev config as-is in "vdpa dev show" output

Jason Wang
2022-10-26T04:45:00Z

在 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@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@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@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
>>>>>
>