[dpdk-dev,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(-)
  

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

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