On Tue, Nov 10, 2026 at 3:21 PM Konstantin Ananyev < [email protected]> wrote:
> > > > > > > > > On Mon, Nov 9, 2026 at 3:40 PM Konstantin Ananyev > > <[email protected]> wrote: > > > > > > > Replace the __m128i members in the PCLMULQDQ CRC context with > > aligned > > > uint64_t arrays to allow portable compile-time initialization. > > > > > > Move CRC16-CCITT and CRC32-ETH folding constants into static > > const context > > > structures in their respective handlers, removing the runtime > > > initialization previously performed by rte_net_crc_sse42_init > > and avoids > > > the extra data segment static variables which were being populated. > > > > > > Load the constants into SIMD registers using aligned loads when > > performing > > > CRC calculations. This was done transparently previously when the > > > __m128i values were assigned, now there is explicit SIMD load done. > > > > It is not clear why(how) new code is better then what we have > > right now. > > Can you probably provide a bit more details? > > > > > > Previously this file was maintaining static variables in data segment > > namely: > > static alignas(16) struct crc_pclmulqdq_ctx crc32_eth_pclmulqdq; > > static alignas(16) struct crc_pclmulqdq_ctx crc16_ccitt_pclmulqdq; > > > > These were populated in the init function with the same calculated > > fold constants. > > The constants already are present in the rodata section in the binary. > > Hence the init is doing extra work in copying it over to the > > static crc_pclmulqdq_ctx > > structures which is redundant. > > > > This patch avoids this extra copy during init and modifies the SIMD > > routines > > to directly read the constants from rodata memory. Thus the data > > segment memory usage is > > reduced by getting rid of the static variables (128 bytes). There > > should be no > > performance impact by this change (minus the init work done previously). > > TBH, it looks to me like a nitpicking: yes we move 128B from .data to > .rodata, but in terms > of functionality or perfomance - does it really matter? > It is not moving from .data to .rodata, same data was already there in .rodata, hence it is removal from .data. Well this was mainly motivated from the rewrite for improving barrett reduction. I've combined this in the other patch and we can drop this patch from review. Kindly review the newer submission which includes this refactory change as well: https://patches.dpdk.org/project/dpdk/patch/[email protected]/ Thanks, Shreesh > Though yes, the code looks a bit simpler and nicer. > If we decided to improve the init code, then my preference would be to > use rte_xmm_t? > Then we can do something like that: > struct crc_pclmulqdq_ctx { > rte_xmm_t rk1_rk2; > ... > }; > ... > static const struct crc_pclmulqdq_ctx crc32_eth_pclmulqdq = { > .rk1_rk2 = { .u64 = {0x1c6e41596LLU, 0x154442bd4LLU }}, > ... > }; > ... > __m128i k = params->rk3_rk4.x; > > > > > > > > > Signed-off-by: Shreesh Adiga <[email protected]> > > > --- > > > lib/net/net_crc_sse.c | 68 > > ++++++++++++++----------------------------- > > > 1 file changed, 22 insertions(+), 46 deletions(-) > > > > > > diff --git a/lib/net/net_crc_sse.c b/lib/net/net_crc_sse.c > > > index e30f8544fc..4361e35e39 100644 > > > --- a/lib/net/net_crc_sse.c > > > +++ b/lib/net/net_crc_sse.c > > > @@ -12,15 +12,13 @@ > > > #include "net_crc.h" > > > > > > /** PCLMULQDQ CRC computation context structure */ > > > -struct crc_pclmulqdq_ctx { > > > - __m128i rk1_rk2; > > > - __m128i rk3_rk4; > > > - __m128i rk5_rk6; > > > - __m128i rk7_rk8; > > > +struct __rte_aligned(XMM_SIZE) crc_pclmulqdq_ctx { > > > + uint64_t rk1_rk2[2]; > > > + uint64_t rk3_rk4[2]; > > > + uint64_t rk5_rk6[2]; > > > + uint64_t rk7_rk8[2]; > > > }; > > > > > > -static alignas(16) struct crc_pclmulqdq_ctx crc32_eth_pclmulqdq; > > > -static alignas(16) struct crc_pclmulqdq_ctx crc16_ccitt_pclmulqdq; > > > /** > > > * @brief Performs one folding round > > > * > > > @@ -162,7 +160,7 @@ crc32_eth_calc_pclmulqdq( > > > fold3 = _mm_loadu_si128((const __m128i *)(data + > 32)); > > > fold4 = _mm_loadu_si128((const __m128i *)(data + > 48)); > > > fold1 = _mm_xor_si128(fold1, temp); > > > - k = params->rk1_rk2; > > > + k = _mm_load_si128((const __m128i *)params->rk1_rk2); > > > > > > for (n = 64; (n + 64) <= data_len; n += 64) { > > > temp1 = _mm_loadu_si128((const __m128i > > *)&data[n]); > > > @@ -175,7 +173,7 @@ crc32_eth_calc_pclmulqdq( > > > fold4 = crcr32_folding_round(temp4, k, > fold4); > > > } > > > > > > - k = params->rk3_rk4; > > > + k = _mm_load_si128((const __m128i *)params->rk3_rk4); > > > fold1 = crcr32_folding_round(fold2, k, fold1); > > > fold1 = crcr32_folding_round(fold3, k, fold1); > > > fold = crcr32_folding_round(fold4, k, fold1); > > > @@ -205,7 +203,7 @@ crc32_eth_calc_pclmulqdq( > > > fold = _mm_xor_si128(fold, temp); > > > > > > /** Single folding loop - the last 16 bytes is processed > > separately */ > > > - k = params->rk3_rk4; > > > + k = _mm_load_si128((const __m128i *)params->rk3_rk4); > > > n = 16; > > > > > > single_fold_loop: > > > @@ -235,11 +233,11 @@ crc32_eth_calc_pclmulqdq( > > > > > > /** Reduction 128 -> 32 Assumes: fold holds 128bit folded > > data */ > > > reduction_128_64: > > > - k = params->rk5_rk6; > > > + k = _mm_load_si128((const __m128i *)params->rk5_rk6); > > > fold = crcr32_reduce_128_to_64(fold, k); > > > > > > barret_reduction: > > > - k = params->rk7_rk8; > > > + k = _mm_load_si128((const __m128i *)params->rk7_rk8); > > > n = crcr32_reduce_64_to_32(fold, k); > > > > > > return n; > > > @@ -248,45 +246,17 @@ crc32_eth_calc_pclmulqdq( > > > void > > > rte_net_crc_sse42_init(void) > > > { > > > - uint64_t k1, k2, k3, k4, k5, k6; > > > - uint64_t p = 0, q = 0; > > > - > > > - /** Initialize CRC16 data */ > > > - k1 = 0x14ff2LLU; > > > - k2 = 0x19a3cLLU; > > > - k3 = 0x189aeLLU; > > > - k4 = 0x8e10LLU; > > > - k5 = 0x189aeLLU; > > > - k6 = 0x114aaLLU; > > > - q = 0x11c581910LLU; > > > - p = 0x10811LLU; > > > - > > > - /** Save the params in context structure */ > > > - crc16_ccitt_pclmulqdq.rk1_rk2 = _mm_set_epi64x(k2, k1); > > > - crc16_ccitt_pclmulqdq.rk3_rk4 = _mm_set_epi64x(k4, k3); > > > - crc16_ccitt_pclmulqdq.rk5_rk6 = _mm_set_epi64x(k6, k5); > > > - crc16_ccitt_pclmulqdq.rk7_rk8 = _mm_set_epi64x(p, q); > > > - > > > - /** Initialize CRC32 data */ > > > - k1 = 0x1c6e41596LLU; > > > - k2 = 0x154442bd4LLU; > > > - k3 = 0xccaa009eLLU; > > > - k4 = 0x1751997d0LLU; > > > - k5 = 0xccaa009eLLU; > > > - k6 = 0x163cd6124LLU; > > > - q = 0x1f7011640LLU; > > > - p = 0x1db710641LLU; > > > - > > > - /** Save the params in context structure */ > > > - crc32_eth_pclmulqdq.rk1_rk2 = _mm_set_epi64x(k2, k1); > > > - crc32_eth_pclmulqdq.rk3_rk4 = _mm_set_epi64x(k4, k3); > > > - crc32_eth_pclmulqdq.rk5_rk6 = _mm_set_epi64x(k6, k5); > > > - crc32_eth_pclmulqdq.rk7_rk8 = _mm_set_epi64x(p, q); > > > } > > > > > > uint32_t > > > rte_crc16_ccitt_sse42_handler(const uint8_t *data, uint32_t > > data_len) > > > { > > > + static const struct crc_pclmulqdq_ctx > > crc16_ccitt_pclmulqdq = { > > > + .rk1_rk2 = { 0x14ff2LLU, 0x19a3cLLU }, > > > + .rk3_rk4 = { 0x189aeLLU, 0x8e10LLU }, > > > + .rk5_rk6 = { 0x189aeLLU, 0x114aaLLU }, > > > + .rk7_rk8 = { 0x11c581910LLU, 0x10811LLU }, > > > + }; > > > /** return 16-bit CRC value */ > > > return (uint16_t)~crc32_eth_calc_pclmulqdq(data, > > > data_len, > > > @@ -297,6 +267,12 @@ rte_crc16_ccitt_sse42_handler(const uint8_t > > *data, uint32_t data_len) > > > uint32_t > > > rte_crc32_eth_sse42_handler(const uint8_t *data, uint32_t > > data_len) > > > { > > > + static const struct crc_pclmulqdq_ctx crc32_eth_pclmulqdq = { > > > + .rk1_rk2 = { 0x1c6e41596LLU, 0x154442bd4LLU }, > > > + .rk3_rk4 = { 0xccaa009eLLU, 0x1751997d0LLU }, > > > + .rk5_rk6 = { 0xccaa009eLLU, 0x163cd6124LLU }, > > > + .rk7_rk8 = { 0x1f7011640LLU, 0x1db710641LLU }, > > > + }; > > > return ~crc32_eth_calc_pclmulqdq(data, > > > data_len, > > > 0xffffffffUL, > > > >
