From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 822EAA04BA; Wed, 7 Oct 2020 13:18:17 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 669A71B70E; Wed, 7 Oct 2020 13:18:16 +0200 (CEST) Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by dpdk.org (Postfix) with ESMTP id 3E68E1B70A for ; Wed, 7 Oct 2020 13:18:14 +0200 (CEST) Received: by mail-wm1-f66.google.com with SMTP id p15so1889660wmi.4 for ; Wed, 07 Oct 2020 04:18:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=YwfL/gDoxLaOFB5jW6HOrINPPumvC3PtPf4ctuamn+U=; b=RuywDbtq2k5kkeZ/MLOlygEmIfBcDpr4svHghbavOsOhhSDb9ebEpOuiKtgo8L83ZX ASyMtEuUELZ7o37gEtLaD+fiA8O7ffX/LYnSNDVHMAkFBpnbwxapiqqdc8QZGNO3dreN ga+IiA3r0briXTHDhS/zr1wvdVJMEvyHIhRS89aiDDri+leMAh3jma7HbDJITPocWiVr fcX8L0119IDjbHq6ikSjwb9M4AH5EMwkU0yiuDOfoYq8TEBjw2mXfmySLtUm2Bd9EWt9 u301GlR1vVAjVmH1HGh+x4hf6pkFfUYMg1yBKW7NGZzBwtON3ikoOOCUMTNM9ilvXid8 EX8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=YwfL/gDoxLaOFB5jW6HOrINPPumvC3PtPf4ctuamn+U=; b=nj9H2H250Jonp4a7XIzY9ouCanJLGhd5LqrGgaRuaIFjiVtItqouoKCG76siqoKkwP 18oZmjgUp507Y8X9DuJ879p+O1pNUnq+06FPlgcTwrYaolyT9CZn4oBvNkSv7dAP3iBs iMiafqp+2chEVna6ixEMYAWmoGTH/9qnMxXNLODr86f6cd5snS/hqwopmxQ7Ws+YdXOp N8dUerQKIfbe9kvy6mIHkYbAcQtpcmWkQ0JhmRSQA8TcUMLjjqgWOC9AyVFR4ONfbejb MARjAYUWfQumsUCBCysANsGkOR9VjCGV055dm1onOGwcark11IfrgQv+/G32CYe8gKxX gnbA== X-Gm-Message-State: AOAM531YS0ZGgt61HYr04rtAxuVpADuXK1G/9WB+9h/k02BKVpi7iFRi 5bxcAwZ45yxswCfrlUr1t/T2qA== X-Google-Smtp-Source: ABdhPJzWwdoZ0hfiZkS1wNmGyMcSQB7++Z2RAY3NItml6g8ZrxZcySwVISUkgVtoV6uqnxpwFYhGUg== X-Received: by 2002:a1c:38c7:: with SMTP id f190mr2601429wma.47.1602069493850; Wed, 07 Oct 2020 04:18:13 -0700 (PDT) Received: from 6wind.com (2a01cb0c0005a600345636f7e65ed1a0.ipv6.abo.wanadoo.fr. [2a01:cb0c:5:a600:3456:36f7:e65e:d1a0]) by smtp.gmail.com with ESMTPSA id c4sm2168236wme.27.2020.10.07.04.18.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Oct 2020 04:18:13 -0700 (PDT) Date: Wed, 7 Oct 2020 13:18:12 +0200 From: Olivier Matz To: "Power, Ciara" Cc: "dev@dpdk.org" , Ray Kinsella , Neil Horman , "Richardson, Bruce" Message-ID: <20201007111812.GR21395@platinum> References: <20200807155859.63888-1-ciara.power@intel.com> <20200930130415.11211-1-ciara.power@intel.com> <20200930130415.11211-2-ciara.power@intel.com> <20201006093217.GG21395@platinum> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [PATCH v3 01/18] eal: add max SIMD bitwidth X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > >Sent: Tuesday 6 October 2020 10:32 > >To: Power, Ciara > >Cc: dev@dpdk.org; Ray Kinsella ; Neil Horman > > > >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 > >> > >> --- > >> 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. > >> --- > > > > >> > >> +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? > > > > > > >> +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. > > > > Thanks, > Ciara