Hi Olivier, >-----Original Message----- >From: Olivier Matz <olivier.m...@6wind.com> >Sent: Tuesday 6 October 2020 11:01 >To: Power, Ciara <ciara.po...@intel.com> >Cc: Coyle, David <david.co...@intel.com>; Singh, Jasvinder ><jasvinder.si...@intel.com>; dev@dpdk.org; O'loingsigh, Mairtin ><mairtin.oloings...@intel.com>; Ryan, Brendan <brendan.r...@intel.com>; >Richardson, Bruce <bruce.richard...@intel.com> >Subject: Re: [dpdk-dev] [PATCH v3 17/18] net: add checks for max SIMD >bitwidth > >Hi, > >On Thu, Oct 01, 2020 at 02:19:37PM +0000, Power, Ciara wrote: >> Hi David, >> >> Thanks for reviewing, >> >> >-----Original Message----- >> >From: Coyle, David <david.co...@intel.com> >> >Sent: Thursday 1 October 2020 15:17 >> >To: Singh, Jasvinder <jasvinder.si...@intel.com>; Power, Ciara >> ><ciara.po...@intel.com>; dev@dpdk.org >> >Cc: Power, Ciara <ciara.po...@intel.com>; Olivier Matz >> ><olivier.m...@6wind.com>; O'loingsigh, Mairtin >> ><mairtin.oloings...@intel.com>; Ryan, Brendan >> ><brendan.r...@intel.com>; Richardson, Bruce >> ><bruce.richard...@intel.com> >> >Subject: RE: [dpdk-dev] [PATCH v3 17/18] net: add checks for max SIMD >> >bitwidth >> > >> >Hi Jasvinder/Ciara >> > >> >> -----Original Message----- >> >> From: Singh, Jasvinder <jasvinder.si...@intel.com> >> >> > >> >> > > From: dev <dev-boun...@dpdk.org> 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.
Thanks for this suggestion, will try this for the next version, it seems it will work quite well, thanks. >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(). > Yes, It is documented in the Doxygen comment for the rte_set_max_simd_bitwidth() function that it should be called early, as you mentioned. <snip> Thanks, Ciara
