| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 16 May 2022 05:46:49 +0000
From: Eli Cohen <elic@...dia.com>
To: Si-Wei Liu <si-wei.liu@...cle.com>,
"mst@...hat.com" <mst@...hat.com>,
"jasowang@...hat.com" <jasowang@...hat.com>
CC: "virtualization@...ts.linux-foundation.org"
<virtualization@...ts.linux-foundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v1 for reference] vdpa: Add support for reading vdpa
device statistics
> -----Original Message-----
> From: Si-Wei Liu <si-wei.liu@...cle.com>
> Sent: Friday, May 13, 2022 12:34 AM
> To: Eli Cohen <elic@...dia.com>; mst@...hat.com; jasowang@...hat.com
> Cc: virtualization@...ts.linux-foundation.org; linux-kernel@...r.kernel.org
> Subject: Re: [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@...dia.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.
I wonder if we're not going in circles. At first I thought it would be better to the kernel driver
digest everything and give it to userspace. For each record we would pass back the attribute
name, attribute value, and an indication of the type of queue: TX, RX or Control.
Regarding the type of queue, we can do it in the vdpa core.
What do you think?
>
> > + 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@PH0PR12MB5481.namprd12.prod.outlook.com/
> >
> > [2]
> > https://lore.kernel.org/virtualization/6175d620-6be3-c249-5482-0a9448499e4a@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;
Powered by blists - more mailing lists