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

["Mcnamara, John" <john.mcnamara@intel.com>]

> -----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.
--