DPDK patches and discussions
 help / color / mirror / Atom feed
From: "O'loingsigh, Mairtin" <mairtin.oloingsigh@intel.com>
To: "De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>,
	"Singh, Jasvinder" <jasvinder.singh@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"Ryan, Brendan" <brendan.ryan@intel.com>,
	"Coyle, David" <david.coyle@intel.com>
Subject: Re: [dpdk-dev] [PATCH] net: add support for AVX512 when generating CRC
Date: Tue, 29 Sep 2020 15:47:25 +0000	[thread overview]
Message-ID: <MN2PR11MB37258FD0D12FECE129FB429F9C320@MN2PR11MB3725.namprd11.prod.outlook.com> (raw)
In-Reply-To: <SN6PR11MB3101F9F890FF914EAE0E92AA84240@SN6PR11MB3101.namprd11.prod.outlook.com>

Hi,

> -----Original Message-----
> From: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Sent: Friday, September 11, 2020 10:58 AM
> To: O'loingsigh, Mairtin <mairtin.oloingsigh@intel.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Cc: dev@dpdk.org; Ryan, Brendan <brendan.ryan@intel.com>; Coyle, David
> <david.coyle@intel.com>
> Subject: RE: [PATCH] net: add support for AVX512 when generating CRC
> 
> Hi Mairtin,
> 
> > -----Original Message-----
> > From: O'loingsigh, Mairtin <mairtin.oloingsigh@intel.com>
> > Sent: Thursday, September 10, 2020 1:01 PM
> > To: Singh, Jasvinder <jasvinder.singh@intel.com>
> > Cc: dev@dpdk.org; Ryan, Brendan <brendan.ryan@intel.com>; Coyle,
> David
> > <david.coyle@intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>; O'loingsigh, Mairtin
> > <mairtin.oloingsigh@intel.com>
> > Subject: [PATCH] net: add support for AVX512 when generating CRC
> >
> > This patch enables the generation of CRC using AVX512 instruction set
> > when available on the host platform.
> >
> > Signed-off-by: Mairtin o Loingsigh <mairtin.oloingsigh@intel.com>
> > ---
> >
> > v1:
> > * Initial version, with AVX512 support for CRC32 Ethernet only
> > (requires further
> > updates)
> >   * AVX512 support for CRC16-CCITT and final implementation of
> >     CRC32 Ethernet will be added in v2
> > ---
> >  doc/guides/rel_notes/release_20_11.rst |    4 +
> >  lib/librte_net/net_crc_avx.h           |  331
> ++++++++++++++++++++++++++++++++
> >  lib/librte_net/rte_net_crc.c           |   23 ++-
> >  lib/librte_net/rte_net_crc.h           |    1 +
> >  4 files changed, 358 insertions(+), 1 deletions(-)  create mode
> > 100644 lib/librte_net/net_crc_avx.h
> >
> > diff --git a/doc/guides/rel_notes/release_20_11.rst
> > b/doc/guides/rel_notes/release_20_11.rst
> > index df227a1..d6a84ca 100644
> > --- a/doc/guides/rel_notes/release_20_11.rst
> > +++ b/doc/guides/rel_notes/release_20_11.rst
> > @@ -55,6 +55,10 @@ New Features
> >       Also, make sure to start the actual text at the margin.
> >       =======================================================
> >
> > +* **Added support for AVX512 in rte_net CRC calculations.**
> > +
> > +  Added new CRC32 calculation code using AVX512 instruction set
> > + Added new CRC16-CCITT calculation code using AVX512 instruction set
> >
> >  Removed Items
> >  -------------
> > diff --git a/lib/librte_net/net_crc_avx.h
> > b/lib/librte_net/net_crc_avx.h new file mode 100644 index
> > 0000000..d9481d5
> > --- /dev/null
> > +++ b/lib/librte_net/net_crc_avx.h
> 
> ...
> 
> > +static __rte_always_inline uint32_t
> > +crc32_eth_calc_pclmulqdq(
> > +	const uint8_t *data,
> > +	uint32_t data_len,
> > +	uint32_t crc,
> > +	const struct crc_pclmulqdq512_ctx *params) {
> > +	__m256i b;
> > +	__m512i temp, k;
> > +	__m512i qw0 = _mm512_set1_epi64(0);
> > +	__m512i fold0;
> > +	uint32_t n;
> 
> This is loading 64 bytes of data, but if seems like only 16 are available, right?
> Should we use _mm_loadu_si128?
> 
> > +			fold0 = _mm512_xor_si512(fold0, temp);
> > +			goto reduction_128_64;
> > +		}
> > +
> > +		if (unlikely(data_len < 16)) {
> > +			/* 0 to 15 bytes */
> > +			uint8_t buffer[16] __rte_aligned(16);
> > +
> > +			memset(buffer, 0, sizeof(buffer));
> > +			memcpy(buffer, data, data_len);
> 
> I would use _mm_maskz_loadu_epi8, passing a mask register with ((1 <<
> data_len) - 1).
> 
> > +
> > +			fold0 = _mm512_load_si512((const __m128i
> *)buffer);
> > +			fold0 = _mm512_xor_si512(fold0, temp);
> > +			if (unlikely(data_len < 4)) {
> > +				fold0 = xmm_shift_left(fold0, 8 - data_len);
> > +				goto barret_reduction;
> > +			}
> > +			fold0 = xmm_shift_left(fold0, 16 - data_len);
> > +			goto reduction_128_64;
> > +		}
> > +		/* 17 to 31 bytes */
> > +		fold0 = _mm512_loadu_si512((const __m512i *)data);
> 
> Same here. Looks like you are loading too much data?
> 
> > +		fold0 = _mm512_xor_si512(fold0, temp);
> > +		n = 16;
> > +		k = params->rk1_rk2;
> > +		goto partial_bytes;
> > +	}
> 
> ...
> 
> > +
> > +		fold0 = _mm512_xor_si512(fold0, temp);
> > +		fold0 = _mm512_xor_si512(fold0, b);
> 
> You could use _mm512_ternarylogic_epi64 with 0x96 as to do 2x XORs in one
> instruction.
> 
> > +	}
> > +
> > +	/** Reduction 128 -> 32 Assumes: fold holds 128bit folded data */
> > +reduction_128_64:
> > +	k = params->rk5_rk6;
> > +
> > +barret_reduction:
> > +	k = params->rk7_rk8;
> > +	n = crcr32_reduce_64_to_32(fold0, k);
> > +
> > +	return n;
> > +}
> > +
> > +

