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

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

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,
};


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

> 
> <snip>
> 
> Thanks,
> Ciara