DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] net/ena: revert redefining memcpy
       [not found] <200~bug-1510-3@http.bugs.dpdk.org>
@ 2024-08-12 15:34 ` Stephen Hemminger
  2024-08-12 20:53   ` Wathsala Wathawana Vithanage
                     ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Stephen Hemminger @ 2024-08-12 15:34 UTC (permalink / raw)
  To: dev
  Cc: Stephen Hemminger, Shai Brandes, Evgeny Schemeilin, Ron Beider,
	Amit Bernstein, Wajeeh Atrash, Igor Chauskin, Michal Krawczyk,
	Artur Rojek

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] net/ena: revert redefining memcpy
  2024-08-12 15:34 ` [PATCH] net/ena: revert redefining memcpy Stephen Hemminger
@ 2024-08-12 20:53   ` Wathsala Wathawana Vithanage
  2024-08-13  8:57   ` Morten Brørup
  2024-09-12  5:12   ` Tyler Retzlaff
  2 siblings, 0 replies; 10+ messages in thread
From: Wathsala Wathawana Vithanage @ 2024-08-12 20:53 UTC (permalink / raw)
  To: Stephen Hemminger, dev
  Cc: Shai Brandes, Evgeny Schemeilin, Ron Beider, Amit Bernstein,
	Wajeeh Atrash, Igor Chauskin, Michal Krawczyk, Artur Rojek, nd


> 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] net/ena: revert redefining memcpy
  2024-08-12 15:34 ` [PATCH] net/ena: revert redefining memcpy Stephen Hemminger
  2024-08-12 20:53   ` Wathsala Wathawana Vithanage
@ 2024-08-13  8:57   ` Morten Brørup
  2024-09-12  5:12   ` Tyler Retzlaff
  2 siblings, 0 replies; 10+ messages in thread
From: Morten Brørup @ 2024-08-13  8:57 UTC (permalink / raw)
  To: Stephen Hemminger, dev
  Cc: Shai Brandes, Evgeny Schemeilin, Ron Beider, Amit Bernstein,
	Wajeeh Atrash, Igor Chauskin, Michal Krawczyk, Artur Rojek

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net/ena: revert redefining memcpy
  2024-08-12 15:34 ` [PATCH] net/ena: revert redefining memcpy Stephen Hemminger
  2024-08-12 20:53   ` Wathsala Wathawana Vithanage
  2024-08-13  8:57   ` Morten Brørup
@ 2024-09-12  5:12   ` Tyler Retzlaff
  2024-09-12  8:19     ` Ferruh Yigit
  2024-09-22  4:08     ` Ferruh Yigit
  2 siblings, 2 replies; 10+ messages in thread
From: Tyler Retzlaff @ 2024-09-12  5:12 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: dev, Shai Brandes, Evgeny Schemeilin, Ron Beider, Amit Bernstein,
	Wajeeh Atrash, Igor Chauskin, Michal Krawczyk, Artur Rojek

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>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net/ena: revert redefining memcpy
  2024-09-12  5:12   ` Tyler Retzlaff
@ 2024-09-12  8:19     ` Ferruh Yigit
  2024-09-12 14:53       ` Brandes, Shai
  2024-09-16  6:33       ` Brandes, Shai
  2024-09-22  4:08     ` Ferruh Yigit
  1 sibling, 2 replies; 10+ messages in thread
From: Ferruh Yigit @ 2024-09-12  8:19 UTC (permalink / raw)
  To: Tyler Retzlaff, Stephen Hemminger, Shai Brandes
  Cc: dev, Evgeny Schemeilin, Ron Beider, Amit Bernstein,
	Wajeeh Atrash, Igor Chauskin, Michal Krawczyk, Artur Rojek

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?


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] net/ena: revert redefining memcpy
  2024-09-12  8:19     ` Ferruh Yigit
@ 2024-09-12 14:53       ` Brandes, Shai
  2024-09-16  6:33       ` Brandes, Shai
  1 sibling, 0 replies; 10+ messages in thread
From: Brandes, Shai @ 2024-09-12 14:53 UTC (permalink / raw)
  To: Ferruh Yigit, Tyler Retzlaff, Stephen Hemminger
  Cc: dev, Schmeilin, Evgeny, Beider, Ron, Bernstein, Amit, Atrash, Wajeeh



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] net/ena: revert redefining memcpy
  2024-09-12  8:19     ` Ferruh Yigit
  2024-09-12 14:53       ` Brandes, Shai
@ 2024-09-16  6:33       ` Brandes, Shai
  2024-09-16 15:11         ` Stephen Hemminger
  1 sibling, 1 reply; 10+ messages in thread
From: Brandes, Shai @ 2024-09-16  6:33 UTC (permalink / raw)
  To: Ferruh Yigit, Tyler Retzlaff, Stephen Hemminger
  Cc: dev, Schmeilin, Evgeny, Beider, Ron, Bernstein, Amit, Atrash, Wajeeh



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net/ena: revert redefining memcpy
  2024-09-16  6:33       ` Brandes, Shai
@ 2024-09-16 15:11         ` Stephen Hemminger
  2024-09-16 16:06           ` Brandes, Shai
  0 siblings, 1 reply; 10+ messages in thread
From: Stephen Hemminger @ 2024-09-16 15:11 UTC (permalink / raw)
  To: Brandes, Shai
  Cc: Ferruh Yigit, Tyler Retzlaff, dev, Schmeilin, Evgeny, Beider,
	Ron, Bernstein, Amit, Atrash, Wajeeh

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] net/ena: revert redefining memcpy
  2024-09-16 15:11         ` Stephen Hemminger
@ 2024-09-16 16:06           ` Brandes, Shai
  0 siblings, 0 replies; 10+ messages in thread
From: Brandes, Shai @ 2024-09-16 16:06 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Ferruh Yigit, Tyler Retzlaff, dev, Schmeilin, Evgeny, Beider,
	Ron, Bernstein,  Amit, Atrash, Wajeeh

[-- 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 --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net/ena: revert redefining memcpy
  2024-09-12  5:12   ` Tyler Retzlaff
  2024-09-12  8:19     ` Ferruh Yigit
@ 2024-09-22  4:08     ` Ferruh Yigit
  1 sibling, 0 replies; 10+ messages in thread
From: Ferruh Yigit @ 2024-09-22  4:08 UTC (permalink / raw)
  To: Tyler Retzlaff, Stephen Hemminger
  Cc: dev, Shai Brandes, Evgeny Schemeilin, Ron Beider, Amit Bernstein,
	Wajeeh Atrash, Igor Chauskin, Michal Krawczyk, Artur Rojek

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

Acked-by: Shai Brandes <shaibran@amazon.com>

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-09-22  4:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <200~bug-1510-3@http.bugs.dpdk.org>
2024-08-12 15:34 ` [PATCH] net/ena: revert redefining memcpy Stephen Hemminger
2024-08-12 20:53   ` Wathsala Wathawana Vithanage
2024-08-13  8:57   ` Morten Brørup
2024-09-12  5:12   ` Tyler Retzlaff
2024-09-12  8:19     ` Ferruh Yigit
2024-09-12 14:53       ` Brandes, Shai
2024-09-16  6:33       ` Brandes, Shai
2024-09-16 15:11         ` Stephen Hemminger
2024-09-16 16:06           ` Brandes, Shai
2024-09-22  4:08     ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).