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 E9552A04BC; Thu, 8 Oct 2020 12:04:05 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CC5241BDDF; Thu, 8 Oct 2020 12:04:04 +0200 (CEST) Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by dpdk.org (Postfix) with ESMTP id 1725C1BD1F for ; Thu, 8 Oct 2020 12:04:03 +0200 (CEST) Received: by mail-wr1-f65.google.com with SMTP id t10so5933490wrv.1 for ; Thu, 08 Oct 2020 03:04:03 -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=fgYwsn4z4U5Asqqp24JAkljMuZxBVuyViBfUykyvEZQ=; b=fIhtXMYhinI40VYUI06h4poXZcmDrcbDltVXVj/e44vKlcS8hTUHNcnXDIZHdG2zmC 2389XwXWltgjlGvL69IVZ878TsMFpl3vGbfKXZvBtFNbasUqD3Qcl4M1l9dIBgcS5Wcj RM6PvudVOI/HLPfA2UGEz+1ZnK0/cEICIwkyWhgM1Wh671DBTOmjxWM2R/MOIcjB6bZ1 Scnmz+9qpOSX7SPCyU5j4ZN6du6sHz1RW4F0S/yJSyyk/ty9n+ETZMcSUAR6VHqei5D/ BIFaMkmwnrnWDsP/6GrZmY+uVGxZtyKhuLUYS1/548wrr+BXLUeU0TNAlzo/4thq1Kp7 Sf9Q== 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=fgYwsn4z4U5Asqqp24JAkljMuZxBVuyViBfUykyvEZQ=; b=kTH2mVMqJmosGExIqfCg+iRbNtmK+bUrP+vdcQ8EEG0KX4JN5nbXi/LY2sxEx6Jrxs QIkApLV2GS8FuhvMQEfIvwww7nOPgmFI2gP6w+Y2lFlAtIdodcvDNEpj6Mh6uecgBrO7 xo8UcSCDWZhhpniD8TorTtGt1HyMKHOetev3x1+eJUkQ0HLQE9R4vSavbMJlhfGHQbyM 3s12BApzqyCHKk9ay3c7/hPeibTaw5hqtLbtD47P+kPT8Uf3iJWOU2cuuRNfpm+jBn6G MadYS3TIqfACkqyPInMWXqT8Z5G/8fvm1LGxX5NTZXRlAd6giaQylad9QpoTRmbnzQBU W3dA== X-Gm-Message-State: AOAM533mx2JppamiDVc6lVR9eD+L+/nzNW9oGzqjj5JRmmRozvCsuEHW P/xHvZb9MdyEMrrkGGo7Sdu+KA== X-Google-Smtp-Source: ABdhPJzT8DgMJ7YHTkBb38yz3uCfrSQXM0Bv/SGNWktrJrGjVo69aRWM4RzfmjhiplOLId7MFE+kSw== X-Received: by 2002:adf:e9c1:: with SMTP id l1mr8353057wrn.68.1602151442725; Thu, 08 Oct 2020 03:04:02 -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 d23sm6160575wmb.6.2020.10.08.03.04.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 08 Oct 2020 03:04:01 -0700 (PDT) Date: Thu, 8 Oct 2020 12:04:01 +0200 From: Olivier Matz To: "Power, Ciara" Cc: "dev@dpdk.org" , Ray Kinsella , Neil Horman , "Richardson, Bruce" Message-ID: <20201008100401.GV21395@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> <20201007111812.GR21395@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 Thu, Oct 08, 2020 at 09:25:42AM +0000, Power, Ciara wrote: > Hi Olivier, > > > >-----Original Message----- > >From: Olivier Matz > >Sent: Wednesday 7 October 2020 12:18 > >To: Power, Ciara > >Cc: dev@dpdk.org; Ray Kinsella ; Neil Horman > >; Richardson, Bruce > >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 > >> >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, > >}; > > > > > > 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