[PATCH v5 0/3] Show statistics for a vdpa device

[PATCH v5 0/3] Show statistics for a vdpa device

[Eli Cohen]

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

The first patch is just a cleanup fix.
The second patch lays the ground to allow an upstream driver to provide
statistics in the form of an attribute name/attribute value pairs.

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

v4 -> v5:
See inside the individual patches

Eli Cohen (3):
  vdpa: Fix error logic in vdpa_nl_cmd_dev_get_doit
  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                | 153 +++++++++++++++++++++++++-
 include/linux/mlx5/mlx5_ifc.h      |   1 +
 include/linux/mlx5/mlx5_ifc_vdpa.h |  39 +++++++
 include/linux/vdpa.h               |   3 +
 include/uapi/linux/vdpa.h          |   6 ++
 7 files changed, 365 insertions(+), 4 deletions(-)

-- 
2.35.1

[PATCH v5 1/3] vdpa: Fix error logic in vdpa_nl_cmd_dev_get_doit

[Eli Cohen]

In vdpa_nl_cmd_dev_get_doit(), if the call to genlmsg_reply() fails we
must not call nlmsg_free() since this is done inside genlmsg_reply().

Fix it.

Fixes: bc0d90ee021f ("vdpa: Enable user to query vdpa device info")
Acked-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/vdpa.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 2b75c00b1005..fac89a0d8178 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -756,14 +756,19 @@ static int vdpa_nl_cmd_dev_get_doit(struct sk_buff *skb, struct genl_info *info)
 		goto mdev_err;
 	}
 	err = vdpa_dev_fill(vdev, msg, info->snd_portid, info->snd_seq, 0, info->extack);
-	if (!err)
-		err = genlmsg_reply(msg, info);
+	if (err)
+		goto mdev_err;
+
+	err = genlmsg_reply(msg, info);
+	put_device(dev);
+	mutex_unlock(&vdpa_dev_mutex);
+	return err;
+
 mdev_err:
 	put_device(dev);
 err:
 	mutex_unlock(&vdpa_dev_mutex);
-	if (err)
-		nlmsg_free(msg);
+	nlmsg_free(msg);
 	return err;
 }
 
-- 
2.35.1

[PATCH v5 2/3] 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>
---
v4 -> v5:
1. Remove unused macro definition
2. Avoid policy on queue index. Do validty check in the implementation.
3. Restrict to VIRTIO_ID_NET devices only.
4. Improve netlink error messages.


 drivers/vdpa/vdpa.c       | 140 ++++++++++++++++++++++++++++++++++++++
 include/linux/vdpa.h      |   3 +
 include/uapi/linux/vdpa.h |   6 ++
 3 files changed, 149 insertions(+)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index fac89a0d8178..91f4c13c7c7c 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -914,6 +914,86 @@ 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;
+	}
+
+	switch (device_id) {
+	case VIRTIO_ID_NET:
+		if (index > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX) {
+			NL_SET_ERR_MSG_MOD(info->extack, "queue index excceeds max value");
+			err = -ERANGE;
+			break;
+		}
+
+		err = vendor_stats_fill(vdev, msg, info, index);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+	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;
@@ -995,6 +1075,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)
+		goto mdev_err;
+
+	err = genlmsg_reply(msg, info);
+
+	put_device(dev);
+	mutex_unlock(&vdpa_dev_mutex);
+
+	return err;
+
+mdev_err:
+	put_device(dev);
+dev_err:
+	nlmsg_free(msg);
+	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 },
@@ -1035,6 +1169,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..2ae8443331e1 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 */
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

[PATCH v5 3/3] 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>
---
v4 -> v5:
 Remove numq_lock mutex. Use reslock instread.


 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..99b0621e7a87 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,7 @@ 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 */
 };
 
 static void free_resources(struct mlx5_vdpa_net *ndev);
@@ -822,6 +824,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 +884,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 +1149,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 +1217,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 +1238,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 +1284,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);
@@ -1681,6 +1739,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 +1763,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 +2383,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;
@@ -2442,6 +2504,108 @@ 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->reslock);
+	if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_FEATURES_OK)) {
+		NL_SET_ERR_MSG_MOD(extack, "feature negotiation not complete");
+		err = -EAGAIN;
+		goto out_err;
+	}
+
+	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->reslock);
+	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 +2615,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,
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 v5 1/3] vdpa: Fix error logic in vdpa_nl_cmd_dev_get_doit

