From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id D3B8B2C7A for ; Mon, 13 Jul 2015 14:56:49 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP; 13 Jul 2015 05:56:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,462,1432623600"; d="scan'208";a="605203484" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by orsmga003.jf.intel.com with ESMTP; 13 Jul 2015 05:56:47 -0700 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.216]) by IRSMSX101.ger.corp.intel.com ([169.254.1.201]) with mapi id 14.03.0224.002; Mon, 13 Jul 2015 13:56:46 +0100 From: "Mcnamara, John" To: Klaus Degner , "dev@dpdk.org" Thread-Topic: [PATCH v3] pcap: add support for rx and tx byte counters Thread-Index: AQHQvNIC+Z/7XkNN50emA9uCnFRefp3ZU6HQ Date: Mon, 13 Jul 2015 12:56:46 +0000 Message-ID: References: <1435664375-25648-1-git-send-email-kd@allegro-packets.com> <1436555593-62452-1-git-send-email-kd@allegro-packets.com> In-Reply-To: <1436555593-62452-1-git-send-email-kd@allegro-packets.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 12:56:50 -0000 > -----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 >=20 > 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 th= e 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 cha= rs per line) #11:=20 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 >=20 > 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 =3D=3D NULL || nb_pkts =3D=3D 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 =3D (uint16_t)header.len; > + bytes_rx +=3D 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 +=3D num_rx; > + pcap_q->rx_bytes +=3D 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 +=3D num_tx; > + dumper_q->tx_bytes +=3D 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 c= ode path but there are two TX code paths (for writing to a file and to an i= nterface). > dumper_q->err_pkts +=3D 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 =3D 0, tx_total =3D 0, tx_err_total =3D 0; > + unsigned long rx_total =3D 0, rx_b_total =3D 0; > + unsigned long tx_total =3D 0, tx_err_total =3D 0, tx_b_total =3D 0; The rx_b_total and tx_b_total variable names aren't very clear. Call them r= x_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 =3D 0, rx_bytes_total =3D 0; unsigned long tx_total =3D 0, tx_bytes_total =3D 0, tx_err_total =3D 0; Also, it would probably be best to rename rx/tx_total to rx/tx_packets_tota= l (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 =3D tx_total; - igb_stats->obytes =3D tx_b_total; + igb_stats->opackets =3D tx_packets_total; + igb_stats->obytes =3D tx_bytes_total; John. --=20