From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id E5C10428B3; Wed, 5 Apr 2023 02:04:43 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7A8A440FDF; Wed, 5 Apr 2023 02:04:43 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 407B940DDC for ; Wed, 5 Apr 2023 02:04:42 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 77AA8210DEA2; Tue, 4 Apr 2023 17:04:41 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 77AA8210DEA2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1680653081; bh=uq1PicO6HeBpejgCmihJl3kRopg6VrUWknSfi1KzJro=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=QBswnUzxeUuoAAbCiGrajZ1Nizo0be+uG1I9DkzJ6ETABTzKqDnoJjQabHzbPS3IU YCpU8Tqw2RKgsOa4Kk2/6WSFhqpJsuqfExV0fmVdhQR6+VQG8457XS2EwrD/ISN6Ky O4cxZayFyjbLmQ0x95B3B+PfVPCKaPRrlA+OTnm8= Date: Tue, 4 Apr 2023 17:04:41 -0700 From: Tyler Retzlaff To: Konstantin Ananyev Cc: Konstantin Ananyev , "dev@dpdk.org" , "bruce.richardson@intel.com" , "david.marchand@redhat.com" , "thomas@monjalon.net" , "mb@smartsharesystems.com" Subject: Re: [PATCH 3/9] eal: use barrier intrinsics when compiling with msvc Message-ID: <20230405000441.GA24769@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1680558751-17931-1-git-send-email-roretzla@linux.microsoft.com> <1680558751-17931-4-git-send-email-roretzla@linux.microsoft.com> <754a877d020a4a199a5310b469e876a7@huawei.com> <20230404154906.GC23247@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Wed, Apr 05, 2023 at 12:49:21AM +0100, Konstantin Ananyev wrote: > 04/04/2023 16:49, Tyler Retzlaff пишет: > >On Tue, Apr 04, 2023 at 12:11:07PM +0000, Konstantin Ananyev wrote: > >> > >> > >>>Inline assembly is not supported for msvc x64 instead use > >>>_{Read,Write,ReadWrite}Barrier() intrinsics. > >>> > >>>Signed-off-by: Tyler Retzlaff > >>>--- > >>> lib/eal/include/generic/rte_atomic.h | 4 ++++ > >>> lib/eal/x86/include/rte_atomic.h | 10 +++++++++- > >>> 2 files changed, 13 insertions(+), 1 deletion(-) > >>> > >>>diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h > >>>index 234b268..e973184 100644 > >>>--- a/lib/eal/include/generic/rte_atomic.h > >>>+++ b/lib/eal/include/generic/rte_atomic.h > >>>@@ -116,9 +116,13 @@ > >>> * Guarantees that operation reordering does not occur at compile time > >>> * for operations directly before and after the barrier. > >>> */ > >>>+#ifndef RTE_TOOLCHAIN_MSVC > >>> #define rte_compiler_barrier() do { \ > >>> asm volatile ("" : : : "memory"); \ > >>> } while(0) > >>>+#else > >>>+#define rte_compiler_barrier() _ReadWriteBarrier() > >>>+#endif > >>> > >>> /** > >>> * Synchronization fence between threads based on the specified memory order. > >>>diff --git a/lib/eal/x86/include/rte_atomic.h b/lib/eal/x86/include/rte_atomic.h > >>>index f2ee1a9..5cce9ba 100644 > >>>--- a/lib/eal/x86/include/rte_atomic.h > >>>+++ b/lib/eal/x86/include/rte_atomic.h > >>>@@ -27,9 +27,13 @@ > >>> > >>> #define rte_rmb() _mm_lfence() > >>> > >>>+#ifndef RTE_TOOLCHAIN_MSVC > >>> #define rte_smp_wmb() rte_compiler_barrier() > >>>- > >>> #define rte_smp_rmb() rte_compiler_barrier() > >>>+#else > >>>+#define rte_smp_wmb() _WriteBarrier() > >>>+#define rte_smp_rmb() _ReadBarrier() > >>>+#endif > >>> > >>> /* > >>> * From Intel Software Development Manual; Vol 3; > >>>@@ -66,11 +70,15 @@ > >>> static __rte_always_inline void > >>> rte_smp_mb(void) > >>> { > >>>+#ifndef RTE_TOOLCHAIN_MSVC > >>> #ifdef RTE_ARCH_I686 > >>> asm volatile("lock addl $0, -128(%%esp); " ::: "memory"); > >>> #else > >>> asm volatile("lock addl $0, -128(%%rsp); " ::: "memory"); > >>> #endif > >>>+#else > >>>+ rte_compiler_barrier(); > >>>+#endif > >> > >>It doesn't look right to me: compiler_barrier is not identical to LOCK-ed operation, > >>and is not enough to serve as a proper memory barrier for SMP. > > > >i think i'm confused by the macro naming here. i'll take another look > >thank you for raising it. > > > >> > >>Another ore generic comment - do we really need to pollute all that code with RTE_TOOLCHAIN_MSVC ifdefs? > >>Right now we have ability to have subdir per arch (x86/arm/etc.). > >>Can we treat x86+windows+msvc as a special arch? > > > >i asked this question previously and confirmed in the technical board > >meeting. the answer i received was that the community did not want new > >directory/headers introduced for compiler support matrix and i should > >use #ifdef in the existing headers. > > Ok, can I then ask at least to minimize number of ifdefs to absolute > minimum? in principal no objection at all, one question though is what to do with comment based documentation attached to macros? e.g. #ifdef SOME_FOO /* some documentation */ #define some_macro #else #define some_macro #endif #ifdef SOME_FOO /* some documentation 2 */ #define some_macro2 #else #define some_macro2 #endif i can either duplicate the documentation for every define so it stays "attached" or i can only document the first expansion. let me know what you expect. so something like this? #ifdef SOME_FOO /* some documentation */ #define some_macro /* some documentation 2 */ #define some_macro2 #else #define some_macro #define some_macro2 #endif or should all documentation be duplicated? which can become a teadious redundancy for anyone maintaining it. keep in mind we might have to make an exception for rte_common.h because it seems doing this would be really ugly there. take a look let me know. > It is really hard to read an follow acode that is heavily ifdefed. > Let say above we probably don't need to re-define > rte_smp_rmb/rte_smp_wmb, as both are boiled down to > compiler_barrier(), which is already redefined. can you take a look at v2 of the patch and re-prescribe your advise here? in v2 only the intel macros expand to the compiler barrier. though i find this vexing since as you pointed out it seems they aren't supposed to be compiler only barriers according to the documentation in generic/rte_atomic.h they are intended to be memory barriers. please help me if i've goofed up in this regard. > Another question - could it be visa-versa approach: > can we replace some inline assembly with common instincts whenever possible? msvc has only intrinsics and the conditional expansion for msvc is to use those intrinsics, gcc doesn't generally define intrinsics for processor specific code does it? thanks