diff mbox series

[v2,01/71] cocci/rte_memcpy: add script to eliminate fixed size rte_memcpy

Message ID 20240301171707.95242-2-stephen@networkplumber.org (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers
Series replace use of fixed size rte_mempcy |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Stephen Hemminger March 1, 2024, 5:14 p.m. UTC 
  Rte_memcpy should not be used for the simple case of copying
a fix size structure because it is slower and will hide problems
from code analysis tools. Coverity, fortify and other analyzers
special case memcpy().

Gcc (and Clang) are smart enough to inline copies which
will be faster.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 devtools/cocci/rte_memcpy.cocci | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 devtools/cocci/rte_memcpy.cocci

Comments

Mattias Rönnblom March 2, 2024, 11:19 a.m. UTC | #1 
On 2024-03-01 18:14, Stephen Hemminger wrote:
> Rte_memcpy should not be used for the simple case of copying
> a fix size structure because it is slower and will hide problems
> from code analysis tools. Coverity, fortify and other analyzers
> special case memcpy().
> 
> Gcc (and Clang) are smart enough to inline copies which
> will be faster.
> 

Are you suggesting rte_memcpy() calls aren't inlined?

> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   devtools/cocci/rte_memcpy.cocci | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>   create mode 100644 devtools/cocci/rte_memcpy.cocci
> 
> diff --git a/devtools/cocci/rte_memcpy.cocci b/devtools/cocci/rte_memcpy.cocci
> new file mode 100644
> index 000000000000..fa1038fc066d
> --- /dev/null
> +++ b/devtools/cocci/rte_memcpy.cocci
> @@ -0,0 +1,11 @@
> +//
> +// rte_memcpy should not be used for simple fixed size structure
> +// because compiler's are smart enough to inline these.
> +//

What do you do in code where it's not known if the size will be constant 
or not?

> +@@
> +expression src, dst; constant size;
> +@@
> +(
> +- rte_memcpy(dst, src, size)
> ++ memcpy(dst, src, size)
> +)
Stephen Hemminger March 2, 2024, 5:02 p.m. UTC | #2 
On Sat, 2 Mar 2024 12:19:13 +0100
Mattias Rönnblom <hofors@lysator.liu.se> wrote:

> On 2024-03-01 18:14, Stephen Hemminger wrote:
> > Rte_memcpy should not be used for the simple case of copying
> > a fix size structure because it is slower and will hide problems
> > from code analysis tools. Coverity, fortify and other analyzers
> > special case memcpy().
> > 
> > Gcc (and Clang) are smart enough to inline copies which
> > will be faster.
> >   
> 
> Are you suggesting rte_memcpy() calls aren't inlined?

With a simple made up example of copying an IPv6 address.

rte_memcpy() inlines to:
rte_copy_addr:
        movdqu  xmm0, XMMWORD PTR [rsi]
        movups  XMMWORD PTR [rdi], xmm0
        movdqu  xmm0, XMMWORD PTR [rsi]
        movups  XMMWORD PTR [rdi], xmm0
        ret

Vs memcpy becomes.

copy_addr:
        movdqu  xmm0, XMMWORD PTR [rsi]
        movups  XMMWORD PTR [rdi], xmm0
        ret

Looks like a bug in rte_memcpy_generic? Why is it not handling
16 bytes correctly.

For me, it was more about getting fortify and compiler warnings to
catch bugs. For most code, the output should be the same.
Morten Brørup March 2, 2024, 5:39 p.m. UTC | #3 
+To: x86 maintainers, another bug in rte_memcpy()

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, 2 March 2024 18.02
> 
> On Sat, 2 Mar 2024 12:19:13 +0100
> Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> 
> > On 2024-03-01 18:14, Stephen Hemminger wrote:
> > > Rte_memcpy should not be used for the simple case of copying
> > > a fix size structure because it is slower and will hide problems
> > > from code analysis tools. Coverity, fortify and other analyzers
> > > special case memcpy().
> > >
> > > Gcc (and Clang) are smart enough to inline copies which
> > > will be faster.
> > >
> >
> > Are you suggesting rte_memcpy() calls aren't inlined?
> 
> With a simple made up example of copying an IPv6 address.
> 
> rte_memcpy() inlines to:
> rte_copy_addr:
>         movdqu  xmm0, XMMWORD PTR [rsi]
>         movups  XMMWORD PTR [rdi], xmm0
>         movdqu  xmm0, XMMWORD PTR [rsi]
>         movups  XMMWORD PTR [rdi], xmm0
>         ret
> 
> Vs memcpy becomes.
> 
> copy_addr:
>         movdqu  xmm0, XMMWORD PTR [rsi]
>         movups  XMMWORD PTR [rdi], xmm0
>         ret
> 
> Looks like a bug in rte_memcpy_generic? Why is it not handling
> 16 bytes correctly.

Looks like you have run into another bunch of off-by-one errors like this one:
https://git.dpdk.org/dpdk/commit/lib/eal/x86/include/rte_memcpy.h?id=2ef17be88e8b26f871cfb0265227341e36f486ea

> 
> For me, it was more about getting fortify and compiler warnings to
> catch bugs. For most code, the output should be the same.
diff mbox series

Patch

diff --git a/devtools/cocci/rte_memcpy.cocci b/devtools/cocci/rte_memcpy.cocci
new file mode 100644
index 000000000000..fa1038fc066d
--- /dev/null
+++ b/devtools/cocci/rte_memcpy.cocci
@@ -0,0 +1,11 @@ 
+//
+// rte_memcpy should not be used for simple fixed size structure
+// because compiler's are smart enough to inline these.
+//
+@@
+expression src, dst; constant size;
+@@
+(
+- rte_memcpy(dst, src, size)
++ memcpy(dst, src, size)
+)