[Si-Wei Liu]

On 5/10/2022 4:27 AM, Eli Cohen wrote:
> In vdpa_nl_cmd_dev_get_doit(), if the call to genlmsg_reply() fails we
> must not call nlmsg_free() since this is done inside genlmsg_reply().
>
> Fix it.
>
> Fixes: bc0d90ee021f ("vdpa: Enable user to query vdpa device info")
> Acked-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
>   drivers/vdpa/vdpa.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 2b75c00b1005..fac89a0d8178 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -756,14 +756,19 @@ static int vdpa_nl_cmd_dev_get_doit(struct sk_buff *skb, struct genl_info *info)
>   		goto mdev_err;
>   	}
>   	err = vdpa_dev_fill(vdev, msg, info->snd_portid, info->snd_seq, 0, info->extack);
> -	if (!err)
> -		err = genlmsg_reply(msg, info);
> +	if (err)
> +		goto mdev_err;
> +
> +	err = genlmsg_reply(msg, info);
> +	put_device(dev);
> +	mutex_unlock(&vdpa_dev_mutex);
> +	return err;
> +
>   mdev_err:
>   	put_device(dev);
>   err:
>   	mutex_unlock(&vdpa_dev_mutex);
> -	if (err)
> -		nlmsg_free(msg);
> +	nlmsg_free(msg);
>   	return err;
>   }
>   

Re: [PATCH v5 1/3] vdpa: Fix error logic in vdpa_nl_cmd_dev_get_doit

[Si-Wei Liu]

On 5/10/2022 4:27 AM, Eli Cohen wrote:
> In vdpa_nl_cmd_dev_get_doit(), if the call to genlmsg_reply() fails we
> must not call nlmsg_free() since this is done inside genlmsg_reply().
>
> Fix it.
>
> Fixes: bc0d90ee021f ("vdpa: Enable user to query vdpa device info")
> Acked-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
>   drivers/vdpa/vdpa.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 2b75c00b1005..fac89a0d8178 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -756,14 +756,19 @@ static int vdpa_nl_cmd_dev_get_doit(struct sk_buff *skb, struct genl_info *info)
>   		goto mdev_err;
>   	}
>   	err = vdpa_dev_fill(vdev, msg, info->snd_portid, info->snd_seq, 0, info->extack);
> -	if (!err)
> -		err = genlmsg_reply(msg, info);
> +	if (err)
> +		goto mdev_err;
> +
> +	err = genlmsg_reply(msg, info);
> +	put_device(dev);
> +	mutex_unlock(&vdpa_dev_mutex);
> +	return err;
> +
>   mdev_err:
>   	put_device(dev);
>   err:
>   	mutex_unlock(&vdpa_dev_mutex);
> -	if (err)
> -		nlmsg_free(msg);
> +	nlmsg_free(msg);
>   	return err;
>   }
>   

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

Re: [PATCH v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics

[Si-Wei Liu]

On 5/10/2022 4:27 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>
> ---
> v4 -> v5:
>   Remove numq_lock mutex. Use reslock instread.
>
>
>   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..99b0621e7a87 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,7 @@ 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 */
Dangling code change?

>   };
>   
>   static void free_resources(struct mlx5_vdpa_net *ndev);
> @@ -822,6 +824,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 +884,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 +1149,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 +1217,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 +1238,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 +1284,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);
> @@ -1681,6 +1739,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 +1763,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 +2383,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;
> @@ -2442,6 +2504,108 @@ 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->reslock);
I wonder if we can change this lock to r/w semaphore too, otherwise it 
almost defeats the merit of converting vdpa_dev_mutex to the same. This 
change would benefit multiple parallel readers.
> +	if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> +		NL_SET_ERR_MSG_MOD(extack, "feature negotiation not complete");
> +		err = -EAGAIN;
> +		goto out_err;
> +	}
> +
> +	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;
Your userspace reference patch doesn't actually use this attribute, but 
instead calls the VDPA_CMD_DEV_CONFIG_GET in prior to get this 
information, which will break consistency. Is it your plan to change the 
userspace code to accommodate what's already piggybacked here and 
display stat query in just one atomic call? Hope all the available attrs 
here would satisfy the userspace need.

