[PATCH] app/testpmd: improve sse based macswap

[PATCH] app/testpmd: improve sse based macswap

[Vipin Varghese]

Goal of the patch is to improve SSE macswap on x86_64 by reducing
the stalls in backend engine. Original implementation of the SSE
macswap makes loop call to multiple load, shuffle & store. Using
SIMD ISA interleaving we can reduce the stalls for
 - load SSE token exhaustion
 - Shuffle and Load dependency

Also other changes which improves packet per second are
 - Filling access to MBUF for offload flags which is separate cacheline,
 - using register keyword

Build test using meson script:
``````````````````````````````

build-gcc-static
buildtools
build-gcc-shared
build-mini
build-clang-static
build-clang-shared
build-x86-generic

Test Results:
`````````````

Platform-1: AMD EPYC SIENA 8594P @2.3GHz, no boost

------------------------------------------------
TEST IO 64B: baseline <NIC : MPPs>
 - mellanox CX-7 2*200Gbps : 42.0
 - intel E810 1*100Gbps : 82.0
 - intel E810 2*200Gbps (2CQ-DA2): 82.45
------------------------------------------------
TEST MACSWAP 64B: <NIC : Before : After>
 - mellanox CX-7 2*200Gbps : 31.533 : 31.90
 - intel E810 1*100Gbps : 50.380 : 47.0
 - intel E810 2*200Gbps (2CQ-DA2): 48.840 : 49.827
------------------------------------------------
TEST MACSWAP 128B: <NIC : Before: After>
 - mellanox CX-7 2*200Gbps: 30.946 : 31.770
 - intel E810 1*100Gbps: 49.386 : 46.366
 - intel E810 2*200Gbps (2CQ-DA2): 47.979 : 49.503
------------------------------------------------
TEST MACSWAP 256B: <NIC: Before: After>
 - mellanox CX-7 2*200Gbps: 32.480 : 33.150
 - intel E810 1 * 100Gbps: 45.29 : 44.571
 - intel E810 2 * 200Gbps (2CQ-DA2): 45.033 : 45.117
------------------------------------------------

Platform-2: AMD EPYC 9554 @3.1GHz, no boost

------------------------------------------------
TEST IO 64B: baseline <NIC : MPPs>
 - intel E810 2*200Gbps (2CQ-DA2): 82.49
------------------------------------------------
<NIC intel E810 2*200Gbps (2CQ-DA2): Before : After>
TEST MACSWAP: 1Q 1C1T
 64B: : 45.0 : 45.54
128B: : 44.48 : 44.43
256B: : 42.0 : 41.99
+++++++++++++++++++++++++
TEST MACSWAP: 2Q 2C2T
 64B: : 59.5 : 60.55
128B: : 56.78 : 58.1
256B: : 41.85 : 41.99
------------------------------------------------

Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
---
 app/test-pmd/macswap_sse.h | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/app/test-pmd/macswap_sse.h b/app/test-pmd/macswap_sse.h
index 223f87a539..6e4ed21924 100644
--- a/app/test-pmd/macswap_sse.h
+++ b/app/test-pmd/macswap_sse.h
@@ -16,16 +16,16 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
 	uint64_t ol_flags;
 	int i;
 	int r;
-	__m128i addr0, addr1, addr2, addr3;
+	register __m128i addr0, addr1, addr2, addr3;
 	/**
 	 * shuffle mask be used to shuffle the 16 bytes.
 	 * byte 0-5 wills be swapped with byte 6-11.
 	 * byte 12-15 will keep unchanged.
 	 */
-	__m128i shfl_msk = _mm_set_epi8(15, 14, 13, 12,
-					5, 4, 3, 2,
-					1, 0, 11, 10,
-					9, 8, 7, 6);
+	register const __m128i shfl_msk = _mm_set_epi8(15, 14, 13, 12,
+							5, 4, 3, 2,
+							1, 0, 11, 10,
+							9, 8, 7, 6);
 
 	ol_flags = ol_flags_init(txp->dev_conf.txmode.offloads);
 	vlan_qinq_set(pkts, nb, ol_flags,
@@ -44,23 +44,24 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
 
 		mb[0] = pkts[i++];
 		eth_hdr[0] = rte_pktmbuf_mtod(mb[0], struct rte_ether_hdr *);
-		addr0 = _mm_loadu_si128((__m128i *)eth_hdr[0]);
-
 		mb[1] = pkts[i++];
 		eth_hdr[1] = rte_pktmbuf_mtod(mb[1], struct rte_ether_hdr *);
-		addr1 = _mm_loadu_si128((__m128i *)eth_hdr[1]);
-
-
 		mb[2] = pkts[i++];
 		eth_hdr[2] = rte_pktmbuf_mtod(mb[2], struct rte_ether_hdr *);
-		addr2 = _mm_loadu_si128((__m128i *)eth_hdr[2]);
-
 		mb[3] = pkts[i++];
 		eth_hdr[3] = rte_pktmbuf_mtod(mb[3], struct rte_ether_hdr *);
-		addr3 = _mm_loadu_si128((__m128i *)eth_hdr[3]);
 
+		/* Interleave loads and shuffle with field set */
+		addr0 = _mm_loadu_si128((__m128i *)eth_hdr[0]);
+		mbuf_field_set(mb[0], ol_flags);
+		addr1 = _mm_loadu_si128((__m128i *)eth_hdr[1]);
+		mbuf_field_set(mb[1], ol_flags);
 		addr0 = _mm_shuffle_epi8(addr0, shfl_msk);
+		addr2 = _mm_loadu_si128((__m128i *)eth_hdr[2]);
+		mbuf_field_set(mb[2], ol_flags);
 		addr1 = _mm_shuffle_epi8(addr1, shfl_msk);
+		addr3 = _mm_loadu_si128((__m128i *)eth_hdr[3]);
+		mbuf_field_set(mb[3], ol_flags);
 		addr2 = _mm_shuffle_epi8(addr2, shfl_msk);
 		addr3 = _mm_shuffle_epi8(addr3, shfl_msk);
 
@@ -69,25 +70,21 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
 		_mm_storeu_si128((__m128i *)eth_hdr[2], addr2);
 		_mm_storeu_si128((__m128i *)eth_hdr[3], addr3);
 
-		mbuf_field_set(mb[0], ol_flags);
-		mbuf_field_set(mb[1], ol_flags);
-		mbuf_field_set(mb[2], ol_flags);
-		mbuf_field_set(mb[3], ol_flags);
 		r -= 4;
 	}
 
 	for ( ; i < nb; i++) {
-		if (i < nb - 1)
+		if (i < (nb - 1))
 			rte_prefetch0(rte_pktmbuf_mtod(pkts[i+1], void *));
 		mb[0] = pkts[i];
 		eth_hdr[0] = rte_pktmbuf_mtod(mb[0], struct rte_ether_hdr *);
 
 		/* Swap dest and src mac addresses. */
 		addr0 = _mm_loadu_si128((__m128i *)eth_hdr[0]);
+		/* invoke field_set as it is on separate cacheline */
+		mbuf_field_set(mb[0], ol_flags);
 		addr0 = _mm_shuffle_epi8(addr0, shfl_msk);
 		_mm_storeu_si128((__m128i *)eth_hdr[0], addr0);
-
-		mbuf_field_set(mb[0], ol_flags);
 	}
 }
 
-- 
2.34.1

Re: [PATCH] app/testpmd: improve sse based macswap

[Ferruh Yigit]

On 7/16/2024 7:37 AM, Vipin Varghese wrote:
> Goal of the patch is to improve SSE macswap on x86_64 by reducing
> the stalls in backend engine. Original implementation of the SSE
> macswap makes loop call to multiple load, shuffle & store. Using
> SIMD ISA interleaving we can reduce the stalls for
>  - load SSE token exhaustion
>  - Shuffle and Load dependency
> 
> Also other changes which improves packet per second are
>  - Filling access to MBUF for offload flags which is separate cacheline,
>  - using register keyword
> 
> Build test using meson script:
> ``````````````````````````````
> 
> build-gcc-static
> buildtools
> build-gcc-shared
> build-mini
> build-clang-static
> build-clang-shared
> build-x86-generic
> 
> Test Results:
> `````````````
> 
> Platform-1: AMD EPYC SIENA 8594P @2.3GHz, no boost
> 
> ------------------------------------------------
> TEST IO 64B: baseline <NIC : MPPs>
>  - mellanox CX-7 2*200Gbps : 42.0
>  - intel E810 1*100Gbps : 82.0
>  - intel E810 2*200Gbps (2CQ-DA2): 82.45
> ------------------------------------------------
> TEST MACSWAP 64B: <NIC : Before : After>
>  - mellanox CX-7 2*200Gbps : 31.533 : 31.90
>  - intel E810 1*100Gbps : 50.380 : 47.0
>  - intel E810 2*200Gbps (2CQ-DA2): 48.840 : 49.827
> ------------------------------------------------
> TEST MACSWAP 128B: <NIC : Before: After>
>  - mellanox CX-7 2*200Gbps: 30.946 : 31.770
>  - intel E810 1*100Gbps: 49.386 : 46.366
>  - intel E810 2*200Gbps (2CQ-DA2): 47.979 : 49.503
> ------------------------------------------------
> TEST MACSWAP 256B: <NIC: Before: After>
>  - mellanox CX-7 2*200Gbps: 32.480 : 33.150
>  - intel E810 1 * 100Gbps: 45.29 : 44.571
>  - intel E810 2 * 200Gbps (2CQ-DA2): 45.033 : 45.117
> ------------------------------------------------
> 
> Platform-2: AMD EPYC 9554 @3.1GHz, no boost
> 
> ------------------------------------------------
> TEST IO 64B: baseline <NIC : MPPs>
>  - intel E810 2*200Gbps (2CQ-DA2): 82.49
> ------------------------------------------------
> <NIC intel E810 2*200Gbps (2CQ-DA2): Before : After>
> TEST MACSWAP: 1Q 1C1T
>  64B: : 45.0 : 45.54
> 128B: : 44.48 : 44.43
> 256B: : 42.0 : 41.99
> +++++++++++++++++++++++++
> TEST MACSWAP: 2Q 2C2T
>  64B: : 59.5 : 60.55
> 128B: : 56.78 : 58.1
> 256B: : 41.85 : 41.99
> ------------------------------------------------
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
>

