net/ena: revert redefining memcpy

PATCH Applied Stephen Hemminger 6 participants 10 messages Aug 12, 2024 – Sep 22, 2024 1 message not in the archive Network drivers

lore

Share & tools

lore

Stephen Hemminger

author

2024-08-12 15:34 UTC #1 lore raw

PATCH 1/1 net/ena: revert redefining memcpy

In reply to a message not in the archive

Redefining memcpy as rte_memcpy has no performance gain on current compilers, and introduced bugs like this one where rte_memcpy() will be detected as referencing past the destination.

Bugzilla ID: 1510

Fixes 142778b3702a ("net/ena: switch memcpy to optimized version")

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/ena/base/ena_plat_dpdk.h | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

drivers/net/ena/base/ena_plat_dpdk.h +1 -8

index 21b96113c7..291db6cabe 100644
@@ -26,7 +26,6 @@
 #include <rte_spinlock.h>
 
 #include <sys/time.h>
-#include <rte_memcpy.h>
 
 typedef uint64_t u64;
 typedef uint32_t u32;

@@ -70,13 +69,7 @@ typedef uint64_t dma_addr_t;
 #define ENA_UDELAY(x) rte_delay_us_block(x)
 
 #define ENA_TOUCH(x) ((void)(x))
-/* Redefine memcpy with caution: rte_memcpy can be simply aliased to memcpy, so
- * make the redefinition only if it's safe (and beneficial) to do so.
- */
-#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64_MEMCPY) || defined(RTE_ARCH_ARM_NEON_MEMCPY)
-#undef memcpy
-#define memcpy rte_memcpy
-#endif
+
 #define wmb rte_wmb
 #define rmb rte_rmb
 #define mb rte_mb

Signature · 1 line

2.43.0

Wathsala Wathawana Vithanage

Acked-by on v1 1/1

2024-08-12 20:53 UTC #2 lore raw

Quoted 9 lines

Subject: [PATCH] net/ena: revert redefining memcpy
Redefining memcpy as rte_memcpy has no performance gain on current compilers, and introduced bugs like this one where rte_memcpy() will be detected as referencing past the destination.
Bugzilla ID: 1510
Fixes 142778b3702a ("net/ena: switch memcpy to optimized version")

Similar observations in Arm when RTE_ARCH_ARM64_MEMCPY flag is on.

Acked-by Wathsala Vithanage <[email protected]>

Quoted 36 lines

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/ena/base/ena_plat_dpdk.h | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)
drivers/net/ena/base/ena_plat_dpdk.h +0 -2
index 21b96113c7..291db6cabe 100644
@@ -26,7 +26,6 @@
 #include <rte_spinlock.h>
 
 #include <sys/time.h>
-#include <rte_memcpy.h>
 
 typedef uint64_t u64;
 typedef uint32_t u32;
@@ -70,13 +69,7 @@ typedef uint64_t dma_addr_t;  #define
 ENA_UDELAY(x) rte_delay_us_block(x)
 
 #define ENA_TOUCH(x) ((void)(x))
-/* Redefine memcpy with caution: rte_memcpy can be simply aliased to
memcpy, so
- * make the redefinition only if it's safe (and beneficial) to do so.
- */
-#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64_MEMCPY) || defined(RTE_ARCH_ARM_NEON_MEMCPY) -#undef memcpy -#define memcpy rte_memcpy -#endif
+
#define wmb rte_wmb
#define rmb rte_rmb
#define mb rte_mb
--
2.43.0

Morten Brørup

Acked-by on v1 1/1

2024-08-13 08:57 UTC #3 lore raw

In reply to Stephen Hemminger

Quoted 11 lines

From: Stephen Hemminger [mailto:[email protected]]
Redefining memcpy as rte_memcpy has no performance gain on current compilers, and introduced bugs like this one where rte_memcpy() will be detected as referencing past the destination.
Bugzilla ID: 1510
Fixes 142778b3702a ("net/ena: switch memcpy to optimized version")
Signed-off-by Stephen Hemminger <[email protected]>
---

Acked-by Morten Brørup <[email protected]>

Tyler Retzlaff

Acked-by on v1 1/1

2024-09-12 05:12 UTC #4 lore raw

In reply to Stephen Hemminger

Quoted 9 lines · Stephen Hemminger

Redefining memcpy as rte_memcpy has no performance gain on current compilers, and introduced bugs like this one where rte_memcpy() will be detected as referencing past the destination.
Bugzilla ID: 1510
Fixes 142778b3702a ("net/ena: switch memcpy to optimized version")
Signed-off-by Stephen Hemminger <[email protected]>
---

Acked-by Tyler Retzlaff <[email protected]>

Ferruh Yigit

2024-09-12 08:19 UTC #5 lore raw

Quoted 5 lines · Tyler Retzlaff

Quoted 4 lines · Stephen Hemminger
Redefining memcpy as rte_memcpy has no performance gain on current compilers, and introduced bugs like this one where rte_memcpy() will be detected as referencing past the destination.

Quoted 8 lines

Quoted 5 lines
Bugzilla ID: 1510
Fixes 142778b3702a ("net/ena: switch memcpy to optimized version")
Signed-off-by Stephen Hemminger <[email protected]>
---
Acked-by Tyler Retzlaff <[email protected]>

Hi Shai,