Thanks,
-Siwei

> +
> +	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->reslock);
> +	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 +2615,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,
> 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_ */

Re: [PATCH v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics

[Si-Wei Liu]

On 5/10/2022 4:27 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>
> ---
> v4 -> v5:
>   Remove numq_lock mutex. Use reslock instread.
>
>
>   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..99b0621e7a87 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,7 @@ 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 */
Dangling code change?

>   };
>   
>   static void free_resources(struct mlx5_vdpa_net *ndev);
> @@ -822,6 +824,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 +884,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 +1149,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 +1217,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 +1238,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 +1284,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);
> @@ -1681,6 +1739,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 +1763,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 +2383,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;
> @@ -2442,6 +2504,108 @@ 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->reslock);
I wonder if we can change this lock to r/w semaphore too, otherwise it 
almost defeats the merit of converting vdpa_dev_mutex to the same. This 
change would benefit multiple parallel readers.
> +	if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> +		NL_SET_ERR_MSG_MOD(extack, "feature negotiation not complete");
> +		err = -EAGAIN;
> +		goto out_err;
> +	}
> +
> +	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;
Your userspace reference patch doesn't actually use this attribute, but 
instead calls the VDPA_CMD_DEV_CONFIG_GET in prior to get this 
information, which will break consistency. Is it your plan to change the 
userspace code to accommodate what's already piggybacked here and 
display stat query in just one atomic call? Hope all the available attrs 
here would satisfy the userspace need.

Thanks,
-Siwei

> +
> +	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->reslock);
> +	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 +2615,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,
> 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 v5 2/3] vdpa: Add support for querying vendor statistics

[Si-Wei Liu]

On 5/10/2022 4:27 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
The [IETF RFC 8259] "The JavaScript Object Notation (JSON) Data 
Interchange Format" states:

... The names within an object SHOULD be unique.
:
:
:
    An object whose names are all unique is interoperable in the sense
    that all software implementations receiving that object will agree on
    the name-value mappings.  When the names within an object are not
    unique, the behavior of software that receives such an object is
    unpredictable.  Many implementations report the last name/value pair
    only.  Other implementations report an error or fail to parse the
    object, and some implementations report all of the name/value pairs,
    including duplicates.


that said, duplicate keys in a JSON object are really not recommended, 
which have the potential of breaking a lot of json parser tools. Are you 
going to revise the example, and implement what Parav [1] or me [2] 
suggested earlier? What worries me is that the  new output would 
ultimately affect the placement of attributes in the kernel side. And 
looking at the userspace implementation, it doesn't seem like the paring 
and the order of "name" and "value" keys are ensured: for e.g. name1 
name2 value2, value1 value2 name2, name1 name2 value1 value2 should all 
be invalid. A buggy vendor driver implementation which doesn't pay 
attention to this could easily break script-able JSON output. IMHO it'd 
be good to have clearly structured and self-describing attribute 
placement in the kernel rather than have userspace validate all invalid 
combinations.

[1] 
https://lore.kernel.org/virtualization/PH0PR12MB5481F6D44451F01814470112DC089@PH0PR12MB5481.namprd12.prod.outlook.com/
[2] 
https://lore.kernel.org/virtualization/6175d620-6be3-c249-5482-0a9448499e4a@oracle.com/


Thanks,
-Siwei
>          }
>      }
> }
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> v4 -> v5:
> 1. Remove unused macro definition
> 2. Avoid policy on queue index. Do validty check in the implementation.
> 3. Restrict to VIRTIO_ID_NET devices only.
> 4. Improve netlink error messages.
>
>
>   drivers/vdpa/vdpa.c       | 140 ++++++++++++++++++++++++++++++++++++++
>   include/linux/vdpa.h      |   3 +
>   include/uapi/linux/vdpa.h |   6 ++
>   3 files changed, 149 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index fac89a0d8178..91f4c13c7c7c 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -914,6 +914,86 @@ 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;
> +	}
> +
> +	switch (device_id) {
> +	case VIRTIO_ID_NET:
> +		if (index > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX) {
> +			NL_SET_ERR_MSG_MOD(info->extack, "queue index excceeds max value");
> +			err = -ERANGE;
> +			break;
> +		}
> +
> +		err = vendor_stats_fill(vdev, msg, info, index);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +		break;
> +	}
> +	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;
> @@ -995,6 +1075,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)
> +		goto mdev_err;
> +
> +	err = genlmsg_reply(msg, info);
> +
> +	put_device(dev);
> +	mutex_unlock(&vdpa_dev_mutex);
> +
> +	return err;
> +
> +mdev_err:
> +	put_device(dev);
> +dev_err:
> +	nlmsg_free(msg);
> +	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 },
> @@ -1035,6 +1169,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..2ae8443331e1 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 */
> 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 v5 2/3] vdpa: Add support for querying vendor statistics