Hi Bruce, John,

Can you please help testing macswap performance with this patch on Intel
platforms, to be sure it is not causing regression?

Other option is to get this patch for -rc3 and tested there, with the
condition to remove it in any regression, if this help testing the patch?

Thanks,
ferruh

Re: [PATCH] app/testpmd: improve sse based macswap

[Bruce Richardson]

On Tue, Jul 23, 2024 at 05:45:57PM +0100, Ferruh Yigit wrote:
> On 7/16/2024 7:37 AM, Vipin Varghese wrote:
> > Goal of the patch is to improve SSE macswap on x86_64 by reducing
> > the stalls in backend engine. Original implementation of the SSE
> > macswap makes loop call to multiple load, shuffle & store. Using
> > SIMD ISA interleaving we can reduce the stalls for
> >  - load SSE token exhaustion
> >  - Shuffle and Load dependency
> > 
> > Also other changes which improves packet per second are
> >  - Filling access to MBUF for offload flags which is separate cacheline,
> >  - using register keyword
> > 
> > Build test using meson script:
> > ``````````````````````````````
> > 
> > build-gcc-static
> > buildtools
> > build-gcc-shared
> > build-mini
> > build-clang-static
> > build-clang-shared
> > build-x86-generic
> > 
> > Test Results:
> > `````````````
> > 
> > Platform-1: AMD EPYC SIENA 8594P @2.3GHz, no boost
> > 
> > ------------------------------------------------
> > TEST IO 64B: baseline <NIC : MPPs>
> >  - mellanox CX-7 2*200Gbps : 42.0
> >  - intel E810 1*100Gbps : 82.0
> >  - intel E810 2*200Gbps (2CQ-DA2): 82.45
> > ------------------------------------------------
> > TEST MACSWAP 64B: <NIC : Before : After>
> >  - mellanox CX-7 2*200Gbps : 31.533 : 31.90
> >  - intel E810 1*100Gbps : 50.380 : 47.0
> >  - intel E810 2*200Gbps (2CQ-DA2): 48.840 : 49.827
> > ------------------------------------------------
> > TEST MACSWAP 128B: <NIC : Before: After>
> >  - mellanox CX-7 2*200Gbps: 30.946 : 31.770
> >  - intel E810 1*100Gbps: 49.386 : 46.366
> >  - intel E810 2*200Gbps (2CQ-DA2): 47.979 : 49.503
> > ------------------------------------------------
> > TEST MACSWAP 256B: <NIC: Before: After>
> >  - mellanox CX-7 2*200Gbps: 32.480 : 33.150
> >  - intel E810 1 * 100Gbps: 45.29 : 44.571
> >  - intel E810 2 * 200Gbps (2CQ-DA2): 45.033 : 45.117
> > ------------------------------------------------
> > 
> > Platform-2: AMD EPYC 9554 @3.1GHz, no boost
> > 
> > ------------------------------------------------
> > TEST IO 64B: baseline <NIC : MPPs>
> >  - intel E810 2*200Gbps (2CQ-DA2): 82.49
> > ------------------------------------------------
> > <NIC intel E810 2*200Gbps (2CQ-DA2): Before : After>
> > TEST MACSWAP: 1Q 1C1T
> >  64B: : 45.0 : 45.54
> > 128B: : 44.48 : 44.43
> > 256B: : 42.0 : 41.99
> > +++++++++++++++++++++++++
> > TEST MACSWAP: 2Q 2C2T
> >  64B: : 59.5 : 60.55
> > 128B: : 56.78 : 58.1
> > 256B: : 41.85 : 41.99
> > ------------------------------------------------
> > 
> > Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
> >
> 
> Hi Bruce, John,
> 
> Can you please help testing macswap performance with this patch on Intel
> platforms, to be sure it is not causing regression?
> 
Hi Ferruh,

We can try and get some Intel numbers for you, but I think at this point it
is better deferred to 24.11 due to lack of discussion and analysis of the
numbers. This is because the numbers above already show that it is causing
regressions - in fact many of the regressions are larger than the benefits
shown. This may be acceptable, but it would imply that we shouldn't be too
hasty in applying the patch.

Regards,
/Bruce

Re: [PATCH] app/testpmd: improve sse based macswap

[Varghese, Vipin]

[-- Attachment #1: Type: text/plain, Size: 3591 bytes --]

Hi Bruce,

Thanks for highlighting the variance. We found this was an internal test 
bed configuration issue. We are sharing the next version of the same 
patch with updated numbers.


On 7/23/2024 10:42 PM, Bruce Richardson wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Tue, Jul 23, 2024 at 05:45:57PM +0100, Ferruh Yigit wrote:
>> On 7/16/2024 7:37 AM, Vipin Varghese wrote:
>>> Goal of the patch is to improve SSE macswap on x86_64 by reducing
>>> the stalls in backend engine. Original implementation of the SSE
>>> macswap makes loop call to multiple load, shuffle & store. Using
>>> SIMD ISA interleaving we can reduce the stalls for
>>>   - load SSE token exhaustion
>>>   - Shuffle and Load dependency
>>>
>>> Also other changes which improves packet per second are
>>>   - Filling access to MBUF for offload flags which is separate cacheline,
>>>   - using register keyword
>>>
>>> Build test using meson script:
>>> ``````````````````````````````
>>>
>>> build-gcc-static
>>> buildtools
>>> build-gcc-shared
>>> build-mini
>>> build-clang-static
>>> build-clang-shared
>>> build-x86-generic
>>>
>>> Test Results:
>>> `````````````
>>>
>>> Platform-1: AMD EPYC SIENA 8594P @2.3GHz, no boost
>>>
>>> ------------------------------------------------
>>> TEST IO 64B: baseline <NIC : MPPs>
>>>   - mellanox CX-7 2*200Gbps : 42.0
>>>   - intel E810 1*100Gbps : 82.0
>>>   - intel E810 2*200Gbps (2CQ-DA2): 82.45
>>> ------------------------------------------------
>>> TEST MACSWAP 64B: <NIC : Before : After>
>>>   - mellanox CX-7 2*200Gbps : 31.533 : 31.90
>>>   - intel E810 1*100Gbps : 50.380 : 47.0
>>>   - intel E810 2*200Gbps (2CQ-DA2): 48.840 : 49.827
>>> ------------------------------------------------
>>> TEST MACSWAP 128B: <NIC : Before: After>
>>>   - mellanox CX-7 2*200Gbps: 30.946 : 31.770
>>>   - intel E810 1*100Gbps: 49.386 : 46.366
>>>   - intel E810 2*200Gbps (2CQ-DA2): 47.979 : 49.503
>>> ------------------------------------------------
>>> TEST MACSWAP 256B: <NIC: Before: After>
>>>   - mellanox CX-7 2*200Gbps: 32.480 : 33.150
>>>   - intel E810 1 * 100Gbps: 45.29 : 44.571
>>>   - intel E810 2 * 200Gbps (2CQ-DA2): 45.033 : 45.117
>>> ------------------------------------------------
>>>
>>> Platform-2: AMD EPYC 9554 @3.1GHz, no boost
>>>
>>> ------------------------------------------------
>>> TEST IO 64B: baseline <NIC : MPPs>
>>>   - intel E810 2*200Gbps (2CQ-DA2): 82.49
>>> ------------------------------------------------
>>> <NIC intel E810 2*200Gbps (2CQ-DA2): Before : After>
>>> TEST MACSWAP: 1Q 1C1T
>>>   64B: : 45.0 : 45.54
>>> 128B: : 44.48 : 44.43
>>> 256B: : 42.0 : 41.99
>>> +++++++++++++++++++++++++
>>> TEST MACSWAP: 2Q 2C2T
>>>   64B: : 59.5 : 60.55
>>> 128B: : 56.78 : 58.1
>>> 256B: : 41.85 : 41.99
>>> ------------------------------------------------
>>>
>>> Signed-off-by: Vipin Varghese<vipin.varghese@amd.com>
>>>
>> Hi Bruce, John,
>>
>> Can you please help testing macswap performance with this patch on Intel
>> platforms, to be sure it is not causing regression?
>>
> Hi Ferruh,
>
> We can try and get some Intel numbers for you, but I think at this point it
> is better deferred to 24.11 due to lack of discussion and analysis of the
> numbers. This is because the numbers above already show that it is causing
> regressions - in fact many of the regressions are larger than the benefits
> shown. This may be acceptable, but it would imply that we shouldn't be too
> hasty in applying the patch.
>
> Regards,
> /Bruce

