* Re: [dpdk-dev] [PATCH] add rx and tx byte counter statistics for PCAP PMD
2015-06-30 11:39 [dpdk-dev] [PATCH] add rx and tx byte counter statistics for PCAP PMD Klaus Degner
@ 2015-07-08 16:11 ` Mcnamara, John
2015-07-09 15:14 ` Klaus Degner
2015-07-10 13:00 ` [dpdk-dev] [PATCH v2] pcap: add support for rx and tx byte counters Klaus Degner
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Mcnamara, John @ 2015-07-08 16:11 UTC (permalink / raw)
To: Klaus Degner, dev
> -----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.
--
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH] add rx and tx byte counter statistics for PCAP PMD
2015-07-08 16:11 ` Mcnamara, John
@ 2015-07-09 15:14 ` Klaus Degner
0 siblings, 0 replies; 11+ messages in thread
From: Klaus Degner @ 2015-07-09 15:14 UTC (permalink / raw)
To: Mcnamara, John, dev
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.
> --
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH v2] pcap: add support for rx and tx byte counters
2015-06-30 11:39 [dpdk-dev] [PATCH] add rx and tx byte counter statistics for PCAP PMD Klaus Degner
2015-07-08 16:11 ` Mcnamara, John
@ 2015-07-10 13:00 ` Klaus Degner
2015-07-10 19:13 ` [dpdk-dev] [PATCH v3] " Klaus Degner
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Klaus Degner @ 2015-07-10 13:00 UTC (permalink / raw)
To: dev
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [dpdk-dev] [PATCH v3] pcap: add support for rx and tx byte counters
2015-06-30 11:39 [dpdk-dev] [PATCH] add rx and tx byte counter statistics for PCAP PMD Klaus Degner
2015-07-08 16:11 ` Mcnamara, John
2015-07-10 13:00 ` [dpdk-dev] [PATCH v2] pcap: add support for rx and tx byte counters Klaus Degner
@ 2015-07-10 19:13 ` Klaus Degner
2015-07-13 12:56 ` Mcnamara, John
2015-07-13 13:51 ` [dpdk-dev] [PATCH] " Klaus Degner
2015-07-13 14:54 ` [dpdk-dev] [PATCH v5] " Klaus Degner
4 siblings, 1 reply; 11+ messages in thread
From: Klaus Degner @ 2015-07-10 19:13 UTC (permalink / raw)
To: dev
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH v3] pcap: add support for rx and tx byte counters
2015-07-10 19:13 ` [dpdk-dev] [PATCH v3] " Klaus Degner
@ 2015-07-13 12:56 ` Mcnamara, John
2015-07-13 13:01 ` Klaus Degner
0 siblings, 1 reply; 11+ messages in thread
From: Mcnamara, John @ 2015-07-13 12:56 UTC (permalink / raw)
To: Klaus Degner, dev
> -----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.
--
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH v3] pcap: add support for rx and tx byte counters
2015-07-13 12:56 ` Mcnamara, John
@ 2015-07-13 13:01 ` Klaus Degner
0 siblings, 0 replies; 11+ messages in thread
From: Klaus Degner @ 2015-07-13 13:01 UTC (permalink / raw)
To: Mcnamara, John, dev
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.
> --
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH] pcap: add support for rx and tx byte counters
2015-06-30 11:39 [dpdk-dev] [PATCH] add rx and tx byte counter statistics for PCAP PMD Klaus Degner
` (2 preceding siblings ...)
2015-07-10 19:13 ` [dpdk-dev] [PATCH v3] " Klaus Degner
@ 2015-07-13 13:51 ` Klaus Degner
2015-07-13 14:54 ` [dpdk-dev] [PATCH v5] " Klaus Degner
4 siblings, 0 replies; 11+ messages in thread
From: Klaus Degner @ 2015-07-13 13:51 UTC (permalink / raw)
To: dev
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [dpdk-dev] [PATCH v5] pcap: add support for rx and tx byte counters
2015-06-30 11:39 [dpdk-dev] [PATCH] add rx and tx byte counter statistics for PCAP PMD Klaus Degner
` (3 preceding siblings ...)
2015-07-13 13:51 ` [dpdk-dev] [PATCH] " Klaus Degner
@ 2015-07-13 14:54 ` Klaus Degner
2015-07-13 15:40 ` Mcnamara, John
4 siblings, 1 reply; 11+ messages in thread
From: Klaus Degner @ 2015-07-13 14:54 UTC (permalink / raw)
To: dev
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH v5] pcap: add support for rx and tx byte counters
2015-07-13 14:54 ` [dpdk-dev] [PATCH v5] " Klaus Degner
@ 2015-07-13 15:40 ` Mcnamara, John
2015-07-13 17:33 ` Thomas Monjalon
0 siblings, 1 reply; 11+ messages in thread
From: Mcnamara, John @ 2015-07-13 15:40 UTC (permalink / raw)
To: Klaus Degner, dev
> -----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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH v5] pcap: add support for rx and tx byte counters
2015-07-13 15:40 ` Mcnamara, John
@ 2015-07-13 17:33 ` Thomas Monjalon
0 siblings, 0 replies; 11+ messages in thread
From: Thomas Monjalon @ 2015-07-13 17:33 UTC (permalink / raw)
To: Klaus Degner; +Cc: dev
> > 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
^ permalink raw reply [flat|nested] 11+ messages in thread