[Si-Wei Liu]

On 5/10/2022 4:27 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
The [IETF RFC 8259] "The JavaScript Object Notation (JSON) Data 
Interchange Format" states:

... The names within an object SHOULD be unique.
:
:
:
    An object whose names are all unique is interoperable in the sense
    that all software implementations receiving that object will agree on
    the name-value mappings.  When the names within an object are not
    unique, the behavior of software that receives such an object is
    unpredictable.  Many implementations report the last name/value pair
    only.  Other implementations report an error or fail to parse the
    object, and some implementations report all of the name/value pairs,
    including duplicates.


that said, duplicate keys in a JSON object are really not recommended, 
which have the potential of breaking a lot of json parser tools. Are you 
going to revise the example, and implement what Parav [1] or me [2] 
suggested earlier? What worries me is that the  new output would 
ultimately affect the placement of attributes in the kernel side. And 
looking at the userspace implementation, it doesn't seem like the paring 
and the order of "name" and "value" keys are ensured: for e.g. name1 
name2 value2, value1 value2 name2, name1 name2 value1 value2 should all 
be invalid. A buggy vendor driver implementation which doesn't pay 
attention to this could easily break script-able JSON output. IMHO it'd 
be good to have clearly structured and self-describing attribute 
placement in the kernel rather than have userspace validate all invalid 
combinations.

[1] 
https://lore.kernel.org/virtualization/PH0PR12MB5481F6D44451F01814470112DC089@PH0PR12MB5481.namprd12.prod.outlook.com/
[2] 
https://lore.kernel.org/virtualization/6175d620-6be3-c249-5482-0a9448499e4a@oracle.com/


Thanks,
-Siwei
>          }
>      }
> }
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> v4 -> v5:
> 1. Remove unused macro definition
> 2. Avoid policy on queue index. Do validty check in the implementation.
> 3. Restrict to VIRTIO_ID_NET devices only.
> 4. Improve netlink error messages.
>
>
>   drivers/vdpa/vdpa.c       | 140 ++++++++++++++++++++++++++++++++++++++
>   include/linux/vdpa.h      |   3 +
>   include/uapi/linux/vdpa.h |   6 ++
>   3 files changed, 149 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index fac89a0d8178..91f4c13c7c7c 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -914,6 +914,86 @@ 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;
> +	}
> +
> +	switch (device_id) {
> +	case VIRTIO_ID_NET:
> +		if (index > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX) {
> +			NL_SET_ERR_MSG_MOD(info->extack, "queue index excceeds max value");
> +			err = -ERANGE;
> +			break;
> +		}
> +
> +		err = vendor_stats_fill(vdev, msg, info, index);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +		break;
> +	}
> +	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;
> @@ -995,6 +1075,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)
> +		goto mdev_err;
> +
> +	err = genlmsg_reply(msg, info);
> +
> +	put_device(dev);
> +	mutex_unlock(&vdpa_dev_mutex);
> +
> +	return err;
> +
> +mdev_err:
> +	put_device(dev);
> +dev_err:
> +	nlmsg_free(msg);
> +	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 },
> @@ -1035,6 +1169,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..2ae8443331e1 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 */
> 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 v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics

[Jason Wang]

