From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f182.google.com (mail-wi0-f182.google.com [209.85.212.182]) by dpdk.org (Postfix) with ESMTP id 4FE075A76 for ; Mon, 9 Mar 2015 19:51:22 +0100 (CET) Received: by wibbs8 with SMTP id bs8so24027493wib.0 for ; Mon, 09 Mar 2015 11:51:22 -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 :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=sT2BQrMbzkx2eEruiQTzA0isIfyBmGKV2XWKEeUltu4=; b=SeiL6+sMx2m8DW9XUlrhtOwTJqbfuEIxx4fvdID1jhyYJ1ExlpqjKAjW9Bdcd7Rep+ Mbkc6p2kewVdPrrvw9OzukZqWpHRj3sv8wU8d/90Y2ShqQC80KL0uKblH9nnkfwAQC9a Imrx0uyfzEtk9oiNqJijvodjBmB3LvKsQDxBx/Fk4BB2+YwRgFiRFYO/ZK4zknmqLBhQ WuCRRBPIN1n6j++NZr+kUT1Nz9csPFtBJl9cJOZCf0NCIjEkgSqEjS+C9IkL6jy43OWT Ru7W4LEdplBkY/a+K97Sx7TeVHZob/ZbqnsxwSb8gPCIsQwE5hpxbguGy2VZVoFrMa7S 5wGA== X-Gm-Message-State: ALoCoQmGUOUPermOXVtMIfuDN5Det3FixeLCpOQym72IBzZr/vrTewTTJvOZXTfkriy1gtfeGk4S X-Received: by 10.180.73.171 with SMTP id m11mr60777269wiv.21.1425927082084; Mon, 09 Mar 2015 11:51:22 -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 dj5sm29413301wjb.28.2015.03.09.11.51.20 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 09 Mar 2015 11:51:21 -0700 (PDT) Message-ID: <54FDEBA7.3020603@cloudius-systems.com> Date: Mon, 09 Mar 2015 20:51:19 +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" 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> In-Reply-To: <2601191342CEEE43887BDE71AB977258213F4E48@irsmsx105.ger.corp.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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 18:51:22 -0000 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_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]? 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. > > 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? Right. > Otherwise there is no much point in using all these '__leXX' types, > except probably to show an intention, correct? 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. 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 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