Skip to contentSkip to conversation

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Stephen Hemminger

author
#1 lore raw

PATCH 1/1 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")
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
Acked-by on v1 1/1
#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

Acked-by on v1 1/1
#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]>
Acked-by on v1 1/1
#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]>
#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?

#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.

#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?

author
#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.

#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.

Acked-by Shai Brandes Applied
#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.