From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 0152BA0526; Tue, 21 Jul 2020 10:57:01 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C4FA41BFEF; Tue, 21 Jul 2020 10:57:00 +0200 (CEST) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 92CE71BFE4 for ; Tue, 21 Jul 2020 10:56:58 +0200 (CEST) IronPort-SDR: Phendh6LRZZHqQDI9avcD6ihrHpmo2tWOF1yggEQbmwelGvbb8TxcKH/T2OIcK9tWkw9qsNw6w 2qajvt28RYGg== X-IronPort-AV: E=McAfee;i="6000,8403,9688"; a="130164930" X-IronPort-AV: E=Sophos;i="5.75,378,1589266800"; d="scan'208";a="130164930" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Jul 2020 01:56:57 -0700 IronPort-SDR: mUKReZVSgk5KbnqvKFw5qVMv90YLn8VqbE4eYtJixFUQrDNrJFw7vjJxcyzxNq4sSXhghIWO7N /X/8RXxPTMBw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,378,1589266800"; d="scan'208";a="431912343" Received: from rnicolau-mobl1.ger.corp.intel.com (HELO [10.213.251.241]) ([10.213.251.241]) by orsmga004.jf.intel.com with ESMTP; 21 Jul 2020 01:56:54 -0700 To: David Marchand Cc: dev , Beilei Xing , Jeff Guo , Bruce Richardson , "Ananyev, Konstantin" , Jerin Jacob , "Trahe, Fiona" , Wei Zhao , "Ruifeng Wang (Arm Technology China)" References: <1591870283-7776-1-git-send-email-radu.nicolau@intel.com> <1595236337-28230-1-git-send-email-radu.nicolau@intel.com> <1595236337-28230-2-git-send-email-radu.nicolau@intel.com> From: "Nicolau, Radu" Message-ID: Date: Tue, 21 Jul 2020 09:56:53 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-GB Subject: Re: [dpdk-dev] [PATCH v9 1/4] eal: add WC store functions X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 7/20/2020 1:20 PM, David Marchand wrote: > On Mon, Jul 20, 2020 at 11:12 AM Radu Nicolau wrote: >> Add rte_write32_wc and rte_write32_wc_relaxed functions >> that implement 32bit stores using write combining memory protocol. >> Provided generic stubs and x86 implementation. > What is the difference of using this new API when compared to the > existing pci driver flag RTE_PCI_DRV_WC_ACTIVATE? > Do we have some overlap between the two? No, the RTE_PCI_DRV_WC_ACTIVATE define will map the BARs as write combining, whereas these functions will use the WC regardless of the mapping, and they can be used for memory areas that won't be mapped using the PCI infrastructure. > > This commitlog is quite short for something that touches performance. > I saw a question from Ruifeng, it is worth adding this to the commitlog. Strictly speaking, this patch only enables support for WC stores, and it does not have any performance implications on its own. I think a fair assumption is that if someone is looking to use WC stores they will know exactly how and where they can be used. > > Which x86 platforms will benefit from it? > What is the impact on performance for existing platforms that have no > MOVDIRI support? x86 platforms supporting MOVDIRI instruction with this particular patch, and any other platform that have similar instructions if they were to be enabled. There is no impact for the ones that don't support it. > > >> Signed-off-by: Radu Nicolau >> Acked-by: Bruce Richardson >> --- >> lib/librte_eal/arm/include/rte_io_64.h | 12 +++++++ >> lib/librte_eal/include/generic/rte_io.h | 48 ++++++++++++++++++++++++++++ >> lib/librte_eal/x86/include/rte_io.h | 56 +++++++++++++++++++++++++++++++++ >> 3 files changed, 116 insertions(+) >> >> diff --git a/lib/librte_eal/arm/include/rte_io_64.h b/lib/librte_eal/arm/include/rte_io_64.h >> index e534624..d07d9cb 100644 >> --- a/lib/librte_eal/arm/include/rte_io_64.h >> +++ b/lib/librte_eal/arm/include/rte_io_64.h >> @@ -164,6 +164,18 @@ rte_write64(uint64_t value, volatile void *addr) >> rte_write64_relaxed(value, addr); >> } >> >> +static __rte_always_inline void >> +rte_write32_wc(uint32_t value, volatile void *addr) >> +{ >> + rte_write32(value, addr); >> +} >> + >> +static __rte_always_inline void >> +rte_write32_wc_relaxed(uint32_t value, volatile void *addr) >> +{ >> + rte_write32_relaxed(value, addr); >> +} >> + > We were using a single knob RTE_OVERRIDE_IO_H for overriding the whole rte_io.h. > Now we would have a special case for an API for x86 and the code is > copy/pasted in the ARM header and keeping the "whole" override mode. > > This leaves an unfinished taste. > > Why did you not flag all relevant "native" helpers? > This would factor some code from the ARM header. I agree that having a more granular approach is better, having a single knob is why ARM header had about half of the functions overridden and half copied and pasted before this patch. But this is outside the scope of this patch. > > >> #ifdef __cplusplus >> } >> #endif >> diff --git a/lib/librte_eal/include/generic/rte_io.h b/lib/librte_eal/include/generic/rte_io.h >> index da457f7..0669baa 100644 >> --- a/lib/librte_eal/include/generic/rte_io.h >> +++ b/lib/librte_eal/include/generic/rte_io.h >> @@ -229,6 +229,40 @@ rte_write32(uint32_t value, volatile void *addr); >> static inline void >> rte_write64(uint64_t value, volatile void *addr); >> >> +/** >> + * Write a 32-bit value to I/O device memory address addr using write >> + * combining memory write protocol. Depending on the platform write combining >> + * may not be available and/or may be treated as a hint and the behavior may >> + * fallback to a regular store. >> + * >> + * @param value >> + * Value to write >> + * @param addr >> + * I/O memory address to write the value to >> + */ >> +__rte_experimental >> +static inline void >> +rte_write32_wc(uint32_t value, volatile void *addr); >> + >> +/** >> + * Write a 32-bit value to I/O device memory address addr using write >> + * combining memory write protocol. Depending on the platform write combining >> + * may not be available and/or may be treated as a hint and the behavior may >> + * fallback to a regular store. >> + * >> + * The relaxed version does not have additional I/O memory barrier, useful in >> + * accessing the device registers of integrated controllers which implicitly >> + * strongly ordered with respect to memory access. > It might be just me, but I have trouble reading the last part of this sentence. > Maybe remove "with respect to"? It was copied and pasted from the other _relaxed functions for consistency reasons. > > >> + * >> + * @param value >> + * Value to write >> + * @param addr >> + * I/O memory address to write the value to >> + */ >> +__rte_experimental >> +static inline void >> +rte_write32_wc_relaxed(uint32_t value, volatile void *addr); >> + >> #endif /* __DOXYGEN__ */ >> >> #ifndef RTE_OVERRIDE_IO_H >> @@ -345,6 +379,20 @@ rte_write64(uint64_t value, volatile void *addr) >> rte_write64_relaxed(value, addr); >> } >> >> +#ifndef RTE_NATIVE_WRITE32_WC >> +static __rte_always_inline void >> +rte_write32_wc(uint32_t value, volatile void *addr) >> +{ >> + rte_write32(value, addr); >> +} >> + >> +static __rte_always_inline void >> +rte_write32_wc_relaxed(uint32_t value, volatile void *addr) >> +{ >> + rte_write32_relaxed(value, addr); >> +} >> +#endif /* RTE_NATIVE_WRITE32_WC */ >> + >> #endif /* RTE_OVERRIDE_IO_H */ >> >> #endif /* _RTE_IO_H_ */ >> diff --git a/lib/librte_eal/x86/include/rte_io.h b/lib/librte_eal/x86/include/rte_io.h >> index 2db71b1..c95ed67 100644 >> --- a/lib/librte_eal/x86/include/rte_io.h >> +++ b/lib/librte_eal/x86/include/rte_io.h >> @@ -9,8 +9,64 @@ >> extern "C" { >> #endif >> >> +#include "rte_cpuflags.h" >> + >> +#define RTE_NATIVE_WRITE32_WC >> #include "generic/rte_io.h" >> >> +/** >> + * @internal >> + * MOVDIRI wrapper. >> + */ >> +static __rte_always_inline void >> +_rte_x86_movdiri(uint32_t value, volatile void *addr) >> +{ >> + asm volatile( >> + /* MOVDIRI */ >> + ".byte 0x40, 0x0f, 0x38, 0xf9, 0x02" >> + : >> + : "a" (value), "d" (addr)); >> +} >> + >> +static __rte_always_inline void >> +rte_write32_wc(uint32_t value, volatile void *addr) >> +{ >> + static int _x86_movdiri_flag = -1; >> + if (_x86_movdiri_flag == 1) { >> + rte_wmb(); >> + _rte_x86_movdiri(value, addr); >> + } else if (_x86_movdiri_flag == 0) { >> + rte_write32(value, addr); >> + } else { >> + _x86_movdiri_flag = >> + (rte_cpu_get_flag_enabled(RTE_CPUFLAG_MOVDIRI) > 0); >> + if (_x86_movdiri_flag == 1) { >> + rte_wmb(); >> + _rte_x86_movdiri(value, addr); >> + } else { >> + rte_write32(value, addr); >> + } >> + } >> +} >> + >> +static __rte_always_inline void >> +rte_write32_wc_relaxed(uint32_t value, volatile void *addr) >> +{ >> + static int _x86_movdiri_flag = -1; >> + if (_x86_movdiri_flag == 1) { >> + _rte_x86_movdiri(value, addr); >> + } else if (_x86_movdiri_flag == 0) { >> + rte_write32_relaxed(value, addr); >> + } else { >> + _x86_movdiri_flag = >> + (rte_cpu_get_flag_enabled(RTE_CPUFLAG_MOVDIRI) > 0); >> + if (_x86_movdiri_flag == 1) >> + _rte_x86_movdiri(value, addr); >> + else >> + rte_write32_relaxed(value, addr); >> + } >> +} >> + > Repeating some comments I made earlier. > > - If a single helper called by both rte_write32_wc and > rte_write32_wc_relaxed with a _constant_ flag is not to your liking (I > don't see where it would have an impact on performance), then maybe > rte_write32_wc() can be simply implemented as: > > +static __rte_always_inline void > +rte_write32_wc(uint32_t value, volatile void *addr) > +{ > + rte_wmb(); > + rte_write32_wc_relaxed(value, addr); > +} > + Yes, it can be written as such, I will update the patch. > > - Looking at this above suggestion, I wonder about the non-relaxed case. > Is rte_io_wmb() not enough? No, we need an actual memory fence, on x86 rte_io_wmb() is defined as rte_compiler_barrier() > > > - The cpuflag check can be resolved at init once and for all. > By this, I mean in lib/librte_eal/x86/rte_cpuflags.c: > > +int rte_x86_movdiri_flag = -1; > + > +RTE_INIT(rte_x86_movdiri_init) > +{ > + rte_x86_movdiri_flag = > + (rte_cpu_get_flag_enabled(RTE_CPUFLAG_MOVDIRI) > 0); > +} > > The variable can be exported in lib/librte_eal/x86/include/rte_cpuflags.h. > > Then rte_write32_wc_relaxed() becomes: > > +static __rte_always_inline void > +rte_write32_wc_relaxed(uint32_t value, volatile void *addr) > +{ > + if (rte_x86_movdiri_flag == 1) { > + asm volatile( > + /* MOVDIRI */ > + ".byte 0x40, 0x0f, 0x38, 0xf9, 0x02" > + : > + : "a" (value), "d" (addr)); > + return; > + } > + > + rte_write32_relaxed(value, addr); > +} > + I tried this before sending the patch, there is an issue when linking the shared library version with the --no-undefined flag. This will require to have the variable exported in the overall map file, requiring a change across all platforms for something that is very x86 specific. > > Thanks. >