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 CA936A04BC; Thu, 8 Oct 2020 13:48:59 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3F3D31BE9F; Thu, 8 Oct 2020 13:48:57 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 4BE9B1BE8C for ; Thu, 8 Oct 2020 13:48:55 +0200 (CEST) IronPort-SDR: vusokAWtz6dBk/Kg+lW1vvt+CFtqMjQiaPcyH7z3fIFp/O9HiovtZLGdkPT8UJ65yB2xavkHnY Bf27D8bheCGQ== X-IronPort-AV: E=McAfee;i="6000,8403,9767"; a="153153589" X-IronPort-AV: E=Sophos;i="5.77,350,1596524400"; d="scan'208";a="153153589" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Oct 2020 04:48:53 -0700 IronPort-SDR: +rBt10d4o+VuWkMYCw0owkW4FXREgFlCCoD78SsmYqEUYSL0BA6UG7FtnT6qceVDaXNLfP5Va8 iGzukSB/ZhQw== X-IronPort-AV: E=Sophos;i="5.77,350,1596524400"; d="scan'208";a="343381240" Received: from bricha3-mobl.ger.corp.intel.com ([10.213.226.107]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 08 Oct 2020 04:48:51 -0700 Date: Thu, 8 Oct 2020 12:48:47 +0100 From: Bruce Richardson To: "Power, Ciara" Cc: Olivier Matz , "dev@dpdk.org" , Ray Kinsella , Neil Horman Message-ID: <20201008114847.GC1106@bricha3-MOBL.ger.corp.intel.com> References: <20200807155859.63888-1-ciara.power@intel.com> <20200930130415.11211-1-ciara.power@intel.com> <20200930130415.11211-2-ciara.power@intel.com> <20201006093217.GG21395@platinum> <20201007111812.GR21395@platinum> <20201008100401.GV21395@platinum> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-dev] [PATCH v3 01/18] eal: add max SIMD bitwidth 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 Thu, Oct 08, 2020 at 11:58:08AM +0100, Power, Ciara wrote: > Hi Olivier, > > > >-----Original Message----- From: Olivier Matz > >Sent: Thursday 8 October 2020 11:04 To: Power, Ciara > > Cc: dev@dpdk.org; Ray Kinsella ; > >Neil Horman ; Richardson, Bruce > > Subject: Re: [dpdk-dev] [PATCH v3 01/18] > >eal: add max SIMD bitwidth > > > >Hi Ciara, > > > >On Thu, Oct 08, 2020 at 09:25:42AM +0000, Power, Ciara wrote: > >> Hi Olivier, > >> > >> > >> >-----Original Message----- From: Olivier Matz > >> > Sent: Wednesday 7 October 2020 12:18 To: > >> >Power, Ciara Cc: dev@dpdk.org; Ray Kinsella > >> >; Neil Horman ; Richardson, > >> >Bruce Subject: Re: [dpdk-dev] [PATCH v3 > >> >01/18] eal: add max SIMD bitwidth > >> > > >> >Hi Ciara, > >> > > >> >On Wed, Oct 07, 2020 at 10:47:34AM +0000, Power, Ciara wrote: > >> >> Hi Olivier, > >> >> > >> >> Thanks for reviewing, some comments below. > >> >> > >> >> > >> >> >-----Original Message----- From: Olivier Matz > >> >> > Sent: Tuesday 6 October 2020 10:32 To: > >> >> >Power, Ciara Cc: dev@dpdk.org; Ray > >> >> >Kinsella ; Neil Horman > >> >> >Subject: Re: [dpdk-dev] [PATCH v3 01/18] eal: add max SIMD > >> >> >bitwidth > >> >> > > >> >> >Hi Ciara, > >> >> > > >> >> >Please find some comments below. > >> >> > > >> >> >On Wed, Sep 30, 2020 at 02:03:57PM +0100, Ciara Power wrote: > >> >> >> This patch adds a max SIMD bitwidth EAL configuration. The API > >> >> >> allows for an app to set this value. It can also be set using > >> >> >> EAL argument --force-max-simd-bitwidth, which will lock the > >> >> >> value and override any modifications made by the app. > >> >> >> > > >> >> > >> >> >> +enum rte_max_simd_t { +RTE_NO_SIMD = 64, +RTE_MAX_128_SIMD = > >> >> >> 128, +RTE_MAX_256_SIMD = 256, +RTE_MAX_512_SIMD = 512, > >> >> >> +RTE_MAX_SIMD_DISABLE = UINT16_MAX, }; > >> >> > > >> >> >What is the difference between RTE_NO_SIMD and > >> >RTE_MAX_SIMD_DISABLE? > >> >> > >> >> RTE_NO_SIMD has value 64 to limit paths to scalar only. > >> >> RTE_MAX_SIMD_DISABLE sets the highest value possible, so > >> >> essentially disables the limit affecting which vector paths are > >> >> taken. This disable option was added to allow for ARM SVE which > >> >> will be later added, Discussed with Honnappa on a previous version: > >> >> https://patchwork.dpdk.org/patch/76097/ > >> > > >> >Ok, so RTE_MAX_SIMD_DISABLE means "disable the max limit", right? > >> > > >> >I feel the name is a bit confusing. What about something like this: > >> > > >> >enum rte_simd { RTE_SIMD_DISABLED = 0, RTE_SIMD_128 = 128, > >> >RTE_SIMD_256 = 256, RTE_SIMD_512 = 512, RTE_SIMD_MAX = UINT16_MAX, }; > >> > > >> > > >> > >> Sure, I can rename these. Although will implement with > >RTE_SIMD_DISABLED=64 to allow for scalar path only. > > > >Out of curiosity, why 64? I thought 0 was a good value for "disabled". > > > > 64 was chosen because it represents the max bitwidth for the scalar path, > 64 bits. Currently, we use 0 on the command-line to represent the > RTE_SIMD_MAX = UINT16_MAX, as it is more user friendly to pass > "--force-max-simd-bitwidth=0" rather than a max value, the 0 is then > internally converted to the max value option. This would not be possible > if we have the scalar option as 0 value. > > >> >> > >> >> >The default value in internal_config is 0, so in my understanding > >> >> >rte_get_max_simd_bitwidth() will return 0 if > >> >> >--force-max-simd-bitwidth is not passed. Is it expected? > >> >> > > >> >> >Maybe I'm missing something, but I don't understand why the value > >> >> >in internal_config is not set to the maximum supported SIMD > >> >> >bitwidth by default, and optionally overriden by the command line > >> >> >argument, or by the API. > >> >> > > >> >> > >> >> The default value for max_simd_bitwidth is set depending on the > >> >> architecture, 256 for x86/ppc, and UINT16_MAX for ARM. So for > >> >> example > >> >the default on x86 allows for AVX2 and under. > >> >> The defaults can be seen in patch 2: > >> >> https://patchwork.dpdk.org/patch/79339/ > >> > > >> >Ok, I was expecting to have a runtime check for this. For instance, > >> >on intel architecture, it is not known at compilation, it depends on > >> >the target which can support up to AVX, AVX2, or AVX512. > >> > > >> > >> Yes, the actual support will vary, but this max SIMD bitwidth is only > >> an > >upper limit on what paths can be taken. > >> So for example with x86 default at 256, the path will still be chosen > >> based > >on what the target can support, but it must be AVX2 or a lesser path. > >> This allows for AVX512 to be enabled at runtime, by increasing the max > >SIMD bitwidth to 512, allowing for that path to be taken where > >supported. > > > >Ah, this means that AVX512 won't be enabled by default on machine that > >support it? Is there a reason for that? > > > > We can't enable the AVX512 by default because it can cause CPU frequency > slowdowns, But this will allow runtime enabling to take that path if the > app/user finds it is the best choice for their use, by setting the max > SIMD bitwidth to 512. > > >Another question: if the default value for max-simd-bitwidth is 256 on > >Intel, and we are running on a target that does not support AVX2, will > >the value be updated to 128 at initialization? In other word, is it > >still up to the dpdk libraries doing vector code to check the > >availability of vector instructions? > > > >Thanks, Olivier > > No the value is not updated depending on the support, it is just a limit. > Libraries still do the checks they had done previously to check what is > supported, and once that supported path is within the max SIMD bitwidth > limit, it is okay to go ahead, otherwise it will need to choose a lesser > path. For example, if a library supports AVX2, SSE and a scalar path, > but the max SIMD bitwidth is set at 128 by app/user, although the library > supports AVX2, it will be limited to choosing the SSE path. Whereas if > for example a library supports only SSE and a scalar path, and the > default max SIMD bitwidth is used (256), the library can still choose SSE > as it is below the 256 bit limit, and the limit remains at 256. > Just to note too that the reason for keeping this separation is that the actual code path selection in each library is going to have to be architecture specific, e.g. to choose SSE or NEON for 128-bit width, whether or not the max-bitwidth functions take the running system into account. By leaving the libs/drivers to check the CPU support themselves, it keeps the max-bitwidth functions generic and saves having architecture-specific code in two places for this.