Re: [PATCH v6 0/7] Optionally have rte_memcpy delegate to compiler memcpy

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 20 Sep 2024 12:27:09 +0200
Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:

> This patch set make DPDK library, driver, and application code
> optionally use the compiler/libc memcpy() when functions in
> <rte_memcpy.h> are invoked.
> 
> The new compiler memcpy-based rte_memcpy() implementations may be
> enabled by means of a build-time option.
> 
> This patch set only make a difference on x86, PPC and ARM. Loongarch
> and RISCV already used compiler/libc memcpy().
> 
> This patch set includes a number of fixes in drivers and libraries
> which errornously relied on <rte_memcpy.h> including header files
> (i.e., <rte_vect.h>) required by its implementation.
> 
> Mattias Rönnblom (7):
>   event/dlb2: include headers for vector and memory copy APIs
>   net/octeon_ep: add missing vector API header include
>   distributor: add missing vector API header include
>   fib: add missing vector API header include
>   eal: provide option to use compiler memcpy instead of RTE
>   ci: test compiler memcpy
>   vhost: optimize memcpy routines when cc memcpy is used
> 
>  .ci/linux-build.sh                     |  5 +++
>  .github/workflows/build.yml            |  7 +++
>  config/meson.build                     |  1 +
>  devtools/test-meson-builds.sh          |  4 +-
>  doc/guides/rel_notes/release_24_11.rst | 20 +++++++++
>  drivers/event/dlb2/dlb2.c              |  2 +
>  drivers/net/octeon_ep/otx_ep_ethdev.c  |  2 +
>  lib/distributor/rte_distributor.c      |  1 +
>  lib/eal/arm/include/rte_memcpy.h       |  9 ++++
>  lib/eal/include/generic/rte_memcpy.h   | 61 +++++++++++++++++++++++---
>  lib/eal/loongarch/include/rte_memcpy.h | 54 +----------------------
>  lib/eal/ppc/include/rte_memcpy.h       |  9 ++++
>  lib/eal/riscv/include/rte_memcpy.h     | 54 +----------------------
>  lib/eal/x86/include/meson.build        |  1 +
>  lib/eal/x86/include/rte_memcpy.h       |  9 ++++
>  lib/fib/trie.c                         |  1 +
>  lib/vhost/virtio_net.c                 | 37 +++++++++++++++-
>  meson_options.txt                      |  2 +
>  18 files changed, 165 insertions(+), 114 deletions(-)
> 

AI code review had useful feedback on this. Please address.

                                                                                                     
## Overall Assessment                                                                                
                                                                                                     
### Errors (Must Fix)                                                                                
1. **Patch 6/7**: Copy-paste error in `.ci/linux-build.sh` - both if branches set `use_cc_memcpy=true`
                                                                                                     
### Warnings (Should Fix)                                                                            
1. **Patch 2/7**: Include order could be improved for consistency                                    
2. **Patch 5/7**: Long line in meson_options.txt description                                         
3. **Patch 5/7**: Extra blank line in release notes before "Removed Items"                           
                                                                                                     
### Info (Consider)                                                                                  
- All patches have proper Signed-off-by tags                                                         
- Subject lines are within 60 character limit                                                        
- Commit messages are well-written and explain the rationale                                         
- Proper use of Acked-by tags from reviewers                                                         
                                                                                                     
The critical bug in Patch 6/7 should be fixed before merging - the else branch should set `-Duse_cc_memcpy=false` instead of `true`.

--- Full Report ---
=== Patch Review: bundle-1682-opt-memcpy.mbox (via Claude) ===
# DPDK Patch Review: bundle-1682-opt-memcpy.mbox

This patch series adds an option to use compiler memcpy instead of DPDK's custom implementation, along with several prerequisite fixes for missing header includes.

---

## Summary

The patch series contains 7 patches:
1. event/dlb2: include headers for vector and memory copy APIs
2. net/octeon_ep: add missing vector API header include
3. distributor: add missing vector API header include
4. fib: add missing vector API header include
5. eal: provide option to use compiler memcpy instead of RTE
6. ci: test compiler memcpy
7. vhost: optimize memcpy routines when cc memcpy is used

---

## Patch 1/7: event/dlb2: include headers for vector and memory copy APIs