The latest version of this patch (v3) reworks a lot of this code and address the issues noted above

Mairtin

  reply	other threads:[~2020-09-29 15:47 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-10 12:01 Mairtin o Loingsigh
2020-09-10 12:27 ` Bruce Richardson
2020-09-10 12:52   ` O'loingsigh, Mairtin
2020-09-29 15:45   ` O'loingsigh, Mairtin
2020-09-11  9:57 ` De Lara Guarch, Pablo
2020-09-29 15:47   ` O'loingsigh, Mairtin [this message]
2020-09-29 15:12 ` [dpdk-dev] [PATCH v2 0/2] net: add CRC run-time checks and AVX512/VPCLMULQDQ based CRC Mairtin o Loingsigh
2020-09-29 15:41   ` O'loingsigh, Mairtin
2020-09-29 15:12 ` [dpdk-dev] [PATCH v2 1/2] net: add run-time architecture specific CRC selection Mairtin o Loingsigh
2020-09-29 15:12 ` [dpdk-dev] [PATCH v2 2/2] net: add support for AVX512/VPCLMULQDQ based CRC Mairtin o Loingsigh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=MN2PR11MB37258FD0D12FECE129FB429F9C320@MN2PR11MB3725.namprd11.prod.outlook.com \
    --to=mairtin.oloingsigh@intel.com \
    --cc=brendan.ryan@intel.com \
    --cc=david.coyle@intel.com \
    --cc=dev@dpdk.org \
    --cc=jasvinder.singh@intel.com \
    --cc=pablo.de.lara.guarch@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).