[-- Attachment #2: Type: text/html, Size: 4183 bytes --]

Re: [PATCH] app/testpmd: improve sse based macswap

[Bruce Richardson]

On Thu, Jul 25, 2024 at 06:17:35PM +0530, Varghese, Vipin wrote:
>    Hi Bruce,
>    Thanks for highlighting the variance. We found this was an internal
>    test bed configuration issue. We are sharing the next version of the
>    same patch with updated numbers.
> 

Great, thanks for the update.

[PATCH v2 0/3] app/testpmd: improve sse based macswap

[Vipin Varghese]

Goal of the patch series is to improve SSE macswap on x86_64 by
reducing the stalls in backend engine. Original implementation of
the SSE-mac-swap makes loop call to multiple load, shuffle & store.

Using SIMD ISA interleaving, register variable and reducing L1 & L2
cache eviction, we can reduce the stalls for
 - load SSE token exhaustion
 - Shuffle and Load dependency

Build test using meson script:
``````````````````````````````
build-gcc-static
buildtools
build-gcc-shared
build-mini
build-clang-static
build-clang-shared
build-x86-generic

Test Results:
`````````````

Platform-1: AMD EPYC SIENA 8594P @2.3GHz, no boost
Platform-2: AMD EPYC 9554 @3.1GHz, no boost

NIC:
 1) mellanox CX-7 1*200Gbps
 2) intel E810 1*100Gbps
 3) intel E810 2*200Gbps (2CQ-DA2) - loopback
 4) braodcom P2100 2*100Gbps - loopback

------------------------------------------------
TEST IO 64B: baseline <NIC : MPPs>
 - NIC-1: 42.0
 - NIC-2: 82.0
 - NIC-3: 82.45
 - NIC-3: 47.03
------------------------------------------------
TEST MACSWAP 64B: <NIC : Before : After>
 - NIC-1: 31.533 : 31.90
 - NIC-2: 48.0   : 48.9 
 - NIC-3: 48.840 : 49.827
 - NIC-4: 44.3   : 45.5
------------------------------------------------
TEST MACSWAP 128B: <NIC : Before: After>
 - NIC-1: 30.946 : 31.770
 - NIC-2: 47.4   : 48.3
 - NIC-3: 47.979 : 48.503
 - NIC-4: 41.53  : 44.59
------------------------------------------------
TEST MACSWAP 256B: <NIC: Before: After>
 - NIC-1: 32.480 : 33.150
 - NIC-2: 45.29  : 45.571
 - NIC-3: 45.033 : 45.117
 - NIC-4: 36.49  : 37.5
------------------------------------------------


------------------------------------------------
TEST IO 64B: baseline <NIC : MPPs>
 - intel E810 2*200Gbps (2CQ-DA2): 82.49
------------------------------------------------
<NIC intel E810 2*200Gbps (2CQ-DA2): Before : After>
TEST MACSWAP: 1Q 1C1T
 64B: : 45.0 : 45.54
128B: : 44.48 : 44.43
256B: : 42.0 : 41.99
+++++++++++++++++++++++++
TEST MACSWAP: 2Q 2C2T
 64B: : 59.5 : 60.55
128B: : 56.78 : 58.1
256B: : 41.85 : 41.99
------------------------------------------------

Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>

Vipin Varghese (3):
  app/testpmd: add register keyword
  app/testpmd: move offload update
  app/testpmd: interleave SSE SIMD

 app/test-pmd/macswap_sse.h | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] app/testpmd: add register keyword

[Vipin Varghese]

Currently SSE SIMD variables are declared as stack variables. Allowing
the use of keyword register for shuffle mask and address variables,
improves the mac-swap Mpps by 1 for single queue.

Test Result:
 * Platform: AMD EPYC 9554 @3.1GHz, no boost
 * Test scenarios: TEST-PMD 64B IO vs MAC-SWAP
 * NIC: broadcom P2100: loopback 2*100Gbps

<mode : Mpps Ingress: Mpps Egress>
------------------------------------------------
 - IO: 47.23 : 46.0
 - MAC-SWAP original: 45.75 : 43.8
 - MAC-SWAP register mod 45.73 : 44.83

Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
---
 app/test-pmd/macswap_sse.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/macswap_sse.h b/app/test-pmd/macswap_sse.h
index 223f87a539..29088843b7 100644
--- a/app/test-pmd/macswap_sse.h
+++ b/app/test-pmd/macswap_sse.h
@@ -16,13 +16,13 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
 	uint64_t ol_flags;
 	int i;
 	int r;
-	__m128i addr0, addr1, addr2, addr3;
+	register __m128i addr0, addr1, addr2, addr3;
 	/**
 	 * shuffle mask be used to shuffle the 16 bytes.
 	 * byte 0-5 wills be swapped with byte 6-11.
 	 * byte 12-15 will keep unchanged.
 	 */
-	__m128i shfl_msk = _mm_set_epi8(15, 14, 13, 12,
+	register const __m128i shfl_msk = _mm_set_epi8(15, 14, 13, 12,
 					5, 4, 3, 2,
 					1, 0, 11, 10,
 					9, 8, 7, 6);
-- 
2.34.1

Re: [PATCH v2 1/3] app/testpmd: add register keyword

[Stephen Hemminger]

On Wed, 21 Aug 2024 20:08:55 +0530
Vipin Varghese <vipin.varghese@amd.com> wrote:

> diff --git a/app/test-pmd/macswap_sse.h b/app/test-pmd/macswap_sse.h
> index 223f87a539..29088843b7 100644
> --- a/app/test-pmd/macswap_sse.h
> +++ b/app/test-pmd/macswap_sse.h
> @@ -16,13 +16,13 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
>  	uint64_t ol_flags;
>  	int i;
>  	int r;
> -	__m128i addr0, addr1, addr2, addr3;
> +	register __m128i addr0, addr1, addr2, addr3;

Some compilers treat register as a no-op. Are you sure? Did you check with godbolt.

Re: [PATCH v2 1/3] app/testpmd: add register keyword

[Varghese, Vipin]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]


On 8/21/2024 8:25 PM, Stephen Hemminger wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Wed, 21 Aug 2024 20:08:55 +0530
> Vipin Varghese<vipin.varghese@amd.com>  wrote:
>
>> diff --git a/app/test-pmd/macswap_sse.h b/app/test-pmd/macswap_sse.h
>> index 223f87a539..29088843b7 100644
>> --- a/app/test-pmd/macswap_sse.h
>> +++ b/app/test-pmd/macswap_sse.h
>> @@ -16,13 +16,13 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
>>        uint64_t ol_flags;
>>        int i;
>>        int r;
>> -     __m128i addr0, addr1, addr2, addr3;
>> +     register __m128i addr0, addr1, addr2, addr3;
> Some compilers treat register as a no-op. Are you sure? Did you check with godbolt.

Thank you Stephen, I have tested the code changes on Linux using GCC and 
Clang compiler.

In both cases in Linux environment, we have seen the the values loaded 
onto register `xmm`.

```
registerconst__m128i shfl_msk = _mm_set_epi8(15, 14, 13, 12, 5, 4, 3, 2, 
1, 0, 11, 10, 9, 8, 7, 6);
vmovdqaxmm0, xmmwordptr[rip+ .LCPI0_0]

```

Both cases we have performance improvement.


Can you please help us understand if we have missed out something?

[-- Attachment #2: Type: text/html, Size: 4172 bytes --]

Re: [PATCH v2 1/3] app/testpmd: add register keyword

[Stephen Hemminger]

On Tue, 27 Aug 2024 21:02:00 +0530
"Varghese, Vipin" <vipin.varghese@amd.com> wrote:

> On 8/21/2024 8:25 PM, Stephen Hemminger wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> >
> >
> > On Wed, 21 Aug 2024 20:08:55 +0530
> > Vipin Varghese<vipin.varghese@amd.com>  wrote:
> >  
> >> diff --git a/app/test-pmd/macswap_sse.h b/app/test-pmd/macswap_sse.h
> >> index 223f87a539..29088843b7 100644
> >> --- a/app/test-pmd/macswap_sse.h
> >> +++ b/app/test-pmd/macswap_sse.h
> >> @@ -16,13 +16,13 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
> >>        uint64_t ol_flags;
> >>        int i;
> >>        int r;
> >> -     __m128i addr0, addr1, addr2, addr3;
> >> +     register __m128i addr0, addr1, addr2, addr3;  
> > Some compilers treat register as a no-op. Are you sure? Did you check with godbolt.  
> 
> Thank you Stephen, I have tested the code changes on Linux using GCC and 
> Clang compiler.
> 
> In both cases in Linux environment, we have seen the the values loaded 
> onto register `xmm`.
> 
> ```
> registerconst__m128i shfl_msk = _mm_set_epi8(15, 14, 13, 12, 5, 4, 3, 2, 
> 1, 0, 11, 10, 9, 8, 7, 6);
> vmovdqaxmm0, xmmwordptr[rip+ .LCPI0_0]
> 
> ```
> 
> Both cases we have performance improvement.
> 
> 
> Can you please help us understand if we have missed out something?

Ok, not sure why compiler would not decide to already use a register here?

Re: [PATCH v2 1/3] app/testpmd: add register keyword

[Varghese, Vipin]

[-- Attachment #1: Type: text/plain, Size: 232 bytes --]


On 8/27/2024 11:09 PM, Stephen Hemminger wrote:
> not sure why compiler would not decide to already use a register here?

Hi Stephen, I totally agree with your point, but in practice it did not 
use registers for code generation.


[-- Attachment #2: Type: text/html, Size: 584 bytes --]

RE: [PATCH v2 1/3] app/testpmd: add register keyword

[Konstantin Ananyev]

> > >
> > >> diff --git a/app/test-pmd/macswap_sse.h b/app/test-pmd/macswap_sse.h
> > >> index 223f87a539..29088843b7 100644
> > >> --- a/app/test-pmd/macswap_sse.h
> > >> +++ b/app/test-pmd/macswap_sse.h
> > >> @@ -16,13 +16,13 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
> > >>        uint64_t ol_flags;
> > >>        int i;
> > >>        int r;
> > >> -     __m128i addr0, addr1, addr2, addr3;
> > >> +     register __m128i addr0, addr1, addr2, addr3;
> > > Some compilers treat register as a no-op. Are you sure? Did you check with godbolt.
> >
> > Thank you Stephen, I have tested the code changes on Linux using GCC and
> > Clang compiler.
> >
> > In both cases in Linux environment, we have seen the the values loaded
> > onto register `xmm`.
> >
> > ```
> > registerconst__m128i shfl_msk = _mm_set_epi8(15, 14, 13, 12, 5, 4, 3, 2,
> > 1, 0, 11, 10, 9, 8, 7, 6);
> > vmovdqaxmm0, xmmwordptr[rip+ .LCPI0_0]

Yep, that what I would probably expect: one time load before the loop starts, right?
Curious  what exactly it would generate then if 'register' keyword is missed?
BTW, on my box,  gcc-11  with '-O3 -msse4.2 ...'  I am seeing expected behavior without 'register' keyword.
Is it some particular compiler version that misbehaves?
 
> >
> > ```
> >
> > Both cases we have performance improvement.
> >
> >
> > Can you please help us understand if we have missed out something?
> 
> Ok, not sure why compiler would not decide to already use a register here?

RE: [PATCH v2 1/3] app/testpmd: add register keyword

[Varghese, Vipin]

[-- Attachment #1: Type: text/plain, Size: 2244 bytes --]

[AMD Official Use Only - AMD Internal Distribution Only]


<snipped>

> > > >> --- a/app/test-pmd/macswap_sse.h
> > > >> +++ b/app/test-pmd/macswap_sse.h
> > > >> @@ -16,13 +16,13 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t
> nb,
> > > >>        uint64_t ol_flags;
> > > >>        int i;
> > > >>        int r;
> > > >> -     __m128i addr0, addr1, addr2, addr3;
> > > >> +     register __m128i addr0, addr1, addr2, addr3;
> > > > Some compilers treat register as a no-op. Are you sure? Did you check
> with godbolt.
> > >
> > > Thank you Stephen, I have tested the code changes on Linux using GCC
> > > and Clang compiler.
> > >
> > > In both cases in Linux environment, we have seen the the values
> > > loaded onto register `xmm`.
> > >
> > > ```
> > > registerconst__m128i shfl_msk = _mm_set_epi8(15, 14, 13, 12, 5, 4,
> > > 3, 2, 1, 0, 11, 10, 9, 8, 7, 6); vmovdqaxmm0, xmmwordptr[rip+
> > > .LCPI0_0]
>
> Yep, that what I would probably expect: one time load before the loop starts,
> right?
> Curious  what exactly it would generate then if 'register' keyword is missed?
> BTW, on my box,  gcc-11  with '-O3 -msse4.2 ...'  I am seeing expected
> behavior without 'register' keyword.
> Is it some particular compiler version that misbehaves?

Thank you, Konstantin, for this pointer. I have been trying this understand this a bit more internally. Here are my observations

1. shuf simd ISA works on XMM register only.
2. Any values from variables has to be loaded to `xmm` register before processing.
3. when compiled for `-march=native` with compiler not aware (SoC Arch gcc weights) without patch might have generating with ` movzx   eax, BYTE PTR [rbp-48]`
4. when register keyword is applied for both shufl_mask and addr, the compiler generates trying to get the variables directly into xmm using ` vmovdqu (%rsi),%xmm1`

So, I think you are right, from gcc12.3 and gcc 13.1 which supports `-march=znver4` this problem will not come.

>
> > >
> > > ```
> > >
> > > Both cases we have performance improvement.
> > >
> > >
> > > Can you please help us understand if we have missed out something?
> >
> > Ok, not sure why compiler would not decide to already use a register here?

[-- Attachment #2: Type: text/html, Size: 7828 bytes --]

Re: [PATCH v2 1/3] app/testpmd: add register keyword

[Ferruh Yigit]

On 9/6/2024 2:02 PM, Varghese, Vipin wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> <snipped>
>  
>> > > >> --- a/app/test-pmd/macswap_sse.h
>> > > >> +++ b/app/test-pmd/macswap_sse.h
>> > > >> @@ -16,13 +16,13 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t
>> nb,
>> > > >>        uint64_t ol_flags;
>> > > >>        int i;
>> > > >>        int r;
>> > > >> -     __m128i addr0, addr1, addr2, addr3;
>> > > >> +     register __m128i addr0, addr1, addr2, addr3;
>> > > > Some compilers treat register as a no-op. Are you sure? Did you check
>> with godbolt.
>> > >
>> > > Thank you Stephen, I have tested the code changes on Linux using GCC
>> > > and Clang compiler.
>> > >
>> > > In both cases in Linux environment, we have seen the the values
>> > > loaded onto register `xmm`.
>> > >
>> > > ```
>> > > registerconst__m128i shfl_msk = _mm_set_epi8(15, 14, 13, 12, 5, 4,
>> > > 3, 2, 1, 0, 11, 10, 9, 8, 7, 6); vmovdqaxmm0, xmmwordptr[rip+
>> > > .LCPI0_0]
>> 
>> Yep, that what I would probably expect: one time load before the loop starts,
>> right?
>> Curious  what exactly it would generate then if 'register' keyword is missed?
>> BTW, on my box,  gcc-11  with '-O3 -msse4.2 ...'  I am seeing expected
>> behavior without 'register' keyword.
>> Is it some particular compiler version that misbehaves?
>  
> Thank you, Konstantin, for this pointer. I have been trying this
> understand this a bit more internally. Here are my observations
>  
> 1. shuf simd ISA works on XMM register only.
> 2. Any values from variables has to be loaded to `xmm` register before
> processing.
> 3. when compiled for `-march=native` with compiler not aware (SoC Arch
> gcc weights) without patch might have generating with `movzx   eax, BYTE
> PTR [rbp-48]`
> 4. when register keyword is applied for both shufl_mask and addr, the
> compiler generates trying to get the variables directly into xmm using `
> vmovdqu (%rsi),%xmm1`
>  
> So, I think you are right, from gcc12.3 and gcc 13.1 which supports `-
> march=znver4` this problem will not come.
>  

Hi Konstantin, Stephen,

There is no negative impact of adding 'register' keyword, right? At
worst it is useless, but Vipin can demonstrate that it has benefit for
some cases, so I think OK to get it.

[PATCH v2 2/3] app/testpmd: move offload update

[Vipin Varghese]

Moving the offload flag update from end to start of the loop,
helps to reduce L1 or L2 cache evictions and amortize shuffle.
This helps to improve RX packet in mac-swap processing.

Test Result:
 * Platform: AMD EPYC 9554 @3.1GHz, no boost
 * Test scenarios: TEST-PMD 64B IO vs MAC-SWAP
 * NIC: broadcom P2100: loopback 2*100Gbps

 <mode : Mpps Ingress: Mpps Egress>
 ------------------------------------------------
  - MAC-SWAP original: 45.75 : 43.8
  - MAC-SWAP register mod: 45.73 : 44.83
  - MAC-SWAP register+ofl modified: 46.36 : 44.79

Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
---
 app/test-pmd/macswap_sse.h | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/app/test-pmd/macswap_sse.h b/app/test-pmd/macswap_sse.h
index 29088843b7..67ff7fdfbb 100644
--- a/app/test-pmd/macswap_sse.h
+++ b/app/test-pmd/macswap_sse.h
@@ -45,19 +45,22 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
 		mb[0] = pkts[i++];
 		eth_hdr[0] = rte_pktmbuf_mtod(mb[0], struct rte_ether_hdr *);
 		addr0 = _mm_loadu_si128((__m128i *)eth_hdr[0]);
+		mbuf_field_set(mb[0], ol_flags);
 
 		mb[1] = pkts[i++];
 		eth_hdr[1] = rte_pktmbuf_mtod(mb[1], struct rte_ether_hdr *);
 		addr1 = _mm_loadu_si128((__m128i *)eth_hdr[1]);
-
+		mbuf_field_set(mb[1], ol_flags);
 
 		mb[2] = pkts[i++];
 		eth_hdr[2] = rte_pktmbuf_mtod(mb[2], struct rte_ether_hdr *);
 		addr2 = _mm_loadu_si128((__m128i *)eth_hdr[2]);
+		mbuf_field_set(mb[2], ol_flags);
 
 		mb[3] = pkts[i++];
 		eth_hdr[3] = rte_pktmbuf_mtod(mb[3], struct rte_ether_hdr *);
 		addr3 = _mm_loadu_si128((__m128i *)eth_hdr[3]);
+		mbuf_field_set(mb[3], ol_flags);
 
 		addr0 = _mm_shuffle_epi8(addr0, shfl_msk);
 		addr1 = _mm_shuffle_epi8(addr1, shfl_msk);
@@ -69,10 +72,6 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
 		_mm_storeu_si128((__m128i *)eth_hdr[2], addr2);
 		_mm_storeu_si128((__m128i *)eth_hdr[3], addr3);
 
-		mbuf_field_set(mb[0], ol_flags);
-		mbuf_field_set(mb[1], ol_flags);
-		mbuf_field_set(mb[2], ol_flags);
-		mbuf_field_set(mb[3], ol_flags);
 		r -= 4;
 	}
 
