Re: [PATCH v7] ip_frag: add IPv4 options fragment and test data

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 17 Jun 2022 11:52:25 +0800 (CST)
"Huichao Cai" <chcchc88@163.com> wrote:

> Hi,Stephen
> 
> 
> There are some things I don't quite understand.Hope you can answer that.
> This will help me avoid similar errors in subsequent patch submissions.Thanks!
> 
> 
> There are places where rte_memcpy functions are used：
> ============================================
> In test_ipfrag.c：
> from func test_get_ipv4_opt: 
> rte_memcpy(expected_opt->data,expected_first_frag_ipv4_opts_copied,sizeof(expected_first_frag_ipv4_opts_copied));
> rte_memcpy(expected_opt>data,expected_first_frag_ipv4_opts_nocopied,sizeof(expected_first_frag_ipv4_opts_nocopied));  
> rte_memcpy(expected_opt->data,expected_sub_frag_ipv4_opts_copied,sizeof(expected_sub_frag_ipv4_opts_copied));
> rte_memcpy(expected_opt->data,expected_sub_frag_ipv4_opts_nocopied,sizeof(expected_sub_frag_ipv4_opts_nocopied));
> from func v4_allocate_packet_of:
> rte_memcpy(hdr + 1, opt.data, opt.len);
> from func test_get_frag_opt:
> rte_memcpy(opt->data, iph_opt, opt_len);
> 
> 
> In rte_ipv4_fragmentation.c:
> from func v4_allocate_packet_of:
> rte_memcpy(dst, src, header_len);
> from func __create_ipopt_frag_hdr:
> 
> rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
> rte_memcpy(ipopt_frag_hdr + ipopt_len, p_opt, p_opt[1]);
> ============================================
> 
> 
> These are the compilation errors:
> ============================================
> test_ipfrag.c:230
> In test_ipfrag.c：
> from func v4_allocate_packet_of:
> rte_memcpy(hdr + 1, opt.data, opt.len);
> rte_ipv4_fragmentation.c:68
> In rte_ipv4_fragmentation.c:
> from func __create_ipopt_frag_hdr:
> rte_memcpy(ipopt_frag_hdr + ipopt_len, p_opt, p_opt[1]);
> ============================================
> 
> 
> 1.Do I need to replace all rte_memcpy with memcpy or only the two rte_memcpy that compile the error are replaced by memcpy?

I would just replace all of the rte_memcpy with memcpy

> 2.
> >Since the copies will all be short why bother using rte_memcpy() all over
> >the place.  Especially in the test code, just use memcpy().  
> For example,in app/test-pmd/cmdline.c:from func cmd_set_vxlan_parsed:rte_memcpy(vxlan_encap_conf.vni, &id.vni[1], 3);Why this place can be used rte_memcpy?
> 3.For example, how such a compilation error occurs:
> ../app/test/test_ipfrag.c:57:17: note: at offset [5, 40] into object‘data’ of size 40
> 4.Under what circumstances can we use rte_memcpy?


It depends. The recommendation here was that fixing warnings is higher priority that saving a few cycles
in an underutilized part of DPDK.

Rte_memcpy() was added in early versions of DPDK because the standard toolchain gcc/glibc
was not using the optimum set of instructions on x86.  Rather than fix glibc, Intel wrote
their own rte_memcpy(). Then DPDK developers, started to assume that rte_memcpy() is always best.

I expect that rte_memcpy() is able to do better than memcpy() for larger copies because it is
likely to use bigger vector instructions and check for alignment.
For small copies just doing the mov's directly is going to be as fast or faster.
In fact, lots of places in DPDK should
replace rte_memcpy() with simple structure assignment to preserve type safety.

This is somewhat historical data, it might be wrong. It would be worthwhile to have benchmarks
across different sizes (variable and fixed), different compilers, and different CPU's.
There might be surprising results.