On Thu, May 12, 2022 at 1:26 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 5/10/2022 4:27 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>
> > ---
> > v4 -> v5:
> >   Remove numq_lock mutex. Use reslock instread.
> >
> >
> >   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..99b0621e7a87 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,7 @@ 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 */
> Dangling code change?
>
> >   };
> >
> >   static void free_resources(struct mlx5_vdpa_net *ndev);
> > @@ -822,6 +824,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 +884,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 +1149,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 +1217,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 +1238,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 +1284,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);
> > @@ -1681,6 +1739,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 +1763,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 +2383,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;
> > @@ -2442,6 +2504,108 @@ 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->reslock);
> I wonder if we can change this lock to r/w semaphore too, otherwise it
> almost defeats the merit of converting vdpa_dev_mutex to the same. This
> change would benefit multiple parallel readers.
> > +     if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> > +             NL_SET_ERR_MSG_MOD(extack, "feature negotiation not complete");
> > +             err = -EAGAIN;
> > +             goto out_err;
> > +     }
> > +
> > +     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;
> Your userspace reference patch doesn't actually use this attribute, but
> instead calls the VDPA_CMD_DEV_CONFIG_GET in prior to get this
> information, which will break consistency. Is it your plan to change the
> userspace code to accommodate what's already piggybacked here and
> display stat query in just one atomic call? Hope all the available attrs
> here would satisfy the userspace need.

Right, I mentioned this in V4. If we depend on the vendor driver to
report the negotiated features, we will end up driver specific code in
the userspace which is sub-optimal.

I think we need to do this in the vdpa core so userspace knows for
sure it can get this.

THanks

>
> Thanks,
> -Siwei
>
> > +
> > +     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->reslock);
> > +     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 +2615,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,
> > 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_ */
>

Re: [PATCH v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics

[Jason Wang]

On Thu, May 12, 2022 at 1:26 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 5/10/2022 4:27 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>
> > ---
> > v4 -> v5:
> >   Remove numq_lock mutex. Use reslock instread.
> >
> >
> >   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..99b0621e7a87 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,7 @@ 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 */
> Dangling code change?
>
> >   };
> >
> >   static void free_resources(struct mlx5_vdpa_net *ndev);
> > @@ -822,6 +824,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 +884,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 +1149,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 +1217,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 +1238,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 +1284,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);
> > @@ -1681,6 +1739,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 +1763,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 +2383,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;
> > @@ -2442,6 +2504,108 @@ 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->reslock);
> I wonder if we can change this lock to r/w semaphore too, otherwise it
> almost defeats the merit of converting vdpa_dev_mutex to the same. This
> change would benefit multiple parallel readers.
> > +     if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> > +             NL_SET_ERR_MSG_MOD(extack, "feature negotiation not complete");
> > +             err = -EAGAIN;
> > +             goto out_err;
> > +     }
> > +
> > +     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;
> Your userspace reference patch doesn't actually use this attribute, but
> instead calls the VDPA_CMD_DEV_CONFIG_GET in prior to get this
> information, which will break consistency. Is it your plan to change the
> userspace code to accommodate what's already piggybacked here and
> display stat query in just one atomic call? Hope all the available attrs
> here would satisfy the userspace need.

Right, I mentioned this in V4. If we depend on the vendor driver to
report the negotiated features, we will end up driver specific code in
the userspace which is sub-optimal.

I think we need to do this in the vdpa core so userspace knows for
sure it can get this.

THanks

>
> Thanks,
> -Siwei
>
> > +
> > +     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->reslock);
> > +     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 +2615,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,
> > 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 v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics

[Eli Cohen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Thursday, May 12, 2022 5:28 AM
> To: Si-Wei Liu <si-wei.liu@oracle.com>
> Cc: Eli Cohen <elic@nvidia.com>; mst <mst@redhat.com>; virtualization <virtualization@lists.linux-foundation.org>; linux-kernel <linux-
> kernel@vger.kernel.org>
> Subject: Re: [PATCH v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics
> 
> On Thu, May 12, 2022 at 1:26 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >
> >
> >
> > On 5/10/2022 4:27 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>
> > > ---
> > > v4 -> v5:
> > >   Remove numq_lock mutex. Use reslock instread.
> > >
> > >
> > >   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..99b0621e7a87 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,7 @@ 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 */
> > Dangling code change?
> >
> > >   };
> > >
> > >   static void free_resources(struct mlx5_vdpa_net *ndev);
> > > @@ -822,6 +824,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 +884,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 +1149,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 +1217,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 +1238,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 +1284,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);
> > > @@ -1681,6 +1739,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 +1763,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 +2383,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;
> > > @@ -2442,6 +2504,108 @@ 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->reslock);
> > I wonder if we can change this lock to r/w semaphore too, otherwise it
> > almost defeats the merit of converting vdpa_dev_mutex to the same. This
> > change would benefit multiple parallel readers.
> > > +     if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> > > +             NL_SET_ERR_MSG_MOD(extack, "feature negotiation not complete");
> > > +             err = -EAGAIN;
> > > +             goto out_err;
> > > +     }
> > > +
> > > +     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;
> > Your userspace reference patch doesn't actually use this attribute, but
> > instead calls the VDPA_CMD_DEV_CONFIG_GET in prior to get this
> > information, which will break consistency. Is it your plan to change the
> > userspace code to accommodate what's already piggybacked here and
> > display stat query in just one atomic call? Hope all the available attrs
> > here would satisfy the userspace need.
> 
> Right, I mentioned this in V4.

