From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-we0-f180.google.com (mail-we0-f180.google.com [74.125.82.180]) by dpdk.org (Postfix) with ESMTP id 65F9E568F for ; Mon, 9 Mar 2015 13:43:25 +0100 (CET) Received: by wesw62 with SMTP id w62so11052704wes.0 for ; Mon, 09 Mar 2015 05:43:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=UsHhvE7wS6DUJKUM8FMDHf1qKZcrFG/jrrxPP1/fQ8o=; b=VN1C+w8IfaRVEeetOeVzNz6p2QMgqVbw0DLgqGzdKGoH3/9hP4HjLgOQn2OPYtX7Pt GLHk1Dyt5Gw8gsgvtkw3AMT7GcosPw5fjaifoohJTJNlMKyrBRVOSPm28gWi9P3/V9dU 6cYSdjX7BZ0eYSc+0iBqHrUQxylQWj9vimOGLMx42G3stgBUx5V5im9TW0ZsAmbds/JO xjY/8U3ChpdoAMF8msO1qavSOeDBLUnouRZUZNZ4vFKbpN6IqW5NlB8jgeAXcVjVgYJG iSlBoYJWW/Wr/T/bmiZDYOKDeA9xykq8Tfd4C0aArumn5+2Kl2Lx1XCzVMBRhiCTKWsy sXEA== X-Gm-Message-State: ALoCoQlKvqpW0Ml/33wmgZiP7tWxR6eGF3nn5fS+u4wEBSc8W0i9fpISGcySXdBACnNTH+YeXFHX X-Received: by 10.194.21.137 with SMTP id v9mr56759966wje.140.1425905005115; Mon, 09 Mar 2015 05:43:25 -0700 (PDT) Received: from [10.0.0.2] (bzq-109-65-117-109.red.bezeqint.net. [109.65.117.109]) by mx.google.com with ESMTPSA id dm6sm15376197wib.22.2015.03.09.05.43.23 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 09 Mar 2015 05:43:24 -0700 (PDT) Message-ID: <54FD956A.1060404@cloudius-systems.com> Date: Mon, 09 Mar 2015 14:43:22 +0200 From: Vlad Zolotarov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: "Ananyev, Konstantin" , "dev@dpdk.org" 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> In-Reply-To: <2601191342CEEE43887BDE71AB977258213F4B04@irsmsx105.ger.corp.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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 12:43:25 -0000 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_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. 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