Si-Wei Liu
2022-May-12 21:33 UTC
[PATCH v1 for reference] vdpa: Add support for reading vdpa device statistics
On 5/11/2022 10:40 PM, Eli Cohen wrote:> Read statistics of a vdpa device. The specific data is a received as a > pair of attribute name and attribute value. > > Examples: > 1. Read statistics for the virtqueue at index 1 > > $ vdpa dev vstats show vdpa-a qidx 1 > vdpa-a: > qtype tx qidx 1 received_desc 4583367 completed_desc 4583367 > > 2. Read statistics for the virtqueue at index 32 > $ vdpa dev vstats show vdpa-a qidx 32 > vdpa-a: > qtype cvq qidx 32 received_desc 0 completed_desc 0 > > 3. Read statisitics for the virtqueue at index 0 with json output > $ vdpa -j dev vstats show vdpa-a qidx 0 > {"vstats":{"vdpa-a":{ > "qtype":"rx","qidx":0,"name":"received_desc","value":490205,"name":"completed_desc","value":489964}}} > > 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": { > > "qtype": "rx", > "qidx": 0, > "name": "received_desc", > "value": 490205, > "name": "completed_desc", > "value": 489964 > } > } > } > > Issue: 2888554 > Change-Id: I137115fd3db258c5b95fa53dabc2cd4c364a174b > Signed-off-by: Eli Cohen <elic at nvidia.com> > --- > vdpa/include/uapi/linux/vdpa.h | 6 ++ > vdpa/vdpa.c | 153 +++++++++++++++++++++++++++++++++ > 2 files changed, 159 insertions(+) > > diff --git a/vdpa/include/uapi/linux/vdpa.h b/vdpa/include/uapi/linux/vdpa.h > index cc575a825a7c..7f52e703f1ad 100644 > --- a/vdpa/include/uapi/linux/vdpa.h > +++ b/vdpa/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_STATS_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, > }; > diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c > index 3ae1b78f4cac..4f217993c73b 100644 > --- a/vdpa/vdpa.c > +++ b/vdpa/vdpa.c > @@ -26,6 +26,7 @@ > #define VDPA_OPT_VDEV_MAC BIT(4) > #define VDPA_OPT_VDEV_MTU BIT(5) > #define VDPA_OPT_MAX_VQP BIT(6) > +#define VDPA_OPT_QUEUE_INDEX BIT(7) > > struct vdpa_opts { > uint64_t present; /* flags of present items */ > @@ -36,6 +37,7 @@ struct vdpa_opts { > char mac[ETH_ALEN]; > uint16_t mtu; > uint16_t max_vqp; > + uint32_t queue_idx; > }; > > struct vdpa { > @@ -174,6 +176,17 @@ static int vdpa_argv_u16(struct vdpa *vdpa, int argc, char **argv, > return get_u16(result, *argv, 10); > } > > +static int vdpa_argv_u32(struct vdpa *vdpa, int argc, char **argv, > + uint32_t *result) > +{ > + if (argc <= 0 || *argv == NULL) { > + fprintf(stderr, "number expected\n"); > + return -EINVAL; > + } > + > + return get_u32(result, *argv, 10); > +} > + > struct vdpa_args_metadata { > uint64_t o_flag; > const char *err_msg; > @@ -183,6 +196,7 @@ static const struct vdpa_args_metadata vdpa_args_required[] = { > {VDPA_OPT_VDEV_MGMTDEV_HANDLE, "management device handle not set."}, > {VDPA_OPT_VDEV_NAME, "device name is not set."}, > {VDPA_OPT_VDEV_HANDLE, "device name is not set."}, > + {VDPA_OPT_QUEUE_INDEX, "queue index is not set."}, > }; > > static int vdpa_args_finding_required_validate(uint64_t o_required, > @@ -228,6 +242,8 @@ static void vdpa_opts_put(struct nlmsghdr *nlh, struct vdpa *vdpa) > mnl_attr_put_u16(nlh, VDPA_ATTR_DEV_NET_CFG_MTU, opts->mtu); > if (opts->present & VDPA_OPT_MAX_VQP) > mnl_attr_put_u16(nlh, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, opts->max_vqp); > + if (opts->present & VDPA_OPT_QUEUE_INDEX) > + mnl_attr_put_u32(nlh, VDPA_ATTR_DEV_QUEUE_INDEX, opts->queue_idx); > } > > static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv, > @@ -304,6 +320,14 @@ static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv, > > NEXT_ARG_FWD(); > o_found |= VDPA_OPT_MAX_VQP; > + } else if ((matches(*argv, "qidx") == 0) && (o_optional & VDPA_OPT_QUEUE_INDEX)) { > + NEXT_ARG_FWD(); > + err = vdpa_argv_u32(vdpa, argc, argv, &opts->queue_idx); > + if (err) > + return err; > + > + NEXT_ARG_FWD(); > + o_found |= VDPA_OPT_QUEUE_INDEX; > } else { > fprintf(stderr, "Unknown option \"%s\"\n", *argv); > return -EINVAL; > @@ -594,6 +618,7 @@ static void cmd_dev_help(void) > fprintf(stderr, " [ max_vqp MAX_VQ_PAIRS ]\n"); > fprintf(stderr, " vdpa dev del DEV\n"); > fprintf(stderr, "Usage: vdpa dev config COMMAND [ OPTIONS ]\n"); > + fprintf(stderr, "Usage: vdpa dev vstats COMMAND\n"); > } > > static const char *device_type_name(uint32_t type) > @@ -819,6 +844,132 @@ static int cmd_dev_config(struct vdpa *vdpa, int argc, char **argv) > return -ENOENT; > } > > +#define MAX_KEY_LEN 200 > +/* 5 bytes for format */ > +#define MAX_FMT_LEN (MAX_KEY_LEN + 5 + 1) > + > +static void pr_out_dev_net_vstats(const struct nlmsghdr *nlh) > +{ > + const char *name = NULL; > + uint64_t features = 0; > + char fmt[MAX_FMT_LEN]; > + uint16_t max_vqp = 0; > + bool is_ctrl = false; > + struct nlattr *attr; > + uint16_t qidx = 0; > + uint64_t v64; > + > + mnl_attr_for_each(attr, nlh, sizeof(struct genlmsghdr)) { > + switch (attr->nla_type) { > + case VDPA_ATTR_DEV_NEGOTIATED_FEATURES: > + features = mnl_attr_get_u64(attr); > + break; > + case VDPA_ATTR_DEV_NET_CFG_MAX_VQP: > + max_vqp = mnl_attr_get_u16(attr); > + break; > + case VDPA_ATTR_DEV_QUEUE_INDEX: > + qidx = mnl_attr_get_u16(attr); > + is_ctrl = features & BIT(VIRTIO_NET_F_CTRL_VQ) && qidx == 2 * max_vqp;Here there's no guarantee in the (userspace) code you could get valid features and max_vqp value although these two attributes may well be present in the table right after the VDPA_ATTR_DEV_QUEUE_INDEX. I think it's with less point to make userspace code complicated to check the ordering and fail buggy driver implementation. Why not follow Jason's suggestion to have vdpa core insert DEV_NEGOTIATED_FEATURES, DEV_NET_CFG_MAX_VQP as well as VDPA_ATTR_DEV_QUEUE_INDEX attrs ahead of other vendor driver stats? Then there's no need to duplicate the logic for the common attrs in each vendor driver.> + if (!is_ctrl) { > + if (qidx & 1) > + print_string(PRINT_ANY, "queue_type", "queue_type %s ", "tx"); > + else > + print_string(PRINT_ANY, "queue_type", "queue_type %s ", "rx"); > + } else { > + print_string(PRINT_ANY, "queue_type", "queue_type %s ", "control_vq"); > + } > + break; > + case VDPA_ATTR_DEV_VENDOR_ATTR_NAME: > + name = mnl_attr_get_str(attr); > + if (strlen(name) > MAX_KEY_LEN) > + return; > + > + strcpy(fmt, name); > + strcat(fmt, " %lu "); > + break; > + case VDPA_ATTR_DEV_VENDOR_ATTR_VALUE: > + v64 = mnl_attr_get_u64(attr); > + print_u64(PRINT_ANY, name, fmt, v64); > + break;Likewise, I believe these vendor stats need more structured message layout to ensure pairing and order.> > [1] > https://lore.kernel.org/virtualization/PH0PR12MB5481F6D44451F01814470112DC089 at PH0PR12MB5481.namprd12.prod.outlook.com/ > > [2] > https://lore.kernel.org/virtualization/6175d620-6be3-c249-5482-0a9448499e4a at oracle.com/Thanks, -Siwei> + } > + } > +} > + > +static void pr_out_dev_vstats(struct vdpa *vdpa, struct nlattr **tb, const struct nlmsghdr *nlh) > +{ > + uint32_t device_id = mnl_attr_get_u32(tb[VDPA_ATTR_DEV_ID]); > + > + pr_out_vdev_handle_start(vdpa, tb); > + switch (device_id) { > + case VIRTIO_ID_NET: > + pr_out_dev_net_vstats(nlh); > + break; > + default: > + break; > + } > + pr_out_vdev_handle_end(vdpa); > +} > + > +static int cmd_dev_vstats_show_cb(const struct nlmsghdr *nlh, void *data) > +{ > + struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh); > + struct nlattr *tb[VDPA_ATTR_MAX + 1] = {}; > + struct vdpa *vdpa = data; > + > + mnl_attr_parse(nlh, sizeof(*genl), attr_cb, tb); > + if (!tb[VDPA_ATTR_DEV_NAME] || !tb[VDPA_ATTR_DEV_ID]) > + return MNL_CB_ERROR; > + pr_out_dev_vstats(vdpa, tb, nlh); > + return MNL_CB_OK; > +} > + > +static void cmd_dev_vstats_help(void) > +{ > + fprintf(stderr, "Usage: vdpa dev vstats show DEV [qidx QUEUE_INDEX]\n"); > +} > + > +static int cmd_dev_vstats_show(struct vdpa *vdpa, int argc, char **argv) > +{ > + uint16_t flags = NLM_F_REQUEST | NLM_F_ACK; > + struct nlmsghdr *nlh; > + int err; > + > + if (argc != 1 && argc != 3) { > + cmd_dev_vstats_help(); > + return -EINVAL; > + } > + > + nlh = mnlu_gen_socket_cmd_prepare(&vdpa->nlg, VDPA_CMD_DEV_STATS_GET, > + flags); > + > + err = vdpa_argv_parse_put(nlh, vdpa, argc, argv, > + VDPA_OPT_VDEV_HANDLE, VDPA_OPT_QUEUE_INDEX); > + if (err) > + return err; > + > + pr_out_section_start(vdpa, "vstats"); > + err = mnlu_gen_socket_sndrcv(&vdpa->nlg, nlh, cmd_dev_vstats_show_cb, vdpa); > + pr_out_section_end(vdpa); > + return 0; > +} > + > +static int cmd_dev_vstats(struct vdpa *vdpa, int argc, char **argv) > +{ > + if (argc < 1) { > + cmd_dev_vstats_help(); > + return -EINVAL; > + } > + > + if (matches(*argv, "help") == 0) { > + cmd_dev_vstats_help(); > + return 0; > + } else if (matches(*argv, "show") == 0) { > + return cmd_dev_vstats_show(vdpa, argc - 1, argv + 1); > + } > + fprintf(stderr, "Command \"%s\" not found\n", *argv); > + return -ENOENT; > +} > + > static int cmd_dev(struct vdpa *vdpa, int argc, char **argv) > { > if (!argc) > @@ -836,6 +987,8 @@ static int cmd_dev(struct vdpa *vdpa, int argc, char **argv) > return cmd_dev_del(vdpa, argc - 1, argv + 1); > } else if (matches(*argv, "config") == 0) { > return cmd_dev_config(vdpa, argc - 1, argv + 1); > + } else if (matches(*argv, "vstats") == 0) { > + return cmd_dev_vstats(vdpa, argc - 1, argv + 1); > } > fprintf(stderr, "Command \"%s\" not found\n", *argv); > return -ENOENT;