@@ -84,10 +83,10 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
 
 		/* Swap dest and src mac addresses. */
 		addr0 = _mm_loadu_si128((__m128i *)eth_hdr[0]);
+		mbuf_field_set(mb[0], ol_flags);
+
 		addr0 = _mm_shuffle_epi8(addr0, shfl_msk);
 		_mm_storeu_si128((__m128i *)eth_hdr[0], addr0);
-
-		mbuf_field_set(mb[0], ol_flags);
 	}
 }
 
-- 
2.34.1

[PATCH v2 3/3] app/testpmd: interleave SSE SIMD

[Vipin Varghese]

Interleaving SSE SIMD load, shuffle, and store, helps to
improve the overall mac-swapp Mpps for both RX and TX.

Test Result:
 * Platform: AMD EPYC 9554 @3.1GHz, no boost
 * Test scenarios: TEST-PMD 64B IO vs MAC-SWAP
 * NIC: broadcom P2100: loopback 2*100Gbps

 <mode : Mpps Ingress: Mpps Egress>
 ------------------------------------------------
  - MAC-SWAP original: 45.75 : 43.8
  - MAC-SWAP register mod: 45.73 : 44.83
  - MAC-SWAP register+ofl mod: 46.36 : 44.79
  - MAC-SWAP register+ofl+interleave mod: 46.0 : 45.1

Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
---
 app/test-pmd/macswap_sse.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/app/test-pmd/macswap_sse.h b/app/test-pmd/macswap_sse.h
