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