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.
>>
>> Signed-off-by: Ciara Power <ciara.po...@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/
>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/
<snip>
Thanks,
Ciara