index 67ff7fdfbb..1f547388b7 100644
--- a/app/test-pmd/macswap_sse.h
+++ b/app/test-pmd/macswap_sse.h
@@ -52,23 +52,25 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
 		addr1 = _mm_loadu_si128((__m128i *)eth_hdr[1]);
 		mbuf_field_set(mb[1], ol_flags);
 
+		addr0 = _mm_shuffle_epi8(addr0, shfl_msk);
+
 		mb[2] = pkts[i++];
 		eth_hdr[2] = rte_pktmbuf_mtod(mb[2], struct rte_ether_hdr *);
 		addr2 = _mm_loadu_si128((__m128i *)eth_hdr[2]);
 		mbuf_field_set(mb[2], ol_flags);
 
+		addr1 = _mm_shuffle_epi8(addr1, shfl_msk);
+		_mm_storeu_si128((__m128i *)eth_hdr[0], addr0);
+
 		mb[3] = pkts[i++];
 		eth_hdr[3] = rte_pktmbuf_mtod(mb[3], struct rte_ether_hdr *);
 		addr3 = _mm_loadu_si128((__m128i *)eth_hdr[3]);
 		mbuf_field_set(mb[3], ol_flags);
 
-		addr0 = _mm_shuffle_epi8(addr0, shfl_msk);
-		addr1 = _mm_shuffle_epi8(addr1, shfl_msk);
 		addr2 = _mm_shuffle_epi8(addr2, shfl_msk);
