diff mbox

[dpdk-dev,v2,04/11] virtio: add xstats() implementation

Message ID 1443606022-13581-5-git-send-email-harry.van.haaren@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Van Haaren, Harry Sept. 30, 2015, 9:40 a.m. UTC 
  Add xstats() functions and statistic strings to virtio PMD.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 82 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 2 deletions(-)

Comments

Stephen Hemminger Sept. 30, 2015, 5:44 p.m. UTC | #1 
On Wed, 30 Sep 2015 10:40:15 +0100
Harry van Haaren <harry.van.haaren@intel.com> wrote:

> +/* [rt]x_qX_ is prepended to the name string here */
> +static const struct rte_virtio_xstats_name_off rte_virtio_q_stat_strings[] = {
> +	{"packets", offsetof(struct virtqueue, packets)},
> +	{"bytes", offsetof(struct virtqueue, bytes)},
> +	{"errors", offsetof(struct virtqueue, errors)},
> +}

I don't see the point of this. The point of xstats is to tell the application
about statistics not available through other means.

These stats should be available already in the per queue stats.
Van Haaren, Harry Oct. 1, 2015, 8 a.m. UTC | #2 
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > +/* [rt]x_qX_ is prepended to the name string here */ static const
> > +struct rte_virtio_xstats_name_off rte_virtio_q_stat_strings[] = {
> > +	{"packets", offsetof(struct virtqueue, packets)},
> > +	{"bytes", offsetof(struct virtqueue, bytes)},
> > +	{"errors", offsetof(struct virtqueue, errors)}, }
> 
> I don't see the point of this. The point of xstats is to tell the application about
> statistics not available through other means.
>
> These stats should be available already in the per queue stats.

You're right - these stats are already available in the per-Q stats part of rte_eth_stats.
The virtio implementation of xstats is mostly framework code so we can add other
stats, for example packet size counters, in the near future.

Thanks for reviewing, -Harry
diff mbox

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 465d3cd..c897d1b 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -80,7 +80,10 @@  static int virtio_dev_link_update(struct rte_eth_dev *dev,
 static void virtio_set_hwaddr(struct virtio_hw *hw);
 static void virtio_get_hwaddr(struct virtio_hw *hw);
 
-static void virtio_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats);
+static void virtio_dev_stats_get(struct rte_eth_dev *dev,
+				 struct rte_eth_stats *stats);
+static int virtio_dev_xstats_get(struct rte_eth_dev *dev,
+				 struct rte_eth_xstats *xstats, unsigned n);
 static void virtio_dev_stats_reset(struct rte_eth_dev *dev);
 static void virtio_dev_free_mbufs(struct rte_eth_dev *dev);
 static int virtio_vlan_filter_set(struct rte_eth_dev *dev,
@@ -109,6 +112,21 @@  static const struct rte_pci_id pci_id_virtio_map[] = {
 { .vendor_id = 0, /* sentinel */ },
 };
 
+struct rte_virtio_xstats_name_off {
+	char name[RTE_ETH_XSTATS_NAME_SIZE];
+	unsigned offset;
+};
+
+/* [rt]x_qX_ is prepended to the name string here */
+static const struct rte_virtio_xstats_name_off rte_virtio_q_stat_strings[] = {
+	{"packets", offsetof(struct virtqueue, packets)},
+	{"bytes", offsetof(struct virtqueue, bytes)},
+	{"errors", offsetof(struct virtqueue, errors)},
+};
+
+#define VIRTIO_NB_Q_XSTATS (sizeof(rte_virtio_q_stat_strings) / \
+			    sizeof(rte_virtio_q_stat_strings[0]))
+
 static int
 virtio_send_command(struct virtqueue *vq, struct virtio_pmd_ctrl *ctrl,
 		int *dlen, int pkt_num)
@@ -568,7 +586,9 @@  static const struct eth_dev_ops virtio_eth_dev_ops = {
 
 	.dev_infos_get           = virtio_dev_info_get,
 	.stats_get               = virtio_dev_stats_get,
+	.xstats_get              = virtio_dev_xstats_get,
 	.stats_reset             = virtio_dev_stats_reset,
+	.xstats_reset            = virtio_dev_stats_reset,
 	.link_update             = virtio_dev_link_update,
 	.rx_queue_setup          = virtio_dev_rx_queue_setup,
 	.rx_queue_release        = virtio_dev_rx_queue_release,
@@ -623,7 +643,7 @@  virtio_dev_atomic_write_link_status(struct rte_eth_dev *dev,
 }
 
 static void
-virtio_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
+virtio_update_stats(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	unsigned i;
 
@@ -660,6 +680,64 @@  virtio_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
 }
 
+static int
+virtio_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats *xstats,
+		      unsigned n)
+{
+	unsigned i;
+	unsigned count = 0;
+
+	unsigned nstats = dev->data->nb_tx_queues * VIRTIO_NB_Q_XSTATS +
+		dev->data->nb_rx_queues * VIRTIO_NB_Q_XSTATS;
+
+	if(n < nstats)
+		return nstats;
+
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		struct virtqueue *rxvq = dev->data->rx_queues[i];
+
+		if(rxvq == NULL)
+			continue;
+
+		unsigned t;
+
+		for(t = 0; t < VIRTIO_NB_Q_XSTATS; t++) {
+			snprintf(xstats[count].name, sizeof(xstats[count].name),
+				 "rx_q%u_%s", i,
+				 rte_virtio_q_stat_strings[t].name);
+			xstats[count].value = *(uint64_t *)(((char *)rxvq) +
+				rte_virtio_q_stat_strings[t].offset);
+			count++;
+		}
+	}
+
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		struct virtqueue *txvq = dev->data->tx_queues[i];
+
+		if(txvq == NULL)
+			continue;
+
+		unsigned t;
+
+		for(t = 0; t < VIRTIO_NB_Q_XSTATS; t++) {
+			snprintf(xstats[count].name, sizeof(xstats[count].name),
+				 "tx_q%u_%s", i,
+				 rte_virtio_q_stat_strings[t].name);
+			xstats[count].value = *(uint64_t *)(((char *)txvq) +
+				rte_virtio_q_stat_strings[t].offset);
+			count++;
+		}
+	}
+
+	return count;
+}
+
+static void
+virtio_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
+{
+	virtio_update_stats(dev, stats);
+}
+
 static void
 virtio_dev_stats_reset(struct rte_eth_dev *dev)
 {