Hi Chenbo,

On 4/21/22 16:09, Xia, Chenbo wrote:
Hi Maxime,

-----Original Message-----
From: Maxime Coquelin <maxime.coque...@redhat.com>
Sent: Thursday, January 27, 2022 10:57 PM
To: dev@dpdk.org; Xia, Chenbo <chenbo....@intel.com>;
david.march...@redhat.com
Cc: Maxime Coquelin <maxime.coque...@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.coque...@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

Reply via email to