-		addr3 = _mm_shuffle_epi8(addr3, shfl_msk);
-
-		_mm_storeu_si128((__m128i *)eth_hdr[0], addr0);
 		_mm_storeu_si128((__m128i *)eth_hdr[1], addr1);
+
+		addr3 = _mm_shuffle_epi8(addr3, shfl_msk);
 		_mm_storeu_si128((__m128i *)eth_hdr[2], addr2);
 		_mm_storeu_si128((__m128i *)eth_hdr[3], addr3);
 
-- 
2.34.1

Re: [PATCH v2 0/3] app/testpmd: improve sse based macswap

[Ferruh Yigit]

On 8/21/2024 3:38 PM, Vipin Varghese wrote:
> Goal of the patch series is to improve SSE macswap on x86_64 by
> reducing the stalls in backend engine. Original implementation of
> the SSE-mac-swap makes loop call to multiple load, shuffle & store.
> 
> Using SIMD ISA interleaving, register variable and reducing L1 & L2
> cache eviction, we can reduce the stalls for
>  - load SSE token exhaustion
>  - Shuffle and Load dependency
> 
> Build test using meson script:
> ``````````````````````````````
> build-gcc-static
> buildtools
> build-gcc-shared
> build-mini
> build-clang-static
> build-clang-shared
> build-x86-generic
> 
> Test Results:
> `````````````
> 
> Platform-1: AMD EPYC SIENA 8594P @2.3GHz, no boost
> Platform-2: AMD EPYC 9554 @3.1GHz, no boost
> 
> NIC:
>  1) mellanox CX-7 1*200Gbps
>  2) intel E810 1*100Gbps
>  3) intel E810 2*200Gbps (2CQ-DA2) - loopback
>  4) braodcom P2100 2*100Gbps - loopback
> 
> ------------------------------------------------
> TEST IO 64B: baseline <NIC : MPPs>
>  - NIC-1: 42.0
>  - NIC-2: 82.0
>  - NIC-3: 82.45
>  - NIC-3: 47.03
> ------------------------------------------------
> TEST MACSWAP 64B: <NIC : Before : After>
>  - NIC-1: 31.533 : 31.90
>  - NIC-2: 48.0   : 48.9 
>  - NIC-3: 48.840 : 49.827
>  - NIC-4: 44.3   : 45.5
> ------------------------------------------------
> TEST MACSWAP 128B: <NIC : Before: After>
>  - NIC-1: 30.946 : 31.770
>  - NIC-2: 47.4   : 48.3
>  - NIC-3: 47.979 : 48.503
>  - NIC-4: 41.53  : 44.59
> ------------------------------------------------
> TEST MACSWAP 256B: <NIC: Before: After>
>  - NIC-1: 32.480 : 33.150
>  - NIC-2: 45.29  : 45.571
>  - NIC-3: 45.033 : 45.117
>  - NIC-4: 36.49  : 37.5
> ------------------------------------------------
> 
> 
> ------------------------------------------------
> TEST IO 64B: baseline <NIC : MPPs>
>  - intel E810 2*200Gbps (2CQ-DA2): 82.49
> ------------------------------------------------
> <NIC intel E810 2*200Gbps (2CQ-DA2): Before : After>
> TEST MACSWAP: 1Q 1C1T
>  64B: : 45.0 : 45.54
> 128B: : 44.48 : 44.43
> 256B: : 42.0 : 41.99
> +++++++++++++++++++++++++
> TEST MACSWAP: 2Q 2C2T
>  64B: : 59.5 : 60.55
> 128B: : 56.78 : 58.1
> 256B: : 41.85 : 41.99
> ------------------------------------------------
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
> 
> Vipin Varghese (3):
>   app/testpmd: add register keyword
>   app/testpmd: move offload update
>   app/testpmd: interleave SSE SIMD
>

For series,
Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>


Bruce, if your testing is not aligned with the presented results please
let us know, otherwise I am planning to get the patch for -rc1.

Re: [PATCH v2 0/3] app/testpmd: improve sse based macswap

[Ferruh Yigit]

On 10/4/2024 6:08 AM, Ferruh Yigit wrote:
> On 8/21/2024 3:38 PM, Vipin Varghese wrote:
>> Goal of the patch series is to improve SSE macswap on x86_64 by
>> reducing the stalls in backend engine. Original implementation of
>> the SSE-mac-swap makes loop call to multiple load, shuffle & store.
>>
>> Using SIMD ISA interleaving, register variable and reducing L1 & L2
>> cache eviction, we can reduce the stalls for
>>  - load SSE token exhaustion
>>  - Shuffle and Load dependency
>>
>> Build test using meson script:
>> ``````````````````````````````
>> build-gcc-static
>> buildtools
>> build-gcc-shared
>> build-mini
>> build-clang-static
>> build-clang-shared
>> build-x86-generic
>>
>> Test Results:
>> `````````````
>>
>> Platform-1: AMD EPYC SIENA 8594P @2.3GHz, no boost
>> Platform-2: AMD EPYC 9554 @3.1GHz, no boost
>>
>> NIC:
>>  1) mellanox CX-7 1*200Gbps
>>  2) intel E810 1*100Gbps
>>  3) intel E810 2*200Gbps (2CQ-DA2) - loopback
>>  4) braodcom P2100 2*100Gbps - loopback
>>
>> ------------------------------------------------
>> TEST IO 64B: baseline <NIC : MPPs>
>>  - NIC-1: 42.0
>>  - NIC-2: 82.0
>>  - NIC-3: 82.45
>>  - NIC-3: 47.03
>> ------------------------------------------------
>> TEST MACSWAP 64B: <NIC : Before : After>
>>  - NIC-1: 31.533 : 31.90
>>  - NIC-2: 48.0   : 48.9 
>>  - NIC-3: 48.840 : 49.827
>>  - NIC-4: 44.3   : 45.5
>> ------------------------------------------------
>> TEST MACSWAP 128B: <NIC : Before: After>
>>  - NIC-1: 30.946 : 31.770
>>  - NIC-2: 47.4   : 48.3
>>  - NIC-3: 47.979 : 48.503
>>  - NIC-4: 41.53  : 44.59
>> ------------------------------------------------
>> TEST MACSWAP 256B: <NIC: Before: After>
>>  - NIC-1: 32.480 : 33.150
>>  - NIC-2: 45.29  : 45.571
>>  - NIC-3: 45.033 : 45.117
>>  - NIC-4: 36.49  : 37.5
>> ------------------------------------------------
>>
>>
>> ------------------------------------------------
>> TEST IO 64B: baseline <NIC : MPPs>
>>  - intel E810 2*200Gbps (2CQ-DA2): 82.49
>> ------------------------------------------------
>> <NIC intel E810 2*200Gbps (2CQ-DA2): Before : After>
>> TEST MACSWAP: 1Q 1C1T
>>  64B: : 45.0 : 45.54
>> 128B: : 44.48 : 44.43
>> 256B: : 42.0 : 41.99
>> +++++++++++++++++++++++++
>> TEST MACSWAP: 2Q 2C2T
>>  64B: : 59.5 : 60.55
>> 128B: : 56.78 : 58.1
>> 256B: : 41.85 : 41.99
>> ------------------------------------------------
>>
>> Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
>>
>> Vipin Varghese (3):
>>   app/testpmd: add register keyword
>>   app/testpmd: move offload update
>>   app/testpmd: interleave SSE SIMD
>>
> 
> For series,
> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> 
> 
> Bruce, if your testing is not aligned with the presented results please
> let us know, otherwise I am planning to get the patch for -rc1.
> 

