Re: [PATCH v1 6/7] vdpa: Add support for querying control virtqueue index

On Thu, Dec 9, 2021 at 5:21 PM Eli Cohen [off-list ref] wrote:

On Thu, Dec 09, 2021 at 05:12:01PM +0800, Jason Wang wrote:

quoted

On Thu, Dec 9, 2021 at 4:26 PM Eli Cohen [off-list ref] wrote:

quoted

On Thu, Dec 09, 2021 at 04:17:22PM +0800, Jason Wang wrote:

quoted

On Thu, Dec 9, 2021 at 4:04 PM Eli Cohen [off-list ref] wrote:

quoted

On Thu, Dec 09, 2021 at 03:55:48PM +0800, Jason Wang wrote:

quoted

On Thu, Dec 9, 2021 at 3:09 PM Eli Cohen [off-list ref] wrote:

quoted

On Thu, Dec 09, 2021 at 01:44:56PM +0800, Jason Wang wrote:

quoted

On Thu, Dec 9, 2021 at 4:15 AM Eli Cohen [off-list ref] wrote:

quoted

Add netlink attribute and callback function to query the control VQ
index of a device.

Example:

$ vdpa dev config show vdpa-a
  vdpa-a: mac 00:00:00:00:88:88 link up link_announce false max_vq_pairs 5 \
  mtu 9000 ctrl_vq_idx 10

I still wonder about the motivation for this.

To be able to show the stats for CVQ.

Right.

quoted

quoted

And as discussed, the
ctrl_vq_idx varies depending on whether MQ is negotiated.

I handle this according to the spec and it depends on MQ.

Yes, but there could be a chance that the cvq index changes after the
vdpa dev config show.

It depends on:
VIRTIO_NET_F_CTRL_VQ
VIRTIO_NET_F_MQ

which can change only if the features are re-negotiated and that happens
on driver in itialization.

And on max_virtqueue_pairs which is also set at driver initialization.

So I don't see any real issue here. Am I missing something?

No. I meant technically there could be a re-negotiation that happens
in the middle:

1) vdpa dev config show
2) feature renegotiation which change the cvq index
3) getting cvq stats

So the cvq index might be changed. E.g it could be changed from a cvq
to a rx queue. It might be easier to make the get index and stats
atomic.

The interface to read VQ stats is based on the user providing the index
and getting the statistics.

If we want to follow your suggestion then we need maybe to use a
slightly modified command:

For regular VQ:
dpa dev vstats show vdpa-a qidx 0

For CVQ:
vdpa dev vstats show vdpa-a cvq

This might be better, but we need to make sure cvq only works for the
device that has a cvq.

OK.

quoted

quoted

And that raises another question. Do we want to report the CVQ index in
config show?

If we go with stats show vdpa-a cvq, we don't need to report the cvq index.

Maybe we should leave some kind of indication as to whether there is a
CVQ. Maybe not the index itself by just "CVQ" so a user will know if
it's worth trying to read cvq stats.

Otherwise, the user will attempt to read and fail.
What do you think?

It should work.

Thanks

quoted

Thanks

quoted

quoted

Thanks

quoted

quoted

Thanks

quoted

quoted

Thanks

quoted

Signed-off-by: Eli Cohen <redacted>
---
v0 -> v1:
1. Use logic defined in the spec to deduce virtqueue index.

 drivers/vdpa/vdpa.c       | 25 +++++++++++++++++++++++++
 include/uapi/linux/vdpa.h |  1 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 3bf016e03512..b4d4b8a7ca4e 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c

@@ -712,6 +712,27 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
        return msg->len;
 }

+static int vdpa_dev_net_ctrl_vq_fill(struct vdpa_device *vdev,
+                                    struct sk_buff *msg,
+                                    struct virtio_net_config *config,
+                                    u64 features)
+{
+       u16 N;
+
+       /* control VQ index, if available, is deduced according to the logic
+        * described in the virtio spec in section 5.1.2
+        */
+       if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
+               return 0;
+
+       if (features & BIT_ULL(VIRTIO_NET_F_MQ))
+               N = le16_to_cpu(config->max_virtqueue_pairs);
+       else
+               N = 1;
+
+       return nla_put_u16(msg, VDPA_ATTR_DEV_CTRL_VQ_IDX, 2 * N);
+}
+
 static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
                                       struct sk_buff *msg, u64 features,
                                       const struct virtio_net_config *config)

@@ -730,6 +751,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
        struct virtio_net_config config = {};
        u64 features;
        u16 val_u16;
+       int err;

        vdpa_get_config(vdev, 0, &config, sizeof(config));

@@ -746,6 +768,9 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
                return -EMSGSIZE;

        features = vdev->config->get_driver_features(vdev);
+       err = vdpa_dev_net_ctrl_vq_fill(vdev, msg, &config, features);
+       if (err)
+               return err;

        return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
 }

diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
index a252f06f9dfd..2e3a7f89f42d 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h

@@ -34,6 +34,7 @@ enum vdpa_attr {
        VDPA_ATTR_DEV_MAX_VQS,                  /* u32 */
        VDPA_ATTR_DEV_MAX_VQ_SIZE,              /* u16 */
        VDPA_ATTR_DEV_MIN_VQ_SIZE,              /* u16 */
+       VDPA_ATTR_DEV_CTRL_VQ_IDX,              /* u16 */

        VDPA_ATTR_DEV_NET_CFG_MACADDR,          /* binary */
        VDPA_ATTR_DEV_NET_STATUS,               /* u8 */
--

2.33.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization