[PATCH v3 0/2] Show statistics for a vdpa device

[PATCH v3 0/2] Show statistics for a vdpa device

[Eli Cohen]

The following two patch series adds support to read vendor statistics
for a vdpa device.

The first patch lays the ground to allow an upstream driver to provide
statistics in the form of an attribute name/attribute value pairs.

The second patch implements this for mlx5_vdpa which gives received
descriptors and completed descriptors information for all the
virtqueues. 

v2 -> v3:
Fix sparse warning in patch 0002

Eli Cohen (2):
  vdpa: Add support for querying vendor statistics
  vdpa/mlx5: Add support for reading descriptor statistics

 drivers/vdpa/mlx5/core/mlx5_vdpa.h |   2 +
 drivers/vdpa/mlx5/net/mlx5_vnet.c  | 165 +++++++++++++++++++++++++++++
 drivers/vdpa/vdpa.c                | 129 ++++++++++++++++++++++
 include/linux/mlx5/mlx5_ifc.h      |   1 +
 include/linux/mlx5/mlx5_ifc_vdpa.h |  39 +++++++
 include/linux/vdpa.h               |   5 +
 include/uapi/linux/vdpa.h          |   6 ++
 7 files changed, 347 insertions(+)

-- 
2.35.1

[PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Eli Cohen]

Allows to read vendor statistics of a vdpa device. The specific
statistics data are received from the upstream driver in the form of an
(attribute name, attribute value) pairs.

An example of statistics for mlx5_vdpa device are:

received_desc - number of descriptors received by the virtqueue
completed_desc - number of descriptors completed by the virtqueue

A descriptor using indirect buffers is still counted as 1. In addition,
N chained descriptors are counted correctly N times as one would expect.

A new callback was added to vdpa_config_ops which provides the means for
the vdpa driver to return statistics results.

The interface allows for reading all the supported virtqueues, including
the control virtqueue if it exists.

Below are some examples taken from mlx5_vdpa which are introduced in the
following patch:

1. Read statistics for the virtqueue at index 1

$ vdpa dev vstats show vdpa-a qidx 1
vdpa-a:
queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836

2. Read statistics for the virtqueue at index 32
$ vdpa dev vstats show vdpa-a qidx 32
vdpa-a:
queue_type control_vq queue_index 32 received_desc 62 completed_desc 62

3. Read statisitics for the virtqueue at index 0 with json output
$ vdpa -j dev vstats show vdpa-a qidx 0
{"vstats":{"vdpa-a":{
"queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
 "name":"completed_desc","value":417548}}}

4. Read statistics for the virtqueue at index 0 with preety json output
$ vdpa -jp dev vstats show vdpa-a qidx 0
{
    "vstats": {
        "vdpa-a": {

            "queue_type": "rx",
            "queue_index": 0,
            "name": "received_desc",
            "value": 417776,
            "name": "completed_desc",
            "value": 417548
        }
    }
}

Signed-off-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
 include/linux/vdpa.h      |   5 ++
 include/uapi/linux/vdpa.h |   6 ++
 3 files changed, 140 insertions(+)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 2b75c00b1005..933466f61ca8 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
 	return err;
 }
 
+static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
+			       struct genl_info *info, u32 index)
+{
+	int err;
+
+	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
+	if (err)
+		return err;
+
+	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
+			     struct genl_info *info, u32 index)
+{
+	int err;
+
+	if (!vdev->config->get_vendor_vq_stats)
+		return -EOPNOTSUPP;
+
+	err = vdpa_fill_stats_rec(vdev, msg, info, index);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
+				      struct sk_buff *msg,
+				      struct genl_info *info, u32 index)
+{
+	u32 device_id;
+	void *hdr;
+	int err;
+	u32 portid = info->snd_portid;
+	u32 seq = info->snd_seq;
+	u32 flags = 0;
+
+	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
+			  VDPA_CMD_DEV_VSTATS_GET);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
+		err = -EMSGSIZE;
+		goto undo_msg;
+	}
+
+	device_id = vdev->config->get_device_id(vdev);
+	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
+		err = -EMSGSIZE;
+		goto undo_msg;
+	}
+
+	err = vendor_stats_fill(vdev, msg, info, index);
+
+	genlmsg_end(msg, hdr);
+
+	return err;
+
+undo_msg:
+	genlmsg_cancel(msg, hdr);
+	return err;
+}
+
 static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct vdpa_device *vdev;
@@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
 	return msg->len;
 }
 
+static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
+					  struct genl_info *info)
+{
+	struct vdpa_device *vdev;
+	struct sk_buff *msg;
+	const char *devname;
+	struct device *dev;
+	u32 index;
+	int err;
+
+	if (!info->attrs[VDPA_ATTR_DEV_NAME])
+		return -EINVAL;
+
+	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
+		return -EINVAL;
+
+	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
+	mutex_lock(&vdpa_dev_mutex);
+	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
+	if (!dev) {
+		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
+		err = -ENODEV;
+		goto dev_err;
+	}
+	vdev = container_of(dev, struct vdpa_device, dev);
+	if (!vdev->mdev) {
+		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
+		err = -EINVAL;
+		goto mdev_err;
+	}
+	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
+	if (!err)
+		err = genlmsg_reply(msg, info);
+
+	put_device(dev);
+	mutex_unlock(&vdpa_dev_mutex);
+
+	if (err)
+		nlmsg_free(msg);
+
+	return err;
+
+mdev_err:
+	put_device(dev);
+dev_err:
+	mutex_unlock(&vdpa_dev_mutex);
+	return err;
+}
+
 static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
 	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
 	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
@@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
 	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
 	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
 	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
+	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
 };
 
 static const struct genl_ops vdpa_nl_ops[] = {
@@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
 		.doit = vdpa_nl_cmd_dev_config_get_doit,
 		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
 	},
+	{
+		.cmd = VDPA_CMD_DEV_VSTATS_GET,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = vdpa_nl_cmd_dev_stats_get_doit,
+		.flags = GENL_ADMIN_PERM,
+	},
 };
 
 static struct genl_family vdpa_nl_family __ro_after_init = {
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 8943a209202e..48ed1fc00830 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -276,6 +276,9 @@ struct vdpa_config_ops {
 			    const struct vdpa_vq_state *state);
 	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
 			    struct vdpa_vq_state *state);
+	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
+				   struct sk_buff *msg,
+				   struct netlink_ext_ack *extack);
 	struct vdpa_notification_area
 	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
 	/* vq irq is not expected to be changed once DRIVER_OK is set */
@@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
 int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
 void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
 
+#define VDPA_INVAL_QUEUE_INDEX 0xffff
+
 #endif /* _LINUX_VDPA_H */
diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
index 1061d8d2d09d..25c55cab3d7c 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h
@@ -18,6 +18,7 @@ enum vdpa_command {
 	VDPA_CMD_DEV_DEL,
 	VDPA_CMD_DEV_GET,		/* can dump */
 	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
+	VDPA_CMD_DEV_VSTATS_GET,
 };
 
 enum vdpa_attr {
@@ -46,6 +47,11 @@ enum vdpa_attr {
 	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
 	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
 	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
+
+	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
+	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
+	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
+
 	/* new attributes must be added above here */
 	VDPA_ATTR_MAX,
 };
-- 
2.35.1

Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Si-Wei Liu]

On 5/2/2022 3:22 AM, Eli Cohen wrote:
> Allows to read vendor statistics of a vdpa device. The specific
> statistics data are received from the upstream driver in the form of an
> (attribute name, attribute value) pairs.
>
> An example of statistics for mlx5_vdpa device are:
>
> received_desc - number of descriptors received by the virtqueue
> completed_desc - number of descriptors completed by the virtqueue
>
> A descriptor using indirect buffers is still counted as 1. In addition,
> N chained descriptors are counted correctly N times as one would expect.
>
> A new callback was added to vdpa_config_ops which provides the means for
> the vdpa driver to return statistics results.
>
> The interface allows for reading all the supported virtqueues, including
> the control virtqueue if it exists.
>
> Below are some examples taken from mlx5_vdpa which are introduced in the
> following patch:
>
> 1. Read statistics for the virtqueue at index 1
>
> $ vdpa dev vstats show vdpa-a qidx 1
> vdpa-a:
> queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
>
> 2. Read statistics for the virtqueue at index 32
> $ vdpa dev vstats show vdpa-a qidx 32
> vdpa-a:
> queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
>
> 3. Read statisitics for the virtqueue at index 0 with json output
> $ vdpa -j dev vstats show vdpa-a qidx 0
> {"vstats":{"vdpa-a":{
> "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
>   "name":"completed_desc","value":417548}}}
>
> 4. Read statistics for the virtqueue at index 0 with preety json output
> $ vdpa -jp dev vstats show vdpa-a qidx 0
> {
>      "vstats": {
>          "vdpa-a": {
>
>              "queue_type": "rx",
>              "queue_index": 0,
>              "name": "received_desc",
>              "value": 417776,
>              "name": "completed_desc",
>              "value": 417548
>          }
>      }
> }
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
>   include/linux/vdpa.h      |   5 ++
>   include/uapi/linux/vdpa.h |   6 ++
>   3 files changed, 140 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 2b75c00b1005..933466f61ca8 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>   	return err;
>   }
>   
> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
> +			       struct genl_info *info, u32 index)
> +{
> +	int err;
> +
> +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
> +	if (err)
> +		return err;
> +
> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
> +		return -EMSGSIZE;
> +
> +	return 0;
> +}
> +
> +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
> +			     struct genl_info *info, u32 index)
> +{
> +	int err;
> +
> +	if (!vdev->config->get_vendor_vq_stats)
> +		return -EOPNOTSUPP;
> +
> +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
> +				      struct sk_buff *msg,
> +				      struct genl_info *info, u32 index)
> +{
> +	u32 device_id;
> +	void *hdr;
> +	int err;
> +	u32 portid = info->snd_portid;
> +	u32 seq = info->snd_seq;
> +	u32 flags = 0;
> +
> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> +			  VDPA_CMD_DEV_VSTATS_GET);
> +	if (!hdr)
> +		return -EMSGSIZE;
> +
> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
> +		err = -EMSGSIZE;
> +		goto undo_msg;
> +	}
> +
> +	device_id = vdev->config->get_device_id(vdev);
> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
> +		err = -EMSGSIZE;
> +		goto undo_msg;
> +	}
> +
You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function, 
otherwise I can't image how you can ensure the atomicity to infer 
queue_type for control vq.
And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call 
into vendor_stats_fill()?

> +	err = vendor_stats_fill(vdev, msg, info, index);
> +
> +	genlmsg_end(msg, hdr);
> +
> +	return err;
> +
> +undo_msg:
> +	genlmsg_cancel(msg, hdr);
> +	return err;
> +}
> +
>   static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
>   {
>   	struct vdpa_device *vdev;
> @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
>   	return msg->len;
>   }
>   
> +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
> +					  struct genl_info *info)
> +{
> +	struct vdpa_device *vdev;
> +	struct sk_buff *msg;
> +	const char *devname;
> +	struct device *dev;
> +	u32 index;
> +	int err;
> +
> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
> +		return -EINVAL;
> +
> +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
> +		return -EINVAL;
> +
> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
> +	mutex_lock(&vdpa_dev_mutex);
Given that stats_get() is a read-only operation that might happen quite 
often from time to time, I wonder if it is now a good timing to convert 
vdpa_dev_mutex to a semaphore?

> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
> +	if (!dev) {
> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> +		err = -ENODEV;
> +		goto dev_err;
Missing nlmsg_free().
> +	}
> +	vdev = container_of(dev, struct vdpa_device, dev);
> +	if (!vdev->mdev) {
> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
> +		err = -EINVAL;
> +		goto mdev_err;
Missing nlmsg_free().

Otherwise looks fine.

Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>


-Siwei
> +	}
> +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
> +	if (!err)
> +		err = genlmsg_reply(msg, info);
> +
> +	put_device(dev);
> +	mutex_unlock(&vdpa_dev_mutex);
> +
> +	if (err)
> +		nlmsg_free(msg);
> +
> +	return err;
> +
> +mdev_err:
> +	put_device(dev);
> +dev_err:
> +	mutex_unlock(&vdpa_dev_mutex);
> +	return err;
> +}
> +
>   static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>   	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>   	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
> @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>   	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
>   	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
>   	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
> +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
>   };
>   
>   static const struct genl_ops vdpa_nl_ops[] = {
> @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>   		.doit = vdpa_nl_cmd_dev_config_get_doit,
>   		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
>   	},
> +	{
> +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
> +		.flags = GENL_ADMIN_PERM,
> +	},
>   };
>   
>   static struct genl_family vdpa_nl_family __ro_after_init = {
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 8943a209202e..48ed1fc00830 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -276,6 +276,9 @@ struct vdpa_config_ops {
>   			    const struct vdpa_vq_state *state);
>   	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
>   			    struct vdpa_vq_state *state);
> +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
> +				   struct sk_buff *msg,
> +				   struct netlink_ext_ack *extack);
>   	struct vdpa_notification_area
>   	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
>   	/* vq irq is not expected to be changed once DRIVER_OK is set */
> @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
>   int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
>   void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
>   
> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
> +
>   #endif /* _LINUX_VDPA_H */
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index 1061d8d2d09d..25c55cab3d7c 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -18,6 +18,7 @@ enum vdpa_command {
>   	VDPA_CMD_DEV_DEL,
>   	VDPA_CMD_DEV_GET,		/* can dump */
>   	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
> +	VDPA_CMD_DEV_VSTATS_GET,
>   };
>   
>   enum vdpa_attr {
> @@ -46,6 +47,11 @@ enum vdpa_attr {
>   	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>   	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>   	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
> +
> +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
> +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
> +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
> +
>   	/* new attributes must be added above here */
>   	VDPA_ATTR_MAX,
>   };

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

Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Si-Wei Liu]

On 5/2/2022 3:22 AM, Eli Cohen wrote:
> Allows to read vendor statistics of a vdpa device. The specific
> statistics data are received from the upstream driver in the form of an
> (attribute name, attribute value) pairs.
>
> An example of statistics for mlx5_vdpa device are:
>
> received_desc - number of descriptors received by the virtqueue
> completed_desc - number of descriptors completed by the virtqueue
>
> A descriptor using indirect buffers is still counted as 1. In addition,
> N chained descriptors are counted correctly N times as one would expect.
>
> A new callback was added to vdpa_config_ops which provides the means for
> the vdpa driver to return statistics results.
>
> The interface allows for reading all the supported virtqueues, including
> the control virtqueue if it exists.
>
> Below are some examples taken from mlx5_vdpa which are introduced in the
> following patch:
>
> 1. Read statistics for the virtqueue at index 1
>
> $ vdpa dev vstats show vdpa-a qidx 1
> vdpa-a:
> queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
>
> 2. Read statistics for the virtqueue at index 32
> $ vdpa dev vstats show vdpa-a qidx 32
> vdpa-a:
> queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
>
> 3. Read statisitics for the virtqueue at index 0 with json output
> $ vdpa -j dev vstats show vdpa-a qidx 0
> {"vstats":{"vdpa-a":{
> "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
>   "name":"completed_desc","value":417548}}}
>
> 4. Read statistics for the virtqueue at index 0 with preety json output
> $ vdpa -jp dev vstats show vdpa-a qidx 0
> {
>      "vstats": {
>          "vdpa-a": {
>
>              "queue_type": "rx",
>              "queue_index": 0,
>              "name": "received_desc",
>              "value": 417776,
>              "name": "completed_desc",
>              "value": 417548
>          }
>      }
> }
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
>   include/linux/vdpa.h      |   5 ++
>   include/uapi/linux/vdpa.h |   6 ++
>   3 files changed, 140 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 2b75c00b1005..933466f61ca8 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>   	return err;
>   }
>   
> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
> +			       struct genl_info *info, u32 index)
> +{
> +	int err;
> +
> +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
> +	if (err)
> +		return err;
> +
> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
> +		return -EMSGSIZE;
> +
> +	return 0;
> +}
> +
> +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
> +			     struct genl_info *info, u32 index)
> +{
> +	int err;
> +
> +	if (!vdev->config->get_vendor_vq_stats)
> +		return -EOPNOTSUPP;
> +
> +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
> +				      struct sk_buff *msg,
> +				      struct genl_info *info, u32 index)
> +{
> +	u32 device_id;
> +	void *hdr;
> +	int err;
> +	u32 portid = info->snd_portid;
> +	u32 seq = info->snd_seq;
> +	u32 flags = 0;
> +
> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> +			  VDPA_CMD_DEV_VSTATS_GET);
> +	if (!hdr)
> +		return -EMSGSIZE;
> +
> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
> +		err = -EMSGSIZE;
> +		goto undo_msg;
> +	}
> +
> +	device_id = vdev->config->get_device_id(vdev);
> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
> +		err = -EMSGSIZE;
> +		goto undo_msg;
> +	}
> +
You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function, 
otherwise I can't image how you can ensure the atomicity to infer 
queue_type for control vq.
And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call 
into vendor_stats_fill()?

> +	err = vendor_stats_fill(vdev, msg, info, index);
> +
> +	genlmsg_end(msg, hdr);
> +
> +	return err;
> +
> +undo_msg:
> +	genlmsg_cancel(msg, hdr);
> +	return err;
> +}
> +
>   static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
>   {
>   	struct vdpa_device *vdev;
> @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
>   	return msg->len;
>   }
>   
> +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
> +					  struct genl_info *info)
> +{
> +	struct vdpa_device *vdev;
> +	struct sk_buff *msg;
> +	const char *devname;
> +	struct device *dev;
> +	u32 index;
> +	int err;
> +
> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
> +		return -EINVAL;
> +
> +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
> +		return -EINVAL;
> +
> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
> +	mutex_lock(&vdpa_dev_mutex);
Given that stats_get() is a read-only operation that might happen quite 
often from time to time, I wonder if it is now a good timing to convert 
vdpa_dev_mutex to a semaphore?

> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
> +	if (!dev) {
> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> +		err = -ENODEV;
> +		goto dev_err;
Missing nlmsg_free().
> +	}
> +	vdev = container_of(dev, struct vdpa_device, dev);
> +	if (!vdev->mdev) {
> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
> +		err = -EINVAL;
> +		goto mdev_err;
Missing nlmsg_free().

Otherwise looks fine.

Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>


-Siwei
> +	}
> +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
> +	if (!err)
> +		err = genlmsg_reply(msg, info);
> +
> +	put_device(dev);
> +	mutex_unlock(&vdpa_dev_mutex);
> +
> +	if (err)
> +		nlmsg_free(msg);
> +
> +	return err;
> +
> +mdev_err:
> +	put_device(dev);
> +dev_err:
> +	mutex_unlock(&vdpa_dev_mutex);
> +	return err;
> +}
> +
>   static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>   	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>   	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
> @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>   	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
>   	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
>   	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
> +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
>   };
>   
>   static const struct genl_ops vdpa_nl_ops[] = {
> @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>   		.doit = vdpa_nl_cmd_dev_config_get_doit,
>   		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
>   	},
> +	{
> +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
> +		.flags = GENL_ADMIN_PERM,
> +	},
>   };
>   
>   static struct genl_family vdpa_nl_family __ro_after_init = {
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 8943a209202e..48ed1fc00830 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -276,6 +276,9 @@ struct vdpa_config_ops {
>   			    const struct vdpa_vq_state *state);
>   	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
>   			    struct vdpa_vq_state *state);
> +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
> +				   struct sk_buff *msg,
> +				   struct netlink_ext_ack *extack);
>   	struct vdpa_notification_area
>   	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
>   	/* vq irq is not expected to be changed once DRIVER_OK is set */
> @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
>   int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
>   void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
>   
> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
> +
>   #endif /* _LINUX_VDPA_H */
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index 1061d8d2d09d..25c55cab3d7c 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -18,6 +18,7 @@ enum vdpa_command {
>   	VDPA_CMD_DEV_DEL,
>   	VDPA_CMD_DEV_GET,		/* can dump */
>   	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
> +	VDPA_CMD_DEV_VSTATS_GET,
>   };
>   
>   enum vdpa_attr {
> @@ -46,6 +47,11 @@ enum vdpa_attr {
>   	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>   	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>   	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
> +
> +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
> +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
> +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
> +
>   	/* new attributes must be added above here */
>   	VDPA_ATTR_MAX,
>   };

RE: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Eli Cohen]

> -----Original Message-----
> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Tuesday, May 3, 2022 2:48 AM
> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
> 
> 
> 
> On 5/2/2022 3:22 AM, Eli Cohen wrote:
> > Allows to read vendor statistics of a vdpa device. The specific
> > statistics data are received from the upstream driver in the form of an
> > (attribute name, attribute value) pairs.
> >
> > An example of statistics for mlx5_vdpa device are:
> >
> > received_desc - number of descriptors received by the virtqueue
> > completed_desc - number of descriptors completed by the virtqueue
> >
> > A descriptor using indirect buffers is still counted as 1. In addition,
> > N chained descriptors are counted correctly N times as one would expect.
> >
> > A new callback was added to vdpa_config_ops which provides the means for
> > the vdpa driver to return statistics results.
> >
> > The interface allows for reading all the supported virtqueues, including
> > the control virtqueue if it exists.
> >
> > Below are some examples taken from mlx5_vdpa which are introduced in the
> > following patch:
> >
> > 1. Read statistics for the virtqueue at index 1
> >
> > $ vdpa dev vstats show vdpa-a qidx 1
> > vdpa-a:
> > queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
> >
> > 2. Read statistics for the virtqueue at index 32
> > $ vdpa dev vstats show vdpa-a qidx 32
> > vdpa-a:
> > queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
> >
> > 3. Read statisitics for the virtqueue at index 0 with json output
> > $ vdpa -j dev vstats show vdpa-a qidx 0
> > {"vstats":{"vdpa-a":{
> > "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
> >   "name":"completed_desc","value":417548}}}
> >
> > 4. Read statistics for the virtqueue at index 0 with preety json output
> > $ vdpa -jp dev vstats show vdpa-a qidx 0
> > {
> >      "vstats": {
> >          "vdpa-a": {
> >
> >              "queue_type": "rx",
> >              "queue_index": 0,
> >              "name": "received_desc",
> >              "value": 417776,
> >              "name": "completed_desc",
> >              "value": 417548
> >          }
> >      }
> > }
> >
> > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > ---
> >   drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
> >   include/linux/vdpa.h      |   5 ++
> >   include/uapi/linux/vdpa.h |   6 ++
> >   3 files changed, 140 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index 2b75c00b1005..933466f61ca8 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
> >   	return err;
> >   }
> >
> > +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
> > +			       struct genl_info *info, u32 index)
> > +{
> > +	int err;
> > +
> > +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
> > +	if (err)
> > +		return err;
> > +
> > +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
> > +		return -EMSGSIZE;
> > +
> > +	return 0;
> > +}
> > +
> > +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
> > +			     struct genl_info *info, u32 index)
> > +{
> > +	int err;
> > +
> > +	if (!vdev->config->get_vendor_vq_stats)
> > +		return -EOPNOTSUPP;
> > +
> > +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
> > +	if (err)
> > +		return err;
> > +
> > +	return 0;
> > +}
> > +
> > +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
> > +				      struct sk_buff *msg,
> > +				      struct genl_info *info, u32 index)
> > +{
> > +	u32 device_id;
> > +	void *hdr;
> > +	int err;
> > +	u32 portid = info->snd_portid;
> > +	u32 seq = info->snd_seq;
> > +	u32 flags = 0;
> > +
> > +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> > +			  VDPA_CMD_DEV_VSTATS_GET);
> > +	if (!hdr)
> > +		return -EMSGSIZE;
> > +
> > +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
> > +		err = -EMSGSIZE;
> > +		goto undo_msg;
> > +	}
> > +
> > +	device_id = vdev->config->get_device_id(vdev);
> > +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
> > +		err = -EMSGSIZE;
> > +		goto undo_msg;
> > +	}
> > +
> You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
> otherwise I can't image how you can ensure the atomicity to infer
> queue_type for control vq.
> And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
> into vendor_stats_fill()?

It is done in the hardware driver. In this case, in mlx5_vdpa.

> 
> > +	err = vendor_stats_fill(vdev, msg, info, index);
> > +
> > +	genlmsg_end(msg, hdr);
> > +
> > +	return err;
> > +
> > +undo_msg:
> > +	genlmsg_cancel(msg, hdr);
> > +	return err;
> > +}
> > +
> >   static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
> >   {
> >   	struct vdpa_device *vdev;
> > @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
> >   	return msg->len;
> >   }
> >
> > +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
> > +					  struct genl_info *info)
> > +{
> > +	struct vdpa_device *vdev;
> > +	struct sk_buff *msg;
> > +	const char *devname;
> > +	struct device *dev;
> > +	u32 index;
> > +	int err;
> > +
> > +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
> > +		return -EINVAL;
> > +
> > +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
> > +		return -EINVAL;
> > +
> > +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> > +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > +	if (!msg)
> > +		return -ENOMEM;
> > +
> > +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
> > +	mutex_lock(&vdpa_dev_mutex);
> Given that stats_get() is a read-only operation that might happen quite
> often from time to time, I wonder if it is now a good timing to convert
> vdpa_dev_mutex to a semaphore?
> 
> > +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
> > +	if (!dev) {
> > +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> > +		err = -ENODEV;
> > +		goto dev_err;
> Missing nlmsg_free().
> > +	}
> > +	vdev = container_of(dev, struct vdpa_device, dev);
> > +	if (!vdev->mdev) {
> > +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
> > +		err = -EINVAL;
> > +		goto mdev_err;
> Missing nlmsg_free().
> 
> Otherwise looks fine.
> 
> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
> 
> 
> -Siwei
> > +	}
> > +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
> > +	if (!err)
> > +		err = genlmsg_reply(msg, info);
> > +
> > +	put_device(dev);
> > +	mutex_unlock(&vdpa_dev_mutex);
> > +
> > +	if (err)
> > +		nlmsg_free(msg);
> > +
> > +	return err;
> > +
> > +mdev_err:
> > +	put_device(dev);
> > +dev_err:
> > +	mutex_unlock(&vdpa_dev_mutex);
> > +	return err;
> > +}
> > +
> >   static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
> >   	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
> >   	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
> > @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
> >   	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
> >   	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
> >   	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
> > +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
> >   };
> >
> >   static const struct genl_ops vdpa_nl_ops[] = {
> > @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
> >   		.doit = vdpa_nl_cmd_dev_config_get_doit,
> >   		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
> >   	},
> > +	{
> > +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
> > +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> > +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
> > +		.flags = GENL_ADMIN_PERM,
> > +	},
> >   };
> >
> >   static struct genl_family vdpa_nl_family __ro_after_init = {
> > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > index 8943a209202e..48ed1fc00830 100644
> > --- a/include/linux/vdpa.h
> > +++ b/include/linux/vdpa.h
> > @@ -276,6 +276,9 @@ struct vdpa_config_ops {
> >   			    const struct vdpa_vq_state *state);
> >   	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
> >   			    struct vdpa_vq_state *state);
> > +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
> > +				   struct sk_buff *msg,
> > +				   struct netlink_ext_ack *extack);
> >   	struct vdpa_notification_area
> >   	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
> >   	/* vq irq is not expected to be changed once DRIVER_OK is set */
> > @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
> >   int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
> >   void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
> >
> > +#define VDPA_INVAL_QUEUE_INDEX 0xffff
> > +
> >   #endif /* _LINUX_VDPA_H */
> > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > index 1061d8d2d09d..25c55cab3d7c 100644
> > --- a/include/uapi/linux/vdpa.h
> > +++ b/include/uapi/linux/vdpa.h
> > @@ -18,6 +18,7 @@ enum vdpa_command {
> >   	VDPA_CMD_DEV_DEL,
> >   	VDPA_CMD_DEV_GET,		/* can dump */
> >   	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
> > +	VDPA_CMD_DEV_VSTATS_GET,
> >   };
> >
> >   enum vdpa_attr {
> > @@ -46,6 +47,11 @@ enum vdpa_attr {
> >   	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
> >   	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
> >   	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
> > +
> > +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
> > +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
> > +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
> > +
> >   	/* new attributes must be added above here */
> >   	VDPA_ATTR_MAX,
> >   };

Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Si-Wei Liu]

On 5/2/2022 10:13 PM, Eli Cohen wrote:
>
>> -----Original Message-----
>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>> Sent: Tuesday, May 3, 2022 2:48 AM
>> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
>> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
>>
>>
>>
>> On 5/2/2022 3:22 AM, Eli Cohen wrote:
>>> Allows to read vendor statistics of a vdpa device. The specific
>>> statistics data are received from the upstream driver in the form of an
>>> (attribute name, attribute value) pairs.
>>>
>>> An example of statistics for mlx5_vdpa device are:
>>>
>>> received_desc - number of descriptors received by the virtqueue
>>> completed_desc - number of descriptors completed by the virtqueue
>>>
>>> A descriptor using indirect buffers is still counted as 1. In addition,
>>> N chained descriptors are counted correctly N times as one would expect.
>>>
>>> A new callback was added to vdpa_config_ops which provides the means for
>>> the vdpa driver to return statistics results.
>>>
>>> The interface allows for reading all the supported virtqueues, including
>>> the control virtqueue if it exists.
>>>
>>> Below are some examples taken from mlx5_vdpa which are introduced in the
>>> following patch:
>>>
>>> 1. Read statistics for the virtqueue at index 1
>>>
>>> $ vdpa dev vstats show vdpa-a qidx 1
>>> vdpa-a:
>>> queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
>>>
>>> 2. Read statistics for the virtqueue at index 32
>>> $ vdpa dev vstats show vdpa-a qidx 32
>>> vdpa-a:
>>> queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
>>>
>>> 3. Read statisitics for the virtqueue at index 0 with json output
>>> $ vdpa -j dev vstats show vdpa-a qidx 0
>>> {"vstats":{"vdpa-a":{
>>> "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
>>>    "name":"completed_desc","value":417548}}}
>>>
>>> 4. Read statistics for the virtqueue at index 0 with preety json output
>>> $ vdpa -jp dev vstats show vdpa-a qidx 0
>>> {
>>>       "vstats": {
>>>           "vdpa-a": {
>>>
>>>               "queue_type": "rx",
>>>               "queue_index": 0,
>>>               "name": "received_desc",
>>>               "value": 417776,
>>>               "name": "completed_desc",
>>>               "value": 417548
>>>           }
>>>       }
>>> }
>>>
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> ---
>>>    drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
>>>    include/linux/vdpa.h      |   5 ++
>>>    include/uapi/linux/vdpa.h |   6 ++
>>>    3 files changed, 140 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 2b75c00b1005..933466f61ca8 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>>>    	return err;
>>>    }
>>>
>>> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
>>> +			       struct genl_info *info, u32 index)
>>> +{
>>> +	int err;
>>> +
>>> +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
>>> +		return -EMSGSIZE;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
>>> +			     struct genl_info *info, u32 index)
>>> +{
>>> +	int err;
>>> +
>>> +	if (!vdev->config->get_vendor_vq_stats)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
>>> +				      struct sk_buff *msg,
>>> +				      struct genl_info *info, u32 index)
>>> +{
>>> +	u32 device_id;
>>> +	void *hdr;
>>> +	int err;
>>> +	u32 portid = info->snd_portid;
>>> +	u32 seq = info->snd_seq;
>>> +	u32 flags = 0;
>>> +
>>> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>> +			  VDPA_CMD_DEV_VSTATS_GET);
>>> +	if (!hdr)
>>> +		return -EMSGSIZE;
>>> +
>>> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
>>> +		err = -EMSGSIZE;
>>> +		goto undo_msg;
>>> +	}
>>> +
>>> +	device_id = vdev->config->get_device_id(vdev);
>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
>>> +		err = -EMSGSIZE;
>>> +		goto undo_msg;
>>> +	}
>>> +
>> You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
>> otherwise I can't image how you can ensure the atomicity to infer
>> queue_type for control vq.
>> And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
>> into vendor_stats_fill()?
> It is done in the hardware driver. In this case, in mlx5_vdpa.
OK... So you think this is not vdpa common code but rather individual 
vendor driver should deal with? Seems fine at the first glance, but with 
some thoughts this would complicate userspace code quite a lot to tell 
apart different cases - say if the VDPA_ATTR_DEV_NEGOTIATED_FEATURES 
attr is missing it would not be possible to display the queue type. On 
the other hand, the queue type itself shouldn't be vendor specific thing 
- only the vendor stats are, right?

Furthermore, without FEATURES_OK the stats returned for a specific queue 
might not be stable/reliable/reasonable at all, not sure how the tool 
can infer such complex state (e.g. negotiation is in progress) if 
somehow the vendor driver doesn't provide the corresponding attribute? 
Should vendor driver expect to fail with explicit message to indicate 
the reason, or it'd be fine to just display zero stats there? Looking at 
mlx5_vdpa implementation it seems to be the former case, but in case of 
device being negotiating, depending on which queue, the vstat query 
might end up with a confusing message of, either

"virtqueue index is not valid"

or,

"failed to query hardware"

but none is helpful for user to indicate what's going on... I wonder if 
mandating presence of FEATURES_OK would simplify userspace tool's 
implementation in this case?


Thanks,
-Siwei

>
>>> +	err = vendor_stats_fill(vdev, msg, info, index);
>>> +
>>> +	genlmsg_end(msg, hdr);
>>> +
>>> +	return err;
>>> +
>>> +undo_msg:
>>> +	genlmsg_cancel(msg, hdr);
>>> +	return err;
>>> +}
>>> +
>>>    static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
>>>    {
>>>    	struct vdpa_device *vdev;
>>> @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
>>>    	return msg->len;
>>>    }
>>>
>>> +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
>>> +					  struct genl_info *info)
>>> +{
>>> +	struct vdpa_device *vdev;
>>> +	struct sk_buff *msg;
>>> +	const char *devname;
>>> +	struct device *dev;
>>> +	u32 index;
>>> +	int err;
>>> +
>>> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
>>> +		return -EINVAL;
>>> +
>>> +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
>>> +		return -EINVAL;
>>> +
>>> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
>>> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>> +	if (!msg)
>>> +		return -ENOMEM;
>>> +
>>> +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
>>> +	mutex_lock(&vdpa_dev_mutex);
>> Given that stats_get() is a read-only operation that might happen quite
>> often from time to time, I wonder if it is now a good timing to convert
>> vdpa_dev_mutex to a semaphore?
>>
>>> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
>>> +	if (!dev) {
>>> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
>>> +		err = -ENODEV;
>>> +		goto dev_err;
>> Missing nlmsg_free().
>>> +	}
>>> +	vdev = container_of(dev, struct vdpa_device, dev);
>>> +	if (!vdev->mdev) {
>>> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
>>> +		err = -EINVAL;
>>> +		goto mdev_err;
>> Missing nlmsg_free().
>>
>> Otherwise looks fine.
>>
>> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>
>>
>> -Siwei
>>> +	}
>>> +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
>>> +	if (!err)
>>> +		err = genlmsg_reply(msg, info);
>>> +
>>> +	put_device(dev);
>>> +	mutex_unlock(&vdpa_dev_mutex);
>>> +
>>> +	if (err)
>>> +		nlmsg_free(msg);
>>> +
>>> +	return err;
>>> +
>>> +mdev_err:
>>> +	put_device(dev);
>>> +dev_err:
>>> +	mutex_unlock(&vdpa_dev_mutex);
>>> +	return err;
>>> +}
>>> +
>>>    static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>    	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>>>    	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
>>> @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>    	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
>>>    	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
>>>    	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
>>> +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
>>>    };
>>>
>>>    static const struct genl_ops vdpa_nl_ops[] = {
>>> @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>>>    		.doit = vdpa_nl_cmd_dev_config_get_doit,
>>>    		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
>>>    	},
>>> +	{
>>> +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
>>> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>>> +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
>>> +		.flags = GENL_ADMIN_PERM,
>>> +	},
>>>    };
>>>
>>>    static struct genl_family vdpa_nl_family __ro_after_init = {
>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>> index 8943a209202e..48ed1fc00830 100644
>>> --- a/include/linux/vdpa.h
>>> +++ b/include/linux/vdpa.h
>>> @@ -276,6 +276,9 @@ struct vdpa_config_ops {
>>>    			    const struct vdpa_vq_state *state);
>>>    	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
>>>    			    struct vdpa_vq_state *state);
>>> +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
>>> +				   struct sk_buff *msg,
>>> +				   struct netlink_ext_ack *extack);
>>>    	struct vdpa_notification_area
>>>    	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
>>>    	/* vq irq is not expected to be changed once DRIVER_OK is set */
>>> @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
>>>    int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
>>>    void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
>>>
>>> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
>>> +
>>>    #endif /* _LINUX_VDPA_H */
>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>>> index 1061d8d2d09d..25c55cab3d7c 100644
>>> --- a/include/uapi/linux/vdpa.h
>>> +++ b/include/uapi/linux/vdpa.h
>>> @@ -18,6 +18,7 @@ enum vdpa_command {
>>>    	VDPA_CMD_DEV_DEL,
>>>    	VDPA_CMD_DEV_GET,		/* can dump */
>>>    	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
>>> +	VDPA_CMD_DEV_VSTATS_GET,
>>>    };
>>>
>>>    enum vdpa_attr {
>>> @@ -46,6 +47,11 @@ enum vdpa_attr {
>>>    	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>>>    	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>>>    	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
>>> +
>>> +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
>>> +
>>>    	/* new attributes must be added above here */
>>>    	VDPA_ATTR_MAX,
>>>    };

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

Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Si-Wei Liu]

On 5/2/2022 10:13 PM, Eli Cohen wrote:
>
>> -----Original Message-----
>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>> Sent: Tuesday, May 3, 2022 2:48 AM
>> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
>> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
>>
>>
>>
>> On 5/2/2022 3:22 AM, Eli Cohen wrote:
>>> Allows to read vendor statistics of a vdpa device. The specific
>>> statistics data are received from the upstream driver in the form of an
>>> (attribute name, attribute value) pairs.
>>>
>>> An example of statistics for mlx5_vdpa device are:
>>>
>>> received_desc - number of descriptors received by the virtqueue
>>> completed_desc - number of descriptors completed by the virtqueue
>>>
>>> A descriptor using indirect buffers is still counted as 1. In addition,
>>> N chained descriptors are counted correctly N times as one would expect.
>>>
>>> A new callback was added to vdpa_config_ops which provides the means for
>>> the vdpa driver to return statistics results.
>>>
>>> The interface allows for reading all the supported virtqueues, including
>>> the control virtqueue if it exists.
>>>
>>> Below are some examples taken from mlx5_vdpa which are introduced in the
>>> following patch:
>>>
>>> 1. Read statistics for the virtqueue at index 1
>>>
>>> $ vdpa dev vstats show vdpa-a qidx 1
>>> vdpa-a:
>>> queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
>>>
>>> 2. Read statistics for the virtqueue at index 32
>>> $ vdpa dev vstats show vdpa-a qidx 32
>>> vdpa-a:
>>> queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
>>>
>>> 3. Read statisitics for the virtqueue at index 0 with json output
>>> $ vdpa -j dev vstats show vdpa-a qidx 0
>>> {"vstats":{"vdpa-a":{
>>> "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
>>>    "name":"completed_desc","value":417548}}}
>>>
>>> 4. Read statistics for the virtqueue at index 0 with preety json output
>>> $ vdpa -jp dev vstats show vdpa-a qidx 0
>>> {
>>>       "vstats": {
>>>           "vdpa-a": {
>>>
>>>               "queue_type": "rx",
>>>               "queue_index": 0,
>>>               "name": "received_desc",
>>>               "value": 417776,
>>>               "name": "completed_desc",
>>>               "value": 417548
>>>           }
>>>       }
>>> }
>>>
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> ---
>>>    drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
>>>    include/linux/vdpa.h      |   5 ++
>>>    include/uapi/linux/vdpa.h |   6 ++
>>>    3 files changed, 140 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 2b75c00b1005..933466f61ca8 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>>>    	return err;
>>>    }
>>>
>>> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
>>> +			       struct genl_info *info, u32 index)
>>> +{
>>> +	int err;
>>> +
>>> +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
>>> +		return -EMSGSIZE;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
>>> +			     struct genl_info *info, u32 index)
>>> +{
>>> +	int err;
>>> +
>>> +	if (!vdev->config->get_vendor_vq_stats)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
>>> +				      struct sk_buff *msg,
>>> +				      struct genl_info *info, u32 index)
>>> +{
>>> +	u32 device_id;
>>> +	void *hdr;
>>> +	int err;
>>> +	u32 portid = info->snd_portid;
>>> +	u32 seq = info->snd_seq;
>>> +	u32 flags = 0;
>>> +
>>> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>> +			  VDPA_CMD_DEV_VSTATS_GET);
>>> +	if (!hdr)
>>> +		return -EMSGSIZE;
>>> +
>>> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
>>> +		err = -EMSGSIZE;
>>> +		goto undo_msg;
>>> +	}
>>> +
>>> +	device_id = vdev->config->get_device_id(vdev);
>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
>>> +		err = -EMSGSIZE;
>>> +		goto undo_msg;
>>> +	}
>>> +
>> You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
>> otherwise I can't image how you can ensure the atomicity to infer
>> queue_type for control vq.
>> And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
>> into vendor_stats_fill()?
> It is done in the hardware driver. In this case, in mlx5_vdpa.
OK... So you think this is not vdpa common code but rather individual 
vendor driver should deal with? Seems fine at the first glance, but with 
some thoughts this would complicate userspace code quite a lot to tell 
apart different cases - say if the VDPA_ATTR_DEV_NEGOTIATED_FEATURES 
attr is missing it would not be possible to display the queue type. On 
the other hand, the queue type itself shouldn't be vendor specific thing 
- only the vendor stats are, right?

Furthermore, without FEATURES_OK the stats returned for a specific queue 
might not be stable/reliable/reasonable at all, not sure how the tool 
can infer such complex state (e.g. negotiation is in progress) if 
somehow the vendor driver doesn't provide the corresponding attribute? 
Should vendor driver expect to fail with explicit message to indicate 
the reason, or it'd be fine to just display zero stats there? Looking at 
mlx5_vdpa implementation it seems to be the former case, but in case of 
device being negotiating, depending on which queue, the vstat query 
might end up with a confusing message of, either

"virtqueue index is not valid"

or,

"failed to query hardware"

but none is helpful for user to indicate what's going on... I wonder if 
mandating presence of FEATURES_OK would simplify userspace tool's 
implementation in this case?


Thanks,
-Siwei

>
>>> +	err = vendor_stats_fill(vdev, msg, info, index);
>>> +
>>> +	genlmsg_end(msg, hdr);
>>> +
>>> +	return err;
>>> +
>>> +undo_msg:
>>> +	genlmsg_cancel(msg, hdr);
>>> +	return err;
>>> +}
>>> +
>>>    static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
>>>    {
>>>    	struct vdpa_device *vdev;
>>> @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
>>>    	return msg->len;
>>>    }
>>>
>>> +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
>>> +					  struct genl_info *info)
>>> +{
>>> +	struct vdpa_device *vdev;
>>> +	struct sk_buff *msg;
>>> +	const char *devname;
>>> +	struct device *dev;
>>> +	u32 index;
>>> +	int err;
>>> +
>>> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
>>> +		return -EINVAL;
>>> +
>>> +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
>>> +		return -EINVAL;
>>> +
>>> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
>>> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>> +	if (!msg)
>>> +		return -ENOMEM;
>>> +
>>> +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
>>> +	mutex_lock(&vdpa_dev_mutex);
>> Given that stats_get() is a read-only operation that might happen quite
>> often from time to time, I wonder if it is now a good timing to convert
>> vdpa_dev_mutex to a semaphore?
>>
>>> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
>>> +	if (!dev) {
>>> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
>>> +		err = -ENODEV;
>>> +		goto dev_err;
>> Missing nlmsg_free().
>>> +	}
>>> +	vdev = container_of(dev, struct vdpa_device, dev);
>>> +	if (!vdev->mdev) {
>>> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
>>> +		err = -EINVAL;
>>> +		goto mdev_err;
>> Missing nlmsg_free().
>>
>> Otherwise looks fine.
>>
>> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>
>>
>> -Siwei
>>> +	}
>>> +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
>>> +	if (!err)
>>> +		err = genlmsg_reply(msg, info);
>>> +
>>> +	put_device(dev);
>>> +	mutex_unlock(&vdpa_dev_mutex);
>>> +
>>> +	if (err)
>>> +		nlmsg_free(msg);
>>> +
>>> +	return err;
>>> +
>>> +mdev_err:
>>> +	put_device(dev);
>>> +dev_err:
>>> +	mutex_unlock(&vdpa_dev_mutex);
>>> +	return err;
>>> +}
>>> +
>>>    static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>    	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>>>    	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
>>> @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>    	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
>>>    	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
>>>    	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
>>> +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
>>>    };
>>>
>>>    static const struct genl_ops vdpa_nl_ops[] = {
>>> @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>>>    		.doit = vdpa_nl_cmd_dev_config_get_doit,
>>>    		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
>>>    	},
>>> +	{
>>> +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
>>> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>>> +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
>>> +		.flags = GENL_ADMIN_PERM,
>>> +	},
>>>    };
>>>
>>>    static struct genl_family vdpa_nl_family __ro_after_init = {
>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>> index 8943a209202e..48ed1fc00830 100644
>>> --- a/include/linux/vdpa.h
>>> +++ b/include/linux/vdpa.h
>>> @@ -276,6 +276,9 @@ struct vdpa_config_ops {
>>>    			    const struct vdpa_vq_state *state);
>>>    	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
>>>    			    struct vdpa_vq_state *state);
>>> +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
>>> +				   struct sk_buff *msg,
>>> +				   struct netlink_ext_ack *extack);
>>>    	struct vdpa_notification_area
>>>    	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
>>>    	/* vq irq is not expected to be changed once DRIVER_OK is set */
>>> @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
>>>    int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
>>>    void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
>>>
>>> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
>>> +
>>>    #endif /* _LINUX_VDPA_H */
>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>>> index 1061d8d2d09d..25c55cab3d7c 100644
>>> --- a/include/uapi/linux/vdpa.h
>>> +++ b/include/uapi/linux/vdpa.h
>>> @@ -18,6 +18,7 @@ enum vdpa_command {
>>>    	VDPA_CMD_DEV_DEL,
>>>    	VDPA_CMD_DEV_GET,		/* can dump */
>>>    	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
>>> +	VDPA_CMD_DEV_VSTATS_GET,
>>>    };
>>>
>>>    enum vdpa_attr {
>>> @@ -46,6 +47,11 @@ enum vdpa_attr {
>>>    	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>>>    	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>>>    	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
>>> +
>>> +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
>>> +
>>>    	/* new attributes must be added above here */
>>>    	VDPA_ATTR_MAX,
>>>    };

RE: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Eli Cohen]

> -----Original Message-----
> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Wednesday, May 4, 2022 7:44 AM
> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
> 
> 
> 
> On 5/2/2022 10:13 PM, Eli Cohen wrote:
> >
> >> -----Original Message-----
> >> From: Si-Wei Liu <si-wei.liu@oracle.com>
> >> Sent: Tuesday, May 3, 2022 2:48 AM
> >> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
> >> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
> >>
> >>
> >>
> >> On 5/2/2022 3:22 AM, Eli Cohen wrote:
> >>> Allows to read vendor statistics of a vdpa device. The specific
> >>> statistics data are received from the upstream driver in the form of an
> >>> (attribute name, attribute value) pairs.
> >>>
> >>> An example of statistics for mlx5_vdpa device are:
> >>>
> >>> received_desc - number of descriptors received by the virtqueue
> >>> completed_desc - number of descriptors completed by the virtqueue
> >>>
> >>> A descriptor using indirect buffers is still counted as 1. In addition,
> >>> N chained descriptors are counted correctly N times as one would expect.
> >>>
> >>> A new callback was added to vdpa_config_ops which provides the means for
> >>> the vdpa driver to return statistics results.
> >>>
> >>> The interface allows for reading all the supported virtqueues, including
> >>> the control virtqueue if it exists.
> >>>
> >>> Below are some examples taken from mlx5_vdpa which are introduced in the
> >>> following patch:
> >>>
> >>> 1. Read statistics for the virtqueue at index 1
> >>>
> >>> $ vdpa dev vstats show vdpa-a qidx 1
> >>> vdpa-a:
> >>> queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
> >>>
> >>> 2. Read statistics for the virtqueue at index 32
> >>> $ vdpa dev vstats show vdpa-a qidx 32
> >>> vdpa-a:
> >>> queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
> >>>
> >>> 3. Read statisitics for the virtqueue at index 0 with json output
> >>> $ vdpa -j dev vstats show vdpa-a qidx 0
> >>> {"vstats":{"vdpa-a":{
> >>> "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
> >>>    "name":"completed_desc","value":417548}}}
> >>>
> >>> 4. Read statistics for the virtqueue at index 0 with preety json output
> >>> $ vdpa -jp dev vstats show vdpa-a qidx 0
> >>> {
> >>>       "vstats": {
> >>>           "vdpa-a": {
> >>>
> >>>               "queue_type": "rx",
> >>>               "queue_index": 0,
> >>>               "name": "received_desc",
> >>>               "value": 417776,
> >>>               "name": "completed_desc",
> >>>               "value": 417548
> >>>           }
> >>>       }
> >>> }
> >>>
> >>> Signed-off-by: Eli Cohen <elic@nvidia.com>
> >>> ---
> >>>    drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
> >>>    include/linux/vdpa.h      |   5 ++
> >>>    include/uapi/linux/vdpa.h |   6 ++
> >>>    3 files changed, 140 insertions(+)
> >>>
> >>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >>> index 2b75c00b1005..933466f61ca8 100644
> >>> --- a/drivers/vdpa/vdpa.c
> >>> +++ b/drivers/vdpa/vdpa.c
> >>> @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
> >>>    	return err;
> >>>    }
> >>>
> >>> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
> >>> +			       struct genl_info *info, u32 index)
> >>> +{
> >>> +	int err;
> >>> +
> >>> +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
> >>> +		return -EMSGSIZE;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
> >>> +			     struct genl_info *info, u32 index)
> >>> +{
> >>> +	int err;
> >>> +
> >>> +	if (!vdev->config->get_vendor_vq_stats)
> >>> +		return -EOPNOTSUPP;
> >>> +
> >>> +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
> >>> +				      struct sk_buff *msg,
> >>> +				      struct genl_info *info, u32 index)
> >>> +{
> >>> +	u32 device_id;
> >>> +	void *hdr;
> >>> +	int err;
> >>> +	u32 portid = info->snd_portid;
> >>> +	u32 seq = info->snd_seq;
> >>> +	u32 flags = 0;
> >>> +
> >>> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> >>> +			  VDPA_CMD_DEV_VSTATS_GET);
> >>> +	if (!hdr)
> >>> +		return -EMSGSIZE;
> >>> +
> >>> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
> >>> +		err = -EMSGSIZE;
> >>> +		goto undo_msg;
> >>> +	}
> >>> +
> >>> +	device_id = vdev->config->get_device_id(vdev);
> >>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
> >>> +		err = -EMSGSIZE;
> >>> +		goto undo_msg;
> >>> +	}
> >>> +
> >> You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
> >> otherwise I can't image how you can ensure the atomicity to infer
> >> queue_type for control vq.
> >> And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
> >> into vendor_stats_fill()?
> > It is done in the hardware driver. In this case, in mlx5_vdpa.
> OK... So you think this is not vdpa common code but rather individual
> vendor driver should deal with? Seems fine at the first glance, but with
> some thoughts this would complicate userspace code quite a lot to tell
> apart different cases - say if the VDPA_ATTR_DEV_NEGOTIATED_FEATURES
> attr is missing it would not be possible to display the queue type. On
> the other hand, the queue type itself shouldn't be vendor specific thing
> - only the vendor stats are, right?
> 
Right, although this feature is really about displaying statistics and queue type
is just supplemental information.

> Furthermore, without FEATURES_OK the stats returned for a specific queue
> might not be stable/reliable/reasonable at all, not sure how the tool
> can infer such complex state (e.g. negotiation is in progress) if
> somehow the vendor driver doesn't provide the corresponding attribute?
> Should vendor driver expect to fail with explicit message to indicate
> the reason, or it'd be fine to just display zero stats there? Looking at
> mlx5_vdpa implementation it seems to be the former case, but in case of
> device being negotiating, depending on which queue, the vstat query
> might end up with a confusing message of, either
> 
> "virtqueue index is not valid"
> 
> or,
> 
> "failed to query hardware"
> 
> but none is helpful for user to indicate what's going on... I wonder if
> mandating presence of FEATURES_OK would simplify userspace tool's
> implementation in this case?

When you say "mandating", do you refer to kernel? The userspace tool
can do that since it will have the negotiated features.

I am reluctant to splitting attributes insertion between hardware driver
and generic vdpa code. If this is vendor specific feature I think all attributes
should come from the vendor driver. But, I don't insist and can move to vdpa
generic code.

> 
> 
> Thanks,
> -Siwei
> 
> >
> >>> +	err = vendor_stats_fill(vdev, msg, info, index);
> >>> +
> >>> +	genlmsg_end(msg, hdr);
> >>> +
> >>> +	return err;
> >>> +
> >>> +undo_msg:
> >>> +	genlmsg_cancel(msg, hdr);
> >>> +	return err;
> >>> +}
> >>> +
> >>>    static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
> >>>    {
> >>>    	struct vdpa_device *vdev;
> >>> @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
> >>>    	return msg->len;
> >>>    }
> >>>
> >>> +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
> >>> +					  struct genl_info *info)
> >>> +{
> >>> +	struct vdpa_device *vdev;
> >>> +	struct sk_buff *msg;
> >>> +	const char *devname;
> >>> +	struct device *dev;
> >>> +	u32 index;
> >>> +	int err;
> >>> +
> >>> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
> >>> +		return -EINVAL;
> >>> +
> >>> +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
> >>> +		return -EINVAL;
> >>> +
> >>> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> >>> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >>> +	if (!msg)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
> >>> +	mutex_lock(&vdpa_dev_mutex);
> >> Given that stats_get() is a read-only operation that might happen quite
> >> often from time to time, I wonder if it is now a good timing to convert
> >> vdpa_dev_mutex to a semaphore?
> >>
> >>> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
> >>> +	if (!dev) {
> >>> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> >>> +		err = -ENODEV;
> >>> +		goto dev_err;
> >> Missing nlmsg_free().
> >>> +	}
> >>> +	vdev = container_of(dev, struct vdpa_device, dev);
> >>> +	if (!vdev->mdev) {
> >>> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
> >>> +		err = -EINVAL;
> >>> +		goto mdev_err;
> >> Missing nlmsg_free().
> >>
> >> Otherwise looks fine.
> >>
> >> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
> >>
> >>
> >> -Siwei
> >>> +	}
> >>> +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
> >>> +	if (!err)
> >>> +		err = genlmsg_reply(msg, info);
> >>> +
> >>> +	put_device(dev);
> >>> +	mutex_unlock(&vdpa_dev_mutex);
> >>> +
> >>> +	if (err)
> >>> +		nlmsg_free(msg);
> >>> +
> >>> +	return err;
> >>> +
> >>> +mdev_err:
> >>> +	put_device(dev);
> >>> +dev_err:
> >>> +	mutex_unlock(&vdpa_dev_mutex);
> >>> +	return err;
> >>> +}
> >>> +
> >>>    static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
> >>>    	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
> >>>    	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
> >>> @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
> >>>    	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
> >>>    	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
> >>>    	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
> >>> +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
> >>>    };
> >>>
> >>>    static const struct genl_ops vdpa_nl_ops[] = {
> >>> @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
> >>>    		.doit = vdpa_nl_cmd_dev_config_get_doit,
> >>>    		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
> >>>    	},
> >>> +	{
> >>> +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
> >>> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> >>> +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
> >>> +		.flags = GENL_ADMIN_PERM,
> >>> +	},
> >>>    };
> >>>
> >>>    static struct genl_family vdpa_nl_family __ro_after_init = {
> >>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> >>> index 8943a209202e..48ed1fc00830 100644
> >>> --- a/include/linux/vdpa.h
> >>> +++ b/include/linux/vdpa.h
> >>> @@ -276,6 +276,9 @@ struct vdpa_config_ops {
> >>>    			    const struct vdpa_vq_state *state);
> >>>    	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
> >>>    			    struct vdpa_vq_state *state);
> >>> +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
> >>> +				   struct sk_buff *msg,
> >>> +				   struct netlink_ext_ack *extack);
> >>>    	struct vdpa_notification_area
> >>>    	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
> >>>    	/* vq irq is not expected to be changed once DRIVER_OK is set */
> >>> @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
> >>>    int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
> >>>    void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
> >>>
> >>> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
> >>> +
> >>>    #endif /* _LINUX_VDPA_H */
> >>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> >>> index 1061d8d2d09d..25c55cab3d7c 100644
> >>> --- a/include/uapi/linux/vdpa.h
> >>> +++ b/include/uapi/linux/vdpa.h
> >>> @@ -18,6 +18,7 @@ enum vdpa_command {
> >>>    	VDPA_CMD_DEV_DEL,
> >>>    	VDPA_CMD_DEV_GET,		/* can dump */
> >>>    	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
> >>> +	VDPA_CMD_DEV_VSTATS_GET,
> >>>    };
> >>>
> >>>    enum vdpa_attr {
> >>> @@ -46,6 +47,11 @@ enum vdpa_attr {
> >>>    	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
> >>>    	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
> >>>    	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
> >>> +
> >>> +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
> >>> +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
> >>> +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
> >>> +
> >>>    	/* new attributes must be added above here */
> >>>    	VDPA_ATTR_MAX,
> >>>    };

Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Jason Wang]

在 2022/5/4 13:44, Eli Cohen 写道:
>
>> -----Original Message-----
>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>> Sent: Wednesday, May 4, 2022 7:44 AM
>> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
>> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
>>
>>
>>
>> On 5/2/2022 10:13 PM, Eli Cohen wrote:
>>>> -----Original Message-----
>>>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>>>> Sent: Tuesday, May 3, 2022 2:48 AM
>>>> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
>>>> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
>>>>
>>>>
>>>>
>>>> On 5/2/2022 3:22 AM, Eli Cohen wrote:
>>>>> Allows to read vendor statistics of a vdpa device. The specific
>>>>> statistics data are received from the upstream driver in the form of an
>>>>> (attribute name, attribute value) pairs.
>>>>>
>>>>> An example of statistics for mlx5_vdpa device are:
>>>>>
>>>>> received_desc - number of descriptors received by the virtqueue
>>>>> completed_desc - number of descriptors completed by the virtqueue
>>>>>
>>>>> A descriptor using indirect buffers is still counted as 1. In addition,
>>>>> N chained descriptors are counted correctly N times as one would expect.
>>>>>
>>>>> A new callback was added to vdpa_config_ops which provides the means for
>>>>> the vdpa driver to return statistics results.
>>>>>
>>>>> The interface allows for reading all the supported virtqueues, including
>>>>> the control virtqueue if it exists.
>>>>>
>>>>> Below are some examples taken from mlx5_vdpa which are introduced in the
>>>>> following patch:
>>>>>
>>>>> 1. Read statistics for the virtqueue at index 1
>>>>>
>>>>> $ vdpa dev vstats show vdpa-a qidx 1
>>>>> vdpa-a:
>>>>> queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
>>>>>
>>>>> 2. Read statistics for the virtqueue at index 32
>>>>> $ vdpa dev vstats show vdpa-a qidx 32
>>>>> vdpa-a:
>>>>> queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
>>>>>
>>>>> 3. Read statisitics for the virtqueue at index 0 with json output
>>>>> $ vdpa -j dev vstats show vdpa-a qidx 0
>>>>> {"vstats":{"vdpa-a":{
>>>>> "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
>>>>>     "name":"completed_desc","value":417548}}}
>>>>>
>>>>> 4. Read statistics for the virtqueue at index 0 with preety json output
>>>>> $ vdpa -jp dev vstats show vdpa-a qidx 0
>>>>> {
>>>>>        "vstats": {
>>>>>            "vdpa-a": {
>>>>>
>>>>>                "queue_type": "rx",
>>>>>                "queue_index": 0,
>>>>>                "name": "received_desc",
>>>>>                "value": 417776,
>>>>>                "name": "completed_desc",
>>>>>                "value": 417548
>>>>>            }
>>>>>        }
>>>>> }
>>>>>
>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>>> ---
>>>>>     drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
>>>>>     include/linux/vdpa.h      |   5 ++
>>>>>     include/uapi/linux/vdpa.h |   6 ++
>>>>>     3 files changed, 140 insertions(+)
>>>>>
>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>>> index 2b75c00b1005..933466f61ca8 100644
>>>>> --- a/drivers/vdpa/vdpa.c
>>>>> +++ b/drivers/vdpa/vdpa.c
>>>>> @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>>>>>     	return err;
>>>>>     }
>>>>>
>>>>> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
>>>>> +			       struct genl_info *info, u32 index)
>>>>> +{
>>>>> +	int err;
>>>>> +
>>>>> +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
>>>>> +	if (err)
>>>>> +		return err;
>>>>> +
>>>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
>>>>> +		return -EMSGSIZE;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
>>>>> +			     struct genl_info *info, u32 index)
>>>>> +{
>>>>> +	int err;
>>>>> +
>>>>> +	if (!vdev->config->get_vendor_vq_stats)
>>>>> +		return -EOPNOTSUPP;
>>>>> +
>>>>> +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
>>>>> +	if (err)
>>>>> +		return err;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
>>>>> +				      struct sk_buff *msg,
>>>>> +				      struct genl_info *info, u32 index)
>>>>> +{
>>>>> +	u32 device_id;
>>>>> +	void *hdr;
>>>>> +	int err;
>>>>> +	u32 portid = info->snd_portid;
>>>>> +	u32 seq = info->snd_seq;
>>>>> +	u32 flags = 0;
>>>>> +
>>>>> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>>>> +			  VDPA_CMD_DEV_VSTATS_GET);
>>>>> +	if (!hdr)
>>>>> +		return -EMSGSIZE;
>>>>> +
>>>>> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
>>>>> +		err = -EMSGSIZE;
>>>>> +		goto undo_msg;
>>>>> +	}
>>>>> +
>>>>> +	device_id = vdev->config->get_device_id(vdev);
>>>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
>>>>> +		err = -EMSGSIZE;
>>>>> +		goto undo_msg;
>>>>> +	}
>>>>> +
>>>> You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
>>>> otherwise I can't image how you can ensure the atomicity to infer
>>>> queue_type for control vq.
>>>> And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
>>>> into vendor_stats_fill()?
>>> It is done in the hardware driver. In this case, in mlx5_vdpa.
>> OK... So you think this is not vdpa common code but rather individual
>> vendor driver should deal with? Seems fine at the first glance, but with
>> some thoughts this would complicate userspace code quite a lot to tell
>> apart different cases - say if the VDPA_ATTR_DEV_NEGOTIATED_FEATURES
>> attr is missing it would not be possible to display the queue type. On
>> the other hand, the queue type itself shouldn't be vendor specific thing
>> - only the vendor stats are, right?
>>
> Right, although this feature is really about displaying statistics and queue type
> is just supplemental information.
>
>> Furthermore, without FEATURES_OK the stats returned for a specific queue
>> might not be stable/reliable/reasonable at all, not sure how the tool
>> can infer such complex state (e.g. negotiation is in progress) if
>> somehow the vendor driver doesn't provide the corresponding attribute?
>> Should vendor driver expect to fail with explicit message to indicate
>> the reason, or it'd be fine to just display zero stats there? Looking at
>> mlx5_vdpa implementation it seems to be the former case, but in case of
>> device being negotiating, depending on which queue, the vstat query
>> might end up with a confusing message of, either
>>
>> "virtqueue index is not valid"
>>
>> or,
>>
>> "failed to query hardware"
>>
>> but none is helpful for user to indicate what's going on... I wonder if
>> mandating presence of FEATURES_OK would simplify userspace tool's
>> implementation in this case?
> When you say "mandating", do you refer to kernel? The userspace tool
> can do that since it will have the negotiated features.


I think it might be helpful if the userspace code could be posted as well.

Thanks


>
> I am reluctant to splitting attributes insertion between hardware driver
> and generic vdpa code. If this is vendor specific feature I think all attributes
> should come from the vendor driver. But, I don't insist and can move to vdpa
> generic code.
>
>>
>> Thanks,
>> -Siwei
>>
>>>>> +	err = vendor_stats_fill(vdev, msg, info, index);
>>>>> +
>>>>> +	genlmsg_end(msg, hdr);
>>>>> +
>>>>> +	return err;
>>>>> +
>>>>> +undo_msg:
>>>>> +	genlmsg_cancel(msg, hdr);
>>>>> +	return err;
>>>>> +}
>>>>> +
>>>>>     static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
>>>>>     {
>>>>>     	struct vdpa_device *vdev;
>>>>> @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
>>>>>     	return msg->len;
>>>>>     }
>>>>>
>>>>> +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
>>>>> +					  struct genl_info *info)
>>>>> +{
>>>>> +	struct vdpa_device *vdev;
>>>>> +	struct sk_buff *msg;
>>>>> +	const char *devname;
>>>>> +	struct device *dev;
>>>>> +	u32 index;
>>>>> +	int err;
>>>>> +
>>>>> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
>>>>> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>>>> +	if (!msg)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
>>>>> +	mutex_lock(&vdpa_dev_mutex);
>>>> Given that stats_get() is a read-only operation that might happen quite
>>>> often from time to time, I wonder if it is now a good timing to convert
>>>> vdpa_dev_mutex to a semaphore?
>>>>
>>>>> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
>>>>> +	if (!dev) {
>>>>> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
>>>>> +		err = -ENODEV;
>>>>> +		goto dev_err;
>>>> Missing nlmsg_free().
>>>>> +	}
>>>>> +	vdev = container_of(dev, struct vdpa_device, dev);
>>>>> +	if (!vdev->mdev) {
>>>>> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
>>>>> +		err = -EINVAL;
>>>>> +		goto mdev_err;
>>>> Missing nlmsg_free().
>>>>
>>>> Otherwise looks fine.
>>>>
>>>> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>>>
>>>>
>>>> -Siwei
>>>>> +	}
>>>>> +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
>>>>> +	if (!err)
>>>>> +		err = genlmsg_reply(msg, info);
>>>>> +
>>>>> +	put_device(dev);
>>>>> +	mutex_unlock(&vdpa_dev_mutex);
>>>>> +
>>>>> +	if (err)
>>>>> +		nlmsg_free(msg);
>>>>> +
>>>>> +	return err;
>>>>> +
>>>>> +mdev_err:
>>>>> +	put_device(dev);
>>>>> +dev_err:
>>>>> +	mutex_unlock(&vdpa_dev_mutex);
>>>>> +	return err;
>>>>> +}
>>>>> +
>>>>>     static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>>>     	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>>>>>     	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
>>>>> @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>>>     	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
>>>>>     	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
>>>>>     	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
>>>>> +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
>>>>>     };
>>>>>
>>>>>     static const struct genl_ops vdpa_nl_ops[] = {
>>>>> @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>>>>>     		.doit = vdpa_nl_cmd_dev_config_get_doit,
>>>>>     		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
>>>>>     	},
>>>>> +	{
>>>>> +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
>>>>> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>>>>> +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
>>>>> +		.flags = GENL_ADMIN_PERM,
>>>>> +	},
>>>>>     };
>>>>>
>>>>>     static struct genl_family vdpa_nl_family __ro_after_init = {
>>>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>>>> index 8943a209202e..48ed1fc00830 100644
>>>>> --- a/include/linux/vdpa.h
>>>>> +++ b/include/linux/vdpa.h
>>>>> @@ -276,6 +276,9 @@ struct vdpa_config_ops {
>>>>>     			    const struct vdpa_vq_state *state);
>>>>>     	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
>>>>>     			    struct vdpa_vq_state *state);
>>>>> +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
>>>>> +				   struct sk_buff *msg,
>>>>> +				   struct netlink_ext_ack *extack);
>>>>>     	struct vdpa_notification_area
>>>>>     	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
>>>>>     	/* vq irq is not expected to be changed once DRIVER_OK is set */
>>>>> @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
>>>>>     int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
>>>>>     void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
>>>>>
>>>>> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
>>>>> +
>>>>>     #endif /* _LINUX_VDPA_H */
>>>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>>>>> index 1061d8d2d09d..25c55cab3d7c 100644
>>>>> --- a/include/uapi/linux/vdpa.h
>>>>> +++ b/include/uapi/linux/vdpa.h
>>>>> @@ -18,6 +18,7 @@ enum vdpa_command {
>>>>>     	VDPA_CMD_DEV_DEL,
>>>>>     	VDPA_CMD_DEV_GET,		/* can dump */
>>>>>     	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
>>>>> +	VDPA_CMD_DEV_VSTATS_GET,
>>>>>     };
>>>>>
>>>>>     enum vdpa_attr {
>>>>> @@ -46,6 +47,11 @@ enum vdpa_attr {
>>>>>     	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>>>>>     	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>>>>>     	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
>>>>> +
>>>>> +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>>>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
>>>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
>>>>> +
>>>>>     	/* new attributes must be added above here */
>>>>>     	VDPA_ATTR_MAX,
>>>>>     };

Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Jason Wang]

在 2022/5/4 13:44, Eli Cohen 写道:
>
>> -----Original Message-----
>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>> Sent: Wednesday, May 4, 2022 7:44 AM
>> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
>> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
>>
>>
>>
>> On 5/2/2022 10:13 PM, Eli Cohen wrote:
>>>> -----Original Message-----
>>>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>>>> Sent: Tuesday, May 3, 2022 2:48 AM
>>>> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
>>>> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
>>>>
>>>>
>>>>
>>>> On 5/2/2022 3:22 AM, Eli Cohen wrote:
>>>>> Allows to read vendor statistics of a vdpa device. The specific
>>>>> statistics data are received from the upstream driver in the form of an
>>>>> (attribute name, attribute value) pairs.
>>>>>
>>>>> An example of statistics for mlx5_vdpa device are:
>>>>>
>>>>> received_desc - number of descriptors received by the virtqueue
>>>>> completed_desc - number of descriptors completed by the virtqueue
>>>>>
>>>>> A descriptor using indirect buffers is still counted as 1. In addition,
>>>>> N chained descriptors are counted correctly N times as one would expect.
>>>>>
>>>>> A new callback was added to vdpa_config_ops which provides the means for
>>>>> the vdpa driver to return statistics results.
>>>>>
>>>>> The interface allows for reading all the supported virtqueues, including
>>>>> the control virtqueue if it exists.
>>>>>
>>>>> Below are some examples taken from mlx5_vdpa which are introduced in the
>>>>> following patch:
>>>>>
>>>>> 1. Read statistics for the virtqueue at index 1
>>>>>
>>>>> $ vdpa dev vstats show vdpa-a qidx 1
>>>>> vdpa-a:
>>>>> queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
>>>>>
>>>>> 2. Read statistics for the virtqueue at index 32
>>>>> $ vdpa dev vstats show vdpa-a qidx 32
>>>>> vdpa-a:
>>>>> queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
>>>>>
>>>>> 3. Read statisitics for the virtqueue at index 0 with json output
>>>>> $ vdpa -j dev vstats show vdpa-a qidx 0
>>>>> {"vstats":{"vdpa-a":{
>>>>> "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
>>>>>     "name":"completed_desc","value":417548}}}
>>>>>
>>>>> 4. Read statistics for the virtqueue at index 0 with preety json output
>>>>> $ vdpa -jp dev vstats show vdpa-a qidx 0
>>>>> {
>>>>>        "vstats": {
>>>>>            "vdpa-a": {
>>>>>
>>>>>                "queue_type": "rx",
>>>>>                "queue_index": 0,
>>>>>                "name": "received_desc",
>>>>>                "value": 417776,
>>>>>                "name": "completed_desc",
>>>>>                "value": 417548
>>>>>            }
>>>>>        }
>>>>> }
>>>>>
>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>>> ---
>>>>>     drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
>>>>>     include/linux/vdpa.h      |   5 ++
>>>>>     include/uapi/linux/vdpa.h |   6 ++
>>>>>     3 files changed, 140 insertions(+)
>>>>>
>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>>> index 2b75c00b1005..933466f61ca8 100644
>>>>> --- a/drivers/vdpa/vdpa.c
>>>>> +++ b/drivers/vdpa/vdpa.c
>>>>> @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>>>>>     	return err;
>>>>>     }
>>>>>
>>>>> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
>>>>> +			       struct genl_info *info, u32 index)
>>>>> +{
>>>>> +	int err;
>>>>> +
>>>>> +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
>>>>> +	if (err)
>>>>> +		return err;
>>>>> +
>>>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
>>>>> +		return -EMSGSIZE;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
>>>>> +			     struct genl_info *info, u32 index)
>>>>> +{
>>>>> +	int err;
>>>>> +
>>>>> +	if (!vdev->config->get_vendor_vq_stats)
>>>>> +		return -EOPNOTSUPP;
>>>>> +
>>>>> +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
>>>>> +	if (err)
>>>>> +		return err;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
>>>>> +				      struct sk_buff *msg,
>>>>> +				      struct genl_info *info, u32 index)
>>>>> +{
>>>>> +	u32 device_id;
>>>>> +	void *hdr;
>>>>> +	int err;
>>>>> +	u32 portid = info->snd_portid;
>>>>> +	u32 seq = info->snd_seq;
>>>>> +	u32 flags = 0;
>>>>> +
>>>>> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>>>> +			  VDPA_CMD_DEV_VSTATS_GET);
>>>>> +	if (!hdr)
>>>>> +		return -EMSGSIZE;
>>>>> +
>>>>> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
>>>>> +		err = -EMSGSIZE;
>>>>> +		goto undo_msg;
>>>>> +	}
>>>>> +
>>>>> +	device_id = vdev->config->get_device_id(vdev);
>>>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
>>>>> +		err = -EMSGSIZE;
>>>>> +		goto undo_msg;
>>>>> +	}
>>>>> +
>>>> You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
>>>> otherwise I can't image how you can ensure the atomicity to infer
>>>> queue_type for control vq.
>>>> And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
>>>> into vendor_stats_fill()?
>>> It is done in the hardware driver. In this case, in mlx5_vdpa.
>> OK... So you think this is not vdpa common code but rather individual
>> vendor driver should deal with? Seems fine at the first glance, but with
>> some thoughts this would complicate userspace code quite a lot to tell
>> apart different cases - say if the VDPA_ATTR_DEV_NEGOTIATED_FEATURES
>> attr is missing it would not be possible to display the queue type. On
>> the other hand, the queue type itself shouldn't be vendor specific thing
>> - only the vendor stats are, right?
>>
> Right, although this feature is really about displaying statistics and queue type
> is just supplemental information.
>
>> Furthermore, without FEATURES_OK the stats returned for a specific queue
>> might not be stable/reliable/reasonable at all, not sure how the tool
>> can infer such complex state (e.g. negotiation is in progress) if
>> somehow the vendor driver doesn't provide the corresponding attribute?
>> Should vendor driver expect to fail with explicit message to indicate
>> the reason, or it'd be fine to just display zero stats there? Looking at
>> mlx5_vdpa implementation it seems to be the former case, but in case of
>> device being negotiating, depending on which queue, the vstat query
>> might end up with a confusing message of, either
>>
>> "virtqueue index is not valid"
>>
>> or,
>>
>> "failed to query hardware"
>>
>> but none is helpful for user to indicate what's going on... I wonder if
>> mandating presence of FEATURES_OK would simplify userspace tool's
>> implementation in this case?
> When you say "mandating", do you refer to kernel? The userspace tool
> can do that since it will have the negotiated features.


I think it might be helpful if the userspace code could be posted as well.

Thanks


>
> I am reluctant to splitting attributes insertion between hardware driver
> and generic vdpa code. If this is vendor specific feature I think all attributes
> should come from the vendor driver. But, I don't insist and can move to vdpa
> generic code.
>
>>
>> Thanks,
>> -Siwei
>>
>>>>> +	err = vendor_stats_fill(vdev, msg, info, index);
>>>>> +
>>>>> +	genlmsg_end(msg, hdr);
>>>>> +
>>>>> +	return err;
>>>>> +
>>>>> +undo_msg:
>>>>> +	genlmsg_cancel(msg, hdr);
>>>>> +	return err;
>>>>> +}
>>>>> +
>>>>>     static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
>>>>>     {
>>>>>     	struct vdpa_device *vdev;
>>>>> @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
>>>>>     	return msg->len;
>>>>>     }
>>>>>
>>>>> +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
>>>>> +					  struct genl_info *info)
>>>>> +{
>>>>> +	struct vdpa_device *vdev;
>>>>> +	struct sk_buff *msg;
>>>>> +	const char *devname;
>>>>> +	struct device *dev;
>>>>> +	u32 index;
>>>>> +	int err;
>>>>> +
>>>>> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
>>>>> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>>>> +	if (!msg)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
>>>>> +	mutex_lock(&vdpa_dev_mutex);
>>>> Given that stats_get() is a read-only operation that might happen quite
>>>> often from time to time, I wonder if it is now a good timing to convert
>>>> vdpa_dev_mutex to a semaphore?
>>>>
>>>>> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
>>>>> +	if (!dev) {
>>>>> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
>>>>> +		err = -ENODEV;
>>>>> +		goto dev_err;
>>>> Missing nlmsg_free().
>>>>> +	}
>>>>> +	vdev = container_of(dev, struct vdpa_device, dev);
>>>>> +	if (!vdev->mdev) {
>>>>> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
>>>>> +		err = -EINVAL;
>>>>> +		goto mdev_err;
>>>> Missing nlmsg_free().
>>>>
>>>> Otherwise looks fine.
>>>>
>>>> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>>>
>>>>
>>>> -Siwei
>>>>> +	}
>>>>> +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
>>>>> +	if (!err)
>>>>> +		err = genlmsg_reply(msg, info);
>>>>> +
>>>>> +	put_device(dev);
>>>>> +	mutex_unlock(&vdpa_dev_mutex);
>>>>> +
>>>>> +	if (err)
>>>>> +		nlmsg_free(msg);
>>>>> +
>>>>> +	return err;
>>>>> +
>>>>> +mdev_err:
>>>>> +	put_device(dev);
>>>>> +dev_err:
>>>>> +	mutex_unlock(&vdpa_dev_mutex);
>>>>> +	return err;
>>>>> +}
>>>>> +
>>>>>     static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>>>     	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>>>>>     	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
>>>>> @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>>>     	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
>>>>>     	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
>>>>>     	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
>>>>> +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
>>>>>     };
>>>>>
>>>>>     static const struct genl_ops vdpa_nl_ops[] = {
>>>>> @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>>>>>     		.doit = vdpa_nl_cmd_dev_config_get_doit,
>>>>>     		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
>>>>>     	},
>>>>> +	{
>>>>> +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
>>>>> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>>>>> +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
>>>>> +		.flags = GENL_ADMIN_PERM,
>>>>> +	},
>>>>>     };
>>>>>
>>>>>     static struct genl_family vdpa_nl_family __ro_after_init = {
>>>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>>>> index 8943a209202e..48ed1fc00830 100644
>>>>> --- a/include/linux/vdpa.h
>>>>> +++ b/include/linux/vdpa.h
>>>>> @@ -276,6 +276,9 @@ struct vdpa_config_ops {
>>>>>     			    const struct vdpa_vq_state *state);
>>>>>     	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
>>>>>     			    struct vdpa_vq_state *state);
>>>>> +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
>>>>> +				   struct sk_buff *msg,
>>>>> +				   struct netlink_ext_ack *extack);
>>>>>     	struct vdpa_notification_area
>>>>>     	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
>>>>>     	/* vq irq is not expected to be changed once DRIVER_OK is set */
>>>>> @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
>>>>>     int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
>>>>>     void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
>>>>>
>>>>> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
>>>>> +
>>>>>     #endif /* _LINUX_VDPA_H */
>>>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>>>>> index 1061d8d2d09d..25c55cab3d7c 100644
>>>>> --- a/include/uapi/linux/vdpa.h
>>>>> +++ b/include/uapi/linux/vdpa.h
>>>>> @@ -18,6 +18,7 @@ enum vdpa_command {
>>>>>     	VDPA_CMD_DEV_DEL,
>>>>>     	VDPA_CMD_DEV_GET,		/* can dump */
>>>>>     	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
>>>>> +	VDPA_CMD_DEV_VSTATS_GET,
>>>>>     };
>>>>>
>>>>>     enum vdpa_attr {
>>>>> @@ -46,6 +47,11 @@ enum vdpa_attr {
>>>>>     	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>>>>>     	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>>>>>     	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
>>>>> +
>>>>> +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>>>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
>>>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
>>>>> +
>>>>>     	/* new attributes must be added above here */
>>>>>     	VDPA_ATTR_MAX,
>>>>>     };

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

Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Si-Wei Liu]

On 5/3/2022 10:44 PM, Eli Cohen wrote:
>
>> -----Original Message-----
>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>> Sent: Wednesday, May 4, 2022 7:44 AM
>> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
>> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
>>
>>
>>
>> On 5/2/2022 10:13 PM, Eli Cohen wrote:
>>>> -----Original Message-----
>>>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>>>> Sent: Tuesday, May 3, 2022 2:48 AM
>>>> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
>>>> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
>>>>
>>>>
>>>>
>>>> On 5/2/2022 3:22 AM, Eli Cohen wrote:
>>>>> Allows to read vendor statistics of a vdpa device. The specific
>>>>> statistics data are received from the upstream driver in the form of an
>>>>> (attribute name, attribute value) pairs.
>>>>>
>>>>> An example of statistics for mlx5_vdpa device are:
>>>>>
>>>>> received_desc - number of descriptors received by the virtqueue
>>>>> completed_desc - number of descriptors completed by the virtqueue
>>>>>
>>>>> A descriptor using indirect buffers is still counted as 1. In addition,
>>>>> N chained descriptors are counted correctly N times as one would expect.
>>>>>
>>>>> A new callback was added to vdpa_config_ops which provides the means for
>>>>> the vdpa driver to return statistics results.
>>>>>
>>>>> The interface allows for reading all the supported virtqueues, including
>>>>> the control virtqueue if it exists.
>>>>>
>>>>> Below are some examples taken from mlx5_vdpa which are introduced in the
>>>>> following patch:
>>>>>
>>>>> 1. Read statistics for the virtqueue at index 1
>>>>>
>>>>> $ vdpa dev vstats show vdpa-a qidx 1
>>>>> vdpa-a:
>>>>> queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
>>>>>
>>>>> 2. Read statistics for the virtqueue at index 32
>>>>> $ vdpa dev vstats show vdpa-a qidx 32
>>>>> vdpa-a:
>>>>> queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
>>>>>
>>>>> 3. Read statisitics for the virtqueue at index 0 with json output
>>>>> $ vdpa -j dev vstats show vdpa-a qidx 0
>>>>> {"vstats":{"vdpa-a":{
>>>>> "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
>>>>>     "name":"completed_desc","value":417548}}}
>>>>>
>>>>> 4. Read statistics for the virtqueue at index 0 with preety json output
>>>>> $ vdpa -jp dev vstats show vdpa-a qidx 0
>>>>> {
>>>>>        "vstats": {
>>>>>            "vdpa-a": {
>>>>>
>>>>>                "queue_type": "rx",
>>>>>                "queue_index": 0,
>>>>>                "name": "received_desc",
>>>>>                "value": 417776,
>>>>>                "name": "completed_desc",
>>>>>                "value": 417548
>>>>>            }
>>>>>        }
>>>>> }
>>>>>
>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>>> ---
>>>>>     drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
>>>>>     include/linux/vdpa.h      |   5 ++
>>>>>     include/uapi/linux/vdpa.h |   6 ++
>>>>>     3 files changed, 140 insertions(+)
>>>>>
>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>>> index 2b75c00b1005..933466f61ca8 100644
>>>>> --- a/drivers/vdpa/vdpa.c
>>>>> +++ b/drivers/vdpa/vdpa.c
>>>>> @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>>>>>     	return err;
>>>>>     }
>>>>>
>>>>> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
>>>>> +			       struct genl_info *info, u32 index)
>>>>> +{
>>>>> +	int err;
>>>>> +
>>>>> +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
>>>>> +	if (err)
>>>>> +		return err;
>>>>> +
>>>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
>>>>> +		return -EMSGSIZE;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
>>>>> +			     struct genl_info *info, u32 index)
>>>>> +{
>>>>> +	int err;
>>>>> +
>>>>> +	if (!vdev->config->get_vendor_vq_stats)
>>>>> +		return -EOPNOTSUPP;
>>>>> +
>>>>> +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
>>>>> +	if (err)
>>>>> +		return err;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
>>>>> +				      struct sk_buff *msg,
>>>>> +				      struct genl_info *info, u32 index)
>>>>> +{
>>>>> +	u32 device_id;
>>>>> +	void *hdr;
>>>>> +	int err;
>>>>> +	u32 portid = info->snd_portid;
>>>>> +	u32 seq = info->snd_seq;
>>>>> +	u32 flags = 0;
>>>>> +
>>>>> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>>>> +			  VDPA_CMD_DEV_VSTATS_GET);
>>>>> +	if (!hdr)
>>>>> +		return -EMSGSIZE;
>>>>> +
>>>>> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
>>>>> +		err = -EMSGSIZE;
>>>>> +		goto undo_msg;
>>>>> +	}
>>>>> +
>>>>> +	device_id = vdev->config->get_device_id(vdev);
>>>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
>>>>> +		err = -EMSGSIZE;
>>>>> +		goto undo_msg;
>>>>> +	}
>>>>> +
>>>> You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
>>>> otherwise I can't image how you can ensure the atomicity to infer
>>>> queue_type for control vq.
>>>> And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
>>>> into vendor_stats_fill()?
>>> It is done in the hardware driver. In this case, in mlx5_vdpa.
>> OK... So you think this is not vdpa common code but rather individual
>> vendor driver should deal with? Seems fine at the first glance, but with
>> some thoughts this would complicate userspace code quite a lot to tell
>> apart different cases - say if the VDPA_ATTR_DEV_NEGOTIATED_FEATURES
>> attr is missing it would not be possible to display the queue type. On
>> the other hand, the queue type itself shouldn't be vendor specific thing
>> - only the vendor stats are, right?
>>
> Right, although this feature is really about displaying statistics and queue type
> is just supplemental information.
Well, in the userspace implementation perspective it might be so, but 
thinking it from user's point of view it might not be easy to infer the 
type just from queue index.

>
>> Furthermore, without FEATURES_OK the stats returned for a specific queue
>> might not be stable/reliable/reasonable at all, not sure how the tool
>> can infer such complex state (e.g. negotiation is in progress) if
>> somehow the vendor driver doesn't provide the corresponding attribute?
>> Should vendor driver expect to fail with explicit message to indicate
>> the reason, or it'd be fine to just display zero stats there? Looking at
>> mlx5_vdpa implementation it seems to be the former case, but in case of
>> device being negotiating, depending on which queue, the vstat query
>> might end up with a confusing message of, either
>>
>> "virtqueue index is not valid"
>>
>> or,
>>
>> "failed to query hardware"
>>
>> but none is helpful for user to indicate what's going on... I wonder if
>> mandating presence of FEATURES_OK would simplify userspace tool's
>> implementation in this case?
> When you say "mandating", do you refer to kernel?
Yep. Either the vendor driver checks the FEATURES_OK status alone and 
rejects incoming request when negotiation is still in progress, or the 
vdpa core could double check on that. Or else the user would get to 
either of the above messages, which is kinda misleading to users.

>   The userspace tool
> can do that since it will have the negotiated features.
Are you suggesting if the userspace tool doesn't see the negotiated 
features attribute but the get_vendor_vq_stats() call was successful, it 
would simply assume no FEATURES_OK is set as yet?

For now I don't see the need of per-queue stat query before device and 
driver get done on feature negotiation. It would be simpler for the 
userspace tool to assume valid per-queue stats are available only until 
feature negotiation is done: the kernel could just reject the 
.get_vendor_vq_stats() call if not seeing FEATURES_OK. We can further 
extend by introducing new attribute to it whenever there's future need. 
That way you don't need to add a lot of future proof code to the 
userspace tool and try to cover all of the cases in the first place.

> I am reluctant to splitting attributes insertion between hardware driver
> and generic vdpa code. If this is vendor specific feature I think all attributes
> should come from the vendor driver. But, I don't insist and can move to vdpa
> generic code.
It's fine to insert all attributes in the driver, but please try to come 
up with sort of common abstraction/interface for userspace to digest. 
And hopefully it would simplify tool's implementation rather than 
complicate things up.

BTW, please pay attention to the comment I made for vdpa_dev_mutex 
below. I'd rather get the semaphore conversion done earlier than later, 
or there'll be side issue popped up with regard to locking and atomicity.

> Given that stats_get() is a read-only operation that might happen quite
> often from time to time, I wonder if it is now a good timing to convert
> vdpa_dev_mutex to a semaphore?
>

Thanks,
-Siwei
>
>>
>> Thanks,
>> -Siwei
>>
>>>>> +	err = vendor_stats_fill(vdev, msg, info, index);
>>>>> +
>>>>> +	genlmsg_end(msg, hdr);
>>>>> +
>>>>> +	return err;
>>>>> +
>>>>> +undo_msg:
>>>>> +	genlmsg_cancel(msg, hdr);
>>>>> +	return err;
>>>>> +}
>>>>> +
>>>>>     static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
>>>>>     {
>>>>>     	struct vdpa_device *vdev;
>>>>> @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
>>>>>     	return msg->len;
>>>>>     }
>>>>>
>>>>> +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
>>>>> +					  struct genl_info *info)
>>>>> +{
>>>>> +	struct vdpa_device *vdev;
>>>>> +	struct sk_buff *msg;
>>>>> +	const char *devname;
>>>>> +	struct device *dev;
>>>>> +	u32 index;
>>>>> +	int err;
>>>>> +
>>>>> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
>>>>> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>>>> +	if (!msg)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
>>>>> +	mutex_lock(&vdpa_dev_mutex);
>>>> Given that stats_get() is a read-only operation that might happen quite
>>>> often from time to time, I wonder if it is now a good timing to convert
>>>> vdpa_dev_mutex to a semaphore?
>>>>
>>>>> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
>>>>> +	if (!dev) {
>>>>> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
>>>>> +		err = -ENODEV;
>>>>> +		goto dev_err;
>>>> Missing nlmsg_free().
>>>>> +	}
>>>>> +	vdev = container_of(dev, struct vdpa_device, dev);
>>>>> +	if (!vdev->mdev) {
>>>>> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
>>>>> +		err = -EINVAL;
>>>>> +		goto mdev_err;
>>>> Missing nlmsg_free().
>>>>
>>>> Otherwise looks fine.
>>>>
>>>> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>>>
>>>>
>>>> -Siwei
>>>>> +	}
>>>>> +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
>>>>> +	if (!err)
>>>>> +		err = genlmsg_reply(msg, info);
>>>>> +
>>>>> +	put_device(dev);
>>>>> +	mutex_unlock(&vdpa_dev_mutex);
>>>>> +
>>>>> +	if (err)
>>>>> +		nlmsg_free(msg);
>>>>> +
>>>>> +	return err;
>>>>> +
>>>>> +mdev_err:
>>>>> +	put_device(dev);
>>>>> +dev_err:
>>>>> +	mutex_unlock(&vdpa_dev_mutex);
>>>>> +	return err;
>>>>> +}
>>>>> +
>>>>>     static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>>>     	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>>>>>     	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
>>>>> @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>>>     	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
>>>>>     	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
>>>>>     	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
>>>>> +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
>>>>>     };
>>>>>
>>>>>     static const struct genl_ops vdpa_nl_ops[] = {
>>>>> @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>>>>>     		.doit = vdpa_nl_cmd_dev_config_get_doit,
>>>>>     		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
>>>>>     	},
>>>>> +	{
>>>>> +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
>>>>> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>>>>> +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
>>>>> +		.flags = GENL_ADMIN_PERM,
>>>>> +	},
>>>>>     };
>>>>>
>>>>>     static struct genl_family vdpa_nl_family __ro_after_init = {
>>>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>>>> index 8943a209202e..48ed1fc00830 100644
>>>>> --- a/include/linux/vdpa.h
>>>>> +++ b/include/linux/vdpa.h
>>>>> @@ -276,6 +276,9 @@ struct vdpa_config_ops {
>>>>>     			    const struct vdpa_vq_state *state);
>>>>>     	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
>>>>>     			    struct vdpa_vq_state *state);
>>>>> +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
>>>>> +				   struct sk_buff *msg,
>>>>> +				   struct netlink_ext_ack *extack);
>>>>>     	struct vdpa_notification_area
>>>>>     	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
>>>>>     	/* vq irq is not expected to be changed once DRIVER_OK is set */
>>>>> @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
>>>>>     int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
>>>>>     void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
>>>>>
>>>>> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
>>>>> +
>>>>>     #endif /* _LINUX_VDPA_H */
>>>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>>>>> index 1061d8d2d09d..25c55cab3d7c 100644
>>>>> --- a/include/uapi/linux/vdpa.h
>>>>> +++ b/include/uapi/linux/vdpa.h
>>>>> @@ -18,6 +18,7 @@ enum vdpa_command {
>>>>>     	VDPA_CMD_DEV_DEL,
>>>>>     	VDPA_CMD_DEV_GET,		/* can dump */
>>>>>     	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
>>>>> +	VDPA_CMD_DEV_VSTATS_GET,
>>>>>     };
>>>>>
>>>>>     enum vdpa_attr {
>>>>> @@ -46,6 +47,11 @@ enum vdpa_attr {
>>>>>     	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>>>>>     	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>>>>>     	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
>>>>> +
>>>>> +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>>>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
>>>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
>>>>> +
>>>>>     	/* new attributes must be added above here */
>>>>>     	VDPA_ATTR_MAX,
>>>>>     };

Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Si-Wei Liu]

On 5/3/2022 10:44 PM, Eli Cohen wrote:
>
>> -----Original Message-----
>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>> Sent: Wednesday, May 4, 2022 7:44 AM
>> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
>> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
>>
>>
>>
>> On 5/2/2022 10:13 PM, Eli Cohen wrote:
>>>> -----Original Message-----
>>>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>>>> Sent: Tuesday, May 3, 2022 2:48 AM
>>>> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
>>>> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
>>>>
>>>>
>>>>
>>>> On 5/2/2022 3:22 AM, Eli Cohen wrote:
>>>>> Allows to read vendor statistics of a vdpa device. The specific
>>>>> statistics data are received from the upstream driver in the form of an
>>>>> (attribute name, attribute value) pairs.
>>>>>
>>>>> An example of statistics for mlx5_vdpa device are:
>>>>>
>>>>> received_desc - number of descriptors received by the virtqueue
>>>>> completed_desc - number of descriptors completed by the virtqueue
>>>>>
>>>>> A descriptor using indirect buffers is still counted as 1. In addition,
>>>>> N chained descriptors are counted correctly N times as one would expect.
>>>>>
>>>>> A new callback was added to vdpa_config_ops which provides the means for
>>>>> the vdpa driver to return statistics results.
>>>>>
>>>>> The interface allows for reading all the supported virtqueues, including
>>>>> the control virtqueue if it exists.
>>>>>
>>>>> Below are some examples taken from mlx5_vdpa which are introduced in the
>>>>> following patch:
>>>>>
>>>>> 1. Read statistics for the virtqueue at index 1
>>>>>
>>>>> $ vdpa dev vstats show vdpa-a qidx 1
>>>>> vdpa-a:
>>>>> queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
>>>>>
>>>>> 2. Read statistics for the virtqueue at index 32
>>>>> $ vdpa dev vstats show vdpa-a qidx 32
>>>>> vdpa-a:
>>>>> queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
>>>>>
>>>>> 3. Read statisitics for the virtqueue at index 0 with json output
>>>>> $ vdpa -j dev vstats show vdpa-a qidx 0
>>>>> {"vstats":{"vdpa-a":{
>>>>> "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
>>>>>     "name":"completed_desc","value":417548}}}
>>>>>
>>>>> 4. Read statistics for the virtqueue at index 0 with preety json output
>>>>> $ vdpa -jp dev vstats show vdpa-a qidx 0
>>>>> {
>>>>>        "vstats": {
>>>>>            "vdpa-a": {
>>>>>
>>>>>                "queue_type": "rx",
>>>>>                "queue_index": 0,
>>>>>                "name": "received_desc",
>>>>>                "value": 417776,
>>>>>                "name": "completed_desc",
>>>>>                "value": 417548
>>>>>            }
>>>>>        }
>>>>> }
>>>>>
>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>>> ---
>>>>>     drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
>>>>>     include/linux/vdpa.h      |   5 ++
>>>>>     include/uapi/linux/vdpa.h |   6 ++
>>>>>     3 files changed, 140 insertions(+)
>>>>>
>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>>> index 2b75c00b1005..933466f61ca8 100644
>>>>> --- a/drivers/vdpa/vdpa.c
>>>>> +++ b/drivers/vdpa/vdpa.c
>>>>> @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>>>>>     	return err;
>>>>>     }
>>>>>
>>>>> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
>>>>> +			       struct genl_info *info, u32 index)
>>>>> +{
>>>>> +	int err;
>>>>> +
>>>>> +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
>>>>> +	if (err)
>>>>> +		return err;
>>>>> +
>>>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
>>>>> +		return -EMSGSIZE;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
>>>>> +			     struct genl_info *info, u32 index)
>>>>> +{
>>>>> +	int err;
>>>>> +
>>>>> +	if (!vdev->config->get_vendor_vq_stats)
>>>>> +		return -EOPNOTSUPP;
>>>>> +
>>>>> +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
>>>>> +	if (err)
>>>>> +		return err;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
>>>>> +				      struct sk_buff *msg,
>>>>> +				      struct genl_info *info, u32 index)
>>>>> +{
>>>>> +	u32 device_id;
>>>>> +	void *hdr;
>>>>> +	int err;
>>>>> +	u32 portid = info->snd_portid;
>>>>> +	u32 seq = info->snd_seq;
>>>>> +	u32 flags = 0;
>>>>> +
>>>>> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>>>> +			  VDPA_CMD_DEV_VSTATS_GET);
>>>>> +	if (!hdr)
>>>>> +		return -EMSGSIZE;
>>>>> +
>>>>> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
>>>>> +		err = -EMSGSIZE;
>>>>> +		goto undo_msg;
>>>>> +	}
>>>>> +
>>>>> +	device_id = vdev->config->get_device_id(vdev);
>>>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
>>>>> +		err = -EMSGSIZE;
>>>>> +		goto undo_msg;
>>>>> +	}
>>>>> +
>>>> You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
>>>> otherwise I can't image how you can ensure the atomicity to infer
>>>> queue_type for control vq.
>>>> And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
>>>> into vendor_stats_fill()?
>>> It is done in the hardware driver. In this case, in mlx5_vdpa.
>> OK... So you think this is not vdpa common code but rather individual
>> vendor driver should deal with? Seems fine at the first glance, but with
>> some thoughts this would complicate userspace code quite a lot to tell
>> apart different cases - say if the VDPA_ATTR_DEV_NEGOTIATED_FEATURES
>> attr is missing it would not be possible to display the queue type. On
>> the other hand, the queue type itself shouldn't be vendor specific thing
>> - only the vendor stats are, right?
>>
> Right, although this feature is really about displaying statistics and queue type
> is just supplemental information.
Well, in the userspace implementation perspective it might be so, but 
thinking it from user's point of view it might not be easy to infer the 
type just from queue index.

>
>> Furthermore, without FEATURES_OK the stats returned for a specific queue
>> might not be stable/reliable/reasonable at all, not sure how the tool
>> can infer such complex state (e.g. negotiation is in progress) if
>> somehow the vendor driver doesn't provide the corresponding attribute?
>> Should vendor driver expect to fail with explicit message to indicate
>> the reason, or it'd be fine to just display zero stats there? Looking at
>> mlx5_vdpa implementation it seems to be the former case, but in case of
>> device being negotiating, depending on which queue, the vstat query
>> might end up with a confusing message of, either
>>
>> "virtqueue index is not valid"
>>
>> or,
>>
>> "failed to query hardware"
>>
>> but none is helpful for user to indicate what's going on... I wonder if
>> mandating presence of FEATURES_OK would simplify userspace tool's
>> implementation in this case?
> When you say "mandating", do you refer to kernel?
Yep. Either the vendor driver checks the FEATURES_OK status alone and 
rejects incoming request when negotiation is still in progress, or the 
vdpa core could double check on that. Or else the user would get to 
either of the above messages, which is kinda misleading to users.

>   The userspace tool
> can do that since it will have the negotiated features.
Are you suggesting if the userspace tool doesn't see the negotiated 
features attribute but the get_vendor_vq_stats() call was successful, it 
would simply assume no FEATURES_OK is set as yet?

For now I don't see the need of per-queue stat query before device and 
driver get done on feature negotiation. It would be simpler for the 
userspace tool to assume valid per-queue stats are available only until 
feature negotiation is done: the kernel could just reject the 
.get_vendor_vq_stats() call if not seeing FEATURES_OK. We can further 
extend by introducing new attribute to it whenever there's future need. 
That way you don't need to add a lot of future proof code to the 
userspace tool and try to cover all of the cases in the first place.

> I am reluctant to splitting attributes insertion between hardware driver
> and generic vdpa code. If this is vendor specific feature I think all attributes
> should come from the vendor driver. But, I don't insist and can move to vdpa
> generic code.
It's fine to insert all attributes in the driver, but please try to come 
up with sort of common abstraction/interface for userspace to digest. 
And hopefully it would simplify tool's implementation rather than 
complicate things up.

BTW, please pay attention to the comment I made for vdpa_dev_mutex 
below. I'd rather get the semaphore conversion done earlier than later, 
or there'll be side issue popped up with regard to locking and atomicity.

> Given that stats_get() is a read-only operation that might happen quite
> often from time to time, I wonder if it is now a good timing to convert
> vdpa_dev_mutex to a semaphore?
>

Thanks,
-Siwei
>
>>
>> Thanks,
>> -Siwei
>>
>>>>> +	err = vendor_stats_fill(vdev, msg, info, index);
>>>>> +
>>>>> +	genlmsg_end(msg, hdr);
>>>>> +
>>>>> +	return err;
>>>>> +
>>>>> +undo_msg:
>>>>> +	genlmsg_cancel(msg, hdr);
>>>>> +	return err;
>>>>> +}
>>>>> +
>>>>>     static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
>>>>>     {
>>>>>     	struct vdpa_device *vdev;
>>>>> @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
>>>>>     	return msg->len;
>>>>>     }
>>>>>
>>>>> +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
>>>>> +					  struct genl_info *info)
>>>>> +{
>>>>> +	struct vdpa_device *vdev;
>>>>> +	struct sk_buff *msg;
>>>>> +	const char *devname;
>>>>> +	struct device *dev;
>>>>> +	u32 index;
>>>>> +	int err;
>>>>> +
>>>>> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
>>>>> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>>>> +	if (!msg)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
>>>>> +	mutex_lock(&vdpa_dev_mutex);
>>>> Given that stats_get() is a read-only operation that might happen quite
>>>> often from time to time, I wonder if it is now a good timing to convert
>>>> vdpa_dev_mutex to a semaphore?
>>>>
>>>>> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
>>>>> +	if (!dev) {
>>>>> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
>>>>> +		err = -ENODEV;
>>>>> +		goto dev_err;
>>>> Missing nlmsg_free().
>>>>> +	}
>>>>> +	vdev = container_of(dev, struct vdpa_device, dev);
>>>>> +	if (!vdev->mdev) {
>>>>> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
>>>>> +		err = -EINVAL;
>>>>> +		goto mdev_err;
>>>> Missing nlmsg_free().
>>>>
>>>> Otherwise looks fine.
>>>>
>>>> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>>>
>>>>
>>>> -Siwei
>>>>> +	}
>>>>> +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
>>>>> +	if (!err)
>>>>> +		err = genlmsg_reply(msg, info);
>>>>> +
>>>>> +	put_device(dev);
>>>>> +	mutex_unlock(&vdpa_dev_mutex);
>>>>> +
>>>>> +	if (err)
>>>>> +		nlmsg_free(msg);
>>>>> +
>>>>> +	return err;
>>>>> +
>>>>> +mdev_err:
>>>>> +	put_device(dev);
>>>>> +dev_err:
>>>>> +	mutex_unlock(&vdpa_dev_mutex);
>>>>> +	return err;
>>>>> +}
>>>>> +
>>>>>     static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>>>     	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>>>>>     	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
>>>>> @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>>>     	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
>>>>>     	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
>>>>>     	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
>>>>> +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
>>>>>     };
>>>>>
>>>>>     static const struct genl_ops vdpa_nl_ops[] = {
>>>>> @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>>>>>     		.doit = vdpa_nl_cmd_dev_config_get_doit,
>>>>>     		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
>>>>>     	},
>>>>> +	{
>>>>> +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
>>>>> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>>>>> +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
>>>>> +		.flags = GENL_ADMIN_PERM,
>>>>> +	},
>>>>>     };
>>>>>
>>>>>     static struct genl_family vdpa_nl_family __ro_after_init = {
>>>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>>>> index 8943a209202e..48ed1fc00830 100644
>>>>> --- a/include/linux/vdpa.h
>>>>> +++ b/include/linux/vdpa.h
>>>>> @@ -276,6 +276,9 @@ struct vdpa_config_ops {
>>>>>     			    const struct vdpa_vq_state *state);
>>>>>     	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
>>>>>     			    struct vdpa_vq_state *state);
>>>>> +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
>>>>> +				   struct sk_buff *msg,
>>>>> +				   struct netlink_ext_ack *extack);
>>>>>     	struct vdpa_notification_area
>>>>>     	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
>>>>>     	/* vq irq is not expected to be changed once DRIVER_OK is set */
>>>>> @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
>>>>>     int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
>>>>>     void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
>>>>>
>>>>> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
>>>>> +
>>>>>     #endif /* _LINUX_VDPA_H */
>>>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>>>>> index 1061d8d2d09d..25c55cab3d7c 100644
>>>>> --- a/include/uapi/linux/vdpa.h
>>>>> +++ b/include/uapi/linux/vdpa.h
>>>>> @@ -18,6 +18,7 @@ enum vdpa_command {
>>>>>     	VDPA_CMD_DEV_DEL,
>>>>>     	VDPA_CMD_DEV_GET,		/* can dump */
>>>>>     	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
>>>>> +	VDPA_CMD_DEV_VSTATS_GET,
>>>>>     };
>>>>>
>>>>>     enum vdpa_attr {
>>>>> @@ -46,6 +47,11 @@ enum vdpa_attr {
>>>>>     	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>>>>>     	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>>>>>     	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
>>>>> +
>>>>> +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>>>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
>>>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
>>>>> +
>>>>>     	/* new attributes must be added above here */
>>>>>     	VDPA_ATTR_MAX,
>>>>>     };

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

RE: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Eli Cohen]

> -----Original Message-----
> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Tuesday, May 3, 2022 2:48 AM
> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
> 
> 
> 
> On 5/2/2022 3:22 AM, Eli Cohen wrote:
> > Allows to read vendor statistics of a vdpa device. The specific
> > statistics data are received from the upstream driver in the form of an
> > (attribute name, attribute value) pairs.
> >
> > An example of statistics for mlx5_vdpa device are:
> >
> > received_desc - number of descriptors received by the virtqueue
> > completed_desc - number of descriptors completed by the virtqueue
> >
> > A descriptor using indirect buffers is still counted as 1. In addition,
> > N chained descriptors are counted correctly N times as one would expect.
> >
> > A new callback was added to vdpa_config_ops which provides the means for
> > the vdpa driver to return statistics results.
> >
> > The interface allows for reading all the supported virtqueues, including
> > the control virtqueue if it exists.
> >
> > Below are some examples taken from mlx5_vdpa which are introduced in the
> > following patch:
> >
> > 1. Read statistics for the virtqueue at index 1
> >
> > $ vdpa dev vstats show vdpa-a qidx 1
> > vdpa-a:
> > queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
> >
> > 2. Read statistics for the virtqueue at index 32
> > $ vdpa dev vstats show vdpa-a qidx 32
> > vdpa-a:
> > queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
> >
> > 3. Read statisitics for the virtqueue at index 0 with json output
> > $ vdpa -j dev vstats show vdpa-a qidx 0
> > {"vstats":{"vdpa-a":{
> > "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
> >   "name":"completed_desc","value":417548}}}
> >
> > 4. Read statistics for the virtqueue at index 0 with preety json output
> > $ vdpa -jp dev vstats show vdpa-a qidx 0
> > {
> >      "vstats": {
> >          "vdpa-a": {
> >
> >              "queue_type": "rx",
> >              "queue_index": 0,
> >              "name": "received_desc",
> >              "value": 417776,
> >              "name": "completed_desc",
> >              "value": 417548
> >          }
> >      }
> > }
> >
> > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > ---
> >   drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
> >   include/linux/vdpa.h      |   5 ++
> >   include/uapi/linux/vdpa.h |   6 ++
> >   3 files changed, 140 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index 2b75c00b1005..933466f61ca8 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
> >   	return err;
> >   }
> >
> > +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
> > +			       struct genl_info *info, u32 index)
> > +{
> > +	int err;
> > +
> > +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
> > +	if (err)
> > +		return err;
> > +
> > +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
> > +		return -EMSGSIZE;
> > +
> > +	return 0;
> > +}
> > +
> > +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
> > +			     struct genl_info *info, u32 index)
> > +{
> > +	int err;
> > +
> > +	if (!vdev->config->get_vendor_vq_stats)
> > +		return -EOPNOTSUPP;
> > +
> > +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
> > +	if (err)
> > +		return err;
> > +
> > +	return 0;
> > +}
> > +
> > +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
> > +				      struct sk_buff *msg,
> > +				      struct genl_info *info, u32 index)
> > +{
> > +	u32 device_id;
> > +	void *hdr;
> > +	int err;
> > +	u32 portid = info->snd_portid;
> > +	u32 seq = info->snd_seq;
> > +	u32 flags = 0;
> > +
> > +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> > +			  VDPA_CMD_DEV_VSTATS_GET);
> > +	if (!hdr)
> > +		return -EMSGSIZE;
> > +
> > +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
> > +		err = -EMSGSIZE;
> > +		goto undo_msg;
> > +	}
> > +
> > +	device_id = vdev->config->get_device_id(vdev);
> > +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
> > +		err = -EMSGSIZE;
> > +		goto undo_msg;
> > +	}
> > +
> You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
> otherwise I can't image how you can ensure the atomicity to infer
> queue_type for control vq.
> And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
> into vendor_stats_fill()?
> 
> > +	err = vendor_stats_fill(vdev, msg, info, index);
> > +
> > +	genlmsg_end(msg, hdr);
> > +
> > +	return err;
> > +
> > +undo_msg:
> > +	genlmsg_cancel(msg, hdr);
> > +	return err;
> > +}
> > +
> >   static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
> >   {
> >   	struct vdpa_device *vdev;
> > @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
> >   	return msg->len;
> >   }
> >
> > +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
> > +					  struct genl_info *info)
> > +{
> > +	struct vdpa_device *vdev;
> > +	struct sk_buff *msg;
> > +	const char *devname;
> > +	struct device *dev;
> > +	u32 index;
> > +	int err;
> > +
> > +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
> > +		return -EINVAL;
> > +
> > +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
> > +		return -EINVAL;
> > +
> > +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> > +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > +	if (!msg)
> > +		return -ENOMEM;
> > +
> > +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
> > +	mutex_lock(&vdpa_dev_mutex);
> Given that stats_get() is a read-only operation that might happen quite
> often from time to time, I wonder if it is now a good timing to convert
> vdpa_dev_mutex to a semaphore?

No sure I follow you. You still need that the process that took the lock will
release it. So in that respect we should still use a mutex.

Did you mean use readers/writers lock?

> 
> > +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
> > +	if (!dev) {
> > +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> > +		err = -ENODEV;
> > +		goto dev_err;
> Missing nlmsg_free().
> > +	}
> > +	vdev = container_of(dev, struct vdpa_device, dev);
> > +	if (!vdev->mdev) {
> > +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
> > +		err = -EINVAL;
> > +		goto mdev_err;
> Missing nlmsg_free().

Thanks will fix.

> 
> Otherwise looks fine.
> 
> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
> 
> 
> -Siwei
> > +	}
> > +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
> > +	if (!err)
> > +		err = genlmsg_reply(msg, info);
> > +
> > +	put_device(dev);
> > +	mutex_unlock(&vdpa_dev_mutex);
> > +
> > +	if (err)
> > +		nlmsg_free(msg);
> > +
> > +	return err;
> > +
> > +mdev_err:
> > +	put_device(dev);
> > +dev_err:
> > +	mutex_unlock(&vdpa_dev_mutex);
> > +	return err;
> > +}
> > +
> >   static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
> >   	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
> >   	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
> > @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
> >   	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
> >   	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
> >   	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
> > +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
> >   };
> >
> >   static const struct genl_ops vdpa_nl_ops[] = {
> > @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
> >   		.doit = vdpa_nl_cmd_dev_config_get_doit,
> >   		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
> >   	},
> > +	{
> > +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
> > +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> > +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
> > +		.flags = GENL_ADMIN_PERM,
> > +	},
> >   };
> >
> >   static struct genl_family vdpa_nl_family __ro_after_init = {
> > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > index 8943a209202e..48ed1fc00830 100644
> > --- a/include/linux/vdpa.h
> > +++ b/include/linux/vdpa.h
> > @@ -276,6 +276,9 @@ struct vdpa_config_ops {
> >   			    const struct vdpa_vq_state *state);
> >   	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
> >   			    struct vdpa_vq_state *state);
> > +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
> > +				   struct sk_buff *msg,
> > +				   struct netlink_ext_ack *extack);
> >   	struct vdpa_notification_area
> >   	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
> >   	/* vq irq is not expected to be changed once DRIVER_OK is set */
> > @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
> >   int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
> >   void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
> >
> > +#define VDPA_INVAL_QUEUE_INDEX 0xffff
> > +
> >   #endif /* _LINUX_VDPA_H */
> > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > index 1061d8d2d09d..25c55cab3d7c 100644
> > --- a/include/uapi/linux/vdpa.h
> > +++ b/include/uapi/linux/vdpa.h
> > @@ -18,6 +18,7 @@ enum vdpa_command {
> >   	VDPA_CMD_DEV_DEL,
> >   	VDPA_CMD_DEV_GET,		/* can dump */
> >   	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
> > +	VDPA_CMD_DEV_VSTATS_GET,
> >   };
> >
> >   enum vdpa_attr {
> > @@ -46,6 +47,11 @@ enum vdpa_attr {
> >   	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
> >   	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
> >   	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
> > +
> > +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
> > +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
> > +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
> > +
> >   	/* new attributes must be added above here */
> >   	VDPA_ATTR_MAX,
> >   };

Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Si-Wei Liu]

