> -----Original Message-----
> From: Klaus Degner [mailto:kd at allegro-packets.com]
> Sent: Friday, July 10, 2015 8:13 PM
> To: dev at 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.
--
