On Thu, Oct 08, 2020 at 12:48:47PM +0100, Bruce Richardson wrote:
> On Thu, Oct 08, 2020 at 11:58:08AM +0100, Power, Ciara wrote:
> > Hi Olivier,
> >
> >
> > >-----Original Message----- From: Olivier Matz <olivier.m...@6wind.com>
> > >Sent: Thursday 8 October 2020 11:04 To: Power, Ciara
> > ><ciara.po...@intel.com> Cc: dev@dpdk.org; Ray Kinsella <m...@ashroe.eu>;
> > >Neil Horman <nhor...@tuxdriver.com>; Richardson, Bruce
> > ><bruce.richard...@intel.com> 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
> > >> ><olivier.m...@6wind.com> Sent: Wednesday 7 October 2020 12:18 To:
> > >> >Power, Ciara <ciara.po...@intel.com> Cc: dev@dpdk.org; Ray Kinsella
> > >> ><m...@ashroe.eu>; Neil Horman <nhor...@tuxdriver.com>; Richardson,
> > >> >Bruce <bruce.richard...@intel.com> 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
> > >> >> ><olivier.m...@6wind.com> Sent: Tuesday 6 October 2020 10:32 To:
> > >> >> >Power, Ciara <ciara.po...@intel.com> Cc: dev@dpdk.org; Ray
> > >> >> >Kinsella <m...@ashroe.eu>; Neil Horman <nhor...@tuxdriver.com>
> > >> >> >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.
> > >> >> >>
> > <snip>
> > >> >>
> > >> >> >> +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.
Ok, that's clearer to me, thanks Ciara and Bruce.
