Virtualization - Mar 2022 - [PATCH v1 1/2] vdpa: Add support for querying vendor statistics

Hi Eli,

Sorry for my so delayed response.
Please see below. 
> From: Eli Cohen <elic at nvidia.com>
> Sent: Wednesday, February 16, 2022 1:30 PM
> 
> Allows to read vendor statistics of a vdpa device. The specific statistics
data is
> received by the upstream driver in the form of an (attribute name,
attribute
> value) pairs.
> +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff
*msg,
> +			       struct genl_info *info, u32 index) {
> +	int err;
> +
> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
> +		return -EMSGSIZE;
> +
> +	err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info-
> >extack);
There is implicit assumption about placement of netlink attributes, like name1,
value1, name2, value2 etc.
Things can break here with such message framing in in place in the future.

Netlink has more generic way for addressing and avoiding above strict placement
of fields etc.

A more elegant way in kernel is to use nested and self-describing attributes.
We also want to use standard netlink infra built in iproute2 to parse in generic
way.

So please change it do like below.
A bit long response, as it contains a pseudo code example.
(ignored all error checks to keep code short)

overview:
---------------
Each vendor stats entry is a nested entry.
This nested entry contains: 
a. stat name ("rx_desc", "cmpl_desc" etc)
b. value of this variable as u64

All of these individual stats entry are put under a new vstats nested entry.
This enables us to parse and reuse existing netlink interface for nested list of
entries.
Such as iproute2 mnl_attr_for_each_nested() API.

pseudo code:
------------------
enum {
	[...]
	VDPA_ATTR_VSTAT_LIST, /* nested, indicating list of vstat entries */
	VDPA_ATTR_VSTAT_ENTRY, /* nested, indiating each vstat entry is self-contained
*/
	VDPA_ATTR_VSTATS_ENTRY_NAME, /* string of the entry */
	VDPA_ATTRS_VSTATS_ENTRY_DATA, /* u64 value of the entry */
	MAX,
}

/**
 * vdpa_vstats_entry_fill - This is an API expose to vendor driver to fill the
vendor specific stats
 * A vendor driver should call this in a loop for all the valid vendor
statistics entry for the specified queue.
 * A vendor driver should call this API in the get_vendor_vq_stats() callback.
 */
int vdpa_vstats_entry_fill(struct sk_buff *msg, const struct vdpa_vstat_entry
*entry, u32 q_index)
{
	/* created a nested attribute in a msg  for this entry */
	vstats_nl_entry = nla_nest_start_noflag(msg, VDPA_ATTR_VSTAT_ENTRY);

	/* now fill value of name + its value in it.
	nla_put_string(msg, VDPA_ATTR_VSTATS_NAME, "string1);
	nla_put_u64_64bit(msg, entry->val.u64);

	/* end this entry nested attribute */
	nla_nest_end(msg, vstats_nl_entry);
	return 0;
}
EXPORT_SYMBOL(vdev_vstats_entry_fill);

static int vdpa_vstat_fill(struct sk_buff *msg,
			   const struct vdpa_vstat_entry *vstats, u32 q_index)
{
	int i = 0;
	int ret;

	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
VDPA_CMD_DEV_VSTATS_GET);

	/* put the device name also, so that same routine can work 
	  * for the dumpit too in future for all the queues
	  */
	nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev)));

	nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index);

	/* start list type to indicate that we will have list of nested */
	vstats = nla_nest_start_noflag(msg, VDPA_ATTR_VSTAT_LIST);
	
	ret = vdev->config->get_vendor_vq_stats(msg, vdev, q_idx);
	nla_nest_end(msg, vstats);

	genlmsg_end(msg, hdr);
}

iproute2 to leverage mnl_attr_for_each_nested(), like below.

vstats_show(...)
{
	mnl_attr_for_each_nested(cur_attr, nla_param[VDPA_ATTR_VSTAT_LIST]) {
		vdpa_vstat_entry_parse(nl);
}

static vdpa_vstat_entry_parse(struct nlattr *nl)
{
	mnl_attr_parse_nested(nl, cb, nla_value);
	/* get the value of each entry placed by driver */
}