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 F0A33A04BB; Tue, 6 Oct 2020 12:00:45 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CF5C5F64; Tue, 6 Oct 2020 12:00:44 +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 B3DACF3E for ; Tue, 6 Oct 2020 12:00:41 +0200 (CEST) Received: by mail-wm1-f66.google.com with SMTP id f21so2279090wml.3 for ; Tue, 06 Oct 2020 03:00:41 -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=velylKQ/MKnlet2qFbKLyYPBrLRcnT/kQRGVw4LTNLE=; b=X725CQbHHiN61yXkOmjxRb/E2HLTo5tyzzncD4fye966OE2HOOWC0M6yseLt4RcGc4 E4yaRjp2GGEzWqOl/n3PAp2BBVouqWNuupvjuKOKDCv9cqpHPgnDs0aLzSYWzy5/UN3l FsrnpHbV2PTi2KZkryajGXsH5egpLB2xaXDGhy9aw82yXm/C1+1M7AIoqYyVRnQxJfdt NF1PNI14DhX65bUMSDwlVUyFDQMeOIy7NhRKqJvNedSpOmhkmGdXSma8D2kRmhPwvWU2 yd/R2ssje21cYT4H1j93xLIWK9m2RuvkTpq0/D+BWDR+GCqhKilFws/NvlA5lw6RK3r1 rdOA== 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=velylKQ/MKnlet2qFbKLyYPBrLRcnT/kQRGVw4LTNLE=; b=PucTN039R9x8gGmP/aGQ7kGhlDoAI/SYrAUCokm12Vstwxax0gQ+IE8cYArONcMJi8 4l2iV+QDUdv5z53R4iEUNs8kcVCsK7R4bURZvPYjJpKhxdzDSvVsxVnf8KxWd54/EdvS NYL2cEYk910jUekXgE8zzKMIr3+c87c1fh3pMxyPAio+3XsEUo8+B9fkHVPy+KsVWd0W bYwrMAb+VOTSgE0mc+xEDFB/TCki+Ow7Toh4RS+6VkAiagkuMDEJFGCVlIv8Aea1KbPu bkYfB+GHxtxzqP4R6/xj3mw6rFcyXT/p4bSrcnzrj8TCO0DMDEELchHGkRkMPUd+9ecK /+ig== X-Gm-Message-State: AOAM531pn/U9VbUksSzIGWqpYW0wHuIsPQmly+0D97a7aMwRVSr8vltB EjEpNAtQdaZuyD3o6u1aqIBuMg== X-Google-Smtp-Source: ABdhPJw5lCCmK+d2wYXNUHVpfmvSrjqpLycp9sXbPzZU1RNnenyXiK+cXsfcirU/FZQ1CeCNIVDTfw== X-Received: by 2002:a1c:2d94:: with SMTP id t142mr3833824wmt.74.1601978440306; Tue, 06 Oct 2020 03:00:40 -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 t16sm3319768wmi.18.2020.10.06.03.00.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 06 Oct 2020 03:00:39 -0700 (PDT) Date: Tue, 6 Oct 2020 12:00:39 +0200 From: Olivier Matz To: "Power, Ciara" Cc: "Coyle, David" , "Singh, Jasvinder" , "dev@dpdk.org" , "O'loingsigh, Mairtin" , "Ryan, Brendan" , "Richardson, Bruce" Message-ID: <20201006100039.GI21395@platinum> References: <20200807155859.63888-1-ciara.power@intel.com> <20200930130415.11211-1-ciara.power@intel.com> <20200930130415.11211-18-ciara.power@intel.com> 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 17/18] net: add checks for 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, On Thu, Oct 01, 2020 at 02:19:37PM +0000, Power, Ciara wrote: > Hi David, > > Thanks for reviewing, > > >-----Original Message----- > >From: Coyle, David > >Sent: Thursday 1 October 2020 15:17 > >To: Singh, Jasvinder ; Power, Ciara > >; dev@dpdk.org > >Cc: Power, Ciara ; Olivier Matz > >; O'loingsigh, Mairtin > >; Ryan, Brendan ; > >Richardson, Bruce > >Subject: RE: [dpdk-dev] [PATCH v3 17/18] net: add checks for max SIMD > >bitwidth > > > >Hi Jasvinder/Ciara > > > >> -----Original Message----- > >> From: Singh, Jasvinder > >> > > >> > > From: dev On Behalf Of Ciara Power When > >> > > choosing a vector path to take, an extra condition must be > >> > > satisfied to ensure the max SIMD bitwidth allows for the CPU enabled > >path. > >> > > > >> > > The vector path was initially chosen in RTE_INIT, however this is > >> > > no longer suitable as we cannot check the max SIMD bitwidth at that > >time. > >> > > The default chosen in RTE_INIT is now scalar. For best performance > >> > > and to use vector paths, apps must explicitly call the set > >> > > algorithm function before using other functions from this library, > >> > > as this is where vector handlers are now chosen. > >> > > >> > [DC] Has it been decided that it is ok to now require applications > >> > to pick the CRC algorithm they want to use? > >> > > >> > An application which previously automatically got SSE4.2 CRC, for > >> > example, will now automatically only get scalar. > >> > > >> > If this is ok, this should probably be called out explicitly in > >> > release notes as it may not be Immediately noticeable to users that > >> > they now need to select the CRC algo. > >> > > >> > Actually, in general, the release notes need to be updated for this > >> patchset. > >> > >> The decision to move rte_set_alg() out of RTE_INIT was taken to avoid > >> check on max_simd_bitwidth in data path for every single time when > >> crc_calc() api is invoked. Based on my understanding, > >> max_simd_bitwidth is set after eal init, and when used in crc_calc(), > >> it might override the default crc algo set during RTE_INIT. Therefore, > >> to avoid extra check on max_simd_bitwidth in data path, better option > >> will be to use this static configuration one time after eal init in the set_algo > >API. > > > >[DC] Yes that is a good change to have made to avoid extra datapath checks. > > > >Based on off-list discussion, I now also know the reason behind now > >defaulting to scalar CRC in RTE_INIT. If a higher bitwidth CRC was chosen by > >RTE_INIT (e.g. > >SSE4.2 CRC) but the max_simd_bitwidth was then set to RTE_NO_SIMD (64) > >through the EAL parameter or call to rte_set_max_simd_bitwidth(), then > >there is a mismatch if rte_net_crc_set_alg() is not then called to reconfigure > >the CRC. Defaulting to scalar avoids this mismatch and works on all archs > > > >As I mentioned before, I think this needs to be called out in release notes, as > >it's an under-the-hood change which could cause app performance to drop if > >app developers aren't aware of it - the API itself hasn't changed, so they may > >not read the doxygen :) > > > > Yes that is a good point, I can add to the release notes for this to call it out. I don't think it is a good idea to have the scalar crc by default. To me, the fastest available CRC has to be enabled by default. I understand the technical reason why you did it like this however: the SIMD bitwidth may not be known at the time the RTE_INIT(rte_net_crc_init) function is called. A simple approach to solve this issue would be to initialize the rte_net_crc_handler pointer to a handlers_default. The first time a crc is called, the rte_crc32_*_default_handler() function would check the configured SIMD bitwidth, and set the handler to the correct one, to avoid to do the test for next time. This approach still does not solve the case where the SIMD bitwidth is modified during the life of the application. In this case, a callback would have to be registered to notify SIMD bitwidth changes... but I don't think it is worth to do it. Instead, it can be documented that rte_set_max_simd_bitwidth() has to be called early, before rte_eal_init(). > >> > >> > >> > > > >> > > Suggested-by: Jasvinder Singh > >> > > > >> > > Signed-off-by: Ciara Power > >> > > > >> > > --- > >> > > v3: > >> > > - Moved choosing vector paths out of RTE_INIT. > >> > > - Moved checking max_simd_bitwidth into the set_alg function. > >> > > --- > >> > > lib/librte_net/rte_net_crc.c | 26 +++++++++++++++++--------- > >> > > lib/librte_net/rte_net_crc.h | 3 ++- > >> > > 2 files changed, 19 insertions(+), 10 deletions(-) > >> > > > >> > > diff --git a/lib/librte_net/rte_net_crc.c > >> > > b/lib/librte_net/rte_net_crc.c index > >> > > 9fd4794a9d..241eb16399 100644 > >> > > --- a/lib/librte_net/rte_net_crc.c > >> > > +++ b/lib/librte_net/rte_net_crc.c > >> > > >> > > >> > > >> > > @@ -145,18 +149,26 @@ rte_crc32_eth_handler(const uint8_t *data, > >> > > uint32_t data_len) void rte_net_crc_set_alg(enum rte_net_crc_alg > >> > > alg) { > >> > > + if (max_simd_bitwidth == 0) > >> > > + max_simd_bitwidth = rte_get_max_simd_bitwidth(); > >> > > + > >> > > switch (alg) { > >> > > #ifdef X86_64_SSE42_PCLMULQDQ > >> > > case RTE_NET_CRC_SSE42: > >> > > - handlers = handlers_sse42; > >> > > - break; > >> > > + if (max_simd_bitwidth >= RTE_MAX_128_SIMD) { > >> > > + handlers = handlers_sse42; > >> > > + return; > >> > > + } > >> > > + RTE_LOG(INFO, NET, "Max SIMD Bitwidth too low, using > >> > > scalar\n"); > >> > > >> > [DC] Not sure if you're aware but there is another patchset which > >> > adds an > >> > AVX512 CRC implementation and run-time checking of cpuflags to > >> > select the CRC path to use: > >> > https://patchwork.dpdk.org/project/dpdk/list/?series=12596 > >> > > >> > There will be a task to merge these 2 patchsets if both are merged. > >> > It looks fairly straightforward to me to merge these, but it would > >> > be good if you take a look too > > I have looked at that patchset, I agree, I think they will be straightforward to merge together. > > Thanks, > Ciara