Re: [PATH v1 1/2] vdpa: Add support for querying vendor statistics

Re: [PATH v1 1/2] vdpa: Add support for querying vendor statistics

[Jason Wang]

On Thu, Nov 25, 2021 at 12:56 AM Eli Cohen <elic@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@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 0xffff

This 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
>

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

RE: [PATH v1 1/2] vdpa: Add support for querying vendor statistics

[Parav Pandit via Virtualization]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Thursday, November 25, 2021 10:21 AM
> 
> On Thu, Nov 25, 2021 at 12:56 AM Eli Cohen <elic@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@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.
 

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

RE: [PATH v1 1/2] vdpa: Add support for querying vendor statistics

[Parav Pandit via Virtualization]

> From: Eli Cohen <elic@nvidia.com>
> Sent: Wednesday, November 24, 2021 10:26 PM
> 
> 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@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;
Better to name it vdpa_vq_vstats as this is reflecting vendor stats.
> +
> +static int vdpa_dev_net_stats_fill(struct vdpa_device *vdev, struct
vdpa_dev_net_vstats_fill

> +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);
We should be really dumping all queue stats of the device in the dumpit() routine.
But there are some limitation from netlink core to enable it.
Given that, I prefer we limit current vstats dump to single q whose index is specified by the user.

In future dumpit and netlink framework can be enhanced to dump for all queues of the specified device.

> 
> +struct vdpa_vq_stats {
vdpa_ vq_vstats

> 
> +#define VDPA_INVAL_QUEUE_INDEX 0xffff
Lets avoid this and let user space the qindex for now.

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

RE: [PATH v1 1/2] vdpa: Add support for querying vendor statistics

[Parav Pandit via Virtualization]

> From: Eli Cohen <elic@nvidia.com>
> Sent: Thursday, November 25, 2021 1:37 PM
> 
> On Thu, Nov 25, 2021 at 07:34:21AM +0200, Parav Pandit wrote:
> >
> >
> > > From: Eli Cohen <elic@nvidia.com>
> > > Sent: Wednesday, November 24, 2021 10:26 PM
> > >
> > > 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@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;
> > Better to name it vdpa_vq_vstats as this is reflecting vendor stats.
> ok
> > > +
> > > +static int vdpa_dev_net_stats_fill(struct vdpa_device *vdev, struct
> > vdpa_dev_net_vstats_fill
> ok
> >
> > > +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);
> > We should be really dumping all queue stats of the device in the dumpit()
> routine.
> > But there are some limitation from netlink core to enable it.
> > Given that, I prefer we limit current vstats dump to single q whose index is
> specified by the user.
> 
> I can live with this. If no one objects, we can simplify the code to return stats
> for a single VQ.
Yeah.

> In that case we can also adopt Jason's suggestion to let the parent driver fill it
> with pairs of field/data for each vendor.
>
Yes, for now, there is single vendor reporting it and stats are generic enough with only two fields.
So it is not really a burden. We should wait for other drivers to grow and actually diverge from current defined stats.
At that point it will be more natural to move in specific drivers.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATH v1 2/2] vdpa/mlx5: Add support for reading descriptor statistics

[Jason Wang]

On Thu, Nov 25, 2021 at 12:56 AM Eli Cohen <elic@nvidia.com> wrote:
>
> Implement the get_vq_stats calback of vdpa_config_ops to return the
> statistics for a virtqueue.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> V0 -> V1:
> Use mutex to sync stats query with change of number of queues
>

[...]

