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

[Klaus Degner <kd@allegro-packets.com>]

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