[dpdk-dev] [PATCH v3 17/18] net: add checks for max SIMD bitwidth

[Power, Ciara]

Hi Olivier,

 
>-----Original Message-----
>From: Olivier Matz <olivier.matz at 6wind.com>
>Sent: Tuesday 6 October 2020 11:01
>To: Power, Ciara <ciara.power at intel.com>
>Cc: Coyle, David <david.coyle at intel.com>; Singh, Jasvinder
><jasvinder.singh at intel.com>; dev at dpdk.org; O'loingsigh, Mairtin
><mairtin.oloingsigh at intel.com>; Ryan, Brendan <brendan.ryan at intel.com>;
>Richardson, Bruce <bruce.richardson at 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.coyle at intel.com>
>> >Sent: Thursday 1 October 2020 15:17
>> >To: Singh, Jasvinder <jasvinder.singh at intel.com>; Power, Ciara
>> ><ciara.power at intel.com>; dev at dpdk.org
>> >Cc: Power, Ciara <ciara.power at intel.com>; Olivier Matz
>> ><olivier.matz at 6wind.com>; O'loingsigh, Mairtin
>> ><mairtin.oloingsigh at intel.com>; Ryan, Brendan
>> ><brendan.ryan at intel.com>; Richardson, Bruce
>> ><bruce.richardson at 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.singh at intel.com>
>> >> >
>> >> > > From: dev <dev-bounces at 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