Jason Wang
2021-Nov-25 04:50 UTC
[PATH v1 1/2] vdpa: Add support for querying vendor statistics
On Thu, Nov 25, 2021 at 12:56 AM Eli Cohen <elic at nvidia.com> wrote:> > Add support for querying virtqueue statistics. Supported statistics are: > > received_desc - number of descriptors received for the virtqueue > completed_desc - number of descriptors completed for the virtqueue > > A descriptors 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, by returning the next queue index to > query. > > Examples: > 1. Read statisitics for the virtqueue at index 1 > $ vdpa dev vstats show vdpa-a qidx 0 > vdpa-a: > qidx 0 rx received_desc 256 completed_desc 9 > > 2. Read statisitics for all the virtqueues > $ vdpa dev vstats show vdpa-a > vdpa-a: > qidx 0 rx received_desc 256 completed_desc 9 > qidx 1 tx received_desc 21 completed_desc 21 > qidx 2 ctrl received_desc 0 completed_desc 0 > > Signed-off-by: Eli Cohen <elic at nvidia.com> > --- > v0 -> v1: > Emphasize that we're dealing with vendor specific counters. > Terminate query loop on error > > drivers/vdpa/vdpa.c | 144 ++++++++++++++++++++++++++++++++++++++ > include/linux/vdpa.h | 10 +++ > include/uapi/linux/vdpa.h | 9 +++ > 3 files changed, 163 insertions(+) > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > index 7332a74a4b00..da658c80ff2a 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -781,6 +781,90 @@ 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, u16 *index) > +{ > + struct vdpa_vq_stats stats; > + u16 idx = *index; > + int err; > + > + err = vdev->config->get_vq_stats(vdev, index, &stats); > + if (err) > + return err;I wonder what happens if some other vendor adds their specific stats. Can we then have very large stats but only few of them is necessary for a specific vendor? If this is ture, is this better to simply pass the msg to the parent instead of a structure like stats?> + > + if (nla_put_u16(msg, VDPA_ATTR_DEV_QUEUE_INDEX, idx)) > + return -EMSGSIZE; > + > + if (nla_put_u16(msg, VDPA_ATTR_DEV_QUEUE_CTRL, !!stats.ctrl_vq)) > + return -EMSGSIZE; > + > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_RECEIVED_DESC, stats.received_desc, > + VDPA_ATTR_PAD)) > + return -EMSGSIZE; > + > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_COMPLETED_DESC, stats.completed_desc, > + VDPA_ATTR_PAD)) > + return -EMSGSIZE; > + > + return 0; > +} > + > +static int vdpa_dev_net_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg, u16 index) > +{ > + int err; > + > + if (!vdev->config->get_vq_stats) > + return -EOPNOTSUPP; > + > + if (index != VDPA_INVAL_QUEUE_INDEX) { > + err = vdpa_fill_stats_rec(vdev, msg, &index); > + if (err) > + return err; > + } else { > + index = 0; > + do { > + err = vdpa_fill_stats_rec(vdev, msg, &index); > + if (err) > + return err; > + } while (index != VDPA_INVAL_QUEUE_INDEX); > + } > + > + return 0; > +} > + > +static int vdpa_dev_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, > + u32 seq, int flags, u16 index) > +{ > + u32 device_id; > + void *hdr; > + int err; > + > + hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags, > + VDPA_CMD_DEV_VSTATS_GET); > + if (!hdr) > + return -EMSGSIZE; > + > + if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) { > + err = -EMSGSIZE; > + goto undo_msg; > + } > + > + device_id = vdev->config->get_device_id(vdev); > + if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) { > + err = -EMSGSIZE; > + goto undo_msg; > + } > + > + err = vdpa_dev_net_stats_fill(vdev, msg, index); > + > + genlmsg_end(msg, hdr); > + > + return err; > + > +undo_msg: > + genlmsg_cancel(msg, hdr); > + return err; > +} > + > static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info) > { > struct vdpa_device *vdev; > @@ -862,6 +946,59 @@ 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; > + u16 index = -1; > + int err; > + > + if (!info->attrs[VDPA_ATTR_DEV_NAME]) > + return -EINVAL; > + > + if (info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]) > + index = nla_get_u16(info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX]); > + > + devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]); > + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); > + if (!msg) > + return -ENOMEM; > + > + 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_stats_fill(vdev, msg, info->snd_portid, info->snd_seq, 0, index); > + if (!err) > + err = genlmsg_reply(msg, info); > + > + put_device(dev); > + mutex_unlock(&vdpa_dev_mutex); > + > + if (err) > + nlmsg_free(msg); > + > + return err; > + > +mdev_err: > + put_device(dev); > +dev_err: > + mutex_unlock(&vdpa_dev_mutex); > + return err; > +} > + > static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = { > [VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING }, > [VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING }, > @@ -869,6 +1006,7 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = { > [VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR, > /* virtio spec 1.1 section 5.1.4.1 for valid MTU range */ > [VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68), > + [VDPA_ATTR_DEV_QUEUE_INDEX] = NLA_POLICY_RANGE(NLA_U16, 0, 32767),It looks to me that the virtio spec allows up to 65535 vqs.> }; > > static const struct genl_ops vdpa_nl_ops[] = { > @@ -902,6 +1040,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 c3011ccda430..1df19a87a9cb 100644 > --- a/include/linux/vdpa.h > +++ b/include/linux/vdpa.h > @@ -58,6 +58,12 @@ struct vdpa_vq_state { > }; > }; > > +struct vdpa_vq_stats { > + u64 received_desc; > + u64 completed_desc; > + bool ctrl_vq; > +}; > + > struct vdpa_mgmt_dev; > > /** > @@ -269,6 +275,8 @@ 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_vq_stats)(struct vdpa_device *vdev, u16 *idx, > + struct vdpa_vq_stats *stats);I'm fine with this but I wonder if it's better to move this to mgmtdev_ops.> 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 */ > @@ -443,4 +451,6 @@ struct vdpa_mgmt_dev { > int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev); > void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev); > > +#define VDPA_INVAL_QUEUE_INDEX 0xffffThis seems a valid index unless I miss something in the spec. Thanks> + > #endif /* _LINUX_VDPA_H */ > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h > index a252f06f9dfd..b749d312dd9c 100644 > --- a/include/uapi/linux/vdpa.h > +++ b/include/uapi/linux/vdpa.h > @@ -18,11 +18,15 @@ 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 { > VDPA_ATTR_UNSPEC, > > + /* Pad attribute for 64b alignment */ > + VDPA_ATTR_PAD = VDPA_ATTR_UNSPEC, > + > /* bus name (optional) + dev name together make the parent device handle */ > VDPA_ATTR_MGMTDEV_BUS_NAME, /* string */ > VDPA_ATTR_MGMTDEV_DEV_NAME, /* string */ > @@ -40,6 +44,11 @@ enum vdpa_attr { > VDPA_ATTR_DEV_NET_CFG_MAX_VQP, /* u16 */ > VDPA_ATTR_DEV_NET_CFG_MTU, /* u16 */ > > + VDPA_ATTR_DEV_QUEUE_INDEX, /* u16 */ > + VDPA_ATTR_DEV_QUEUE_CTRL, /* u16 */ > + VDPA_ATTR_DEV_VENDOR_RECEIVED_DESC, /* u64 */ > + VDPA_ATTR_DEV_VENDOR_COMPLETED_DESC, /* u64 */ > + > /* new attributes must be added above here */ > VDPA_ATTR_MAX, > }; > -- > 2.33.1 >
Parav Pandit
2021-Nov-30 04:09 UTC
[PATH v1 1/2] vdpa: Add support for querying vendor statistics
> From: Jason Wang <jasowang at redhat.com> > Sent: Thursday, November 25, 2021 10:21 AM > > On Thu, Nov 25, 2021 at 12:56 AM Eli Cohen <elic at nvidia.com> wrote: > > > > Add support for querying virtqueue statistics. Supported statistics are: > > > > received_desc - number of descriptors received for the virtqueue > > completed_desc - number of descriptors completed for the virtqueue > > > > A descriptors 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, by returning the next > > queue index to query. > > > > Examples: > > 1. Read statisitics for the virtqueue at index 1 $ vdpa dev vstats > > show vdpa-a qidx 0 > > vdpa-a: > > qidx 0 rx received_desc 256 completed_desc 9 > > > > 2. Read statisitics for all the virtqueues $ vdpa dev vstats show > > vdpa-a > > vdpa-a: > > qidx 0 rx received_desc 256 completed_desc 9 qidx 1 tx received_desc > > 21 completed_desc 21 qidx 2 ctrl received_desc 0 completed_desc 0 > > > > Signed-off-by: Eli Cohen <elic at nvidia.com> > > --- > > v0 -> v1: > > Emphasize that we're dealing with vendor specific counters. > > Terminate query loop on error > > > > drivers/vdpa/vdpa.c | 144 ++++++++++++++++++++++++++++++++++++++ > > include/linux/vdpa.h | 10 +++ > > include/uapi/linux/vdpa.h | 9 +++ > > 3 files changed, 163 insertions(+) > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index > > 7332a74a4b00..da658c80ff2a 100644 > > --- a/drivers/vdpa/vdpa.c > > +++ b/drivers/vdpa/vdpa.c > > @@ -781,6 +781,90 @@ 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, u16 *index) { > > + struct vdpa_vq_stats stats; > > + u16 idx = *index; > > + int err; > > + > > + err = vdev->config->get_vq_stats(vdev, index, &stats); > > + if (err) > > + return err; > > I wonder what happens if some other vendor adds their specific stats. > Can we then have very large stats but only few of them is necessary for a > specific vendor? If this is ture, is this better to simply pass the msg to the > parent instead of a structure like stats? >It is better to have vdpa defined vendor stats structure, so that this subsystem has well defined statistics. If vdpa enables every vendor driver to put the messages of its choice stats will become a assorted items bag. While it may seem great for one vendor to put whatever they want there, it is inconvenient for end user to understand/parse those stats differently, if they are not defined by vdpa subsystem. Additionally integrating it to monitoring tools such as prometheous becomes another pain point. It also requires moving VDPA_ATTR_DEV_VENDOR_COMPLETED_DESC out of UAPI and defining vendor defined string which can easily go out of sync. So I am inclined towards vendor specific stats to be well defined by vdpa subsystem like how its down in this patch. This still allows different stats among multiple vendors or multiple generation of single vendor.