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

[Singh, Jasvinder]

> -----Original Message-----
> From: Coyle, David <david.coyle at intel.com>
> Sent: Wednesday, September 30, 2020 4:04 PM
> To: Power, Ciara <ciara.power at intel.com>; dev at dpdk.org
> Cc: Power, Ciara <ciara.power at intel.com>; Singh, Jasvinder
> <jasvinder.singh 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 Ciara,
> 
> > 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. 

 
> >
> > Suggested-by: Jasvinder Singh <jasvinder.singh at intel.com>
> >
> > Signed-off-by: Ciara Power <ciara.power at 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