Did you have any chance to check/test this patch?

Brandes, Shai

2024-09-12 14:53 UTC #6 lore raw

Quoted 32 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Thursday, September 12, 2024 11:20 AM
To: Tyler Retzlaff <[email protected]>; Stephen Hemminger <[email protected]>; Brandes, Shai <[email protected]>
Cc: [email protected]; Schmeilin, Evgeny <[email protected]>; Beider, Ron <[email protected]>; Bernstein, Amit <[email protected]>; Atrash, Wajeeh <[email protected]>; Chauskin, Igor <[email protected]>; Michal Krawczyk <[email protected]>; Artur Rojek <[email protected]>
Subject: RE: [EXTERNAL] [PATCH] net/ena: revert redefining memcpy
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
Quoted 11 lines · Tyler Retzlaff
Quoted 9 lines · Stephen Hemminger
Redefining memcpy as rte_memcpy has no performance gain on current compilers, and introduced bugs like this one where rte_memcpy() will be detected as referencing past the destination.
Bugzilla ID: 1510
Fixes 142778b3702a ("net/ena: switch memcpy to optimized version")
Signed-off-by Stephen Hemminger <[email protected]>
---

Quoted 6 lines

Quoted 2 lines
Acked-by Tyler Retzlaff <[email protected]>
Hi Shai,
Did you have any chance to check/test this patch?

[Brandes, Shai] sorry, I missed this. The change will be tested by the upcoming Monday is this is fine.

Brandes, Shai

2024-09-16 06:33 UTC #7 lore raw

In reply to Ferruh Yigit

Quoted 32 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Thursday, September 12, 2024 11:20 AM
To: Tyler Retzlaff <[email protected]>; Stephen Hemminger <[email protected]>; Brandes, Shai <[email protected]>
Cc: [email protected]; Schmeilin, Evgeny <[email protected]>; Beider, Ron <[email protected]>; Bernstein, Amit <[email protected]>; Atrash, Wajeeh <[email protected]>; Chauskin, Igor <[email protected]>; Michal Krawczyk <[email protected]>; Artur Rojek <[email protected]>
Subject: RE: [EXTERNAL] [PATCH] net/ena: revert redefining memcpy
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
Quoted 11 lines · Tyler Retzlaff
Quoted 9 lines · Stephen Hemminger
Redefining memcpy as rte_memcpy has no performance gain on current compilers, and introduced bugs like this one where rte_memcpy() will be detected as referencing past the destination.
Bugzilla ID: 1510
Fixes 142778b3702a ("net/ena: switch memcpy to optimized version")
Signed-off-by Stephen Hemminger <[email protected]>
---

Quoted 6 lines

Quoted 2 lines
Acked-by Tyler Retzlaff <[email protected]>
Hi Shai,
Did you have any chance to check/test this patch?

[Brandes, Shai] We are currently conducting tests and will provide an update shortly. In the meantime, could you advise whether it is recommended to entirely avoid using rte_memcpy in our driver, considering we have direct calls to it?

Stephen Hemminger

author

2024-09-16 15:11 UTC #8 lore raw

Quoted 2 lines · Shai

Quoted 1 line
Did you have any chance to check/test this patch?
[Brandes, Shai] We are currently conducting tests and will provide an update shortly. In the meantime, could you advise whether it is recommended to entirely avoid using rte_memcpy in our driver, considering we have direct calls to it?

There is a long term goal to remove rte_memcpy(). It exists only as workaround for cases where older compilers do not produce optimium code.

When rte_memcpy() is used the checks done by fortify, gcc, coverity etc are less and there is higher probability of bugs going undetected.

My current recommendation is to only use rte_memcpy() in the data path and for variable size data items.

Brandes, Shai

2024-09-16 16:06 UTC #9 lore raw

Thanks for the clarification.
We are okay to move forward with the revert.

בתאריך 16 בספט׳ 2024 18:12,‏ Stephen Hemminger <[email protected]> כתב:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.

Quoted 2 lines · Shai

Quoted 1 line
Did you have any chance to check/test this patch?
[Brandes, Shai] We are currently conducting tests and will provide an update shortly. In the meantime, could you advise whether it is recommended to entirely avoid using rte_memcpy in our driver, considering we have direct calls to it?

There is a long term goal to remove rte_memcpy(). It exists only as workaround for cases where older compilers do not produce optimium code.

When rte_memcpy() is used the checks done by fortify, gcc, coverity etc are less and there is higher probability of bugs going undetected.

My current recommendation is to only use rte_memcpy() in the data path and for variable size data items.

Ferruh Yigit

Acked-by Shai Brandes Applied

2024-09-22 04:08 UTC #10 lore raw

In reply to Tyler Retzlaff

Quoted 5 lines · Tyler Retzlaff

Quoted 4 lines · Stephen Hemminger
Redefining memcpy as rte_memcpy has no performance gain on current compilers, and introduced bugs like this one where rte_memcpy() will be detected as referencing past the destination.

Quoted 8 lines

Quoted 5 lines
Bugzilla ID: 1510
Fixes 142778b3702a ("net/ena: switch memcpy to optimized version")
Signed-off-by Stephen Hemminger <[email protected]>
---
Acked-by Tyler Retzlaff <[email protected]>

Acked-by Shai Brandes <[email protected]>

Applied to dpdk-next-net/main, thanks.