> +static int mlx5_get_vq_stats(struct vdpa_device *vdev, u16 *idx,
> +                            struct vdpa_vq_stats *stats)
> +{
> +       struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> +       struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> +       struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[*idx];
> +       struct mlx5_control_vq *cvq;
> +       int err;
> +
> +       mutex_lock(&ndev->numq_lock);
> +       if ((!ctrl_vq_active(mvdev) && *idx >= ndev->cur_num_vqs) ||
> +           (*idx != ctrl_vq_idx(mvdev) && *idx >= ndev->cur_num_vqs)) {
> +               err = -EINVAL;
> +               goto out;

Interesting, I wonder if it's simpler that we just allow stats up to
the max vqs. It's sometimes useful to dump the stats of all the vqs so
we can let that policy to userspace. Then we can get rid of the mutex.

Thanks

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

Re: [PATH v1 2/2] vdpa/mlx5: Add support for reading descriptor statistics

[Jason Wang]

On Thu, Nov 25, 2021 at 3:59 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Thu, Nov 25, 2021 at 12:57:53PM +0800, Jason Wang wrote:
> > On Thu, Nov 25, 2021 at 12:56 AM Eli Cohen <elic@nvidia.com> wrote:
> > >
> > > Implement the get_vq_stats calback of vdpa_config_ops to return the
> > > statistics for a virtqueue.
> > >
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > ---
> > > V0 -> V1:
> > > Use mutex to sync stats query with change of number of queues
> > >
> >
> > [...]
> >
> > > +static int mlx5_get_vq_stats(struct vdpa_device *vdev, u16 *idx,
> > > +                            struct vdpa_vq_stats *stats)
> > > +{
> > > +       struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > +       struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > +       struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[*idx];
> > > +       struct mlx5_control_vq *cvq;
> > > +       int err;
> > > +
> > > +       mutex_lock(&ndev->numq_lock);
> > > +       if ((!ctrl_vq_active(mvdev) && *idx >= ndev->cur_num_vqs) ||
> > > +           (*idx != ctrl_vq_idx(mvdev) && *idx >= ndev->cur_num_vqs)) {
> > > +               err = -EINVAL;
> > > +               goto out;
> >
> > Interesting, I wonder if it's simpler that we just allow stats up to
> > the max vqs. It's sometimes useful to dump the stats of all the vqs so
> > we can let that policy to userspace. Then we can get rid of the mutex.
> >
> If a VQ is not active then I don't have stats for it. The hardware
> object is not available to be queried.

I wonder if it's ok that we just return 0 in this case?

Btw, the cvq counters:

+
+ if (*idx == ctrl_vq_idx(mvdev)) {
+ cvq = &mvdev->cvq;
+ stats->received_desc = cvq->head;
+ stats->completed_desc = cvq->head;
+ stats->ctrl_vq = true;
+ *idx = VDPA_INVAL_QUEUE_INDEX;
+ err = 0;
+ goto out;
+ }

Seems not to consider the case that the head can wrap around.

Thanks


>
> > Thanks
> >
>

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

RE: [PATH v1 2/2] vdpa/mlx5: Add support for reading descriptor statistics

[Parav Pandit via Virtualization]

> From: Eli Cohen <elic@nvidia.com>
> Sent: Wednesday, November 24, 2021 10:26 PM
> Implement the get_vq_stats calback of vdpa_config_ops to return the statistics
> for a virtqueue.
> 
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> V0 -> V1:
> Use mutex to sync stats query with change of number of queues
> 
>  drivers/vdpa/mlx5/net/mlx5_vnet.c  | 143 +++++++++++++++++++++++++++++
>  include/linux/mlx5/mlx5_ifc.h      |   1 +
>  include/linux/mlx5/mlx5_ifc_vdpa.h |  39 ++++++++
>  3 files changed, 183 insertions(+)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 9b7d8c721354..465e832f2ad1 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -119,6 +119,7 @@ struct mlx5_vdpa_virtqueue {
>  	struct mlx5_vdpa_umem umem2;
>  	struct mlx5_vdpa_umem umem3;
> 
> +	u32 counter_set_id;
>  	bool initialized;
>  	int index;
>  	u32 virtq_id;
> @@ -164,6 +165,8 @@ struct mlx5_vdpa_net {
>  	u32 cur_num_vqs;
>  	struct notifier_block nb;
>  	struct vdpa_callback config_cb;
> +	/* sync access to virtqueues statistics */
> +	struct mutex numq_lock;
It is better to use rw_semaphore, than mutex. 
So get_stats do down/up_read() and change_num_qps() do down/up_write().
This helps to better annotate stats getter is only a reader and handle_ctrl_mq() is updater.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATH v1 0/2] Support reading vendor statistics through iproute2/vdpa tool

[Michael S. Tsirkin]

On Wed, Nov 24, 2021 at 06:55:29PM +0200, Eli Cohen wrote:
> The following patches add support for reading vdpa statistics
> information from a vdpa instance using netlink messages.
> 
> mlx5_vdpa is also implemented to provide that information.
> 
> The supported statistics data are number of descriptors recieved by the
> virtqueue and the number of descriptors completed by it.

So I assume a v2 of this will be forthcoming, right?

> v0 -> v1:
> Emphasize that we're dealing with vendor specific counters.
> Add mutex to synchronize query stats with changing of number of queues
> 
> Eli Cohen (2):
>   vdpa: Add support for querying vendor statistics
>   vdpa/mlx5: Add support for reading descriptor statistics
> 
>  drivers/vdpa/mlx5/net/mlx5_vnet.c  | 143 ++++++++++++++++++++++++++++
>  drivers/vdpa/vdpa.c                | 144 +++++++++++++++++++++++++++++
>  include/linux/mlx5/mlx5_ifc.h      |   1 +
>  include/linux/mlx5/mlx5_ifc_vdpa.h |  39 ++++++++
>  include/linux/vdpa.h               |  10 ++
>  include/uapi/linux/vdpa.h          |   9 ++
>  6 files changed, 346 insertions(+)
> 
> -- 
> 2.33.1

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