Series applied to dpdk-next-net/main, thanks.

[PATCH] app/testpmd: improve sse based macswap

[Vipin Varghese]

Goal of the patch is to improve SSE macswap on x86_64 by reducing
the stalls in backend engine. Original implementation of the SSE
macswap makes loop call to multiple load, shuffle & store. Using
SIMD ISA interleaving we can reduce the stalls for
 - load SSE token exhaustion
 - Shuffle and Load dependency

Also other changes which improves packet per second are
 - Filling access to MBUF for offload flags which is separate cacheline,
 - using register keyword

Test results:
------------
Platform: AMD EPYC SIENA 8594P @2.3GHz, no boost
DPDK: 24.03

------------------------------------------------
TEST IO 64B: baseline <NIC : MPPs>
 - mellanox CX-7 2*200Gbps : 42.0
 - intel E810 1*100Gbps : 82.0
 - intel E810 2*200Gbps (2CQ-DA2): 83.0
------------------------------------------------
TEST MACSWAP 64B: <NIC : Before : After>
 - mellanox CX-7 2*200Gbps : 31.533 : 31.90
 - intel E810 1*100Gbps : 50.380 : 47.0
 - intel E810 2*200Gbps (2CQ-DA2): 48.840 : 49.827
------------------------------------------------
TEST MACSWAP 128B: <NIC : Before: After>
 - mellanox CX-7 2*200Gbps: 30.946 : 31.770
 - intel E810 1*100Gbps: 49.386 : 46.366
 - intel E810 2*200Gbps (2CQ-DA2): 47.979 : 49.503
------------------------------------------------
TEST MACSWAP 256B: <NIC: Before: After>
 - mellanox CX-7 2*200Gbps: 32.480 : 33.150
 - intel E810 1 * 100Gbps: 45.29 : 44.571
 - intel E810 2 * 200Gbps (2CQ-DA2): 45.033 : 45.117
------------------------------------------------

using multiple queues and lcore there is linear increase in MPPs.

Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
---
 app/test-pmd/macswap_sse.h | 40 ++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/app/test-pmd/macswap_sse.h b/app/test-pmd/macswap_sse.h
index 223f87a539..a3d3a274e5 100644
--- a/app/test-pmd/macswap_sse.h
+++ b/app/test-pmd/macswap_sse.h
@@ -11,21 +11,21 @@ static inline void
 do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
 		struct rte_port *txp)
 {
-	struct rte_ether_hdr *eth_hdr[4];
-	struct rte_mbuf *mb[4];
+	register struct rte_ether_hdr *eth_hdr[8];
+	register struct rte_mbuf *mb[8];
 	uint64_t ol_flags;
 	int i;
 	int r;
-	__m128i addr0, addr1, addr2, addr3;
+	register __m128i addr0, addr1, addr2, addr3;
 	/**
 	 * shuffle mask be used to shuffle the 16 bytes.
 	 * byte 0-5 wills be swapped with byte 6-11.
 	 * byte 12-15 will keep unchanged.
 	 */
-	__m128i shfl_msk = _mm_set_epi8(15, 14, 13, 12,
-					5, 4, 3, 2,
-					1, 0, 11, 10,
-					9, 8, 7, 6);
+	register const __m128i shfl_msk = _mm_set_epi8(15, 14, 13, 12,
+							5, 4, 3, 2,
+							1, 0, 11, 10,
+							9, 8, 7, 6);
 
 	ol_flags = ol_flags_init(txp->dev_conf.txmode.offloads);
 	vlan_qinq_set(pkts, nb, ol_flags,
@@ -44,23 +44,24 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
 
 		mb[0] = pkts[i++];
 		eth_hdr[0] = rte_pktmbuf_mtod(mb[0], struct rte_ether_hdr *);
-		addr0 = _mm_loadu_si128((__m128i *)eth_hdr[0]);
-
 		mb[1] = pkts[i++];
 		eth_hdr[1] = rte_pktmbuf_mtod(mb[1], struct rte_ether_hdr *);
-		addr1 = _mm_loadu_si128((__m128i *)eth_hdr[1]);
-
-
 		mb[2] = pkts[i++];
 		eth_hdr[2] = rte_pktmbuf_mtod(mb[2], struct rte_ether_hdr *);
-		addr2 = _mm_loadu_si128((__m128i *)eth_hdr[2]);
-
 		mb[3] = pkts[i++];
 		eth_hdr[3] = rte_pktmbuf_mtod(mb[3], struct rte_ether_hdr *);
-		addr3 = _mm_loadu_si128((__m128i *)eth_hdr[3]);
 
+		/* Interleave load, shuffle & set */
+		addr0 = _mm_loadu_si128((__m128i *)eth_hdr[0]);
+		mbuf_field_set(mb[0], ol_flags);
+		addr1 = _mm_loadu_si128((__m128i *)eth_hdr[1]);
+		mbuf_field_set(mb[1], ol_flags);
 		addr0 = _mm_shuffle_epi8(addr0, shfl_msk);
+		addr2 = _mm_loadu_si128((__m128i *)eth_hdr[2]);
+		mbuf_field_set(mb[2], ol_flags);
 		addr1 = _mm_shuffle_epi8(addr1, shfl_msk);
+		addr3 = _mm_loadu_si128((__m128i *)eth_hdr[3]);
+		mbuf_field_set(mb[3], ol_flags);
 		addr2 = _mm_shuffle_epi8(addr2, shfl_msk);
 		addr3 = _mm_shuffle_epi8(addr3, shfl_msk);
 
@@ -69,25 +70,22 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
 		_mm_storeu_si128((__m128i *)eth_hdr[2], addr2);
 		_mm_storeu_si128((__m128i *)eth_hdr[3], addr3);
 
-		mbuf_field_set(mb[0], ol_flags);
-		mbuf_field_set(mb[1], ol_flags);
-		mbuf_field_set(mb[2], ol_flags);
-		mbuf_field_set(mb[3], ol_flags);
 		r -= 4;
 	}
 
 	for ( ; i < nb; i++) {
 		if (i < nb - 1)
 			rte_prefetch0(rte_pktmbuf_mtod(pkts[i+1], void *));
+
 		mb[0] = pkts[i];
 		eth_hdr[0] = rte_pktmbuf_mtod(mb[0], struct rte_ether_hdr *);
 
 		/* Swap dest and src mac addresses. */
 		addr0 = _mm_loadu_si128((__m128i *)eth_hdr[0]);
+		/* MBUF and Ethernet are 2 separate cacheline */
+		mbuf_field_set(mb[0], ol_flags);
 		addr0 = _mm_shuffle_epi8(addr0, shfl_msk);
 		_mm_storeu_si128((__m128i *)eth_hdr[0], addr0);
-
-		mbuf_field_set(mb[0], ol_flags);
 	}
 }
 
-- 
2.34.1