On 5/8/2022 12:44 AM, Eli Cohen wrote:
>> -----Original Message-----
>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>> Sent: Tuesday, May 3, 2022 2:48 AM
>> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
>> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
>>
>>
>>
>> On 5/2/2022 3:22 AM, Eli Cohen wrote:
>>> Allows to read vendor statistics of a vdpa device. The specific
>>> statistics data are received from the upstream driver in the form of an
>>> (attribute name, attribute value) pairs.
>>>
>>> An example of statistics for mlx5_vdpa device are:
>>>
>>> received_desc - number of descriptors received by the virtqueue
>>> completed_desc - number of descriptors completed by the virtqueue
>>>
>>> A descriptor using indirect buffers is still counted as 1. In addition,
>>> N chained descriptors are counted correctly N times as one would expect.
>>>
>>> A new callback was added to vdpa_config_ops which provides the means for
>>> the vdpa driver to return statistics results.
>>>
>>> The interface allows for reading all the supported virtqueues, including
>>> the control virtqueue if it exists.
>>>
>>> Below are some examples taken from mlx5_vdpa which are introduced in the
>>> following patch:
>>>
>>> 1. Read statistics for the virtqueue at index 1
>>>
>>> $ vdpa dev vstats show vdpa-a qidx 1
>>> vdpa-a:
>>> queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
>>>
>>> 2. Read statistics for the virtqueue at index 32
>>> $ vdpa dev vstats show vdpa-a qidx 32
>>> vdpa-a:
>>> queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
>>>
>>> 3. Read statisitics for the virtqueue at index 0 with json output
>>> $ vdpa -j dev vstats show vdpa-a qidx 0
>>> {"vstats":{"vdpa-a":{
>>> "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
>>>    "name":"completed_desc","value":417548}}}
>>>
>>> 4. Read statistics for the virtqueue at index 0 with preety json output
>>> $ vdpa -jp dev vstats show vdpa-a qidx 0
>>> {
>>>       "vstats": {
>>>           "vdpa-a": {
>>>
>>>               "queue_type": "rx",
>>>               "queue_index": 0,
>>>               "name": "received_desc",
>>>               "value": 417776,
>>>               "name": "completed_desc",
>>>               "value": 417548
>>>           }
>>>       }
>>> }
>>>
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> ---
>>>    drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
>>>    include/linux/vdpa.h      |   5 ++
>>>    include/uapi/linux/vdpa.h |   6 ++
>>>    3 files changed, 140 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 2b75c00b1005..933466f61ca8 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>>>    	return err;
>>>    }
>>>
>>> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
>>> +			       struct genl_info *info, u32 index)
>>> +{
>>> +	int err;
>>> +
>>> +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
>>> +		return -EMSGSIZE;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
>>> +			     struct genl_info *info, u32 index)
>>> +{
>>> +	int err;
>>> +
>>> +	if (!vdev->config->get_vendor_vq_stats)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
>>> +				      struct sk_buff *msg,
>>> +				      struct genl_info *info, u32 index)
>>> +{
>>> +	u32 device_id;
>>> +	void *hdr;
>>> +	int err;
>>> +	u32 portid = info->snd_portid;
>>> +	u32 seq = info->snd_seq;
>>> +	u32 flags = 0;
>>> +
>>> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>> +			  VDPA_CMD_DEV_VSTATS_GET);
>>> +	if (!hdr)
>>> +		return -EMSGSIZE;
>>> +
>>> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
>>> +		err = -EMSGSIZE;
>>> +		goto undo_msg;
>>> +	}
>>> +
>>> +	device_id = vdev->config->get_device_id(vdev);
>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
>>> +		err = -EMSGSIZE;
>>> +		goto undo_msg;
>>> +	}
>>> +
>> You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
>> otherwise I can't image how you can ensure the atomicity to infer
>> queue_type for control vq.
>> And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
>> into vendor_stats_fill()?
>>
>>> +	err = vendor_stats_fill(vdev, msg, info, index);
>>> +
>>> +	genlmsg_end(msg, hdr);
>>> +
>>> +	return err;
>>> +
>>> +undo_msg:
>>> +	genlmsg_cancel(msg, hdr);
>>> +	return err;
>>> +}
>>> +
>>>    static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
>>>    {
>>>    	struct vdpa_device *vdev;
>>> @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
>>>    	return msg->len;
>>>    }
>>>
>>> +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
>>> +					  struct genl_info *info)
>>> +{
>>> +	struct vdpa_device *vdev;
>>> +	struct sk_buff *msg;
>>> +	const char *devname;
>>> +	struct device *dev;
>>> +	u32 index;
>>> +	int err;
>>> +
>>> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
>>> +		return -EINVAL;
>>> +
>>> +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
>>> +		return -EINVAL;
>>> +
>>> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
>>> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>> +	if (!msg)
>>> +		return -ENOMEM;
>>> +
>>> +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
>>> +	mutex_lock(&vdpa_dev_mutex);
>> Given that stats_get() is a read-only operation that might happen quite
>> often from time to time, I wonder if it is now a good timing to convert
>> vdpa_dev_mutex to a semaphore?
> No sure I follow you. You still need that the process that took the lock will
> release it. So in that respect we should still use a mutex.
>
> Did you mean use readers/writers lock?
Yeah, I meant Reader/Writer semaphore, sorry.

-Siwei

>
>>> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
>>> +	if (!dev) {
>>> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
>>> +		err = -ENODEV;
>>> +		goto dev_err;
>> Missing nlmsg_free().
>>> +	}
>>> +	vdev = container_of(dev, struct vdpa_device, dev);
>>> +	if (!vdev->mdev) {
>>> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
>>> +		err = -EINVAL;
>>> +		goto mdev_err;
>> Missing nlmsg_free().
> Thanks will fix.
>
>> Otherwise looks fine.
>>
>> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>
>>
>> -Siwei
>>> +	}
>>> +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
>>> +	if (!err)
>>> +		err = genlmsg_reply(msg, info);
>>> +
>>> +	put_device(dev);
>>> +	mutex_unlock(&vdpa_dev_mutex);
>>> +
>>> +	if (err)
>>> +		nlmsg_free(msg);
>>> +
>>> +	return err;
>>> +
>>> +mdev_err:
>>> +	put_device(dev);
>>> +dev_err:
>>> +	mutex_unlock(&vdpa_dev_mutex);
>>> +	return err;
>>> +}
>>> +
>>>    static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>    	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>>>    	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
>>> @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>    	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
>>>    	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
>>>    	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
>>> +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
>>>    };
>>>
>>>    static const struct genl_ops vdpa_nl_ops[] = {
>>> @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>>>    		.doit = vdpa_nl_cmd_dev_config_get_doit,
>>>    		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
>>>    	},
>>> +	{
>>> +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
>>> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>>> +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
>>> +		.flags = GENL_ADMIN_PERM,
>>> +	},
>>>    };
>>>
>>>    static struct genl_family vdpa_nl_family __ro_after_init = {
>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>> index 8943a209202e..48ed1fc00830 100644
>>> --- a/include/linux/vdpa.h
>>> +++ b/include/linux/vdpa.h
>>> @@ -276,6 +276,9 @@ struct vdpa_config_ops {
>>>    			    const struct vdpa_vq_state *state);
>>>    	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
>>>    			    struct vdpa_vq_state *state);
>>> +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
>>> +				   struct sk_buff *msg,
>>> +				   struct netlink_ext_ack *extack);
>>>    	struct vdpa_notification_area
>>>    	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
>>>    	/* vq irq is not expected to be changed once DRIVER_OK is set */
>>> @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
>>>    int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
>>>    void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
>>>
>>> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
>>> +
>>>    #endif /* _LINUX_VDPA_H */
>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>>> index 1061d8d2d09d..25c55cab3d7c 100644
>>> --- a/include/uapi/linux/vdpa.h
>>> +++ b/include/uapi/linux/vdpa.h
>>> @@ -18,6 +18,7 @@ enum vdpa_command {
>>>    	VDPA_CMD_DEV_DEL,
>>>    	VDPA_CMD_DEV_GET,		/* can dump */
>>>    	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
>>> +	VDPA_CMD_DEV_VSTATS_GET,
>>>    };
>>>
>>>    enum vdpa_attr {
>>> @@ -46,6 +47,11 @@ enum vdpa_attr {
>>>    	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>>>    	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>>>    	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
>>> +
>>> +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
>>> +
>>>    	/* new attributes must be added above here */
>>>    	VDPA_ATTR_MAX,
>>>    };

Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Si-Wei Liu]

