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

[Olivier Matz <olivier.matz@6wind.com>]

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

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 <jasvinder.singh@intel.com>
> >> > >
> >> > > Signed-off-by: Ciara Power <ciara.power@intel.com>
> >> > >
> >> > > ---
> >> > > 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
> >> >
> >> > <snip>
> >> >
> >> > > @@ -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