Re: [PATCH 2/5] vhost: add per-virtqueue statistics support

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Chenbo,

On 4/21/22 16:09, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, January 27, 2022 10:57 PM
>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>;
>> david.marchand@redhat.com
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [PATCH 2/5] vhost: add per-virtqueue statistics support
>>
>> This patch introduces new APIs for the application
>> to query and reset per-virtqueue statistics. The
>> patch also introduces generic counters.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   lib/vhost/rte_vhost.h  |  89 +++++++++++++++++++++++++++++++++
>>   lib/vhost/socket.c     |   4 +-
>>   lib/vhost/version.map  |   5 ++
>>   lib/vhost/vhost.c      | 109 ++++++++++++++++++++++++++++++++++++++++-
>>   lib/vhost/vhost.h      |  18 ++++++-
>>   lib/vhost/virtio_net.c |  53 ++++++++++++++++++++
>>   6 files changed, 274 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
>> index b454c05868..e739091ca0 100644
>> --- a/lib/vhost/rte_vhost.h
>> +++ b/lib/vhost/rte_vhost.h
>> @@ -37,6 +37,7 @@ extern "C" {
>>   #define RTE_VHOST_USER_LINEARBUF_SUPPORT	(1ULL << 6)
>>   #define RTE_VHOST_USER_ASYNC_COPY	(1ULL << 7)
>>   #define RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS	(1ULL << 8)
>> +#define RTE_VHOST_USER_NET_STATS_ENABLE	(1ULL << 9)
>>
>>   /* Features. */
>>   #ifndef VIRTIO_NET_F_GUEST_ANNOUNCE
>> @@ -317,6 +318,32 @@ struct rte_vhost_power_monitor_cond {
>>   	uint8_t match;
>>   };
>>
>> +/** Maximum name length for the statistics counters */
>> +#define RTE_VHOST_STATS_NAME_SIZE 64
>> +
>> +/**
>> + * Vhost virtqueue statistics structure
>> + *
>> + * This structure is used by rte_vhost_vring_stats_get() to provide
>> + * virtqueue statistics to the calling application.
>> + * It maps a name ID, corresponding to an index in the array returned
>> + * by rte_vhost_vring_stats_get_names(), to a statistic value.
>> + */
>> +struct rte_vhost_stat {
>> +	uint64_t id;    /**< The index in xstats name array. */
>> +	uint64_t value; /**< The statistic counter value. */
>> +};
>> +
>> +/**
>> + * Vhost virtqueue statistic name element
>> + *
>> + * This structure is used by rte_vhost_vring_stats_get_anmes() to
> 
> Anmes -> names
> 
>> + * provide virtqueue statistics names to the calling application.
>> + */
>> +struct rte_vhost_stat_name {
>> +	char name[RTE_VHOST_STATS_NAME_SIZE]; /**< The statistic name. */
> 
> Should we consider using ethdev one? Since vhost lib already depends on ethdev
> lib.

I initially thought about it, but this is not a good idea IMHO, as it
might confuse the user, which would think it could use the ethdev API to
control it.

>> +};
>> +
>>   /**
>>    * Convert guest physical address to host virtual address
>>    *
>> @@ -1059,6 +1086,68 @@ __rte_experimental
>>   int
>>   rte_vhost_slave_config_change(int vid, bool need_reply);
>>
>> +/**
>> + * Retrieve names of statistics of a Vhost virtqueue.
>> + *
>> + * There is an assumption that 'stat_names' and 'stats' arrays are
>> matched
>> + * by array index: stats_names[i].name => stats[i].value
>> + *
>> + * @param vid
>> + *   vhost device ID
>> + * @param queue_id
>> + *   vhost queue index
>> + * @param stats_names
>> + *   array of at least size elements to be filled.
>> + *   If set to NULL, the function returns the required number of elements.
>> + * @param size
>> + *   The number of elements in stats_names array.
>> + * @return
>> + *   A negative value on error, otherwise the number of entries filled in
>> the
>> + *   stats name array.
>> + */
>> +__rte_experimental
>> +int
>> +rte_vhost_vring_stats_get_names(int vid, uint16_t queue_id,
>> +		struct rte_vhost_stat_name *name, unsigned int size);
> 
> '@param stats_names' and 'struct rte_vhost_stat_name *name' do not align and reports
> error:
> 
> http://mails.dpdk.org/archives/test-report/2022-March/270275.html

Ha yes, it slept through the cracks when I reworked the series, thanks
for the heads-up.

>> +
>> +/**
>> + * Retrieve statistics of a Vhost virtqueue.
>> + *
>> + * There is an assumption that 'stat_names' and 'stats' arrays are
>> matched
>> + * by array index: stats_names[i].name => stats[i].value
>> + *
>> + * @param vid
>> + *   vhost device ID
>> + * @param queue_id
>> + *   vhost queue index
>> + * @param stats
>> + *   A pointer to a table of structure of type rte_vhost_stat to be
>> filled with
>> + *   virtqueue statistics ids and values.
>> + * @param n
>> + *   The number of elements in stats array.
>> + * @return
>> + *   A negative value on error, otherwise the number of entries filled in
>> the
>> + *   stats table.
>> + */
>> +__rte_experimental
>> +int
>> +rte_vhost_vring_stats_get(int vid, uint16_t queue_id,
>> +		struct rte_vhost_stat *stats, unsigned int n);
>> +
>> +/**
>> + * Reset statistics of a Vhost virtqueue.
>> + *
>> + * @param vid
>> + *   vhost device ID
>> + * @param queue_id
>> + *   vhost queue index
>> + * @return
>> + *   0 on success, a negative value on error.
>> + */
>> +__rte_experimental
>> +int
>> +rte_vhost_vring_stats_reset(int vid, uint16_t queue_id);
>> +
>>   #ifdef __cplusplus
>>   }
>>   #endif
>> diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
>> index c2f8013cd5..6020565fb6 100644
>> --- a/lib/vhost/socket.c
>> +++ b/lib/vhost/socket.c
>> @@ -43,6 +43,7 @@ struct vhost_user_socket {
>>   	bool linearbuf;
>>   	bool async_copy;
>>   	bool net_compliant_ol_flags;
>> +	bool stats_enabled;
>>
>>   	/*
>>   	 * The "supported_features" indicates the feature bits the
>> @@ -228,7 +229,7 @@ vhost_user_add_connection(int fd, struct
>> vhost_user_socket *vsocket)
>>   	vhost_set_ifname(vid, vsocket->path, size);
>>
>>   	vhost_setup_virtio_net(vid, vsocket->use_builtin_virtio_net,
>> -		vsocket->net_compliant_ol_flags);
>> +		vsocket->net_compliant_ol_flags, vsocket->stats_enabled);
>>
>>   	vhost_attach_vdpa_device(vid, vsocket->vdpa_dev);
>>
>> @@ -864,6 +865,7 @@ rte_vhost_driver_register(const char *path, uint64_t
>> flags)
>>   	vsocket->linearbuf = flags & RTE_VHOST_USER_LINEARBUF_SUPPORT;
>>   	vsocket->async_copy = flags & RTE_VHOST_USER_ASYNC_COPY;
>>   	vsocket->net_compliant_ol_flags = flags &
>> RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS;
>> +	vsocket->stats_enabled = flags & RTE_VHOST_USER_NET_STATS_ENABLE;
>>
>>   	if (vsocket->async_copy &&
>>   		(flags & (RTE_VHOST_USER_IOMMU_SUPPORT |
>> diff --git a/lib/vhost/version.map b/lib/vhost/version.map
>> index a7ef7f1976..b83f79c87f 100644
>> --- a/lib/vhost/version.map
>> +++ b/lib/vhost/version.map
>> @@ -84,6 +84,11 @@ EXPERIMENTAL {
>>
>>   	# added in 21.11
>>   	rte_vhost_get_monitor_addr;
>> +
>> +	# added in 22.03
>> +	rte_vhost_vring_stats_get_names;
>> +	rte_vhost_vring_stats_get;
>> +	rte_vhost_vring_stats_reset;
>>   };
>>
>>   INTERNAL {
>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
>> index 42c01abf25..0c6a737aca 100644
>> --- a/lib/vhost/vhost.c
>> +++ b/lib/vhost/vhost.c
>> @@ -28,6 +28,28 @@
>>   struct virtio_net *vhost_devices[MAX_VHOST_DEVICE];
>>   pthread_mutex_t vhost_dev_lock = PTHREAD_MUTEX_INITIALIZER;
>>
>> +struct vhost_vq_stats_name_off {
>> +	char name[RTE_VHOST_STATS_NAME_SIZE];
>> +	unsigned int offset;
>> +};
>> +
>> +static const struct vhost_vq_stats_name_off vhost_vq_stat_strings[] = {
>> +	{"good_packets",           offsetof(struct vhost_virtqueue,
>> stats.packets)},
>> +	{"good_bytes",             offsetof(struct vhost_virtqueue,
>> stats.bytes)},
>> +	{"multicast_packets",      offsetof(struct vhost_virtqueue,
>> stats.multicast)},
>> +	{"broadcast_packets",      offsetof(struct vhost_virtqueue,
>> stats.broadcast)},
>> +	{"undersize_packets",      offsetof(struct vhost_virtqueue,
>> stats.size_bins[0])},
>> +	{"size_64_packets",        offsetof(struct vhost_virtqueue,
>> stats.size_bins[1])},
>> +	{"size_65_127_packets",    offsetof(struct vhost_virtqueue,
>> stats.size_bins[2])},
>> +	{"size_128_255_packets",   offsetof(struct vhost_virtqueue,
>> stats.size_bins[3])},
>> +	{"size_256_511_packets",   offsetof(struct vhost_virtqueue,
>> stats.size_bins[4])},
>> +	{"size_512_1023_packets",  offsetof(struct vhost_virtqueue,
>> stats.size_bins[5])},
>> +	{"size_1024_1518_packets", offsetof(struct vhost_virtqueue,
>> stats.size_bins[6])},
>> +	{"size_1519_max_packets",  offsetof(struct vhost_virtqueue,
>> stats.size_bins[7])},
>> +};
>> +
>> +#define VHOST_NB_VQ_STATS RTE_DIM(vhost_vq_stat_strings)
>> +
>>   /* Called with iotlb_lock read-locked */
>>   uint64_t
>>   __vhost_iova_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq,
>> @@ -758,7 +780,7 @@ vhost_set_ifname(int vid, const char *if_name,
>> unsigned int if_len)
>>   }
>>
>>   void
>> -vhost_setup_virtio_net(int vid, bool enable, bool compliant_ol_flags)
>> +vhost_setup_virtio_net(int vid, bool enable, bool compliant_ol_flags,
>> bool stats_enabled)
>>   {
>>   	struct virtio_net *dev = get_device(vid);
>>
>> @@ -773,6 +795,10 @@ vhost_setup_virtio_net(int vid, bool enable, bool
>> compliant_ol_flags)
>>   		dev->flags |= VIRTIO_DEV_LEGACY_OL_FLAGS;
>>   	else
>>   		dev->flags &= ~VIRTIO_DEV_LEGACY_OL_FLAGS;
>> +	if (stats_enabled)
>> +		dev->flags |= VIRTIO_DEV_STATS_ENABLED;
>> +	else
>> +		dev->flags &= ~VIRTIO_DEV_STATS_ENABLED;
>>   }
>>
>>   void
>> @@ -1908,5 +1934,86 @@ rte_vhost_get_monitor_addr(int vid, uint16_t
>> queue_id,
>>   	return 0;
>>   }
>>
>> +
>> +int
>> +rte_vhost_vring_stats_get_names(int vid, uint16_t queue_id,
>> +		struct rte_vhost_stat_name *name, unsigned int size)
>> +{
>> +	struct virtio_net *dev = get_device(vid);
>> +	unsigned int i;
>> +
>> +	if (dev == NULL)
>> +		return -1;
>> +
>> +	if (queue_id >= dev->nr_vring)
>> +		return -1;
>> +
>> +	if (!(dev->flags & VIRTIO_DEV_STATS_ENABLED))
>> +		return -1;
>> +
>> +	if (name == NULL || size < VHOST_NB_VQ_STATS)
>> +		return VHOST_NB_VQ_STATS;
>> +
>> +	for (i = 0; i < VHOST_NB_VQ_STATS; i++)
>> +		snprintf(name[i].name, sizeof(name[i].name), "%s_q%u_%s",
>> +				(queue_id & 1) ? "rx" : "tx",
>> +				queue_id / 2, vhost_vq_stat_strings[i].name);
>> +
>> +	return VHOST_NB_VQ_STATS;
>> +}
>> +
>> +int
>> +rte_vhost_vring_stats_get(int vid, uint16_t queue_id,
>> +		struct rte_vhost_stat *stats, unsigned int n)
>> +{
>> +	struct virtio_net *dev = get_device(vid);
>> +	struct vhost_virtqueue *vq;
>> +	unsigned int i;
>> +
>> +	if (dev == NULL)
>> +		return -1;
>> +
>> +	if (queue_id >= dev->nr_vring)
>> +		return -1;
>> +
>> +	if (!(dev->flags & VIRTIO_DEV_STATS_ENABLED))
>> +		return -1;
>> +
>> +	if (stats == NULL || n < VHOST_NB_VQ_STATS)
>> +		return VHOST_NB_VQ_STATS;
>> +
>> +	vq = dev->virtqueue[queue_id];
>> +
>> +	rte_spinlock_lock(&vq->access_lock);
>> +	for (i = 0; i < VHOST_NB_VQ_STATS; i++) {
>> +		stats[i].value =
>> +			*(uint64_t *)(((char *)vq) +
>> vhost_vq_stat_strings[i].offset);
>> +		stats[i].id = i;
>> +	}
>> +	rte_spinlock_unlock(&vq->access_lock);
>> +
>> +	return VHOST_NB_VQ_STATS;
>> +}
>> +
>> +int rte_vhost_vring_stats_reset(int vid, uint16_t queue_id)
>> +{
>> +	struct virtio_net *dev = get_device(vid);
>> +	struct vhost_virtqueue *vq;
>> +
>> +	if (dev == NULL)
>> +		return -1;
>> +
>> +	if (queue_id >= dev->nr_vring)
>> +		return -1;
>> +
> 
> Guess you forgot to check VIRTIO_DEV_STATS_ENABLED?

Yes, I will add it in next revision.

>> +	vq = dev->virtqueue[queue_id];
>> +
>> +	rte_spinlock_lock(&vq->access_lock);
>> +	memset(&vq->stats, 0, sizeof(vq->stats));
>> +	rte_spinlock_unlock(&vq->access_lock);
>> +
>> +	return 0;
>> +}
>> +
>>   RTE_LOG_REGISTER_SUFFIX(vhost_config_log_level, config, INFO);
>>   RTE_LOG_REGISTER_SUFFIX(vhost_data_log_level, data, WARNING);
>> diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
>> index 7085e0885c..4c151244c7 100644
>> --- a/lib/vhost/vhost.h
>> +++ b/lib/vhost/vhost.h
>> @@ -38,6 +38,8 @@
>>   #define VIRTIO_DEV_FEATURES_FAILED ((uint32_t)1 << 4)
>>   /* Used to indicate that the virtio_net tx code should fill TX ol_flags
>> */
>>   #define VIRTIO_DEV_LEGACY_OL_FLAGS ((uint32_t)1 << 5)
>> +/*  Used to indicate the application has requested statistics collection
> 
> Should be only one space between '/*' and 'Used' :P
> 
> Thanks,
> Chenbo
> 

Thanks for the reivew,
Maxime