[dpdk-dev] [PATCH v3] pcap: add support for rx and tx byte counters
Klaus Degner
kd at allegro-packets.com
Mon Jul 13 15:01:49 CEST 2015
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 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.
> --
>
>
More information about the dev
mailing list