> -----Original Message----- > From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com] > Sent: Monday, March 09, 2015 12:43 PM > To: Ananyev, Konstantin; dev at 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 at dpdk.org] On Behalf Of Vlad Zolotarov > >> Sent: Monday, March 09, 2015 10:13 AM > >> To: dev at 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 <vladz at cloudius-systems.com> > >> --- > >> 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, > > 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 code > > 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 = rxq->port_id; > >> > >> /* populate the descriptors */ > >> - dma_addr = (uint64_t)mb->buf_physaddr + RTE_PKTMBUF_HEADROOM; > >> + dma_addr = rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(mb)); > >> rxdp[i].read.hdr_addr = dma_addr; > >> rxdp[i].read.pkt_addr = 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 PMD > 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 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' or something similar, right? Otherwise there is no much point in using all these '__leXX' types, except probably to show an intention, correct? 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 very > 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, struct > >> rte_mbuf **rx_pkts, > >> first_seg->ol_flags = pkt_flags; > >> > >> if (likely(pkt_flags & PKT_RX_RSS_HASH)) > >> - first_seg->hash.rss = rxd.wb.lower.hi_dword.rss; > >> + first_seg->hash.rss = > >> + rte_le_to_cpu_32(rxd.wb.lower.hi_dword.rss); > >> else if (pkt_flags & PKT_RX_FDIR) { > >> first_seg->hash.fdir.hash = > >> - (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 = > >> - 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
