[dpdk-dev] [PATCH] add rx and tx byte counter statistics for PCAP PMD

[dpdk-dev] [PATCH] add rx and tx byte counter statistics for PCAP PMD

[Klaus Degner]

PCAP PMD vdev accounts only rx and tx packet statistics.
This patch adds support for rx and tx bytes statistics.

Signed-off-by: Klaus Degner <kd@allegro-packets.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index a6ed5bd..cca75ec 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -68,6 +68,7 @@ struct pcap_rx_queue {
 	uint8_t in_port;
 	struct rte_mempool *mb_pool;
 	volatile unsigned long rx_pkts;
+	volatile unsigned long rx_bytes;
 	volatile unsigned long err_pkts;
 	char name[PATH_MAX];
 	char type[ETH_PCAP_ARG_MAXLEN];
@@ -77,6 +78,7 @@ struct pcap_tx_queue {
 	pcap_dumper_t *dumper;
 	pcap_t *pcap;
 	volatile unsigned long tx_pkts;
+	volatile unsigned long tx_bytes;
 	volatile unsigned long err_pkts;
 	char name[PATH_MAX];
 	char type[ETH_PCAP_ARG_MAXLEN];
@@ -140,6 +142,7 @@ eth_pcap_rx(void *queue,
 	struct pcap_rx_queue *pcap_q = queue;
 	uint16_t num_rx = 0;
 	uint16_t buf_size;
+	uint32_t bytes_rx = 0;
 
 	if (unlikely(pcap_q->pcap == NULL || nb_pkts == 0))
 		return 0;
@@ -170,6 +173,7 @@ eth_pcap_rx(void *queue,
 			mbuf->port = pcap_q->in_port;
 			bufs[num_rx] = mbuf;
 			num_rx++;
+			bytes_rx += header.len;
 		} else {
 			/* pcap packet will not fit in the mbuf, so drop packet */
 			RTE_LOG(ERR, PMD,
@@ -179,6 +183,7 @@ eth_pcap_rx(void *queue,
 		}
 	}
 	pcap_q->rx_pkts += num_rx;
+	pcap_q->rx_bytes += bytes_rx;
 	return num_rx;
 }
 
@@ -205,6 +210,7 @@ eth_pcap_tx_dumper(void *queue,
 	struct rte_mbuf *mbuf;
 	struct pcap_tx_queue *dumper_q = queue;
 	uint16_t num_tx = 0;
+	uint32_t bytes_tx = 0;
 	struct pcap_pkthdr header;
 
 	if (dumper_q->dumper == NULL || nb_pkts == 0)
@@ -220,6 +226,7 @@ eth_pcap_tx_dumper(void *queue,
 				rte_pktmbuf_mtod(mbuf, void*));
 		rte_pktmbuf_free(mbuf);
 		num_tx++;
+		bytes_tx += header.len;
 	}
 
 	/*
@@ -229,6 +236,7 @@ eth_pcap_tx_dumper(void *queue,
 	 */
 	pcap_dump_flush(dumper_q->dumper);
 	dumper_q->tx_pkts += num_tx;
+	dumper_q->tx_bytes += bytes_tx;
 	dumper_q->err_pkts += nb_pkts - num_tx;
 	return num_tx;
 }
@@ -402,25 +410,31 @@ eth_stats_get(struct rte_eth_dev *dev,
 		struct rte_eth_stats *igb_stats)
 {
 	unsigned i;
-	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
+	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0, rx_b_total = 0, tx_b_total = 0;
 	const struct pmd_internals *internal = dev->data->dev_private;
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internal->nb_rx_queues;
 			i++) {
 		igb_stats->q_ipackets[i] = internal->rx_queue[i].rx_pkts;
+		igb_stats->q_ibytes[i] = internal->rx_queue[i].rx_bytes;
 		rx_total += igb_stats->q_ipackets[i];
+		rx_b_total += igb_stats->q_ibytes[i];
 	}
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internal->nb_tx_queues;
 			i++) {
 		igb_stats->q_opackets[i] = internal->tx_queue[i].tx_pkts;
+		igb_stats->q_obytes[i] = internal->tx_queue[i].tx_bytes;
 		igb_stats->q_errors[i] = internal->tx_queue[i].err_pkts;
 		tx_total += igb_stats->q_opackets[i];
+		tx_b_total += igb_stats->q_obytes[i];
 		tx_err_total += igb_stats->q_errors[i];
 	}
 
 	igb_stats->ipackets = rx_total;
+	igb_stats->ibytes = rx_b_total;
 	igb_stats->opackets = tx_total;
+	igb_stats->obytes = tx_b_total;
 	igb_stats->oerrors = tx_err_total;
 }
 
@@ -429,10 +443,13 @@ eth_stats_reset(struct rte_eth_dev *dev)
 {
 	unsigned i;
 	struct pmd_internals *internal = dev->data->dev_private;
-	for (i = 0; i < internal->nb_rx_queues; i++)
+	for (i = 0; i < internal->nb_rx_queues; i++) {
 		internal->rx_queue[i].rx_pkts = 0;
+		internal->rx_queue[i].rx_bytes = 0;
+	}
 	for (i = 0; i < internal->nb_tx_queues; i++) {
 		internal->tx_queue[i].tx_pkts = 0;
+		internal->tx_queue[i].tx_bytes = 0;
 		internal->tx_queue[i].err_pkts = 0;
 	}
 }
