From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id B405A58CB for ; Mon, 9 Mar 2015 17:35:11 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga101.jf.intel.com with ESMTP; 09 Mar 2015 09:35:10 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,368,1422950400"; d="scan'208";a="538168687" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by orsmga003.jf.intel.com with ESMTP; 09 Mar 2015 09:34:40 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.117]) by IRSMSX101.ger.corp.intel.com ([163.33.3.153]) with mapi id 14.03.0195.001; Mon, 9 Mar 2015 16:35:08 +0000 From: "Ananyev, Konstantin" To: Vlad Zolotarov Thread-Topic: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring descriptor fields Thread-Index: AQHQWmatf6gZunXTSk+NXwjVdBt07Z0USDng Date: Mon, 9 Mar 2015 16:35:08 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258213F4E48@irsmsx105.ger.corp.intel.com> References: <1425895968-8597-1-git-send-email-vladz@cloudius-systems.com> <1425895968-8597-2-git-send-email-vladz@cloudius-systems.com> <2601191342CEEE43887BDE71AB977258213F4B04@irsmsx105.ger.corp.intel.com> <54FD956A.1060404@cloudius-systems.com> In-Reply-To: <54FD956A.1060404@cloudius-systems.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring descriptor fields 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, 09 Mar 2015 16:35:12 -0000 > -----Original Message----- > From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] > Sent: Monday, March 09, 2015 12:43 PM > To: Ananyev, Konstantin; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/= rte_cpu_to_le_xx() when reading/setting HW ring > descriptor fields >=20 >=20 >=20 > On 03/09/15 12:29, Ananyev, Konstantin wrote: > > Hi Vlad, > > > >> -----Original Message----- > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vlad Zolotarov > >> Sent: Monday, March 09, 2015 10:13 AM > >> To: dev@dpdk.org > >> Subject: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/r= te_cpu_to_le_xx() when reading/setting HW ring > descriptor > >> fields > >> > >> Fixed the above in ixgbe_rx_alloc_bufs() and in ixgbe_recv_scattered_p= kts(). > >> > >> Signed-off-by: Vlad Zolotarov > >> --- > >> lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 13 +++++++------ > >> 1 file changed, 7 insertions(+), 6 deletions(-) > >> > >> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/= ixgbe_rxtx.c > >> index 9ecf3e5..b033e04 100644 > >> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > >> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > >> @@ -1028,7 +1028,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq) > >> struct igb_rx_entry *rxep; > >> struct rte_mbuf *mb; > >> uint16_t alloc_idx; > >> - uint64_t dma_addr; > >> + __le64 dma_addr; > > Wonder Why you changed from uint64_t to __le64 here? > > Effectively __le64 is the same a uint64_t, >=20 > I'm afraid the above it's not completely correct. See below. >=20 > > and I think it is better to use always the same type across all PMD cod= e for consistency. >=20 > Pls., note that "dma_addr" is only used (see below)... >=20 > > Konstantin > > > > > >> int diag, i; > >> > >> /* allocate buffers in bulk directly into the S/W ring */ > >> @@ -1051,7 +1051,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq) > >> mb->port =3D rxq->port_id; > >> > >> /* populate the descriptors */ > >> - dma_addr =3D (uint64_t)mb->buf_physaddr + RTE_PKTMBUF_HEADROOM; > >> + dma_addr =3D rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(mb)); > >> rxdp[i].read.hdr_addr =3D dma_addr; > >> rxdp[i].read.pkt_addr =3D dma_addr; >=20 > here. ;) And the type of both hdr_addr and pkt_addr is __le64. > I don't exactly understand what do u mean by "use the same type across > all PMD code for consistency" - there are a lot of types used in the PMD > code and __le64 is one of them... ;) >=20 > Now more seriously, let's recall what is the semantics of the __leXX > types - they represent the integer in the "little endian" format. Here, > NIC expects the physical address in a little endian format, thus the > descriptor is defined the way it is defined - using __le64. The same > relates to dma_addr local variable in this patch - it contains the > physical (more correctly "DMA-able") address of the Rx buffer in the > form NIC expects it to be written in the descriptor. >=20 > So, why to use __leXX anyway? - Debugging the (invalid) endianess is a > real headache. Therefore there are a few static code analysis tools like > "sparse" that allow to detect such inconsistencies (see here > http://en.wikipedia.org/wiki/Sparse) and __leXX is a helper to allow > tools like sparse to detect such problems. I meant that for librte_pmd_ixgbe these types are equivalent: lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h: #ifndef __le64 #define __le64 u64 #endif lib/librte_pmd_ixgbe/ixgbe/ixgbe_osdep.h: typedef uint64_t u64; So why not to use just uint64_t as the rest if librt_pmd_ixgbe/ixgbe_*.[c,h= ]? Have to admit, didn't know about the sparse and that ability. Seems like useful one. Though, as I understand, to make any use of it with DPDK, we'll have to use sparse specific attributes: In one of our files define __le64 as '__attribute__((bitwise)) uint64_t'=20 or something similar, right? Otherwise there is no much point in using all these '__leXX' types, except probably to show an intention, correct?=20 Konstantin >=20 > In addition after spending some time writing patches for Linux netdev > list u develop a strong habit for such stuff - Dave and others are very > strict about such things... ;) >=20 > So, is it the same as uint64_t? I guess now it's clear why it is now... ;= ) >=20 > >> } > >> @@ -1559,13 +1559,14 @@ ixgbe_recv_scattered_pkts(void *rx_queue, stru= ct rte_mbuf **rx_pkts, > >> first_seg->ol_flags =3D pkt_flags; > >> > >> if (likely(pkt_flags & PKT_RX_RSS_HASH)) > >> - first_seg->hash.rss =3D rxd.wb.lower.hi_dword.rss; > >> + first_seg->hash.rss =3D > >> + rte_le_to_cpu_32(rxd.wb.lower.hi_dword.rss); > >> else if (pkt_flags & PKT_RX_FDIR) { > >> first_seg->hash.fdir.hash =3D > >> - (uint16_t)((rxd.wb.lower.hi_dword.csum_ip.csum) > >> - & IXGBE_ATR_HASH_MASK); > >> + rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.csum) > >> + & IXGBE_ATR_HASH_MASK; > >> first_seg->hash.fdir.id =3D > >> - rxd.wb.lower.hi_dword.csum_ip.ip_id; > >> + rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.ip_id); > >> } > >> > >> /* Prefetch data of first segment, if configured to do so. */ > >> -- > >> 2.1.0