On 5/8/2022 12:44 AM, Eli Cohen wrote:
>> -----Original Message-----
>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>> Sent: Tuesday, May 3, 2022 2:48 AM
>> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
>> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
>>
>>
>>
>> On 5/2/2022 3:22 AM, Eli Cohen wrote:
>>> Allows to read vendor statistics of a vdpa device. The specific
>>> statistics data are received from the upstream driver in the form of an
>>> (attribute name, attribute value) pairs.
>>>
>>> An example of statistics for mlx5_vdpa device are:
>>>
>>> received_desc - number of descriptors received by the virtqueue
>>> completed_desc - number of descriptors completed by the virtqueue
>>>
>>> A descriptor using indirect buffers is still counted as 1. In addition,
>>> N chained descriptors are counted correctly N times as one would expect.
>>>
>>> A new callback was added to vdpa_config_ops which provides the means for
>>> the vdpa driver to return statistics results.
>>>
>>> The interface allows for reading all the supported virtqueues, including
>>> the control virtqueue if it exists.
>>>
>>> Below are some examples taken from mlx5_vdpa which are introduced in the
>>> following patch:
>>>
>>> 1. Read statistics for the virtqueue at index 1
>>>
>>> $ vdpa dev vstats show vdpa-a qidx 1
>>> vdpa-a:
>>> queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
>>>
>>> 2. Read statistics for the virtqueue at index 32
>>> $ vdpa dev vstats show vdpa-a qidx 32
>>> vdpa-a:
>>> queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
>>>
>>> 3. Read statisitics for the virtqueue at index 0 with json output
>>> $ vdpa -j dev vstats show vdpa-a qidx 0
>>> {"vstats":{"vdpa-a":{
>>> "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
>>>    "name":"completed_desc","value":417548}}}
>>>
>>> 4. Read statistics for the virtqueue at index 0 with preety json output
>>> $ vdpa -jp dev vstats show vdpa-a qidx 0
>>> {
>>>       "vstats": {
>>>           "vdpa-a": {
>>>
>>>               "queue_type": "rx",
>>>               "queue_index": 0,
>>>               "name": "received_desc",
>>>               "value": 417776,
>>>               "name": "completed_desc",
>>>               "value": 417548
>>>           }
>>>       }
>>> }
>>>
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> ---
>>>    drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
>>>    include/linux/vdpa.h      |   5 ++
>>>    include/uapi/linux/vdpa.h |   6 ++
>>>    3 files changed, 140 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 2b75c00b1005..933466f61ca8 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>>>    	return err;
>>>    }
>>>
>>> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
>>> +			       struct genl_info *info, u32 index)
>>> +{
>>> +	int err;
>>> +
>>> +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
>>> +		return -EMSGSIZE;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
>>> +			     struct genl_info *info, u32 index)
>>> +{
>>> +	int err;
>>> +
>>> +	if (!vdev->config->get_vendor_vq_stats)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
>>> +				      struct sk_buff *msg,
>>> +				      struct genl_info *info, u32 index)
>>> +{
>>> +	u32 device_id;
>>> +	void *hdr;
>>> +	int err;
>>> +	u32 portid = info->snd_portid;
>>> +	u32 seq = info->snd_seq;
>>> +	u32 flags = 0;
>>> +
>>> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>> +			  VDPA_CMD_DEV_VSTATS_GET);
>>> +	if (!hdr)
>>> +		return -EMSGSIZE;
>>> +
>>> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
>>> +		err = -EMSGSIZE;
>>> +		goto undo_msg;
>>> +	}
>>> +
>>> +	device_id = vdev->config->get_device_id(vdev);
>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
>>> +		err = -EMSGSIZE;
>>> +		goto undo_msg;
>>> +	}
>>> +
>> You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
>> otherwise I can't image how you can ensure the atomicity to infer
>> queue_type for control vq.
>> And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
>> into vendor_stats_fill()?
>>
>>> +	err = vendor_stats_fill(vdev, msg, info, index);
>>> +
>>> +	genlmsg_end(msg, hdr);
>>> +
>>> +	return err;
>>> +
>>> +undo_msg:
>>> +	genlmsg_cancel(msg, hdr);
>>> +	return err;
>>> +}
>>> +
>>>    static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
>>>    {
>>>    	struct vdpa_device *vdev;
>>> @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
>>>    	return msg->len;
>>>    }
>>>
>>> +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
>>> +					  struct genl_info *info)
>>> +{
>>> +	struct vdpa_device *vdev;
>>> +	struct sk_buff *msg;
>>> +	const char *devname;
>>> +	struct device *dev;
>>> +	u32 index;
>>> +	int err;
>>> +
>>> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
>>> +		return -EINVAL;
>>> +
>>> +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
>>> +		return -EINVAL;
>>> +
>>> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
>>> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>> +	if (!msg)
>>> +		return -ENOMEM;
>>> +
>>> +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
>>> +	mutex_lock(&vdpa_dev_mutex);
>> Given that stats_get() is a read-only operation that might happen quite
>> often from time to time, I wonder if it is now a good timing to convert
>> vdpa_dev_mutex to a semaphore?
> No sure I follow you. You still need that the process that took the lock will
> release it. So in that respect we should still use a mutex.
>
> Did you mean use readers/writers lock?
Yeah, I meant Reader/Writer semaphore, sorry.

-Siwei

>
>>> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
>>> +	if (!dev) {
>>> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
>>> +		err = -ENODEV;
>>> +		goto dev_err;
>> Missing nlmsg_free().
>>> +	}
>>> +	vdev = container_of(dev, struct vdpa_device, dev);
>>> +	if (!vdev->mdev) {
>>> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
>>> +		err = -EINVAL;
>>> +		goto mdev_err;
>> Missing nlmsg_free().
> Thanks will fix.
>
>> Otherwise looks fine.
>>
>> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>
>>
>> -Siwei
>>> +	}
>>> +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
>>> +	if (!err)
>>> +		err = genlmsg_reply(msg, info);
>>> +
>>> +	put_device(dev);
>>> +	mutex_unlock(&vdpa_dev_mutex);
>>> +
>>> +	if (err)
>>> +		nlmsg_free(msg);
>>> +
>>> +	return err;
>>> +
>>> +mdev_err:
>>> +	put_device(dev);
>>> +dev_err:
>>> +	mutex_unlock(&vdpa_dev_mutex);
>>> +	return err;
>>> +}
>>> +
>>>    static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>    	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>>>    	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
>>> @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>>>    	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
>>>    	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
>>>    	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
>>> +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
>>>    };
>>>
>>>    static const struct genl_ops vdpa_nl_ops[] = {
>>> @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>>>    		.doit = vdpa_nl_cmd_dev_config_get_doit,
>>>    		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
>>>    	},
>>> +	{
>>> +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
>>> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>>> +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
>>> +		.flags = GENL_ADMIN_PERM,
>>> +	},
>>>    };
>>>
>>>    static struct genl_family vdpa_nl_family __ro_after_init = {
>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>> index 8943a209202e..48ed1fc00830 100644
>>> --- a/include/linux/vdpa.h
>>> +++ b/include/linux/vdpa.h
>>> @@ -276,6 +276,9 @@ struct vdpa_config_ops {
>>>    			    const struct vdpa_vq_state *state);
>>>    	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
>>>    			    struct vdpa_vq_state *state);
>>> +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
>>> +				   struct sk_buff *msg,
>>> +				   struct netlink_ext_ack *extack);
>>>    	struct vdpa_notification_area
>>>    	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
>>>    	/* vq irq is not expected to be changed once DRIVER_OK is set */
>>> @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
>>>    int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
>>>    void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
>>>
>>> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
>>> +
>>>    #endif /* _LINUX_VDPA_H */
>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>>> index 1061d8d2d09d..25c55cab3d7c 100644
>>> --- a/include/uapi/linux/vdpa.h
>>> +++ b/include/uapi/linux/vdpa.h
>>> @@ -18,6 +18,7 @@ enum vdpa_command {
>>>    	VDPA_CMD_DEV_DEL,
>>>    	VDPA_CMD_DEV_GET,		/* can dump */
>>>    	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
>>> +	VDPA_CMD_DEV_VSTATS_GET,
>>>    };
>>>
>>>    enum vdpa_attr {
>>> @@ -46,6 +47,11 @@ enum vdpa_attr {
>>>    	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>>>    	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>>>    	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
>>> +
>>> +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
>>> +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
>>> +
>>>    	/* new attributes must be added above here */
>>>    	VDPA_ATTR_MAX,
>>>    };

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

RE: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics

[Eli Cohen]

> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Tuesday, May 10, 2022 8:58 PM
> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
> 
> 
> 
> On 5/8/2022 12:44 AM, Eli Cohen wrote:
> >> -----Original Message-----
> >> From: Si-Wei Liu <si-wei.liu@oracle.com>
> >> Sent: Tuesday, May 3, 2022 2:48 AM
> >> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com
> >> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
> >>
> >>
> >>
> >> On 5/2/2022 3:22 AM, Eli Cohen wrote:
> >>> Allows to read vendor statistics of a vdpa device. The specific
> >>> statistics data are received from the upstream driver in the form of an
> >>> (attribute name, attribute value) pairs.
> >>>
> >>> An example of statistics for mlx5_vdpa device are:
> >>>
> >>> received_desc - number of descriptors received by the virtqueue
> >>> completed_desc - number of descriptors completed by the virtqueue
> >>>
> >>> A descriptor using indirect buffers is still counted as 1. In addition,
> >>> N chained descriptors are counted correctly N times as one would expect.
> >>>
> >>> A new callback was added to vdpa_config_ops which provides the means for
> >>> the vdpa driver to return statistics results.
> >>>
> >>> The interface allows for reading all the supported virtqueues, including
> >>> the control virtqueue if it exists.
> >>>
> >>> Below are some examples taken from mlx5_vdpa which are introduced in the
> >>> following patch:
> >>>
> >>> 1. Read statistics for the virtqueue at index 1
> >>>
> >>> $ vdpa dev vstats show vdpa-a qidx 1
> >>> vdpa-a:
> >>> queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
> >>>
> >>> 2. Read statistics for the virtqueue at index 32
> >>> $ vdpa dev vstats show vdpa-a qidx 32
> >>> vdpa-a:
> >>> queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
> >>>
> >>> 3. Read statisitics for the virtqueue at index 0 with json output
> >>> $ vdpa -j dev vstats show vdpa-a qidx 0
> >>> {"vstats":{"vdpa-a":{
> >>> "queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
> >>>    "name":"completed_desc","value":417548}}}
> >>>
> >>> 4. Read statistics for the virtqueue at index 0 with preety json output
> >>> $ vdpa -jp dev vstats show vdpa-a qidx 0
> >>> {
> >>>       "vstats": {
> >>>           "vdpa-a": {
> >>>
> >>>               "queue_type": "rx",
> >>>               "queue_index": 0,
> >>>               "name": "received_desc",
> >>>               "value": 417776,
> >>>               "name": "completed_desc",
> >>>               "value": 417548
> >>>           }
> >>>       }
> >>> }
> >>>
> >>> Signed-off-by: Eli Cohen <elic@nvidia.com>
> >>> ---
> >>>    drivers/vdpa/vdpa.c       | 129 ++++++++++++++++++++++++++++++++++++++
> >>>    include/linux/vdpa.h      |   5 ++
> >>>    include/uapi/linux/vdpa.h |   6 ++
> >>>    3 files changed, 140 insertions(+)
> >>>
> >>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >>> index 2b75c00b1005..933466f61ca8 100644
> >>> --- a/drivers/vdpa/vdpa.c
> >>> +++ b/drivers/vdpa/vdpa.c
> >>> @@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
> >>>    	return err;
> >>>    }
> >>>
> >>> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
> >>> +			       struct genl_info *info, u32 index)
> >>> +{
> >>> +	int err;
> >>> +
> >>> +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
> >>> +		return -EMSGSIZE;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
> >>> +			     struct genl_info *info, u32 index)
> >>> +{
> >>> +	int err;
> >>> +
> >>> +	if (!vdev->config->get_vendor_vq_stats)
> >>> +		return -EOPNOTSUPP;
> >>> +
> >>> +	err = vdpa_fill_stats_rec(vdev, msg, info, index);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
> >>> +				      struct sk_buff *msg,
> >>> +				      struct genl_info *info, u32 index)
> >>> +{
> >>> +	u32 device_id;
> >>> +	void *hdr;
> >>> +	int err;
> >>> +	u32 portid = info->snd_portid;
> >>> +	u32 seq = info->snd_seq;
> >>> +	u32 flags = 0;
> >>> +
> >>> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> >>> +			  VDPA_CMD_DEV_VSTATS_GET);
> >>> +	if (!hdr)
> >>> +		return -EMSGSIZE;
> >>> +
> >>> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
> >>> +		err = -EMSGSIZE;
> >>> +		goto undo_msg;
> >>> +	}
> >>> +
> >>> +	device_id = vdev->config->get_device_id(vdev);
> >>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
> >>> +		err = -EMSGSIZE;
> >>> +		goto undo_msg;
> >>> +	}
> >>> +
> >> You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
> >> otherwise I can't image how you can ensure the atomicity to infer
> >> queue_type for control vq.
> >> And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
> >> into vendor_stats_fill()?
> >>
> >>> +	err = vendor_stats_fill(vdev, msg, info, index);
> >>> +
> >>> +	genlmsg_end(msg, hdr);
> >>> +
> >>> +	return err;
> >>> +
> >>> +undo_msg:
> >>> +	genlmsg_cancel(msg, hdr);
> >>> +	return err;
> >>> +}
> >>> +
> >>>    static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
> >>>    {
> >>>    	struct vdpa_device *vdev;
> >>> @@ -990,6 +1058,60 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
> >>>    	return msg->len;
> >>>    }
> >>>
> >>> +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
> >>> +					  struct genl_info *info)
> >>> +{
> >>> +	struct vdpa_device *vdev;
> >>> +	struct sk_buff *msg;
> >>> +	const char *devname;
> >>> +	struct device *dev;
> >>> +	u32 index;
> >>> +	int err;
> >>> +
> >>> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
> >>> +		return -EINVAL;
> >>> +
> >>> +	if (!info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
> >>> +		return -EINVAL;
> >>> +
> >>> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> >>> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >>> +	if (!msg)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	index = nla_get_u32(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
> >>> +	mutex_lock(&vdpa_dev_mutex);
> >> Given that stats_get() is a read-only operation that might happen quite
> >> often from time to time, I wonder if it is now a good timing to convert
> >> vdpa_dev_mutex to a semaphore?
> > No sure I follow you. You still need that the process that took the lock will
> > release it. So in that respect we should still use a mutex.
> >
> > Did you mean use readers/writers lock?
> Yeah, I meant Reader/Writer semaphore, sorry.

Makes sense. I can post a patch for that.

> 
> -Siwei
> 
> >
> >>> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
> >>> +	if (!dev) {
> >>> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> >>> +		err = -ENODEV;
> >>> +		goto dev_err;
> >> Missing nlmsg_free().
> >>> +	}
> >>> +	vdev = container_of(dev, struct vdpa_device, dev);
> >>> +	if (!vdev->mdev) {
> >>> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
> >>> +		err = -EINVAL;
> >>> +		goto mdev_err;
> >> Missing nlmsg_free().
> > Thanks will fix.
> >
> >> Otherwise looks fine.
> >>
> >> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
> >>
> >>
> >> -Siwei
> >>> +	}
> >>> +	err = vdpa_dev_vendor_stats_fill(vdev, msg, info, index);
> >>> +	if (!err)
> >>> +		err = genlmsg_reply(msg, info);
> >>> +
> >>> +	put_device(dev);
> >>> +	mutex_unlock(&vdpa_dev_mutex);
> >>> +
> >>> +	if (err)
> >>> +		nlmsg_free(msg);
> >>> +
> >>> +	return err;
> >>> +
> >>> +mdev_err:
> >>> +	put_device(dev);
> >>> +dev_err:
> >>> +	mutex_unlock(&vdpa_dev_mutex);
> >>> +	return err;
> >>> +}
> >>> +
> >>>    static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
> >>>    	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
> >>>    	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
> >>> @@ -997,6 +1119,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
> >>>    	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
> >>>    	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
> >>>    	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
> >>> +	[VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U32, 0, 65535),
> >>>    };
> >>>
> >>>    static const struct genl_ops vdpa_nl_ops[] = {
> >>> @@ -1030,6 +1153,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
> >>>    		.doit = vdpa_nl_cmd_dev_config_get_doit,
> >>>    		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
> >>>    	},
> >>> +	{
> >>> +		.cmd = VDPA_CMD_DEV_VSTATS_GET,
> >>> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> >>> +		.doit = vdpa_nl_cmd_dev_stats_get_doit,
> >>> +		.flags = GENL_ADMIN_PERM,
> >>> +	},
> >>>    };
> >>>
> >>>    static struct genl_family vdpa_nl_family __ro_after_init = {
> >>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> >>> index 8943a209202e..48ed1fc00830 100644
> >>> --- a/include/linux/vdpa.h
> >>> +++ b/include/linux/vdpa.h
> >>> @@ -276,6 +276,9 @@ struct vdpa_config_ops {
> >>>    			    const struct vdpa_vq_state *state);
> >>>    	int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
> >>>    			    struct vdpa_vq_state *state);
> >>> +	int (*get_vendor_vq_stats)(struct vdpa_device *vdev, u16 idx,
> >>> +				   struct sk_buff *msg,
> >>> +				   struct netlink_ext_ack *extack);
> >>>    	struct vdpa_notification_area
> >>>    	(*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
> >>>    	/* vq irq is not expected to be changed once DRIVER_OK is set */
> >>> @@ -473,4 +476,6 @@ struct vdpa_mgmt_dev {
> >>>    int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
> >>>    void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
> >>>
> >>> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
> >>> +
> >>>    #endif /* _LINUX_VDPA_H */
> >>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> >>> index 1061d8d2d09d..25c55cab3d7c 100644
> >>> --- a/include/uapi/linux/vdpa.h
> >>> +++ b/include/uapi/linux/vdpa.h
> >>> @@ -18,6 +18,7 @@ enum vdpa_command {
> >>>    	VDPA_CMD_DEV_DEL,
> >>>    	VDPA_CMD_DEV_GET,		/* can dump */
> >>>    	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
> >>> +	VDPA_CMD_DEV_VSTATS_GET,
> >>>    };
> >>>
> >>>    enum vdpa_attr {
> >>> @@ -46,6 +47,11 @@ enum vdpa_attr {
> >>>    	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
> >>>    	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
> >>>    	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
> >>> +
> >>> +	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
> >>> +	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
> >>> +	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
> >>> +
> >>>    	/* new attributes must be added above here */
> >>>    	VDPA_ATTR_MAX,
> >>>    };

[PATCH v3 2/2] vdpa/mlx5: Add support for reading descriptor statistics

[Eli Cohen]

Implement the get_vq_stats calback of vdpa_config_ops to return the
statistics for a virtqueue.

The statistics are provided as vendor specific statistics where the
driver provides a pair of attribute name and attribute value.

In addition to the attribute name/attribute value pair, the driver
returns the negotiated features and max virtqueue pairs for userspace
can decide for a given queue index whether it is a data or control
virtqueue.

Currently supported are received descriptors and completed descriptors.

Acked-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Eli Cohen <elic@nvidia.com>

v2 -> v3:
Fix sparse warning
---
 drivers/vdpa/mlx5/core/mlx5_vdpa.h |   2 +
 drivers/vdpa/mlx5/net/mlx5_vnet.c  | 165 +++++++++++++++++++++++++++++
 include/linux/mlx5/mlx5_ifc.h      |   1 +
 include/linux/mlx5/mlx5_ifc_vdpa.h |  39 +++++++
 4 files changed, 207 insertions(+)

diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
index daaf7b503677..44104093163b 100644
--- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
+++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
@@ -61,6 +61,8 @@ struct mlx5_control_vq {
 	struct vringh_kiov riov;
 	struct vringh_kiov wiov;
 	unsigned short head;
+	unsigned int received_desc;
+	unsigned int completed_desc;
 };
 
 struct mlx5_vdpa_wq_ent {
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 79001301b383..473ec481813c 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -119,6 +119,7 @@ struct mlx5_vdpa_virtqueue {
 	struct mlx5_vdpa_umem umem2;
 	struct mlx5_vdpa_umem umem3;
 
+	u32 counter_set_id;
 	bool initialized;
 	int index;
 	u32 virtq_id;
@@ -164,6 +165,8 @@ struct mlx5_vdpa_net {
 	struct notifier_block nb;
 	struct vdpa_callback config_cb;
 	struct mlx5_vdpa_wq_ent cvq_ent;
+	/* sync access to virtqueues statistics */
+	struct mutex numq_lock;
 };
 
 static void free_resources(struct mlx5_vdpa_net *ndev);
@@ -822,6 +825,12 @@ static u16 get_features_12_3(u64 features)
 	       (!!(features & BIT_ULL(VIRTIO_NET_F_GUEST_CSUM)) << 6);
 }
 
+static bool counters_supported(const struct mlx5_vdpa_dev *mvdev)
+{
+	return MLX5_CAP_GEN_64(mvdev->mdev, general_obj_types) &
+	       BIT_ULL(MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS);
+}
+
 static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
 {
 	int inlen = MLX5_ST_SZ_BYTES(create_virtio_net_q_in);
@@ -876,6 +885,8 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
 	MLX5_SET(virtio_q, vq_ctx, umem_3_id, mvq->umem3.id);
 	MLX5_SET(virtio_q, vq_ctx, umem_3_size, mvq->umem3.size);
 	MLX5_SET(virtio_q, vq_ctx, pd, ndev->mvdev.res.pdn);
+	if (counters_supported(&ndev->mvdev))
+		MLX5_SET(virtio_q, vq_ctx, counter_set_id, mvq->counter_set_id);
 
 	err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
 	if (err)
@@ -1139,6 +1150,47 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
 	return err;
 }
 
+static int counter_set_alloc(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
+{
+	u32 in[MLX5_ST_SZ_DW(create_virtio_q_counters_in)] = {};
+	u32 out[MLX5_ST_SZ_DW(create_virtio_q_counters_out)] = {};
+	void *cmd_hdr;
+	int err;
+
+	if (!counters_supported(&ndev->mvdev))
+		return 0;
+
+	cmd_hdr = MLX5_ADDR_OF(create_virtio_q_counters_in, in, hdr);
+
+	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, opcode, MLX5_CMD_OP_CREATE_GENERAL_OBJECT);
+	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, obj_type, MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS);
+	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);
+
+	err = mlx5_cmd_exec(ndev->mvdev.mdev, in, sizeof(in), out, sizeof(out));
+	if (err)
+		return err;
+
+	mvq->counter_set_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
+
+	return 0;
+}
+
+static void counter_set_dealloc(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
+{
+	u32 in[MLX5_ST_SZ_DW(destroy_virtio_q_counters_in)] = {};
+	u32 out[MLX5_ST_SZ_DW(destroy_virtio_q_counters_out)] = {};
+
+	if (!counters_supported(&ndev->mvdev))
+		return;
+
+	MLX5_SET(destroy_virtio_q_counters_in, in, hdr.opcode, MLX5_CMD_OP_DESTROY_GENERAL_OBJECT);
+	MLX5_SET(destroy_virtio_q_counters_in, in, hdr.obj_id, mvq->counter_set_id);
+	MLX5_SET(destroy_virtio_q_counters_in, in, hdr.uid, ndev->mvdev.res.uid);
+	MLX5_SET(destroy_virtio_q_counters_in, in, hdr.obj_type, MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS);
+	if (mlx5_cmd_exec(ndev->mvdev.mdev, in, sizeof(in), out, sizeof(out)))
+		mlx5_vdpa_warn(&ndev->mvdev, "dealloc counter set 0x%x\n", mvq->counter_set_id);
+}
+
 static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
 {
 	u16 idx = mvq->index;
@@ -1166,6 +1218,10 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
 	if (err)
 		goto err_connect;
 
+	err = counter_set_alloc(ndev, mvq);
+	if (err)
+		goto err_counter;
+
 	err = create_virtqueue(ndev, mvq);
 	if (err)
 		goto err_connect;
@@ -1183,6 +1239,8 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
 	return 0;
 
 err_connect:
+	counter_set_dealloc(ndev, mvq);
+err_counter:
 	qp_destroy(ndev, &mvq->vqqp);
 err_vqqp:
 	qp_destroy(ndev, &mvq->fwqp);
@@ -1227,6 +1285,7 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
 
 	suspend_vq(ndev, mvq);
 	destroy_virtqueue(ndev, mvq);
+	counter_set_dealloc(ndev, mvq);
 	qp_destroy(ndev, &mvq->vqqp);
 	qp_destroy(ndev, &mvq->fwqp);
 	cq_destroy(ndev, mvq->index);
@@ -1633,8 +1692,10 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
 			break;
 		}
 
+		mutex_lock(&ndev->numq_lock);
 		if (!change_num_qps(mvdev, newqps))
 			status = VIRTIO_NET_OK;
+		mutex_unlock(&ndev->numq_lock);
 
 		break;
 	default:
@@ -1681,6 +1742,7 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 		if (read != sizeof(ctrl))
 			break;
 
+		cvq->received_desc++;
 		switch (ctrl.class) {
 		case VIRTIO_NET_CTRL_MAC:
 			status = handle_ctrl_mac(mvdev, ctrl.cmd);
@@ -1704,6 +1766,7 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 		if (vringh_need_notify_iotlb(&cvq->vring))
 			vringh_notify(&cvq->vring);
 
+		cvq->completed_desc++;
 		queue_work(mvdev->wq, &wqent->work);
 		break;
 	}
@@ -2323,6 +2386,8 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
 	mlx5_vdpa_destroy_mr(&ndev->mvdev);
 	ndev->mvdev.status = 0;
 	ndev->cur_num_vqs = 0;
+	ndev->mvdev.cvq.received_desc = 0;
+	ndev->mvdev.cvq.completed_desc = 0;
 	memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
 	ndev->mvdev.actual_features = 0;
 	++mvdev->generation;
@@ -2401,6 +2466,7 @@ static void mlx5_vdpa_free(struct vdpa_device *vdev)
 		mlx5_mpfs_del_mac(pfmdev, ndev->config.mac);
 	}
 	mlx5_vdpa_free_resources(&ndev->mvdev);
+	mutex_destroy(&ndev->numq_lock);
 	mutex_destroy(&ndev->reslock);
 	kfree(ndev->event_cbs);
 	kfree(ndev->vqs);
@@ -2442,6 +2508,102 @@ static u64 mlx5_vdpa_get_driver_features(struct vdpa_device *vdev)
 	return mvdev->actual_features;
 }
 
+static int counter_set_query(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq,
+			     u64 *received_desc, u64 *completed_desc)
+{
+	u32 in[MLX5_ST_SZ_DW(query_virtio_q_counters_in)] = {};
+	u32 out[MLX5_ST_SZ_DW(query_virtio_q_counters_out)] = {};
+	void *cmd_hdr;
+	void *ctx;
+	int err;
+
+	if (!counters_supported(&ndev->mvdev))
+		return -EOPNOTSUPP;
+
+	if (mvq->fw_state != MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY)
+		return -EAGAIN;
+
+	cmd_hdr = MLX5_ADDR_OF(query_virtio_q_counters_in, in, hdr);
+
+	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, opcode, MLX5_CMD_OP_QUERY_GENERAL_OBJECT);
+	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, obj_type, MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS);
+	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);
+	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, obj_id, mvq->counter_set_id);
+
+	err = mlx5_cmd_exec(ndev->mvdev.mdev, in, sizeof(in), out, sizeof(out));
+	if (err)
+		return err;
+
+	ctx = MLX5_ADDR_OF(query_virtio_q_counters_out, out, counters);
+	*received_desc = MLX5_GET64(virtio_q_counters, ctx, received_desc);
+	*completed_desc = MLX5_GET64(virtio_q_counters, ctx, completed_desc);
+	return 0;
+}
+
+static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
+					 struct sk_buff *msg,
+					 struct netlink_ext_ack *extack)
+{
+	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
+	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
+	struct mlx5_vdpa_virtqueue *mvq;
+	struct mlx5_control_vq *cvq;
+	u64 received_desc;
+	u64 completed_desc;
+	int err = 0;
+	u16 max_vqp;
+
+	mutex_lock(&ndev->numq_lock);
+	if (!is_index_valid(mvdev, idx)) {
+		NL_SET_ERR_MSG_MOD(extack, "virtqueue index is not valid");
+		err = -EINVAL;
+		goto out_err;
+	}
+
+	if (idx == ctrl_vq_idx(mvdev)) {
+		cvq = &mvdev->cvq;
+		received_desc = cvq->received_desc;
+		completed_desc = cvq->completed_desc;
+		goto out;
+	}
+
+	mvq = &ndev->vqs[idx];
+	err = counter_set_query(ndev, mvq, &received_desc, &completed_desc);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "failed to query hardware");
+		goto out_err;
+	}
+
+out:
+	err = -EMSGSIZE;
+	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES,
+			      mvdev->actual_features, VDPA_ATTR_PAD))
+		goto out_err;
+
+	max_vqp = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
+	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, max_vqp))
+		goto out_err;
+
+	if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, "received_desc"))
+		goto out_err;
+
+	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, received_desc,
+			      VDPA_ATTR_PAD))
+		goto out_err;
+
+	if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, "completed_desc"))
+		goto out_err;
+
+	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, completed_desc,
+			      VDPA_ATTR_PAD))
+		goto out_err;
+
+	err = 0;
+out_err:
+	mutex_unlock(&ndev->numq_lock);
+	return err;
+}
+
 static const struct vdpa_config_ops mlx5_vdpa_ops = {
 	.set_vq_address = mlx5_vdpa_set_vq_address,
 	.set_vq_num = mlx5_vdpa_set_vq_num,
@@ -2451,6 +2613,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
 	.get_vq_ready = mlx5_vdpa_get_vq_ready,
 	.set_vq_state = mlx5_vdpa_set_vq_state,
 	.get_vq_state = mlx5_vdpa_get_vq_state,
+	.get_vendor_vq_stats = mlx5_vdpa_get_vendor_vq_stats,
 	.get_vq_notification = mlx5_get_vq_notification,
 	.get_vq_irq = mlx5_get_vq_irq,
 	.get_vq_align = mlx5_vdpa_get_vq_align,
@@ -2706,6 +2869,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 
 	init_mvqs(ndev);
 	mutex_init(&ndev->reslock);
+	mutex_init(&ndev->numq_lock);
 	config = &ndev->config;
 
 	if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) {
@@ -2788,6 +2952,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 	if (!is_zero_ether_addr(config->mac))
 		mlx5_mpfs_del_mac(pfmdev, config->mac);
 err_mtu:
+	mutex_destroy(&ndev->numq_lock);
 	mutex_destroy(&ndev->reslock);
 err_alloc:
 	put_device(&mvdev->vdev.dev);
diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index 49a48d7709ac..1d193d9b6029 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -94,6 +94,7 @@ enum {
 enum {
 	MLX5_OBJ_TYPE_GENEVE_TLV_OPT = 0x000b,
 	MLX5_OBJ_TYPE_VIRTIO_NET_Q = 0x000d,
+	MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS = 0x001c,
 	MLX5_OBJ_TYPE_MATCH_DEFINER = 0x0018,
 	MLX5_OBJ_TYPE_MKEY = 0xff01,
 	MLX5_OBJ_TYPE_QP = 0xff02,
diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
index 1a9c9d94cb59..4414ed5b6ed2 100644
--- a/include/linux/mlx5/mlx5_ifc_vdpa.h
+++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
@@ -165,4 +165,43 @@ struct mlx5_ifc_modify_virtio_net_q_out_bits {
 	struct mlx5_ifc_general_obj_out_cmd_hdr_bits general_obj_out_cmd_hdr;
 };
 
+struct mlx5_ifc_virtio_q_counters_bits {
+	u8    modify_field_select[0x40];
+	u8    reserved_at_40[0x40];
+	u8    received_desc[0x40];
+	u8    completed_desc[0x40];
+	u8    error_cqes[0x20];
+	u8    bad_desc_errors[0x20];
+	u8    exceed_max_chain[0x20];
+	u8    invalid_buffer[0x20];
+	u8    reserved_at_180[0x280];
+};
+
+struct mlx5_ifc_create_virtio_q_counters_in_bits {
+	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
+	struct mlx5_ifc_virtio_q_counters_bits virtio_q_counters;
+};
+
+struct mlx5_ifc_create_virtio_q_counters_out_bits {
+	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
+	struct mlx5_ifc_virtio_q_counters_bits virtio_q_counters;
+};
+
+struct mlx5_ifc_destroy_virtio_q_counters_in_bits {
+	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
+};
+
+struct mlx5_ifc_destroy_virtio_q_counters_out_bits {
+	struct mlx5_ifc_general_obj_out_cmd_hdr_bits hdr;
+};
+
+struct mlx5_ifc_query_virtio_q_counters_in_bits {
+	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
+};
+
+struct mlx5_ifc_query_virtio_q_counters_out_bits {
+	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
+	struct mlx5_ifc_virtio_q_counters_bits counters;
+};
+
 #endif /* __MLX5_IFC_VDPA_H_ */
-- 
2.35.1

Re: [PATCH v3 2/2] vdpa/mlx5: Add support for reading descriptor statistics

[Si-Wei Liu]

On 5/2/2022 3:22 AM, Eli Cohen wrote:
> Implement the get_vq_stats calback of vdpa_config_ops to return the
> statistics for a virtqueue.
>
> The statistics are provided as vendor specific statistics where the
> driver provides a pair of attribute name and attribute value.
>
> In addition to the attribute name/attribute value pair, the driver
> returns the negotiated features and max virtqueue pairs for userspace
> can decide for a given queue index whether it is a data or control
> virtqueue.
>
> Currently supported are received descriptors and completed descriptors.
>
> Acked-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
>
> v2 -> v3:
> Fix sparse warning
> ---
>   drivers/vdpa/mlx5/core/mlx5_vdpa.h |   2 +
>   drivers/vdpa/mlx5/net/mlx5_vnet.c  | 165 +++++++++++++++++++++++++++++
>   include/linux/mlx5/mlx5_ifc.h      |   1 +
>   include/linux/mlx5/mlx5_ifc_vdpa.h |  39 +++++++
>   4 files changed, 207 insertions(+)
>
> diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> index daaf7b503677..44104093163b 100644
> --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> @@ -61,6 +61,8 @@ struct mlx5_control_vq {
>   	struct vringh_kiov riov;
>   	struct vringh_kiov wiov;
>   	unsigned short head;
> +	unsigned int received_desc;
> +	unsigned int completed_desc;
>   };
>   
>   struct mlx5_vdpa_wq_ent {
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 79001301b383..473ec481813c 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -119,6 +119,7 @@ struct mlx5_vdpa_virtqueue {
>   	struct mlx5_vdpa_umem umem2;
>   	struct mlx5_vdpa_umem umem3;
>   
> +	u32 counter_set_id;
>   	bool initialized;
>   	int index;
>   	u32 virtq_id;
> @@ -164,6 +165,8 @@ struct mlx5_vdpa_net {
>   	struct notifier_block nb;
>   	struct vdpa_callback config_cb;
>   	struct mlx5_vdpa_wq_ent cvq_ent;
> +	/* sync access to virtqueues statistics */
> +	struct mutex numq_lock;
>   };
>   
>   static void free_resources(struct mlx5_vdpa_net *ndev);
> @@ -822,6 +825,12 @@ static u16 get_features_12_3(u64 features)
>   	       (!!(features & BIT_ULL(VIRTIO_NET_F_GUEST_CSUM)) << 6);
>   }
>   
> +static bool counters_supported(const struct mlx5_vdpa_dev *mvdev)
> +{
> +	return MLX5_CAP_GEN_64(mvdev->mdev, general_obj_types) &
> +	       BIT_ULL(MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS);
> +}
> +
>   static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
>   {
>   	int inlen = MLX5_ST_SZ_BYTES(create_virtio_net_q_in);
> @@ -876,6 +885,8 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
>   	MLX5_SET(virtio_q, vq_ctx, umem_3_id, mvq->umem3.id);
>   	MLX5_SET(virtio_q, vq_ctx, umem_3_size, mvq->umem3.size);
>   	MLX5_SET(virtio_q, vq_ctx, pd, ndev->mvdev.res.pdn);
> +	if (counters_supported(&ndev->mvdev))
> +		MLX5_SET(virtio_q, vq_ctx, counter_set_id, mvq->counter_set_id);
>   
>   	err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
>   	if (err)
> @@ -1139,6 +1150,47 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
>   	return err;
>   }
>   
> +static int counter_set_alloc(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> +{
> +	u32 in[MLX5_ST_SZ_DW(create_virtio_q_counters_in)] = {};
> +	u32 out[MLX5_ST_SZ_DW(create_virtio_q_counters_out)] = {};
> +	void *cmd_hdr;
> +	int err;
> +
> +	if (!counters_supported(&ndev->mvdev))
> +		return 0;
> +
> +	cmd_hdr = MLX5_ADDR_OF(create_virtio_q_counters_in, in, hdr);
> +
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, opcode, MLX5_CMD_OP_CREATE_GENERAL_OBJECT);
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, obj_type, MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS);
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);
> +
> +	err = mlx5_cmd_exec(ndev->mvdev.mdev, in, sizeof(in), out, sizeof(out));
> +	if (err)
> +		return err;
> +
> +	mvq->counter_set_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> +
> +	return 0;
> +}
> +
> +static void counter_set_dealloc(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> +{
> +	u32 in[MLX5_ST_SZ_DW(destroy_virtio_q_counters_in)] = {};
> +	u32 out[MLX5_ST_SZ_DW(destroy_virtio_q_counters_out)] = {};
> +
> +	if (!counters_supported(&ndev->mvdev))
> +		return;
> +
> +	MLX5_SET(destroy_virtio_q_counters_in, in, hdr.opcode, MLX5_CMD_OP_DESTROY_GENERAL_OBJECT);
> +	MLX5_SET(destroy_virtio_q_counters_in, in, hdr.obj_id, mvq->counter_set_id);
> +	MLX5_SET(destroy_virtio_q_counters_in, in, hdr.uid, ndev->mvdev.res.uid);
> +	MLX5_SET(destroy_virtio_q_counters_in, in, hdr.obj_type, MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS);
> +	if (mlx5_cmd_exec(ndev->mvdev.mdev, in, sizeof(in), out, sizeof(out)))
> +		mlx5_vdpa_warn(&ndev->mvdev, "dealloc counter set 0x%x\n", mvq->counter_set_id);
> +}
> +
>   static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
>   {
>   	u16 idx = mvq->index;
> @@ -1166,6 +1218,10 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
>   	if (err)
>   		goto err_connect;
>   
> +	err = counter_set_alloc(ndev, mvq);
> +	if (err)
> +		goto err_counter;
> +
The setup_vq() can be called from change_num_qps() to recreate vq stat 
counters with zero value when changing (increasing) the number of 
queues. See below.
>   	err = create_virtqueue(ndev, mvq);
>   	if (err)
>   		goto err_connect;
> @@ -1183,6 +1239,8 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
>   	return 0;
>   
>   err_connect:
> +	counter_set_dealloc(ndev, mvq);
> +err_counter:
>   	qp_destroy(ndev, &mvq->vqqp);
>   err_vqqp:
>   	qp_destroy(ndev, &mvq->fwqp);
> @@ -1227,6 +1285,7 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
>   
>   	suspend_vq(ndev, mvq);
>   	destroy_virtqueue(ndev, mvq);
> +	counter_set_dealloc(ndev, mvq);
Likewise, teardown_vq() can be called from change_num_qps() to destroy 
vq stat counters when changing (decreasing) the number of queues.

This is not ideal, at least from usability point of view it's quite 
confusing for e.g. if guest user changes the queue number via ethtool 
from 8 to 4 then back to 8, the vq stat on, say queue #6, will not 
persist. Perhaps here it needs some clarifications in the VirtIO spec 
about what should be the expected behavior for these cases:

- if queue number changes without going through a device reset, should 
any device side stat count reset to zero?
- if an individual vq gets reset but the device does not, should device 
side stat counter for the vq reset to zero?

Anyway, the above could be good enhancement with future patches. Given that,

Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>


>   	qp_destroy(ndev, &mvq->vqqp);
>   	qp_destroy(ndev, &mvq->fwqp);
>   	cq_destroy(ndev, mvq->index);
> @@ -1633,8 +1692,10 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>   			break;
>   		}
>   
> +		mutex_lock(&ndev->numq_lock);
>   		if (!change_num_qps(mvdev, newqps))
>   			status = VIRTIO_NET_OK;
> +		mutex_unlock(&ndev->numq_lock);
>   
>   		break;
>   	default:
> @@ -1681,6 +1742,7 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>   		if (read != sizeof(ctrl))
>   			break;
>   
> +		cvq->received_desc++;
>   		switch (ctrl.class) {
>   		case VIRTIO_NET_CTRL_MAC:
>   			status = handle_ctrl_mac(mvdev, ctrl.cmd);
> @@ -1704,6 +1766,7 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>   		if (vringh_need_notify_iotlb(&cvq->vring))
>   			vringh_notify(&cvq->vring);
>   
> +		cvq->completed_desc++;
>   		queue_work(mvdev->wq, &wqent->work);
>   		break;
>   	}
> @@ -2323,6 +2386,8 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>   	mlx5_vdpa_destroy_mr(&ndev->mvdev);
>   	ndev->mvdev.status = 0;
>   	ndev->cur_num_vqs = 0;
> +	ndev->mvdev.cvq.received_desc = 0;
> +	ndev->mvdev.cvq.completed_desc = 0;
>   	memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
>   	ndev->mvdev.actual_features = 0;
>   	++mvdev->generation;
> @@ -2401,6 +2466,7 @@ static void mlx5_vdpa_free(struct vdpa_device *vdev)
>   		mlx5_mpfs_del_mac(pfmdev, ndev->config.mac);
>   	}
>   	mlx5_vdpa_free_resources(&ndev->mvdev);
> +	mutex_destroy(&ndev->numq_lock);
>   	mutex_destroy(&ndev->reslock);
>   	kfree(ndev->event_cbs);
>   	kfree(ndev->vqs);
> @@ -2442,6 +2508,102 @@ static u64 mlx5_vdpa_get_driver_features(struct vdpa_device *vdev)
>   	return mvdev->actual_features;
>   }
>   
> +static int counter_set_query(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq,
> +			     u64 *received_desc, u64 *completed_desc)
> +{
> +	u32 in[MLX5_ST_SZ_DW(query_virtio_q_counters_in)] = {};
> +	u32 out[MLX5_ST_SZ_DW(query_virtio_q_counters_out)] = {};
> +	void *cmd_hdr;
> +	void *ctx;
> +	int err;
> +
> +	if (!counters_supported(&ndev->mvdev))
> +		return -EOPNOTSUPP;
> +
> +	if (mvq->fw_state != MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY)
> +		return -EAGAIN;
> +
> +	cmd_hdr = MLX5_ADDR_OF(query_virtio_q_counters_in, in, hdr);
> +
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, opcode, MLX5_CMD_OP_QUERY_GENERAL_OBJECT);
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, obj_type, MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS);
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, obj_id, mvq->counter_set_id);
> +
> +	err = mlx5_cmd_exec(ndev->mvdev.mdev, in, sizeof(in), out, sizeof(out));
> +	if (err)
> +		return err;
> +
> +	ctx = MLX5_ADDR_OF(query_virtio_q_counters_out, out, counters);
> +	*received_desc = MLX5_GET64(virtio_q_counters, ctx, received_desc);
> +	*completed_desc = MLX5_GET64(virtio_q_counters, ctx, completed_desc);
> +	return 0;
> +}
> +
> +static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> +					 struct sk_buff *msg,
> +					 struct netlink_ext_ack *extack)
> +{
> +	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> +	struct mlx5_vdpa_virtqueue *mvq;
> +	struct mlx5_control_vq *cvq;
> +	u64 received_desc;
> +	u64 completed_desc;
> +	int err = 0;
> +	u16 max_vqp;
> +
> +	mutex_lock(&ndev->numq_lock);
> +	if (!is_index_valid(mvdev, idx)) {
> +		NL_SET_ERR_MSG_MOD(extack, "virtqueue index is not valid");
> +		err = -EINVAL;
> +		goto out_err;
> +	}
> +
> +	if (idx == ctrl_vq_idx(mvdev)) {
> +		cvq = &mvdev->cvq;
> +		received_desc = cvq->received_desc;
> +		completed_desc = cvq->completed_desc;
> +		goto out;
> +	}
> +
> +	mvq = &ndev->vqs[idx];
> +	err = counter_set_query(ndev, mvq, &received_desc, &completed_desc);
> +	if (err) {
> +		NL_SET_ERR_MSG_MOD(extack, "failed to query hardware");
> +		goto out_err;
> +	}
> +
> +out:
> +	err = -EMSGSIZE;
> +	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES,
> +			      mvdev->actual_features, VDPA_ATTR_PAD))
> +		goto out_err;
> +
> +	max_vqp = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, max_vqp))
> +		goto out_err;
> +
> +	if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, "received_desc"))
> +		goto out_err;
> +
> +	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, received_desc,
> +			      VDPA_ATTR_PAD))
> +		goto out_err;
> +
> +	if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, "completed_desc"))
> +		goto out_err;
> +
> +	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, completed_desc,
> +			      VDPA_ATTR_PAD))
> +		goto out_err;
> +
> +	err = 0;
> +out_err:
> +	mutex_unlock(&ndev->numq_lock);
> +	return err;
> +}
> +
>   static const struct vdpa_config_ops mlx5_vdpa_ops = {
>   	.set_vq_address = mlx5_vdpa_set_vq_address,
>   	.set_vq_num = mlx5_vdpa_set_vq_num,
> @@ -2451,6 +2613,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
>   	.get_vq_ready = mlx5_vdpa_get_vq_ready,
>   	.set_vq_state = mlx5_vdpa_set_vq_state,
>   	.get_vq_state = mlx5_vdpa_get_vq_state,
> +	.get_vendor_vq_stats = mlx5_vdpa_get_vendor_vq_stats,
>   	.get_vq_notification = mlx5_get_vq_notification,
>   	.get_vq_irq = mlx5_get_vq_irq,
>   	.get_vq_align = mlx5_vdpa_get_vq_align,
> @@ -2706,6 +2869,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>   
>   	init_mvqs(ndev);
>   	mutex_init(&ndev->reslock);
> +	mutex_init(&ndev->numq_lock);
>   	config = &ndev->config;
>   
>   	if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) {
> @@ -2788,6 +2952,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>   	if (!is_zero_ether_addr(config->mac))
>   		mlx5_mpfs_del_mac(pfmdev, config->mac);
>   err_mtu:
> +	mutex_destroy(&ndev->numq_lock);
>   	mutex_destroy(&ndev->reslock);
>   err_alloc:
>   	put_device(&mvdev->vdev.dev);
> diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
> index 49a48d7709ac..1d193d9b6029 100644
> --- a/include/linux/mlx5/mlx5_ifc.h
> +++ b/include/linux/mlx5/mlx5_ifc.h
> @@ -94,6 +94,7 @@ enum {
>   enum {
>   	MLX5_OBJ_TYPE_GENEVE_TLV_OPT = 0x000b,
>   	MLX5_OBJ_TYPE_VIRTIO_NET_Q = 0x000d,
> +	MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS = 0x001c,
>   	MLX5_OBJ_TYPE_MATCH_DEFINER = 0x0018,
>   	MLX5_OBJ_TYPE_MKEY = 0xff01,
>   	MLX5_OBJ_TYPE_QP = 0xff02,
> diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> index 1a9c9d94cb59..4414ed5b6ed2 100644
> --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> @@ -165,4 +165,43 @@ struct mlx5_ifc_modify_virtio_net_q_out_bits {
>   	struct mlx5_ifc_general_obj_out_cmd_hdr_bits general_obj_out_cmd_hdr;
>   };
>   
> +struct mlx5_ifc_virtio_q_counters_bits {
> +	u8    modify_field_select[0x40];
> +	u8    reserved_at_40[0x40];
> +	u8    received_desc[0x40];
> +	u8    completed_desc[0x40];
> +	u8    error_cqes[0x20];
> +	u8    bad_desc_errors[0x20];
> +	u8    exceed_max_chain[0x20];
> +	u8    invalid_buffer[0x20];
> +	u8    reserved_at_180[0x280];
> +};
> +
> +struct mlx5_ifc_create_virtio_q_counters_in_bits {
> +	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
> +	struct mlx5_ifc_virtio_q_counters_bits virtio_q_counters;
> +};
> +
> +struct mlx5_ifc_create_virtio_q_counters_out_bits {
> +	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
> +	struct mlx5_ifc_virtio_q_counters_bits virtio_q_counters;
> +};
> +
> +struct mlx5_ifc_destroy_virtio_q_counters_in_bits {
> +	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
> +};
> +
> +struct mlx5_ifc_destroy_virtio_q_counters_out_bits {
> +	struct mlx5_ifc_general_obj_out_cmd_hdr_bits hdr;
> +};
> +
> +struct mlx5_ifc_query_virtio_q_counters_in_bits {
> +	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
> +};
> +
> +struct mlx5_ifc_query_virtio_q_counters_out_bits {
> +	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
> +	struct mlx5_ifc_virtio_q_counters_bits counters;
> +};
> +
>   #endif /* __MLX5_IFC_VDPA_H_ */

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

Re: [PATCH v3 2/2] vdpa/mlx5: Add support for reading descriptor statistics

[Si-Wei Liu]

On 5/2/2022 3:22 AM, Eli Cohen wrote:
> Implement the get_vq_stats calback of vdpa_config_ops to return the
> statistics for a virtqueue.
>
> The statistics are provided as vendor specific statistics where the
> driver provides a pair of attribute name and attribute value.
>
> In addition to the attribute name/attribute value pair, the driver
> returns the negotiated features and max virtqueue pairs for userspace
> can decide for a given queue index whether it is a data or control
> virtqueue.
>
> Currently supported are received descriptors and completed descriptors.
>
> Acked-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
>
> v2 -> v3:
> Fix sparse warning
> ---
>   drivers/vdpa/mlx5/core/mlx5_vdpa.h |   2 +
>   drivers/vdpa/mlx5/net/mlx5_vnet.c  | 165 +++++++++++++++++++++++++++++
>   include/linux/mlx5/mlx5_ifc.h      |   1 +
>   include/linux/mlx5/mlx5_ifc_vdpa.h |  39 +++++++
>   4 files changed, 207 insertions(+)
>
> diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> index daaf7b503677..44104093163b 100644
> --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> @@ -61,6 +61,8 @@ struct mlx5_control_vq {
>   	struct vringh_kiov riov;
>   	struct vringh_kiov wiov;
>   	unsigned short head;
> +	unsigned int received_desc;
> +	unsigned int completed_desc;
>   };
>   
>   struct mlx5_vdpa_wq_ent {
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 79001301b383..473ec481813c 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -119,6 +119,7 @@ struct mlx5_vdpa_virtqueue {
>   	struct mlx5_vdpa_umem umem2;
>   	struct mlx5_vdpa_umem umem3;
>   
> +	u32 counter_set_id;
>   	bool initialized;
>   	int index;
>   	u32 virtq_id;
> @@ -164,6 +165,8 @@ struct mlx5_vdpa_net {
>   	struct notifier_block nb;
>   	struct vdpa_callback config_cb;
>   	struct mlx5_vdpa_wq_ent cvq_ent;
> +	/* sync access to virtqueues statistics */
> +	struct mutex numq_lock;
>   };
>   
>   static void free_resources(struct mlx5_vdpa_net *ndev);
> @@ -822,6 +825,12 @@ static u16 get_features_12_3(u64 features)
>   	       (!!(features & BIT_ULL(VIRTIO_NET_F_GUEST_CSUM)) << 6);
>   }
>   
> +static bool counters_supported(const struct mlx5_vdpa_dev *mvdev)
> +{
> +	return MLX5_CAP_GEN_64(mvdev->mdev, general_obj_types) &
> +	       BIT_ULL(MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS);
> +}
> +
>   static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
>   {
>   	int inlen = MLX5_ST_SZ_BYTES(create_virtio_net_q_in);
> @@ -876,6 +885,8 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
>   	MLX5_SET(virtio_q, vq_ctx, umem_3_id, mvq->umem3.id);
>   	MLX5_SET(virtio_q, vq_ctx, umem_3_size, mvq->umem3.size);
>   	MLX5_SET(virtio_q, vq_ctx, pd, ndev->mvdev.res.pdn);
> +	if (counters_supported(&ndev->mvdev))
> +		MLX5_SET(virtio_q, vq_ctx, counter_set_id, mvq->counter_set_id);
>   
>   	err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
>   	if (err)
> @@ -1139,6 +1150,47 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
>   	return err;
>   }
>   
> +static int counter_set_alloc(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> +{
> +	u32 in[MLX5_ST_SZ_DW(create_virtio_q_counters_in)] = {};
> +	u32 out[MLX5_ST_SZ_DW(create_virtio_q_counters_out)] = {};
> +	void *cmd_hdr;
> +	int err;
> +
> +	if (!counters_supported(&ndev->mvdev))
> +		return 0;
> +
> +	cmd_hdr = MLX5_ADDR_OF(create_virtio_q_counters_in, in, hdr);
> +
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, opcode, MLX5_CMD_OP_CREATE_GENERAL_OBJECT);
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, obj_type, MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS);
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);
> +
> +	err = mlx5_cmd_exec(ndev->mvdev.mdev, in, sizeof(in), out, sizeof(out));
> +	if (err)
> +		return err;
> +
> +	mvq->counter_set_id = MLX5_GET(general_obj_out_cmd_hdr, out, obj_id);
> +
> +	return 0;
> +}
> +
> +static void counter_set_dealloc(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> +{
> +	u32 in[MLX5_ST_SZ_DW(destroy_virtio_q_counters_in)] = {};
> +	u32 out[MLX5_ST_SZ_DW(destroy_virtio_q_counters_out)] = {};
> +
> +	if (!counters_supported(&ndev->mvdev))
> +		return;
> +
> +	MLX5_SET(destroy_virtio_q_counters_in, in, hdr.opcode, MLX5_CMD_OP_DESTROY_GENERAL_OBJECT);
> +	MLX5_SET(destroy_virtio_q_counters_in, in, hdr.obj_id, mvq->counter_set_id);
> +	MLX5_SET(destroy_virtio_q_counters_in, in, hdr.uid, ndev->mvdev.res.uid);
> +	MLX5_SET(destroy_virtio_q_counters_in, in, hdr.obj_type, MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS);
> +	if (mlx5_cmd_exec(ndev->mvdev.mdev, in, sizeof(in), out, sizeof(out)))
> +		mlx5_vdpa_warn(&ndev->mvdev, "dealloc counter set 0x%x\n", mvq->counter_set_id);
> +}
> +
>   static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
>   {
>   	u16 idx = mvq->index;
> @@ -1166,6 +1218,10 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
>   	if (err)
>   		goto err_connect;
>   
> +	err = counter_set_alloc(ndev, mvq);
> +	if (err)
> +		goto err_counter;
> +
The setup_vq() can be called from change_num_qps() to recreate vq stat 
counters with zero value when changing (increasing) the number of 
queues. See below.
>   	err = create_virtqueue(ndev, mvq);
>   	if (err)
>   		goto err_connect;
> @@ -1183,6 +1239,8 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
>   	return 0;
>   
>   err_connect:
> +	counter_set_dealloc(ndev, mvq);
> +err_counter:
>   	qp_destroy(ndev, &mvq->vqqp);
>   err_vqqp:
>   	qp_destroy(ndev, &mvq->fwqp);
> @@ -1227,6 +1285,7 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
>   
>   	suspend_vq(ndev, mvq);
>   	destroy_virtqueue(ndev, mvq);
> +	counter_set_dealloc(ndev, mvq);
Likewise, teardown_vq() can be called from change_num_qps() to destroy 
vq stat counters when changing (decreasing) the number of queues.

This is not ideal, at least from usability point of view it's quite 
confusing for e.g. if guest user changes the queue number via ethtool 
from 8 to 4 then back to 8, the vq stat on, say queue #6, will not 
persist. Perhaps here it needs some clarifications in the VirtIO spec 
about what should be the expected behavior for these cases:

- if queue number changes without going through a device reset, should 
any device side stat count reset to zero?
- if an individual vq gets reset but the device does not, should device 
side stat counter for the vq reset to zero?

Anyway, the above could be good enhancement with future patches. Given that,

Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>


>   	qp_destroy(ndev, &mvq->vqqp);
>   	qp_destroy(ndev, &mvq->fwqp);
>   	cq_destroy(ndev, mvq->index);
> @@ -1633,8 +1692,10 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>   			break;
>   		}
>   
> +		mutex_lock(&ndev->numq_lock);
>   		if (!change_num_qps(mvdev, newqps))
>   			status = VIRTIO_NET_OK;
> +		mutex_unlock(&ndev->numq_lock);
>   
>   		break;
>   	default:
> @@ -1681,6 +1742,7 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>   		if (read != sizeof(ctrl))
>   			break;
>   
> +		cvq->received_desc++;
>   		switch (ctrl.class) {
>   		case VIRTIO_NET_CTRL_MAC:
>   			status = handle_ctrl_mac(mvdev, ctrl.cmd);
> @@ -1704,6 +1766,7 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>   		if (vringh_need_notify_iotlb(&cvq->vring))
>   			vringh_notify(&cvq->vring);
>   
> +		cvq->completed_desc++;
>   		queue_work(mvdev->wq, &wqent->work);
>   		break;
>   	}
> @@ -2323,6 +2386,8 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>   	mlx5_vdpa_destroy_mr(&ndev->mvdev);
>   	ndev->mvdev.status = 0;
>   	ndev->cur_num_vqs = 0;
> +	ndev->mvdev.cvq.received_desc = 0;
> +	ndev->mvdev.cvq.completed_desc = 0;
>   	memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
>   	ndev->mvdev.actual_features = 0;
>   	++mvdev->generation;
> @@ -2401,6 +2466,7 @@ static void mlx5_vdpa_free(struct vdpa_device *vdev)
>   		mlx5_mpfs_del_mac(pfmdev, ndev->config.mac);
>   	}
>   	mlx5_vdpa_free_resources(&ndev->mvdev);
> +	mutex_destroy(&ndev->numq_lock);
>   	mutex_destroy(&ndev->reslock);
>   	kfree(ndev->event_cbs);
>   	kfree(ndev->vqs);
> @@ -2442,6 +2508,102 @@ static u64 mlx5_vdpa_get_driver_features(struct vdpa_device *vdev)
>   	return mvdev->actual_features;
>   }
>   
> +static int counter_set_query(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq,
> +			     u64 *received_desc, u64 *completed_desc)
> +{
> +	u32 in[MLX5_ST_SZ_DW(query_virtio_q_counters_in)] = {};
> +	u32 out[MLX5_ST_SZ_DW(query_virtio_q_counters_out)] = {};
> +	void *cmd_hdr;
> +	void *ctx;
> +	int err;
> +
> +	if (!counters_supported(&ndev->mvdev))
> +		return -EOPNOTSUPP;
> +
> +	if (mvq->fw_state != MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY)
> +		return -EAGAIN;
> +
> +	cmd_hdr = MLX5_ADDR_OF(query_virtio_q_counters_in, in, hdr);
> +
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, opcode, MLX5_CMD_OP_QUERY_GENERAL_OBJECT);
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, obj_type, MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS);
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);
> +	MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, obj_id, mvq->counter_set_id);
> +
> +	err = mlx5_cmd_exec(ndev->mvdev.mdev, in, sizeof(in), out, sizeof(out));
> +	if (err)
> +		return err;
> +
> +	ctx = MLX5_ADDR_OF(query_virtio_q_counters_out, out, counters);
> +	*received_desc = MLX5_GET64(virtio_q_counters, ctx, received_desc);
> +	*completed_desc = MLX5_GET64(virtio_q_counters, ctx, completed_desc);
> +	return 0;
> +}
> +
> +static int mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 idx,
> +					 struct sk_buff *msg,
> +					 struct netlink_ext_ack *extack)
> +{
> +	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> +	struct mlx5_vdpa_virtqueue *mvq;
> +	struct mlx5_control_vq *cvq;
> +	u64 received_desc;
> +	u64 completed_desc;
> +	int err = 0;
> +	u16 max_vqp;
> +
> +	mutex_lock(&ndev->numq_lock);
> +	if (!is_index_valid(mvdev, idx)) {
> +		NL_SET_ERR_MSG_MOD(extack, "virtqueue index is not valid");
> +		err = -EINVAL;
> +		goto out_err;
> +	}
> +
> +	if (idx == ctrl_vq_idx(mvdev)) {
> +		cvq = &mvdev->cvq;
> +		received_desc = cvq->received_desc;
> +		completed_desc = cvq->completed_desc;
> +		goto out;
> +	}
> +
> +	mvq = &ndev->vqs[idx];
> +	err = counter_set_query(ndev, mvq, &received_desc, &completed_desc);
> +	if (err) {
> +		NL_SET_ERR_MSG_MOD(extack, "failed to query hardware");
> +		goto out_err;
> +	}
> +
> +out:
> +	err = -EMSGSIZE;
> +	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES,
> +			      mvdev->actual_features, VDPA_ATTR_PAD))
> +		goto out_err;
> +
> +	max_vqp = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, max_vqp))
> +		goto out_err;
> +
> +	if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, "received_desc"))
> +		goto out_err;
> +
> +	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, received_desc,
> +			      VDPA_ATTR_PAD))
> +		goto out_err;
> +
> +	if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, "completed_desc"))
> +		goto out_err;
> +
> +	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, completed_desc,
> +			      VDPA_ATTR_PAD))
> +		goto out_err;
> +
> +	err = 0;
> +out_err:
> +	mutex_unlock(&ndev->numq_lock);
> +	return err;
> +}
> +
>   static const struct vdpa_config_ops mlx5_vdpa_ops = {
>   	.set_vq_address = mlx5_vdpa_set_vq_address,
>   	.set_vq_num = mlx5_vdpa_set_vq_num,
> @@ -2451,6 +2613,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
>   	.get_vq_ready = mlx5_vdpa_get_vq_ready,
>   	.set_vq_state = mlx5_vdpa_set_vq_state,
>   	.get_vq_state = mlx5_vdpa_get_vq_state,
> +	.get_vendor_vq_stats = mlx5_vdpa_get_vendor_vq_stats,
>   	.get_vq_notification = mlx5_get_vq_notification,
>   	.get_vq_irq = mlx5_get_vq_irq,
>   	.get_vq_align = mlx5_vdpa_get_vq_align,
> @@ -2706,6 +2869,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>   
>   	init_mvqs(ndev);
>   	mutex_init(&ndev->reslock);
> +	mutex_init(&ndev->numq_lock);
>   	config = &ndev->config;
>   
>   	if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) {
> @@ -2788,6 +2952,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>   	if (!is_zero_ether_addr(config->mac))
>   		mlx5_mpfs_del_mac(pfmdev, config->mac);
>   err_mtu:
> +	mutex_destroy(&ndev->numq_lock);
>   	mutex_destroy(&ndev->reslock);
>   err_alloc:
>   	put_device(&mvdev->vdev.dev);
> diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
> index 49a48d7709ac..1d193d9b6029 100644
> --- a/include/linux/mlx5/mlx5_ifc.h
> +++ b/include/linux/mlx5/mlx5_ifc.h
> @@ -94,6 +94,7 @@ enum {
>   enum {
>   	MLX5_OBJ_TYPE_GENEVE_TLV_OPT = 0x000b,
>   	MLX5_OBJ_TYPE_VIRTIO_NET_Q = 0x000d,
> +	MLX5_OBJ_TYPE_VIRTIO_Q_COUNTERS = 0x001c,
>   	MLX5_OBJ_TYPE_MATCH_DEFINER = 0x0018,
>   	MLX5_OBJ_TYPE_MKEY = 0xff01,
>   	MLX5_OBJ_TYPE_QP = 0xff02,
> diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> index 1a9c9d94cb59..4414ed5b6ed2 100644
> --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> @@ -165,4 +165,43 @@ struct mlx5_ifc_modify_virtio_net_q_out_bits {
>   	struct mlx5_ifc_general_obj_out_cmd_hdr_bits general_obj_out_cmd_hdr;
>   };
>   
> +struct mlx5_ifc_virtio_q_counters_bits {
> +	u8    modify_field_select[0x40];
> +	u8    reserved_at_40[0x40];
> +	u8    received_desc[0x40];
> +	u8    completed_desc[0x40];
> +	u8    error_cqes[0x20];
> +	u8    bad_desc_errors[0x20];
> +	u8    exceed_max_chain[0x20];
> +	u8    invalid_buffer[0x20];
> +	u8    reserved_at_180[0x280];
> +};
> +
> +struct mlx5_ifc_create_virtio_q_counters_in_bits {
> +	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
> +	struct mlx5_ifc_virtio_q_counters_bits virtio_q_counters;
> +};
> +
> +struct mlx5_ifc_create_virtio_q_counters_out_bits {
> +	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
> +	struct mlx5_ifc_virtio_q_counters_bits virtio_q_counters;
> +};
> +
> +struct mlx5_ifc_destroy_virtio_q_counters_in_bits {
> +	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
> +};
> +
> +struct mlx5_ifc_destroy_virtio_q_counters_out_bits {
> +	struct mlx5_ifc_general_obj_out_cmd_hdr_bits hdr;
> +};
> +
> +struct mlx5_ifc_query_virtio_q_counters_in_bits {
> +	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
> +};
> +
> +struct mlx5_ifc_query_virtio_q_counters_out_bits {
> +	struct mlx5_ifc_general_obj_in_cmd_hdr_bits hdr;
> +	struct mlx5_ifc_virtio_q_counters_bits counters;
> +};
> +
>   #endif /* __MLX5_IFC_VDPA_H_ */