I may have missed that email.

Anyways, seems like I sent over the old version of the patch which matched the older kernel implementation.
Let me send it over again.

> If we depend on the vendor driver to
> report the negotiated features, we will end up driver specific code in
> the userspace which is sub-optimal.
> 
> I think we need to do this in the vdpa core so userspace knows for
> sure it can get this.

OK,  so I will resend a new series and include the rw_semaphore patch in the list.

> 
> THanks
> 
> >
> > Thanks,
> > -Siwei
> >
> > > +
> > > +     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->reslock);
> > > +     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 +2615,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,
> > > 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_ */
> >

Re: [PATCH v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics

[Si-Wei Liu]

[-- Attachment #1.1: Type: text/plain, Size: 921 bytes --]



On 5/11/2022 10:25 AM, Si-Wei Liu wrote:
>>
>> +
>> +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->reslock);
> I wonder if we can change this lock to r/w semaphore too, otherwise it 
> almost defeats the merit of converting vdpa_dev_mutex to the same. 
> This change would benefit multiple parallel readers. 
It looks like the lately posted v6 series missed this change somehow?

-Siwei

[-- Attachment #1.2: Type: text/html, Size: 1878 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

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

Re: [PATCH v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics

[Si-Wei Liu]

On 5/11/2022 10:25 AM, Si-Wei Liu wrote:
>>
>> +
>> +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->reslock);
> I wonder if we can change this lock to r/w semaphore too, otherwise it 
> almost defeats the merit of converting vdpa_dev_mutex to the same. 
> This change would benefit multiple parallel readers. 
It looks like the lately posted v6 series missed this change somehow?

-Siwei

Re: [PATCH v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics

[Si-Wei Liu]

On 5/11/2022 10:25 AM, Si-Wei Liu wrote:
>>
>> +
>> +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->reslock);
> I wonder if we can change this lock to r/w semaphore too, otherwise it 
> almost defeats the merit of converting vdpa_dev_mutex to the same. 
> This change would benefit multiple parallel readers. 
It looks like the lately posted v6 series missed this change somehow?

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

Re: [PATCH v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics

[Si-Wei Liu]

On 5/11/2022 10:25 AM, Si-Wei Liu wrote:
>>
>> @@ -164,6 +165,7 @@ 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 */
> Dangling code change?
... and this.

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

Re: [PATCH v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics

[Si-Wei Liu]

On 5/11/2022 10:25 AM, Si-Wei Liu wrote:
>>
>> @@ -164,6 +165,7 @@ 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 */
> Dangling code change?
... and this.

Thanks,
-Siwei

Re: [PATCH v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics

[Si-Wei Liu]

On 5/11/2022 7:27 PM, Jason Wang wrote:
> On Thu, May 12, 2022 at 1:26 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 5/10/2022 4:27 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>
>>> ---
>>> v4 -> v5:
>>>    Remove numq_lock mutex. Use reslock instread.
>>>
>>>
>>>    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..99b0621e7a87 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,7 @@ 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 */
>> Dangling code change?
>>
>>>    };
>>>
>>>    static void free_resources(struct mlx5_vdpa_net *ndev);
>>> @@ -822,6 +824,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 +884,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 +1149,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 +1217,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 +1238,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 +1284,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);
>>> @@ -1681,6 +1739,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 +1763,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 +2383,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;
>>> @@ -2442,6 +2504,108 @@ 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->reslock);
>> I wonder if we can change this lock to r/w semaphore too, otherwise it
>> almost defeats the merit of converting vdpa_dev_mutex to the same. This
>> change would benefit multiple parallel readers.
>>> +     if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_FEATURES_OK)) {
>>> +             NL_SET_ERR_MSG_MOD(extack, "feature negotiation not complete");
>>> +             err = -EAGAIN;
>>> +             goto out_err;
>>> +     }
>>> +
>>> +     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;
>> Your userspace reference patch doesn't actually use this attribute, but
>> instead calls the VDPA_CMD_DEV_CONFIG_GET in prior to get this
>> information, which will break consistency. Is it your plan to change the
>> userspace code to accommodate what's already piggybacked here and
>> display stat query in just one atomic call? Hope all the available attrs
>> here would satisfy the userspace need.
> Right, I mentioned this in V4. If we depend on the vendor driver to
> report the negotiated features, we will end up driver specific code in
> the userspace which is sub-optimal.
>
> I think we need to do this in the vdpa core so userspace knows for
> sure it can get this.
Yes, that's what I thought and once suggested, too. This would make 
userspace simple and also ease driver implementation.