Re: [PATCH] app/testpmd: improve sse based macswap

[Bruce Richardson]

On Sat, Jul 13, 2024 at 08:49:49PM +0530, Vipin Varghese wrote:
> Goal of the patch is to improve SSE macswap on x86_64 by reducing
> the stalls in backend engine. Original implementation of the SSE
> macswap makes loop call to multiple load, shuffle & store. Using
> SIMD ISA interleaving we can reduce the stalls for
>  - load SSE token exhaustion
>  - Shuffle and Load dependency
> 
> Also other changes which improves packet per second are
>  - Filling access to MBUF for offload flags which is separate cacheline,
>  - using register keyword
> 
> Test results:
> ------------
> Platform: AMD EPYC SIENA 8594P @2.3GHz, no boost
> DPDK: 24.03
> 
> ------------------------------------------------
> TEST IO 64B: baseline <NIC : MPPs>
>  - mellanox CX-7 2*200Gbps : 42.0
>  - intel E810 1*100Gbps : 82.0
>  - intel E810 2*200Gbps (2CQ-DA2): 83.0
> ------------------------------------------------
> TEST MACSWAP 64B: <NIC : Before : After>
>  - mellanox CX-7 2*200Gbps : 31.533 : 31.90
>  - intel E810 1*100Gbps : 50.380 : 47.0
>  - intel E810 2*200Gbps (2CQ-DA2): 48.840 : 49.827
> ------------------------------------------------
> TEST MACSWAP 128B: <NIC : Before: After>
>  - mellanox CX-7 2*200Gbps: 30.946 : 31.770
>  - intel E810 1*100Gbps: 49.386 : 46.366
>  - intel E810 2*200Gbps (2CQ-DA2): 47.979 : 49.503
> ------------------------------------------------
> TEST MACSWAP 256B: <NIC: Before: After>
>  - mellanox CX-7 2*200Gbps: 32.480 : 33.150
>  - intel E810 1 * 100Gbps: 45.29 : 44.571
>  - intel E810 2 * 200Gbps (2CQ-DA2): 45.033 : 45.117
> ------------------------------------------------
> 

Hi,

interesting patch. Do you know why we see regressions in some of the cases
above? For 1x100G at 64B and 128B packet sizes we see perf drops of 3mpps
vs smaller gains in the other two cases at each size (much smaller in the
64B case).

Couple of other questions inline below too.

Thanks,
/Bruce

> using multiple queues and lcore there is linear increase in MPPs.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
> ---
>  app/test-pmd/macswap_sse.h | 40 ++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/app/test-pmd/macswap_sse.h b/app/test-pmd/macswap_sse.h
> index 223f87a539..a3d3a274e5 100644
> --- a/app/test-pmd/macswap_sse.h
> +++ b/app/test-pmd/macswap_sse.h
> @@ -11,21 +11,21 @@ static inline void
>  do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
>  		struct rte_port *txp)
>  {
> -	struct rte_ether_hdr *eth_hdr[4];
> -	struct rte_mbuf *mb[4];
> +	register struct rte_ether_hdr *eth_hdr[8];
> +	register struct rte_mbuf *mb[8];

Does using "register" actually make a difference to the generated code?

Also, why increasing the array sizes from 4 to 8 - the actual code only
uses 4 elements of each array below anyway? Is it for cache alignment
purposes perhaps - if so, please use explicit cache alignment attributes to
specify this rather than having it implicit in the array sizes.

>  	uint64_t ol_flags;
>  	int i;
>  	int r;
> -	__m128i addr0, addr1, addr2, addr3;
> +	register __m128i addr0, addr1, addr2, addr3;
>  	/**
>  	 * shuffle mask be used to shuffle the 16 bytes.
>  	 * byte 0-5 wills be swapped with byte 6-11.
>  	 * byte 12-15 will keep unchanged.
>  	 */
> -	__m128i shfl_msk = _mm_set_epi8(15, 14, 13, 12,
> -					5, 4, 3, 2,
> -					1, 0, 11, 10,
> -					9, 8, 7, 6);
> +	register const __m128i shfl_msk = _mm_set_epi8(15, 14, 13, 12,
> +							5, 4, 3, 2,
> +							1, 0, 11, 10,
> +							9, 8, 7, 6);
>  
>  	ol_flags = ol_flags_init(txp->dev_conf.txmode.offloads);
>  	vlan_qinq_set(pkts, nb, ol_flags,
> @@ -44,23 +44,24 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
>  
>  		mb[0] = pkts[i++];
>  		eth_hdr[0] = rte_pktmbuf_mtod(mb[0], struct rte_ether_hdr *);
> -		addr0 = _mm_loadu_si128((__m128i *)eth_hdr[0]);
> -
>  		mb[1] = pkts[i++];
>  		eth_hdr[1] = rte_pktmbuf_mtod(mb[1], struct rte_ether_hdr *);
> -		addr1 = _mm_loadu_si128((__m128i *)eth_hdr[1]);
> -
> -
>  		mb[2] = pkts[i++];
>  		eth_hdr[2] = rte_pktmbuf_mtod(mb[2], struct rte_ether_hdr *);
> -		addr2 = _mm_loadu_si128((__m128i *)eth_hdr[2]);
> -
>  		mb[3] = pkts[i++];
>  		eth_hdr[3] = rte_pktmbuf_mtod(mb[3], struct rte_ether_hdr *);
> -		addr3 = _mm_loadu_si128((__m128i *)eth_hdr[3]);
>  
> +		/* Interleave load, shuffle & set */
> +		addr0 = _mm_loadu_si128((__m128i *)eth_hdr[0]);
> +		mbuf_field_set(mb[0], ol_flags);
> +		addr1 = _mm_loadu_si128((__m128i *)eth_hdr[1]);
> +		mbuf_field_set(mb[1], ol_flags);
>  		addr0 = _mm_shuffle_epi8(addr0, shfl_msk);
> +		addr2 = _mm_loadu_si128((__m128i *)eth_hdr[2]);
> +		mbuf_field_set(mb[2], ol_flags);
>  		addr1 = _mm_shuffle_epi8(addr1, shfl_msk);
> +		addr3 = _mm_loadu_si128((__m128i *)eth_hdr[3]);
> +		mbuf_field_set(mb[3], ol_flags);
>  		addr2 = _mm_shuffle_epi8(addr2, shfl_msk);
>  		addr3 = _mm_shuffle_epi8(addr3, shfl_msk);
>  
> @@ -69,25 +70,22 @@ do_macswap(struct rte_mbuf *pkts[], uint16_t nb,
>  		_mm_storeu_si128((__m128i *)eth_hdr[2], addr2);
>  		_mm_storeu_si128((__m128i *)eth_hdr[3], addr3);
>  
> -		mbuf_field_set(mb[0], ol_flags);
> -		mbuf_field_set(mb[1], ol_flags);
> -		mbuf_field_set(mb[2], ol_flags);
> -		mbuf_field_set(mb[3], ol_flags);
>  		r -= 4;
>  	}
>  
>  	for ( ; i < nb; i++) {
>  		if (i < nb - 1)
>  			rte_prefetch0(rte_pktmbuf_mtod(pkts[i+1], void *));
> +
>  		mb[0] = pkts[i];
>  		eth_hdr[0] = rte_pktmbuf_mtod(mb[0], struct rte_ether_hdr *);
>  
>  		/* Swap dest and src mac addresses. */
>  		addr0 = _mm_loadu_si128((__m128i *)eth_hdr[0]);
> +		/* MBUF and Ethernet are 2 separate cacheline */
> +		mbuf_field_set(mb[0], ol_flags);
>  		addr0 = _mm_shuffle_epi8(addr0, shfl_msk);
>  		_mm_storeu_si128((__m128i *)eth_hdr[0], addr0);
> -
> -		mbuf_field_set(mb[0], ol_flags);
>  	}

Since final loop is only for the odd elements at the end of the array of
buffers. Does changing the instruction ordering here really make perf
difference?