Re: [dpdk-dev] [PATCH v3 01/18] eal: add max SIMD bitwidth

[Olivier Matz <olivier.matz@6wind.com>]

Hi Ciara,

On Thu, Oct 08, 2020 at 09:25:42AM +0000, Power, Ciara wrote:
> Hi Olivier,
> 
> 
> >-----Original Message-----
> >From: Olivier Matz <olivier.matz@6wind.com>
> >Sent: Wednesday 7 October 2020 12:18
> >To: Power, Ciara <ciara.power@intel.com>
> >Cc: dev@dpdk.org; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
> ><nhorman@tuxdriver.com>; Richardson, Bruce <bruce.richardson@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.matz@6wind.com>
> >> >Sent: Tuesday 6 October 2020 10:32
> >> >To: Power, Ciara <ciara.power@intel.com>
> >> >Cc: dev@dpdk.org; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
> >> ><nhorman@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.
> >> >>
> >> >> Signed-off-by: Ciara Power <ciara.power@intel.com>
> >> >>
> >> >> ---
> >> >> v3:
> >> >>   - Added enum value to essentially disable using max SIMD to choose
> >> >>     paths, intended for use by ARM SVE.
> >> >>   - Fixed parsing bitwidth argument to return an error for values
> >> >>     greater than uint16_t.
> >> >> v2: Added to Doxygen comment for API.
> >> >> ---
> >>
> >> <snip>
> >>
> >> >>
> >> >> +uint16_t
> >> >> +rte_get_max_simd_bitwidth(void)
> >> >> +{
> >> >> +	const struct internal_config *internal_conf =
> >> >> +		eal_get_internal_configuration();
> >> >> +	return internal_conf->max_simd_bitwidth.bitwidth;
> >> >> +}
> >> >
> >> >Should the return value be enum rte_max_simd_t?
> >> >If not, do we really need the enum definition?
> >> >
> >>
> >> I kept the return value and param value below as uint16_t to allow for
> >> arbitrary values, and will allow it be more flexible for future additions as
> >new enums won't need to be added.
> >> For the set function below, this is used when a user passes the EAL
> >> command line flag, which passes an integer value rather than an enum one.
> >> The enums are useful when checking the max_simd_bitwidth in
> >> drivers/libs, for example using "RTE_MAX_256_SIMD" instead of "256" in
> >the condition checks.
> >>
> >> >> +
> >> >> +int
> >> >> +rte_set_max_simd_bitwidth(uint16_t bitwidth) {
> >> >> +	struct internal_config *internal_conf =
> >> >> +		eal_get_internal_configuration();
> >> >> +	if (internal_conf->max_simd_bitwidth.locked) {
> >> >> +		RTE_LOG(NOTICE, EAL, "Cannot set max SIMD bitwidth - user
> >> >runtime override enabled");
> >> >> +		return -EPERM;
> >> >> +	}
> >> >> +
> >> >> +	if (bitwidth != RTE_MAX_SIMD_DISABLE && (bitwidth <
> >> >RTE_NO_SIMD ||
> >> >> +			!rte_is_power_of_2(bitwidth))) {
> >> >> +		RTE_LOG(ERR, EAL, "Invalid bitwidth value!\n");
> >> >> +		return -EINVAL;
> >> >> +	}
> >> >> +	internal_conf->max_simd_bitwidth.bitwidth = bitwidth;
> >> >> +	return 0;
> >> >> +}
> >> >
> >> >Same question, should the parameter be enum rte_max_simd_t?
> >> >
> >>
> >> <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".

> >>
> >> >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?

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