-- 
2.4.5

Re: [dpdk-dev] [PATCH] add rx and tx byte counter statistics for PCAP PMD

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Klaus Degner
> Sent: Tuesday, June 30, 2015 12:40 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] add rx and tx byte counter statistics for 
> PCAP PMD
> 
> PCAP PMD vdev accounts only rx and tx packet statistics.
> This patch adds support for rx and tx bytes statistics.

Hi,

Thanks for that. 

Just a few minor comments.

The subject line should contain the library/module being updated and the body is repeats, more or less, the same comment twice. Do a git log on the file to see the standard format.

>  	unsigned i;
> -	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
> +	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0, 
> +rx_b_total = 0, tx_b_total = 0;

This line exceeds the 80 character limit. Maybe separate the rx and tx initialisations onto separate lines. Or else put them all on separate lines in line with the Coding Guidelines:

   http://dpdk.readthedocs.org/en/latest/guidelines/coding_style.html#local-variables

Run checkpatch on the patch before submission to pick up any issues like this.

If you make those changes and resubmit as a V2 I'll ack it.

John.
-- 

Re: [dpdk-dev] [PATCH] add rx and tx byte counter statistics for PCAP PMD

[Klaus Degner]

Hi John,

Thank you for your review.

I will submit a v2 of the patch.

Klaus

Am 08.07.15 um 18:11 schrieb Mcnamara, John:
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Klaus Degner
>> Sent: Tuesday, June 30, 2015 12:40 PM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH] add rx and tx byte counter statistics for 
>> PCAP PMD
>>
>> PCAP PMD vdev accounts only rx and tx packet statistics.
>> This patch adds support for rx and tx bytes statistics.
> Hi,
>
> Thanks for that. 
>
> Just a few minor comments.
>
> The subject line should contain the library/module being updated and the body is repeats, more or less, the same comment twice. Do a git log on the file to see the standard format.
>
>>  	unsigned i;
>> -	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
>> +	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0, 
>> +rx_b_total = 0, tx_b_total = 0;
> This line exceeds the 80 character limit. Maybe separate the rx and tx initialisations onto separate lines. Or else put them all on separate lines in line with the Coding Guidelines:
>
>    http://dpdk.readthedocs.org/en/latest/guidelines/coding_style.html#local-variables
>
> Run checkpatch on the patch before submission to pick up any issues like this.
>
> If you make those changes and resubmit as a V2 I'll ack it.
>
> John.
> -- 
>
>
>

[dpdk-dev] [PATCH v2] pcap: add support for rx and tx byte counters

[Klaus Degner]

pcap: add support for rx and tx byte counters in addition to existing rx and tx packet counters

Signed-off-by: Klaus Degner <kd@allegro-packets.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index a6ed5bd..ec8c989 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -68,6 +68,7 @@ struct pcap_rx_queue {
 	uint8_t in_port;
 	struct rte_mempool *mb_pool;
 	volatile unsigned long rx_pkts;
+	volatile unsigned long rx_bytes;
 	volatile unsigned long err_pkts;
 	char name[PATH_MAX];
 	char type[ETH_PCAP_ARG_MAXLEN];
@@ -77,6 +78,7 @@ struct pcap_tx_queue {
 	pcap_dumper_t *dumper;
 	pcap_t *pcap;
 	volatile unsigned long tx_pkts;
+	volatile unsigned long tx_bytes;
 	volatile unsigned long err_pkts;
 	char name[PATH_MAX];
 	char type[ETH_PCAP_ARG_MAXLEN];
@@ -140,6 +142,7 @@ eth_pcap_rx(void *queue,
 	struct pcap_rx_queue *pcap_q = queue;
 	uint16_t num_rx = 0;
 	uint16_t buf_size;
+	uint32_t bytes_rx = 0;
 
 	if (unlikely(pcap_q->pcap == NULL || nb_pkts == 0))
 		return 0;
@@ -170,6 +173,7 @@ eth_pcap_rx(void *queue,
 			mbuf->port = pcap_q->in_port;
 			bufs[num_rx] = mbuf;
 			num_rx++;
+			bytes_rx += header.len;
 		} else {
 			/* pcap packet will not fit in the mbuf, so drop packet */
 			RTE_LOG(ERR, PMD,
@@ -179,6 +183,7 @@ eth_pcap_rx(void *queue,
 		}
 	}
 	pcap_q->rx_pkts += num_rx;
+	pcap_q->rx_bytes += bytes_rx;
 	return num_rx;
 }
 
@@ -205,6 +210,7 @@ eth_pcap_tx_dumper(void *queue,
 	struct rte_mbuf *mbuf;
 	struct pcap_tx_queue *dumper_q = queue;
 	uint16_t num_tx = 0;
+	uint32_t bytes_tx = 0;
 	struct pcap_pkthdr header;
 
 	if (dumper_q->dumper == NULL || nb_pkts == 0)
@@ -220,6 +226,7 @@ eth_pcap_tx_dumper(void *queue,
 				rte_pktmbuf_mtod(mbuf, void*));
 		rte_pktmbuf_free(mbuf);
 		num_tx++;
+		bytes_tx += header.len;
 	}
 
 	/*
@@ -229,6 +236,7 @@ eth_pcap_tx_dumper(void *queue,
 	 */
 	pcap_dump_flush(dumper_q->dumper);
 	dumper_q->tx_pkts += num_tx;
+	dumper_q->tx_bytes += bytes_tx;
 	dumper_q->err_pkts += nb_pkts - num_tx;
 	return num_tx;
 }
@@ -402,25 +410,32 @@ eth_stats_get(struct rte_eth_dev *dev,
 		struct rte_eth_stats *igb_stats)
 {
 	unsigned i;
-	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
+	unsigned long rx_total = 0, rx_b_total = 0;
+	unsigned long tx_total = 0, tx_err_total = 0, tx_b_total = 0;
 	const struct pmd_internals *internal = dev->data->dev_private;
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internal->nb_rx_queues;
 			i++) {
 		igb_stats->q_ipackets[i] = internal->rx_queue[i].rx_pkts;
+		igb_stats->q_ibytes[i] = internal->rx_queue[i].rx_bytes;
 		rx_total += igb_stats->q_ipackets[i];
+		rx_b_total += igb_stats->q_ibytes[i];
 	}
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internal->nb_tx_queues;
 			i++) {
 		igb_stats->q_opackets[i] = internal->tx_queue[i].tx_pkts;
+		igb_stats->q_obytes[i] = internal->tx_queue[i].tx_bytes;
 		igb_stats->q_errors[i] = internal->tx_queue[i].err_pkts;
 		tx_total += igb_stats->q_opackets[i];
+		tx_b_total += igb_stats->q_obytes[i];
 		tx_err_total += igb_stats->q_errors[i];
 	}
 
 	igb_stats->ipackets = rx_total;
+	igb_stats->ibytes = rx_b_total;
 	igb_stats->opackets = tx_total;
+	igb_stats->obytes = tx_b_total;
 	igb_stats->oerrors = tx_err_total;
 }
 
