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 DD20DA00C5; Mon, 6 Jul 2020 11:15:08 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EF0BD1D9FF; Mon, 6 Jul 2020 11:15:07 +0200 (CEST) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id F39211D9E7 for ; Mon, 6 Jul 2020 11:15:05 +0200 (CEST) IronPort-SDR: Mu8drxA9lYX75W6xulKJEp8yDnYdS8aTYcliFQeiPDPKqT/SroCsFZ00gSk8RY1lsI5eFMN3Ch MhS8VTWic1Ug== X-IronPort-AV: E=McAfee;i="6000,8403,9673"; a="144883777" X-IronPort-AV: E=Sophos;i="5.75,318,1589266800"; d="scan'208";a="144883777" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Jul 2020 02:15:03 -0700 IronPort-SDR: MhhhhnhVvzJLwI01Kvav6xeWsfjd4QjUv69R0fnsfqWQkzgpGz9n39l0iVy1TNP7B2gs/r4OtL Rawdho8InL4w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,318,1589266800"; d="scan'208";a="322331145" Received: from unknown (HELO [10.252.19.116]) ([10.252.19.116]) by FMSMGA003.fm.intel.com with ESMTP; 06 Jul 2020 02:15:02 -0700 From: "Nicolau, Radu" To: David Marchand Cc: dev , Beilei Xing , Jeff Guo , Bruce Richardson , "Ananyev, Konstantin" , Jerin Jacob References: <1591870283-7776-1-git-send-email-radu.nicolau@intel.com> <1593681821-22357-1-git-send-email-radu.nicolau@intel.com> <1593681821-22357-2-git-send-email-radu.nicolau@intel.com> Message-ID: Date: Mon, 6 Jul 2020 10:15:01 +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 v4 1/2] 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" Hi David, thanks for reviewing! Some comments inline. On 7/3/2020 4:19 PM, David Marchand wrote: > On Thu, Jul 2, 2020 at 11:24 AM Radu Nicolau wrote: >> +static inline void >> +rte_write32_wc(uint32_t value, volatile void *addr); > This is a new API, and even if inlined, it should be marked experimental. Will do. > Is volatile necessary? Yes, most of these functions will be called on mmio addresses/volatile pointers. All other io functions have it. > > +static inline void > +rte_write32_wc_relaxed(uint32_t value, volatile void *addr); > + > + > Double empty line (there are some other in this patch that I won't flag again). I will check all occurrences. > > >> #endif /* __DOXYGEN__ */ >> >> #ifndef RTE_OVERRIDE_IO_H >> @@ -345,6 +378,20 @@ rte_write64(uint64_t value, volatile void *addr) >> rte_write64_relaxed(value, addr); >> } >> >> +#ifndef RTE_NATIVE_WRITE32_WC > Missing return type, this causes build failure on anything but x86. Yes, got lost in the copy/paste. I will fix it in the next version > > >> +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" > Inclusion of this header should be out of the extern "C" block. Why? It is used elsewhere inside the extern "C" block e.g. x86/rte_spinlock.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); > Can't this cpu flag check be moved in a constructor? > This would avoid this copy/paste. We evaluated this approach but it creates more problems than if fixes - it will need a variable that needs to be exported and there is no good place to put it. > > >> + 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; > Same check with a static variable with the same name. It should be no problem, they are static local variables. > > > I wonder if wrapping all of this in a single function would be more elegant. > Then rte_write32_wc(|_relaxed) would call it with a flag. Yes, it will be more elegant but also it will cost more, it was written like this to minimize the number of branches taken for the movdiri path.