-Siwei

>
> THanks
>
>> Thanks,
>> -Siwei
>>
>>> +
>>> +     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->reslock);
>>> +     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 +2615,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,
>>> 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_ */

Re: [PATCH v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics

[Si-Wei Liu]

On 5/11/2022 7:27 PM, Jason Wang wrote:
> On Thu, May 12, 2022 at 1:26 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 5/10/2022 4:27 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>
>>> ---
>>> v4 -> v5:
>>>    Remove numq_lock mutex. Use reslock instread.
>>>
>>>
>>>    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..99b0621e7a87 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,7 @@ 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 */
>> Dangling code change?
>>
>>>    };
>>>
>>>    static void free_resources(struct mlx5_vdpa_net *ndev);
>>> @@ -822,6 +824,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 +884,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 +1149,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 +1217,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 +1238,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 +1284,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);
>>> @@ -1681,6 +1739,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 +1763,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 +2383,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;
>>> @@ -2442,6 +2504,108 @@ 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->reslock);
>> I wonder if we can change this lock to r/w semaphore too, otherwise it
>> almost defeats the merit of converting vdpa_dev_mutex to the same. This
>> change would benefit multiple parallel readers.
>>> +     if (!(ndev->mvdev.status & VIRTIO_CONFIG_S_FEATURES_OK)) {
>>> +             NL_SET_ERR_MSG_MOD(extack, "feature negotiation not complete");
>>> +             err = -EAGAIN;
>>> +             goto out_err;
>>> +     }
>>> +
>>> +     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;
>> Your userspace reference patch doesn't actually use this attribute, but
>> instead calls the VDPA_CMD_DEV_CONFIG_GET in prior to get this
>> information, which will break consistency. Is it your plan to change the
>> userspace code to accommodate what's already piggybacked here and
>> display stat query in just one atomic call? Hope all the available attrs
>> here would satisfy the userspace need.
> Right, I mentioned this in V4. If we depend on the vendor driver to
> report the negotiated features, we will end up driver specific code in
> the userspace which is sub-optimal.
>
> I think we need to do this in the vdpa core so userspace knows for
> sure it can get this.
Yes, that's what I thought and once suggested, too. This would make 
userspace simple and also ease driver implementation.

-Siwei

>
> THanks
>
>> Thanks,
>> -Siwei
>>
>>> +
>>> +     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->reslock);
>>> +     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 +2615,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,
>>> 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 v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics

[Eli Cohen]

> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Friday, May 13, 2022 12:15 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 v5 3/3] vdpa/mlx5: Add support for reading descriptor statistics
> 
> 
> On 5/11/2022 10:25 AM, Si-Wei Liu wrote:
> >>
> >> +
> >> +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->reslock);
> > I wonder if we can change this lock to r/w semaphore too, otherwise it
> > almost defeats the merit of converting vdpa_dev_mutex to the same.
> > This change would benefit multiple parallel readers.
> It looks like the lately posted v6 series missed this change somehow?

Yes, I missed that. I will add it as an additional patch.


> 
> -Siwei