[PATCH] net/ena: revert redefining memcpy

[PATCH] net/ena: revert redefining memcpy

[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 <stephen@networkplumber.org>
---
 drivers/net/ena/base/ena_plat_dpdk.h | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/net/ena/base/ena_plat_dpdk.h b/drivers/net/ena/base/ena_plat_dpdk.h
index 21b96113c7..291db6cabe 100644
--- a/drivers/net/ena/base/ena_plat_dpdk.h
+++ b/drivers/net/ena/base/ena_plat_dpdk.h
@@ -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

RE: [PATCH] net/ena: revert redefining memcpy

[Wathsala Wathawana Vithanage]

> 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 <wathsala.vithanage@arm.com>

> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/ena/base/ena_plat_dpdk.h | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ena/base/ena_plat_dpdk.h
> b/drivers/net/ena/base/ena_plat_dpdk.h
> index 21b96113c7..291db6cabe 100644
> --- a/drivers/net/ena/base/ena_plat_dpdk.h
> +++ b/drivers/net/ena/base/ena_plat_dpdk.h
> @@ -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

RE: [PATCH] net/ena: revert redefining memcpy

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> 
> 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 <stephen@networkplumber.org>
> ---

Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH] net/ena: revert redefining memcpy

[Tyler Retzlaff]

On Mon, Aug 12, 2024 at 08:34:17AM -0700, Stephen Hemminger wrote:
> 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 <stephen@networkplumber.org>
> ---

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Re: [PATCH] net/ena: revert redefining memcpy

[Ferruh Yigit]

On 9/12/2024 6:12 AM, Tyler Retzlaff wrote:
> On Mon, Aug 12, 2024 at 08:34:17AM -0700, Stephen Hemminger wrote:
>> 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 <stephen@networkplumber.org>
>> ---
> 
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> 

Hi Shai,

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

RE: [PATCH] net/ena: revert redefining memcpy

[Brandes, Shai]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, September 12, 2024 11:20 AM
> To: Tyler Retzlaff <roretzla@linux.microsoft.com>; Stephen Hemminger
> <stephen@networkplumber.org>; Brandes, Shai <shaibran@amazon.com>
> Cc: dev@dpdk.org; Schmeilin, Evgeny <evgenys@amazon.com>; Beider, Ron
> <rbeider@amazon.com>; Bernstein, Amit <amitbern@amazon.com>;
> Atrash, Wajeeh <atrwajee@amazon.com>; Chauskin, Igor
> <igorch@amazon.com>; Michal Krawczyk <mk@semihalf.com>; Artur Rojek
> <ar@semihalf.com>
> 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.
> 
> 
> 
> On 9/12/2024 6:12 AM, Tyler Retzlaff wrote:
> > On Mon, Aug 12, 2024 at 08:34:17AM -0700, Stephen Hemminger wrote:
> >> 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 <stephen@networkplumber.org>
> >> ---
> >
> > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> >
> 
> 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.

RE: [PATCH] net/ena: revert redefining memcpy

[Brandes, Shai]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, September 12, 2024 11:20 AM
> To: Tyler Retzlaff <roretzla@linux.microsoft.com>; Stephen Hemminger
> <stephen@networkplumber.org>; Brandes, Shai <shaibran@amazon.com>
> Cc: dev@dpdk.org; Schmeilin, Evgeny <evgenys@amazon.com>; Beider, Ron
> <rbeider@amazon.com>; Bernstein, Amit <amitbern@amazon.com>;
> Atrash, Wajeeh <atrwajee@amazon.com>; Chauskin, Igor
> <igorch@amazon.com>; Michal Krawczyk <mk@semihalf.com>; Artur Rojek
> <ar@semihalf.com>
> 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.
> 
> 
> 
> On 9/12/2024 6:12 AM, Tyler Retzlaff wrote:
> > On Mon, Aug 12, 2024 at 08:34:17AM -0700, Stephen Hemminger wrote:
> >> 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 <stephen@networkplumber.org>
> >> ---
> >
> > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> >
> 
> 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?

Re: [PATCH] net/ena: revert redefining memcpy

[Stephen Hemminger]

On Mon, 16 Sep 2024 06:33:26 +0000
"Brandes, Shai" <shaibran@amazon.com> wrote:

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

RE: [PATCH] net/ena: revert redefining memcpy

[Brandes, Shai]

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

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

בתאריך 16 בספט׳ 2024 18:12,‏ Stephen Hemminger <stephen@networkplumber.org> כתב:
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.



On Mon, 16 Sep 2024 06:33:26 +0000
"Brandes, Shai" <shaibran@amazon.com> wrote:

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

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