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 AA5525A84 for ; Mon, 9 Mar 2015 20:27:08 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP; 09 Mar 2015 12:27:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,369,1422950400"; d="scan'208";a="689356014" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by fmsmga002.fm.intel.com with ESMTP; 09 Mar 2015 12:27:05 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.117]) by IRSMSX151.ger.corp.intel.com ([169.254.4.7]) with mapi id 14.03.0195.001; Mon, 9 Mar 2015 19:27:05 +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+NXwjVdBt07Z0USDnggAA3CICAAAiSAA== Date: Mon, 9 Mar 2015 19:27:04 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258213F4F96@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> <2601191342CEEE43887BDE71AB977258213F4E48@irsmsx105.ger.corp.intel.com> <54FDEBA7.3020603@cloudius-systems.com> In-Reply-To: <54FDEBA7.3020603@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 19:27:09 -0000 > -----Original Message----- > From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] > Sent: Monday, March 09, 2015 6:51 PM > To: Ananyev, Konstantin > 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 >=20 >=20 >=20 > On 03/09/15 18:35, Ananyev, Konstantin wrote: > > > >> -----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 > >> > >> > >> > >> 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()= /rte_cpu_to_le_xx() when reading/setting HW ring > >> descriptor > >>>> fields > >>>> > >>>> Fixed the above in ixgbe_rx_alloc_bufs() and in ixgbe_recv_scattered= _pkts(). > >>>> > >>>> 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_ixgb= e/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, > >> I'm afraid the above it's not completely correct. See below. > >> > >>> and I think it is better to use always the same type across all PMD c= ode for consistency. > >> Pls., note that "dma_addr" is only used (see below)... > >> > >>> 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; > >> 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 P= MD > >> code and __le64 is one of them... ;) > >> > >> 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. > >> > >> So, why to use __leXX anyway? - Debugging the (invalid) endianess is a > >> real headache. Therefore there are a few static code analysis tools li= ke > >> "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]? >=20 > I'm sorry but it seems to me that I have already mentioned that it > wasn't the first time __leXX is used in the ixgbe_*.[ch]. All structs > describing the descriptors of HW rings in ixgbe_type.h use them, so I'm > just continuing what has already been done. >=20 > > > > 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= ' > > or something similar, right? >=20 > Right. >=20 > > Otherwise there is no much point in using all these '__leXX' types, > > except probably to show an intention, correct? >=20 > Not exactly. If u use these types everywhere where it's needed it's only > 6 lines to patch (__le16,32,64 + __be16,32,64) to make sparse work. And > if u don't - there are thousands of lines to check somehow. Yeh, though the thing is - we don't use it in all other similar places... But probably you right and it is never too late to start with good habits.= =20 So I am convinced :) Thanks Konstantin >=20 > thanks, > vlad > > Konstantin > > > >> In addition after spending some time writing patches for Linux netdev > >> list u develop a strong habit for such stuff - Dave and others are ver= y > >> strict about such things... ;) > >> > >> So, is it the same as uint64_t? I guess now it's clear why it is now..= . ;) > >> > >>>> } > >>>> @@ -1559,13 +1559,14 @@ ixgbe_recv_scattered_pkts(void *rx_queue, st= ruct 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