@@ -429,10 +444,13 @@ eth_stats_reset(struct rte_eth_dev *dev)
 {
 	unsigned i;
 	struct pmd_internals *internal = dev->data->dev_private;
-	for (i = 0; i < internal->nb_rx_queues; i++)
+	for (i = 0; i < internal->nb_rx_queues; i++) {
 		internal->rx_queue[i].rx_pkts = 0;
+		internal->rx_queue[i].rx_bytes = 0;
+	}
 	for (i = 0; i < internal->nb_tx_queues; i++) {
 		internal->tx_queue[i].tx_pkts = 0;
+		internal->tx_queue[i].tx_bytes = 0;
 		internal->tx_queue[i].err_pkts = 0;
 	}
 }
-- 
2.4.5

[dpdk-dev] [PATCH v3] pcap: add support for rx and tx byte counters

[Klaus Degner]

add support for rx and tx byte counters in addition to existing rx and tx packet counters, updated with new dpdk master branch
---
 drivers/net/pcap/rte_eth_pcap.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 682628f..0fd04b5 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -69,6 +69,7 @@ struct pcap_rx_queue {
 	uint8_t in_port;
 	struct rte_mempool *mb_pool;
 	volatile unsigned long rx_pkts;
+	volatile unsigned long rx_bytes;
 	volatile unsigned long err_pkts;
 	char name[PATH_MAX];
 	char type[ETH_PCAP_ARG_MAXLEN];
@@ -78,6 +79,7 @@ struct pcap_tx_queue {
 	pcap_dumper_t *dumper;
 	pcap_t *pcap;
 	volatile unsigned long tx_pkts;
+	volatile unsigned long tx_bytes;
 	volatile unsigned long err_pkts;
 	char name[PATH_MAX];
 	char type[ETH_PCAP_ARG_MAXLEN];
@@ -196,6 +198,7 @@ eth_pcap_rx(void *queue,
 	struct pcap_rx_queue *pcap_q = queue;
 	uint16_t num_rx = 0;
 	uint16_t buf_size;
+	uint32_t bytes_rx = 0;
 
 	if (unlikely(pcap_q->pcap == NULL || nb_pkts == 0))
 		return 0;
@@ -222,6 +225,7 @@ eth_pcap_rx(void *queue,
 			rte_memcpy(rte_pktmbuf_mtod(mbuf, void *), packet,
 					header.len);
 			mbuf->data_len = (uint16_t)header.len;
+			bytes_rx += header.len;
 		} else {
 			/* Try read jumbo frame into multi mbufs. */
 			if (unlikely(eth_pcap_rx_jumbo(pcap_q->mb_pool,
@@ -237,6 +241,7 @@ eth_pcap_rx(void *queue,
 		num_rx++;
 	}
 	pcap_q->rx_pkts += num_rx;
+	pcap_q->rx_bytes += bytes_rx;
 	return num_rx;
 }
 
@@ -263,6 +268,7 @@ eth_pcap_tx_dumper(void *queue,
 	struct rte_mbuf *mbuf;
 	struct pcap_tx_queue *dumper_q = queue;
 	uint16_t num_tx = 0;
+	uint32_t bytes_tx = 0;
 	struct pcap_pkthdr header;
 
 	if (dumper_q->dumper == NULL || nb_pkts == 0)
@@ -297,6 +303,7 @@ eth_pcap_tx_dumper(void *queue,
 
 		rte_pktmbuf_free(mbuf);
 		num_tx++;
+		bytes_tx += header.len;
 	}
 
 	/*
@@ -306,6 +313,7 @@ eth_pcap_tx_dumper(void *queue,
 	 */
 	pcap_dump_flush(dumper_q->dumper);
 	dumper_q->tx_pkts += num_tx;
+	dumper_q->tx_bytes += bytes_tx;
 	dumper_q->err_pkts += nb_pkts - num_tx;
 	return num_tx;
 }
@@ -499,25 +507,32 @@ eth_stats_get(struct rte_eth_dev *dev,
 		struct rte_eth_stats *igb_stats)
 {
 	unsigned i;
-	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
+	unsigned long rx_total = 0, rx_b_total = 0;
+	unsigned long tx_total = 0, tx_err_total = 0, tx_b_total = 0;
 	const struct pmd_internals *internal = dev->data->dev_private;
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internal->nb_rx_queues;
 			i++) {
 		igb_stats->q_ipackets[i] = internal->rx_queue[i].rx_pkts;
+		igb_stats->q_ibytes[i] = internal->rx_queue[i].rx_bytes;
 		rx_total += igb_stats->q_ipackets[i];
+		rx_b_total += igb_stats->q_ibytes[i];
 	}
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internal->nb_tx_queues;
 			i++) {
 		igb_stats->q_opackets[i] = internal->tx_queue[i].tx_pkts;
+		igb_stats->q_obytes[i] = internal->tx_queue[i].tx_bytes;
 		igb_stats->q_errors[i] = internal->tx_queue[i].err_pkts;
 		tx_total += igb_stats->q_opackets[i];
+		tx_b_total += igb_stats->q_obytes[i];
 		tx_err_total += igb_stats->q_errors[i];
 	}
 
 	igb_stats->ipackets = rx_total;
+	igb_stats->ibytes = rx_b_total;
 	igb_stats->opackets = tx_total;
+	igb_stats->obytes = tx_b_total;
 	igb_stats->oerrors = tx_err_total;
 }
 
@@ -526,10 +541,13 @@ eth_stats_reset(struct rte_eth_dev *dev)
 {
 	unsigned i;
 	struct pmd_internals *internal = dev->data->dev_private;
-	for (i = 0; i < internal->nb_rx_queues; i++)
+	for (i = 0; i < internal->nb_rx_queues; i++) {
 		internal->rx_queue[i].rx_pkts = 0;
+		internal->rx_queue[i].rx_bytes = 0;
+	}
 	for (i = 0; i < internal->nb_tx_queues; i++) {
 		internal->tx_queue[i].tx_pkts = 0;
+		internal->tx_queue[i].tx_bytes = 0;
 		internal->tx_queue[i].err_pkts = 0;
 	}
 }
-- 
2.4.5

Re: [dpdk-dev] [PATCH v3] pcap: add support for rx and tx byte counters

[Mcnamara, John]

> -----Original Message-----
> From: Klaus Degner [mailto:kd@allegro-packets.com]
> Sent: Friday, July 10, 2015 8:13 PM
> To: dev@dpdk.org
> Cc: Mcnamara, John; Klaus Degner
> Subject: [PATCH v3] pcap: add support for rx and tx byte counters
> 
> add support for rx and tx byte counters in addition to existing rx and tx
> packet counters, updated with new dpdk master branch
> ---
>  drivers/net/pcap/rte_eth_pcap.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)

Hi Klaus,

Thanks, getting better.

Patches need to be signed off by doing --signoff/-s when committing. See the contributing guidelines here:

    http://dpdk.org/dev

This and other issues could be picked up using Linux checkpatch utility:

    $ checkpatch.pl --ignore PREFER_KERNEL_TYPES,SPLIT_STRING,VOLATILE -q *.patch

    WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
    #11: 
    add support for rx and tx byte counters in addition to existing rx and tx packet counters, updated with new dpdk master branch

    (Cut 2 warnings that don't apply to the patch.)

    ERROR: Missing Signed-off-by: line(s)

    total: 1 errors, 3 warnings, 0 checks, 103 lines checked


> 
> diff --git a/drivers/net/pcap/rte_eth_pcap.c
> b/drivers/net/pcap/rte_eth_pcap.c index 682628f..0fd04b5 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> ...
>  	if (unlikely(pcap_q->pcap == NULL || nb_pkts == 0))
>  		return 0;
> @@ -222,6 +225,7 @@ eth_pcap_rx(void *queue,
>  			rte_memcpy(rte_pktmbuf_mtod(mbuf, void *), packet,
>  					header.len);
>  			mbuf->data_len = (uint16_t)header.len;
> +			bytes_rx += header.len;

This looks like it should be outside the if() statement to calculate the RX bytes for both normal and (non-error) jumbo frames.


>  		} else {
>  			/* Try read jumbo frame into multi mbufs. */
>  			if (unlikely(eth_pcap_rx_jumbo(pcap_q->mb_pool,
> @@ -237,6 +241,7 @@ eth_pcap_rx(void *queue,
>  		num_rx++;
>  	}
>  	pcap_q->rx_pkts += num_rx;
> +	pcap_q->rx_bytes += bytes_rx;

Rename bytes_rx to rx_bytes for consistency with the existing member name.


>  	/*
> @@ -306,6 +313,7 @@ eth_pcap_tx_dumper(void *queue,
>  	 */
>  	pcap_dump_flush(dumper_q->dumper);
>  	dumper_q->tx_pkts += num_tx;
> +	dumper_q->tx_bytes += bytes_tx;

Rename bytes_tx, same as previous comment.

Also, this code needs to be added to eth_pcap_tx() as well. The is one RX code path but there are two TX code paths (for writing to a file and to an interface).


>  	dumper_q->err_pkts += nb_pkts - num_tx;
>  	return num_tx;
>  }
> @@ -499,25 +507,32 @@ eth_stats_get(struct rte_eth_dev *dev,
>  		struct rte_eth_stats *igb_stats)
>  {
>  	unsigned i;
> -	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
> +	unsigned long rx_total = 0, rx_b_total = 0;
> +	unsigned long tx_total = 0, tx_err_total = 0, tx_b_total = 0;

The rx_b_total and tx_b_total variable names aren't very clear. Call them rx_bytes_total and tx_bytes_total. In general try to keep the variable names consistent with the existing code.

Also, this would be better to align the rx and tx variables:

	unsigned long rx_total = 0, rx_bytes_total = 0;
	unsigned long tx_total = 0, tx_bytes_total = 0, tx_err_total = 0;

Also, it would probably be best to rename rx/tx_total to rx/tx_packets_total (and the tx_err_total) in this function since there are now two types of totals. That would make code like this clearer:

-	igb_stats->opackets = tx_total;
-	igb_stats->obytes = tx_b_total;

+	igb_stats->opackets = tx_packets_total;
+	igb_stats->obytes = tx_bytes_total;

John.
-- 

Re: [dpdk-dev] [PATCH v3] pcap: add support for rx and tx byte counters

[Klaus Degner]

Hi John,

Thank you again for your review. I will work on a v4 with your input.

Klaus

Am 13.07.15 um 14:56 schrieb Mcnamara, John:
>> -----Original Message-----
>> From: Klaus Degner [mailto:kd@allegro-packets.com]
>> Sent: Friday, July 10, 2015 8:13 PM
>> To: dev@dpdk.org
>> Cc: Mcnamara, John; Klaus Degner
>> Subject: [PATCH v3] pcap: add support for rx and tx byte counters
>>
>> add support for rx and tx byte counters in addition to existing rx and tx
>> packet counters, updated with new dpdk master branch
>> ---
>>  drivers/net/pcap/rte_eth_pcap.c | 22 ++++++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
> Hi Klaus,
>
> Thanks, getting better.
>
> Patches need to be signed off by doing --signoff/-s when committing. See the contributing guidelines here:
>
>     http://dpdk.org/dev
>
> This and other issues could be picked up using Linux checkpatch utility:
>
>     $ checkpatch.pl --ignore PREFER_KERNEL_TYPES,SPLIT_STRING,VOLATILE -q *.patch
>
>     WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
>     #11: 
>     add support for rx and tx byte counters in addition to existing rx and tx packet counters, updated with new dpdk master branch
>
>     (Cut 2 warnings that don't apply to the patch.)
>
>     ERROR: Missing Signed-off-by: line(s)
>
>     total: 1 errors, 3 warnings, 0 checks, 103 lines checked
>
>
>> diff --git a/drivers/net/pcap/rte_eth_pcap.c
>> b/drivers/net/pcap/rte_eth_pcap.c index 682628f..0fd04b5 100644
>> --- a/drivers/net/pcap/rte_eth_pcap.c
>> +++ b/drivers/net/pcap/rte_eth_pcap.c
>> ...
>>  	if (unlikely(pcap_q->pcap == NULL || nb_pkts == 0))
>>  		return 0;
>> @@ -222,6 +225,7 @@ eth_pcap_rx(void *queue,
>>  			rte_memcpy(rte_pktmbuf_mtod(mbuf, void *), packet,
>>  					header.len);
>>  			mbuf->data_len = (uint16_t)header.len;
>> +			bytes_rx += header.len;
> This looks like it should be outside the if() statement to calculate the RX bytes for both normal and (non-error) jumbo frames.
>
>
>>  		} else {
>>  			/* Try read jumbo frame into multi mbufs. */
>>  			if (unlikely(eth_pcap_rx_jumbo(pcap_q->mb_pool,
>> @@ -237,6 +241,7 @@ eth_pcap_rx(void *queue,
>>  		num_rx++;
>>  	}
>>  	pcap_q->rx_pkts += num_rx;
>> +	pcap_q->rx_bytes += bytes_rx;
> Rename bytes_rx to rx_bytes for consistency with the existing member name.
>
>
>>  	/*
>> @@ -306,6 +313,7 @@ eth_pcap_tx_dumper(void *queue,
>>  	 */
>>  	pcap_dump_flush(dumper_q->dumper);
>>  	dumper_q->tx_pkts += num_tx;
>> +	dumper_q->tx_bytes += bytes_tx;
> Rename bytes_tx, same as previous comment.
>
> Also, this code needs to be added to eth_pcap_tx() as well. The is one RX code path but there are two TX code paths (for writing to a file and to an interface).
>
>
>>  	dumper_q->err_pkts += nb_pkts - num_tx;
>>  	return num_tx;
>>  }
>> @@ -499,25 +507,32 @@ eth_stats_get(struct rte_eth_dev *dev,
>>  		struct rte_eth_stats *igb_stats)
>>  {
>>  	unsigned i;
>> -	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
>> +	unsigned long rx_total = 0, rx_b_total = 0;
>> +	unsigned long tx_total = 0, tx_err_total = 0, tx_b_total = 0;
> The rx_b_total and tx_b_total variable names aren't very clear. Call them rx_bytes_total and tx_bytes_total. In general try to keep the variable names consistent with the existing code.
>
> Also, this would be better to align the rx and tx variables:
>
> 	unsigned long rx_total = 0, rx_bytes_total = 0;
> 	unsigned long tx_total = 0, tx_bytes_total = 0, tx_err_total = 0;
>
> Also, it would probably be best to rename rx/tx_total to rx/tx_packets_total (and the tx_err_total) in this function since there are now two types of totals. That would make code like this clearer:
>
> -	igb_stats->opackets = tx_total;
> -	igb_stats->obytes = tx_b_total;
>
> +	igb_stats->opackets = tx_packets_total;
> +	igb_stats->obytes = tx_bytes_total;
>
> John.
> -- 
>
>

[dpdk-dev] [PATCH] pcap: add support for rx and tx byte counters

[Klaus Degner]

add support for rx and tx byte counters
added support for tx pcap dumper and interface
renamed packet counters for consistency

Signed-off-by: Klaus Degner <kd@allegro-packets.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 682628f..7e4f294 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -69,6 +69,7 @@ struct pcap_rx_queue {
 	uint8_t in_port;
 	struct rte_mempool *mb_pool;
 	volatile unsigned long rx_pkts;
+	volatile unsigned long rx_bytes;
 	volatile unsigned long err_pkts;
 	char name[PATH_MAX];
 	char type[ETH_PCAP_ARG_MAXLEN];
@@ -78,6 +79,7 @@ struct pcap_tx_queue {
 	pcap_dumper_t *dumper;
 	pcap_t *pcap;
 	volatile unsigned long tx_pkts;
+	volatile unsigned long tx_bytes;
 	volatile unsigned long err_pkts;
 	char name[PATH_MAX];
 	char type[ETH_PCAP_ARG_MAXLEN];
@@ -196,6 +198,7 @@ eth_pcap_rx(void *queue,
 	struct pcap_rx_queue *pcap_q = queue;
 	uint16_t num_rx = 0;
 	uint16_t buf_size;
+	uint32_t rx_bytes = 0;
 
 	if (unlikely(pcap_q->pcap == NULL || nb_pkts == 0))
 		return 0;
@@ -235,8 +238,10 @@ eth_pcap_rx(void *queue,
 		mbuf->port = pcap_q->in_port;
 		bufs[num_rx] = mbuf;
 		num_rx++;
+		rx_bytes += header.len;
 	}
 	pcap_q->rx_pkts += num_rx;
+	pcap_q->rx_bytes += rx_bytes;
 	return num_rx;
 }
 
@@ -263,6 +268,7 @@ eth_pcap_tx_dumper(void *queue,
 	struct rte_mbuf *mbuf;
 	struct pcap_tx_queue *dumper_q = queue;
 	uint16_t num_tx = 0;
+	uint32_t tx_bytes = 0;
 	struct pcap_pkthdr header;
 
 	if (dumper_q->dumper == NULL || nb_pkts == 0)
@@ -297,6 +303,7 @@ eth_pcap_tx_dumper(void *queue,
 
 		rte_pktmbuf_free(mbuf);
 		num_tx++;
+		tx_bytes += header.len;
 	}
 
 	/*
@@ -306,6 +313,7 @@ eth_pcap_tx_dumper(void *queue,
 	 */
 	pcap_dump_flush(dumper_q->dumper);
 	dumper_q->tx_pkts += num_tx;
+	dumper_q->tx_bytes += tx_bytes;
 	dumper_q->err_pkts += nb_pkts - num_tx;
 	return num_tx;
 }
@@ -323,6 +331,7 @@ eth_pcap_tx(void *queue,
 	struct rte_mbuf *mbuf;
 	struct pcap_tx_queue *tx_queue = queue;
 	uint16_t num_tx = 0;
+	uint32_t tx_bytes = 0;
 
 	if (unlikely(nb_pkts == 0 || tx_queue->pcap == NULL))
 		return 0;
@@ -355,10 +364,12 @@ eth_pcap_tx(void *queue,
 		if (unlikely(ret != 0))
 			break;
 		num_tx++;
+		tx_bytes += mbuf->pkt_len;
 		rte_pktmbuf_free(mbuf);
 	}
 
 	tx_queue->tx_pkts += num_tx;
+	tx_queue->tx_bytes += tx_bytes;
 	tx_queue->err_pkts += nb_pkts - num_tx;
 	return num_tx;
 }
@@ -499,26 +510,34 @@ eth_stats_get(struct rte_eth_dev *dev,
 		struct rte_eth_stats *igb_stats)
 {
 	unsigned i;
-	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
+	unsigned long rx_packets_total = 0, rx_bytes_total = 0;
+	unsigned long tx_packets_total = 0, tx_bytes_total = 0;
+	unsigned long tx_packets_err_total = 0;
 	const struct pmd_internals *internal = dev->data->dev_private;
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internal->nb_rx_queues;
 			i++) {
 		igb_stats->q_ipackets[i] = internal->rx_queue[i].rx_pkts;
-		rx_total += igb_stats->q_ipackets[i];
+		igb_stats->q_ibytes[i] = internal->rx_queue[i].rx_bytes;
+		rx_packets_total += igb_stats->q_ipackets[i];
+		rx_bytes_total += igb_stats->q_ibytes[i];
 	}
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internal->nb_tx_queues;
 			i++) {
 		igb_stats->q_opackets[i] = internal->tx_queue[i].tx_pkts;
+		igb_stats->q_obytes[i] = internal->tx_queue[i].tx_bytes;
 		igb_stats->q_errors[i] = internal->tx_queue[i].err_pkts;
-		tx_total += igb_stats->q_opackets[i];
-		tx_err_total += igb_stats->q_errors[i];
+		tx_packets_total += igb_stats->q_opackets[i];
+		tx_bytes_total += igb_stats->q_obytes[i];
+		tx_packets_err_total += igb_stats->q_errors[i];
 	}
 
-	igb_stats->ipackets = rx_total;
-	igb_stats->opackets = tx_total;
-	igb_stats->oerrors = tx_err_total;
+	igb_stats->ipackets = rx_packets_total;
+	igb_stats->ibytes = rx_bytes_total;
+	igb_stats->opackets = tx_packets_total;
+	igb_stats->obytes = tx_bytes_total;
+	igb_stats->oerrors = tx_packets_err_total;
 }
 
 static void
@@ -526,10 +545,13 @@ eth_stats_reset(struct rte_eth_dev *dev)
 {
 	unsigned i;
 	struct pmd_internals *internal = dev->data->dev_private;
-	for (i = 0; i < internal->nb_rx_queues; i++)
+	for (i = 0; i < internal->nb_rx_queues; i++) {
 		internal->rx_queue[i].rx_pkts = 0;
+		internal->rx_queue[i].rx_bytes = 0;
+	}
 	for (i = 0; i < internal->nb_tx_queues; i++) {
 		internal->tx_queue[i].tx_pkts = 0;
+		internal->tx_queue[i].tx_bytes = 0;
 		internal->tx_queue[i].err_pkts = 0;
 	}
 }
-- 
2.4.5

[dpdk-dev] [PATCH v5] pcap: add support for rx and tx byte counters

[Klaus Degner]

Added RX and TX bytes counter support to the PCAP statistics.
Added TX counter support for pcap dumper and interface functions.
Renamed RX and TX packet counters for consistency.

Signed-off-by: Klaus Degner <kd@allegro-packets.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 682628f..f2e4634 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -69,6 +69,7 @@ struct pcap_rx_queue {
 	uint8_t in_port;
 	struct rte_mempool *mb_pool;
 	volatile unsigned long rx_pkts;
+	volatile unsigned long rx_bytes;
 	volatile unsigned long err_pkts;
 	char name[PATH_MAX];
 	char type[ETH_PCAP_ARG_MAXLEN];
@@ -78,6 +79,7 @@ struct pcap_tx_queue {
 	pcap_dumper_t *dumper;
 	pcap_t *pcap;
 	volatile unsigned long tx_pkts;
+	volatile unsigned long tx_bytes;
 	volatile unsigned long err_pkts;
 	char name[PATH_MAX];
 	char type[ETH_PCAP_ARG_MAXLEN];
@@ -196,6 +198,7 @@ eth_pcap_rx(void *queue,
 	struct pcap_rx_queue *pcap_q = queue;
 	uint16_t num_rx = 0;
 	uint16_t buf_size;
+	uint32_t rx_bytes = 0;
 
 	if (unlikely(pcap_q->pcap == NULL || nb_pkts == 0))
 		return 0;
@@ -235,8 +238,10 @@ eth_pcap_rx(void *queue,
 		mbuf->port = pcap_q->in_port;
 		bufs[num_rx] = mbuf;
 		num_rx++;
+		rx_bytes += header.len;
 	}
 	pcap_q->rx_pkts += num_rx;
+	pcap_q->rx_bytes += rx_bytes;
 	return num_rx;
 }
 
@@ -263,6 +268,7 @@ eth_pcap_tx_dumper(void *queue,
 	struct rte_mbuf *mbuf;
 	struct pcap_tx_queue *dumper_q = queue;
 	uint16_t num_tx = 0;
+	uint32_t tx_bytes = 0;
 	struct pcap_pkthdr header;
 
 	if (dumper_q->dumper == NULL || nb_pkts == 0)
@@ -297,6 +303,7 @@ eth_pcap_tx_dumper(void *queue,
 
 		rte_pktmbuf_free(mbuf);
 		num_tx++;
+		tx_bytes += mbuf->pkt_len;
 	}
 
 	/*
@@ -306,6 +313,7 @@ eth_pcap_tx_dumper(void *queue,
 	 */
 	pcap_dump_flush(dumper_q->dumper);
 	dumper_q->tx_pkts += num_tx;
+	dumper_q->tx_bytes += tx_bytes;
 	dumper_q->err_pkts += nb_pkts - num_tx;
 	return num_tx;
 }
@@ -323,6 +331,7 @@ eth_pcap_tx(void *queue,
 	struct rte_mbuf *mbuf;
 	struct pcap_tx_queue *tx_queue = queue;
 	uint16_t num_tx = 0;
+	uint32_t tx_bytes = 0;
 
 	if (unlikely(nb_pkts == 0 || tx_queue->pcap == NULL))
 		return 0;
@@ -355,10 +364,12 @@ eth_pcap_tx(void *queue,
 		if (unlikely(ret != 0))
 			break;
 		num_tx++;
+		tx_bytes += mbuf->pkt_len;
 		rte_pktmbuf_free(mbuf);
 	}
 
 	tx_queue->tx_pkts += num_tx;
+	tx_queue->tx_bytes += tx_bytes;
 	tx_queue->err_pkts += nb_pkts - num_tx;
 	return num_tx;
 }
@@ -499,26 +510,34 @@ eth_stats_get(struct rte_eth_dev *dev,
 		struct rte_eth_stats *igb_stats)
 {
 	unsigned i;
-	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
+	unsigned long rx_packets_total = 0, rx_bytes_total = 0;
+	unsigned long tx_packets_total = 0, tx_bytes_total = 0;
+	unsigned long tx_packets_err_total = 0;
 	const struct pmd_internals *internal = dev->data->dev_private;
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internal->nb_rx_queues;
 			i++) {
 		igb_stats->q_ipackets[i] = internal->rx_queue[i].rx_pkts;
-		rx_total += igb_stats->q_ipackets[i];
+		igb_stats->q_ibytes[i] = internal->rx_queue[i].rx_bytes;
+		rx_packets_total += igb_stats->q_ipackets[i];
+		rx_bytes_total += igb_stats->q_ibytes[i];
 	}
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internal->nb_tx_queues;
 			i++) {
 		igb_stats->q_opackets[i] = internal->tx_queue[i].tx_pkts;
+		igb_stats->q_obytes[i] = internal->tx_queue[i].tx_bytes;
 		igb_stats->q_errors[i] = internal->tx_queue[i].err_pkts;
-		tx_total += igb_stats->q_opackets[i];
-		tx_err_total += igb_stats->q_errors[i];
+		tx_packets_total += igb_stats->q_opackets[i];
+		tx_bytes_total += igb_stats->q_obytes[i];
+		tx_packets_err_total += igb_stats->q_errors[i];
 	}
 
-	igb_stats->ipackets = rx_total;
-	igb_stats->opackets = tx_total;
-	igb_stats->oerrors = tx_err_total;
+	igb_stats->ipackets = rx_packets_total;
+	igb_stats->ibytes = rx_bytes_total;
+	igb_stats->opackets = tx_packets_total;
+	igb_stats->obytes = tx_bytes_total;
+	igb_stats->oerrors = tx_packets_err_total;
 }
 
 static void
@@ -526,10 +545,13 @@ eth_stats_reset(struct rte_eth_dev *dev)
 {
 	unsigned i;
 	struct pmd_internals *internal = dev->data->dev_private;
-	for (i = 0; i < internal->nb_rx_queues; i++)
+	for (i = 0; i < internal->nb_rx_queues; i++) {
 		internal->rx_queue[i].rx_pkts = 0;
+		internal->rx_queue[i].rx_bytes = 0;
+	}
 	for (i = 0; i < internal->nb_tx_queues; i++) {
 		internal->tx_queue[i].tx_pkts = 0;
+		internal->tx_queue[i].tx_bytes = 0;
 		internal->tx_queue[i].err_pkts = 0;
 	}
 }
-- 
2.4.5

V5. Minor refactoring.

V4. Incorporated comments.

V3. Rebased to new master.

V2. Incorporated comments.

V1. Initial version.

Re: [dpdk-dev] [PATCH v5] pcap: add support for rx and tx byte counters

[Mcnamara, John]

> -----Original Message-----
> From: Klaus Degner [mailto:kd@allegro-packets.com]
> Sent: Monday, July 13, 2015 3:54 PM
> To: dev@dpdk.org
> Cc: Mcnamara, John; Klaus Degner
> Subject: [PATCH v5] pcap: add support for rx and tx byte counters
> 
> Added RX and TX bytes counter support to the PCAP statistics.
> Added TX counter support for pcap dumper and interface functions.
> Renamed RX and TX packet counters for consistency.
> 
> Signed-off-by: Klaus Degner <kd@allegro-packets.com>

Thanks.

Tested-by: John McNamara <john.mcnamara@intel.com>
Acked-by: John McNamara <john.mcnamara@intel.com>

Re: [dpdk-dev] [PATCH v5] pcap: add support for rx and tx byte counters

[Thomas Monjalon]

> > Added RX and TX bytes counter support to the PCAP statistics.
> > Added TX counter support for pcap dumper and interface functions.
> > Renamed RX and TX packet counters for consistency.
> > 
> > Signed-off-by: Klaus Degner <kd@allegro-packets.com>
> 
> Thanks.
> 
> Tested-by: John McNamara <john.mcnamara@intel.com>
> Acked-by: John McNamara <john.mcnamara@intel.com>

Applied, thanks