From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtprelay06.ispgateway.de (smtprelay06.ispgateway.de [80.67.31.104]) by dpdk.org (Postfix) with ESMTP id 838892A5E for ; Mon, 13 Jul 2015 15:01:51 +0200 (CEST) Received: from [87.172.131.115] (helo=nb-klaus.allegro) by smtprelay06.ispgateway.de with esmtpsa (TLSv1.2:DHE-RSA-AES128-SHA:128) (Exim 4.84) (envelope-from ) id 1ZEdMj-0002dH-M1; Mon, 13 Jul 2015 15:01:49 +0200 To: "Mcnamara, John" , "dev@dpdk.org" References: <1435664375-25648-1-git-send-email-kd@allegro-packets.com> <1436555593-62452-1-git-send-email-kd@allegro-packets.com> From: Klaus Degner Message-ID: <55A3B6BD.30205@allegro-packets.com> Date: Mon, 13 Jul 2015 15:01:49 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:38.0) Gecko/20100101 Thunderbird/38.0.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Df-Sender: a2RAYWxsZWdyby1wYWNrZXRzLmNvbQ== Subject: Re: [dpdk-dev] [PATCH v3] pcap: add support for rx and tx byte counters X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Jul 2015 13:01:51 -0000 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. > -- > >