### Info
- Well-structured commit message explaining the issue and fix
- Proper Signed-off-by and Acked-by tags
- Correctly formatted subject line

### No Issues Found
This patch is clean and follows guidelines.

---

## Patch 2/7: net/octeon_ep: add missing vector API header include

### Warning
**Include order issue**: The `<rte_vect.h>` include is placed after `<ethdev_pci.h>`, but according to AGENTS.md guidelines, DPDK EAL includes should come before DPDK misc library includes, and there should be a blank line separating groups.

```c
#include <inttypes.h>
#include <ethdev_pci.h>

#include <rte_vect.h>
```

Consider placing `<rte_vect.h>` in the proper order relative to other DPDK includes.

### No Other Issues Found
Otherwise clean patch.

---

## Patch 3/7: distributor: add missing vector API header include

### Info
- Clean patch with proper formatting

### No Issues Found
This patch is clean and follows guidelines.

---

## Patch 4/7: fib: add missing vector API header include

### Info
- Clean patch with proper formatting

### No Issues Found
This patch is clean and follows guidelines.

---

## Patch 5/7: eal: provide option to use compiler memcpy instead of RTE

### Error
**Release notes formatting issue**: The release notes have an extra blank line after the "destination buffers)." line:

```rst
  destination buffers).
                        <-- extra blank line here
Removed Items
```

The blank line should be removed or the section should end properly before "Removed Items".

### Warning
**Line length in meson_options.txt**: The description line for `use_cc_memcpy` may exceed typical line length conventions:

```python
option('use_cc_memcpy', type: 'boolean', value: false, description:
       'Have the functions of <rte_memcpy.h> delegate to compiler/libc memcpy() instead of using custom implementation.')
```

This is a long line (approximately 119 characters). Consider breaking it.

### Info
- Good use of `#ifdef __cplusplus` guards in generic/rte_memcpy.h
- Proper SPDX identifiers maintained
- Good documentation in release notes

---

## Patch 6/7: ci: test compiler memcpy

### Error
**Copy-paste error in linux-build.sh**: Both branches of the if statement set the same value:

```bash
if [ "$CCMEMCPY" = "true" ]; then
	OPTS="$OPTS -Duse_cc_memcpy=true"
else
	OPTS="$OPTS -Duse_cc_memcpy=true"  # <-- Should be false
fi
```

The `else` branch should set `-Duse_cc_memcpy=false`.

### Info
- Good addition of CI coverage for the new feature

---

## Patch 7/7: vhost: optimize memcpy routines when cc memcpy is used

### Warning
**Brace style inconsistency**: The `if-else` statement uses braces for the `if` block but not for the `else` block:

```c
	if (len <= 256) {
		size_t left;

		for (left = len; left >= 32; left -= 32) {
			memcpy(dst, src, 32);
			dst = RTE_PTR_ADD(dst, 32);
			src = RTE_PTR_ADD(src, 32);
		}

		memcpy(dst, src, left);
	} else
		memcpy(dst, src, len);
```

According to DPDK coding style, when one branch has braces, consistency suggests the other should too, or both should be without braces if they're single statements. However, the DPDK guidelines show examples where this mixed style is acceptable.

### Info
- Good performance documentation in the commit message
- Proper use of `__rte_always_inline`
- Good use of `restrict` keyword
- Appropriate use of `__builtin_assume_aligned`
- Well-documented rationale for the optimization

### No Other Issues Found
This patch is otherwise clean.

---

## Overall Assessment

### Errors (Must Fix)
1. **Patch 6/7**: Copy-paste error in `.ci/linux-build.sh` - both if branches set `use_cc_memcpy=true`

### Warnings (Should Fix)
1. **Patch 2/7**: Include order could be improved for consistency
2. **Patch 5/7**: Long line in meson_options.txt description
3. **Patch 5/7**: Extra blank line in release notes before "Removed Items"

### Info (Consider)
- All patches have proper Signed-off-by tags
- Subject lines are within 60 character limit
- Commit messages are well-written and explain the rationale
- Proper use of Acked-by tags from reviewers

The critical bug in Patch 6/7 should be fixed before merging - the else branch should set `-Duse_cc_memcpy=false` instead of `true`.