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