ip_frag: replace the rte memcpy with memcpy
Checks
Commit Message
To resolve the compilation warning,replace the rte_memcpy with memcpy.
Modify in file test_ipfrag.c and rte_ipv4_fragmentation.c.
Signed-off-by: Huichao Cai <chcchc88@163.com>
---
app/test/test_ipfrag.c | 13 ++++++-------
lib/ip_frag/rte_ipv4_fragmentation.c | 7 +++----
2 files changed, 9 insertions(+), 11 deletions(-)
Comments
On Sat, Jun 18, 2022 at 4:10 PM Huichao Cai <chcchc88@163.com> wrote:
>
> To resolve the compilation warning,replace the rte_memcpy with memcpy.
> Modify in file test_ipfrag.c and rte_ipv4_fragmentation.c.
>
> Signed-off-by: Huichao Cai <chcchc88@163.com>
Fixed Konstantin mail address.
> ---
> app/test/test_ipfrag.c | 13 ++++++-------
> lib/ip_frag/rte_ipv4_fragmentation.c | 7 +++----
> 2 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
> index dc62b0e..ba0ffd0 100644
> --- a/app/test/test_ipfrag.c
> +++ b/app/test/test_ipfrag.c
> @@ -23,7 +23,6 @@
>
> #include <rte_ip_frag.h>
> #include <rte_mbuf.h>
> -#include <rte_memcpy.h>
> #include <rte_random.h>
>
> #define NUM_MBUFS 128
> @@ -147,13 +146,13 @@ static void ut_teardown(void)
> if (opt_copied) {
> expected_opt->len =
> sizeof(expected_first_frag_ipv4_opts_copied);
> - rte_memcpy(expected_opt->data,
> + memcpy(expected_opt->data,
> expected_first_frag_ipv4_opts_copied,
> sizeof(expected_first_frag_ipv4_opts_copied));
> } else {
> expected_opt->len =
> sizeof(expected_first_frag_ipv4_opts_nocopied);
> - rte_memcpy(expected_opt->data,
> + memcpy(expected_opt->data,
> expected_first_frag_ipv4_opts_nocopied,
> sizeof(expected_first_frag_ipv4_opts_nocopied));
> }
> @@ -161,13 +160,13 @@ static void ut_teardown(void)
> if (opt_copied) {
> expected_opt->len =
> sizeof(expected_sub_frag_ipv4_opts_copied);
> - rte_memcpy(expected_opt->data,
> + memcpy(expected_opt->data,
> expected_sub_frag_ipv4_opts_copied,
> sizeof(expected_sub_frag_ipv4_opts_copied));
> } else {
> expected_opt->len =
> sizeof(expected_sub_frag_ipv4_opts_nocopied);
> - rte_memcpy(expected_opt->data,
> + memcpy(expected_opt->data,
> expected_sub_frag_ipv4_opts_nocopied,
> sizeof(expected_sub_frag_ipv4_opts_nocopied));
> }
> @@ -227,7 +226,7 @@ static void ut_teardown(void)
> hdr->src_addr = rte_cpu_to_be_32(0x8080808);
> hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
>
> - rte_memcpy(hdr + 1, opt.data, opt.len);
> + memcpy(hdr + 1, opt.data, opt.len);
> }
>
> static void
> @@ -312,7 +311,7 @@ static void ut_teardown(void)
> char *iph_opt = rte_pktmbuf_mtod_offset(mb[i],
> char *, sizeof(struct rte_ipv4_hdr));
> opt->len = opt_len;
> - rte_memcpy(opt->data, iph_opt, opt_len);
> + memcpy(opt->data, iph_opt, opt_len);
> } else {
> opt->len = RTE_IPV4_HDR_OPT_MAX_LEN;
> memset(opt->data, RTE_IPV4_HDR_OPT_EOL,
> diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
> index a19f6fd..27a8ad2 100644
> --- a/lib/ip_frag/rte_ipv4_fragmentation.c
> +++ b/lib/ip_frag/rte_ipv4_fragmentation.c
> @@ -5,7 +5,6 @@
> #include <stddef.h>
> #include <errno.h>
>
> -#include <rte_memcpy.h>
> #include <rte_ether.h>
>
> #include "ip_frag_common.h"
> @@ -26,7 +25,7 @@ static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
> const struct rte_ipv4_hdr *src, uint16_t header_len,
> uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
> {
> - rte_memcpy(dst, src, header_len);
> + memcpy(dst, src, header_len);
> fofs = (uint16_t)(fofs + (dofs >> RTE_IPV4_HDR_FO_SHIFT));
> fofs = (uint16_t)(fofs | mf << RTE_IPV4_HDR_MF_SHIFT);
> dst->fragment_offset = rte_cpu_to_be_16(fofs);
> @@ -48,7 +47,7 @@ static inline uint16_t __create_ipopt_frag_hdr(uint8_t *iph,
> struct rte_ipv4_hdr *iph_opt = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
>
> ipopt_len = 0;
> - rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
> + memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
> ipopt_frag_hdr += sizeof(struct rte_ipv4_hdr);
>
> uint8_t *p_opt = iph + sizeof(struct rte_ipv4_hdr);
> @@ -65,7 +64,7 @@ static inline uint16_t __create_ipopt_frag_hdr(uint8_t *iph,
> break;
>
> if (RTE_IPV4_HDR_OPT_COPIED(*p_opt)) {
> - rte_memcpy(ipopt_frag_hdr + ipopt_len,
> + memcpy(ipopt_frag_hdr + ipopt_len,
> p_opt, p_opt[1]);
> ipopt_len += p_opt[1];
> }
> --
> 1.8.3.1
>
18/06/2022 15:09, Huichao Cai пишет:
> To resolve the compilation warning,replace the rte_memcpy with memcpy.
> Modify in file test_ipfrag.c and rte_ipv4_fragmentation.c.
>
> Signed-off-by: Huichao Cai <chcchc88@163.com>
> ---
> app/test/test_ipfrag.c | 13 ++++++-------
> lib/ip_frag/rte_ipv4_fragmentation.c | 7 +++----
> 2 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
> index dc62b0e..ba0ffd0 100644
> --- a/app/test/test_ipfrag.c
> +++ b/app/test/test_ipfrag.c
> @@ -23,7 +23,6 @@
>
> #include <rte_ip_frag.h>
> #include <rte_mbuf.h>
> -#include <rte_memcpy.h>
> #include <rte_random.h>
>
> #define NUM_MBUFS 128
> @@ -147,13 +146,13 @@ static void ut_teardown(void)
> if (opt_copied) {
> expected_opt->len =
> sizeof(expected_first_frag_ipv4_opts_copied);
> - rte_memcpy(expected_opt->data,
> + memcpy(expected_opt->data,
> expected_first_frag_ipv4_opts_copied,
> sizeof(expected_first_frag_ipv4_opts_copied));
> } else {
> expected_opt->len =
> sizeof(expected_first_frag_ipv4_opts_nocopied);
> - rte_memcpy(expected_opt->data,
> + memcpy(expected_opt->data,
> expected_first_frag_ipv4_opts_nocopied,
> sizeof(expected_first_frag_ipv4_opts_nocopied));
> }
> @@ -161,13 +160,13 @@ static void ut_teardown(void)
> if (opt_copied) {
> expected_opt->len =
> sizeof(expected_sub_frag_ipv4_opts_copied);
> - rte_memcpy(expected_opt->data,
> + memcpy(expected_opt->data,
> expected_sub_frag_ipv4_opts_copied,
> sizeof(expected_sub_frag_ipv4_opts_copied));
> } else {
> expected_opt->len =
> sizeof(expected_sub_frag_ipv4_opts_nocopied);
> - rte_memcpy(expected_opt->data,
> + memcpy(expected_opt->data,
> expected_sub_frag_ipv4_opts_nocopied,
> sizeof(expected_sub_frag_ipv4_opts_nocopied));
> }
> @@ -227,7 +226,7 @@ static void ut_teardown(void)
> hdr->src_addr = rte_cpu_to_be_32(0x8080808);
> hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
>
> - rte_memcpy(hdr + 1, opt.data, opt.len);
> + memcpy(hdr + 1, opt.data, opt.len);
> }
>
> static void
> @@ -312,7 +311,7 @@ static void ut_teardown(void)
> char *iph_opt = rte_pktmbuf_mtod_offset(mb[i],
> char *, sizeof(struct rte_ipv4_hdr));
> opt->len = opt_len;
> - rte_memcpy(opt->data, iph_opt, opt_len);
> + memcpy(opt->data, iph_opt, opt_len);
> } else {
> opt->len = RTE_IPV4_HDR_OPT_MAX_LEN;
> memset(opt->data, RTE_IPV4_HDR_OPT_EOL,
> diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
> index a19f6fd..27a8ad2 100644
> --- a/lib/ip_frag/rte_ipv4_fragmentation.c
> +++ b/lib/ip_frag/rte_ipv4_fragmentation.c
> @@ -5,7 +5,6 @@
> #include <stddef.h>
> #include <errno.h>
>
> -#include <rte_memcpy.h>
> #include <rte_ether.h>
>
> #include "ip_frag_common.h"
> @@ -26,7 +25,7 @@ static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
> const struct rte_ipv4_hdr *src, uint16_t header_len,
> uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
> {
> - rte_memcpy(dst, src, header_len);
> + memcpy(dst, src, header_len);
I am fine with replacements in test and inside the lib, for cases
where 'len' parameter is constant value.
Though as I said before, here 'header_len' is not a constant value.
Are you sure it will not introduce any performance regression?
> fofs = (uint16_t)(fofs + (dofs >> RTE_IPV4_HDR_FO_SHIFT));
> fofs = (uint16_t)(fofs | mf << RTE_IPV4_HDR_MF_SHIFT);
> dst->fragment_offset = rte_cpu_to_be_16(fofs);
> @@ -48,7 +47,7 @@ static inline uint16_t __create_ipopt_frag_hdr(uint8_t *iph,
> struct rte_ipv4_hdr *iph_opt = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
>
> ipopt_len = 0;
> - rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
> + memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
> ipopt_frag_hdr += sizeof(struct rte_ipv4_hdr);
>
> uint8_t *p_opt = iph + sizeof(struct rte_ipv4_hdr);
> @@ -65,7 +64,7 @@ static inline uint16_t __create_ipopt_frag_hdr(uint8_t *iph,
> break;
>
> if (RTE_IPV4_HDR_OPT_COPIED(*p_opt)) {
> - rte_memcpy(ipopt_frag_hdr + ipopt_len,
> + memcpy(ipopt_frag_hdr + ipopt_len,
> p_opt, p_opt[1]);
> ipopt_len += p_opt[1];
> }
On Wed, 22 Jun 2022 23:49:39 +0100
Konstantin Ananyev <konstantin.v.ananyev@yandex.ru> wrote:
> > @@ -26,7 +25,7 @@ static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
> > const struct rte_ipv4_hdr *src, uint16_t header_len,
> > uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
> > {
> > - rte_memcpy(dst, src, header_len);
> > + memcpy(dst, src, header_len);
>
>
> I am fine with replacements in test and inside the lib, for cases
> where 'len' parameter is constant value.
> Though as I said before, here 'header_len' is not a constant value.
> Are you sure it will not introduce any performance regression?
Do you have any performance tests. The ip header options are very small.
On Thu, Jun 23, 2022 at 4:35 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Wed, 22 Jun 2022 23:49:39 +0100
> Konstantin Ananyev <konstantin.v.ananyev@yandex.ru> wrote:
>
> > > @@ -26,7 +25,7 @@ static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
> > > const struct rte_ipv4_hdr *src, uint16_t header_len,
> > > uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
> > > {
> > > - rte_memcpy(dst, src, header_len);
> > > + memcpy(dst, src, header_len);
> >
> >
> > I am fine with replacements in test and inside the lib, for cases
> > where 'len' parameter is constant value.
> > Though as I said before, here 'header_len' is not a constant value.
> > Are you sure it will not introduce any performance regression?
>
> Do you have any performance tests. The ip header options are very small.
We have no alternative to this patch for fixing build with gcc 12
(which we want for rc2).
As I mentionned during the maintainers call, I will be merging this
patch for rc2 and wait for non regression tests.
We can still revert this patch if the performance is impacted and go
with an alternative approach.
On Sat, Jun 18, 2022 at 4:10 PM Huichao Cai <chcchc88@163.com> wrote:
>
> To resolve the compilation warning,replace the rte_memcpy with memcpy.
> Modify in file test_ipfrag.c and rte_ipv4_fragmentation.c.
>
These warnings appeared with:
Fixes: b50a14a853aa ("ip_frag: add IPv4 options fragment")
> Signed-off-by: Huichao Cai <chcchc88@163.com>
I updated the commitlog to give some detail.
Applied to fix compilation with GCC 12 for -rc2.
Thanks.
23/06/2022 03:35, Stephen Hemminger пишет:
> On Wed, 22 Jun 2022 23:49:39 +0100
> Konstantin Ananyev <konstantin.v.ananyev@yandex.ru> wrote:
>
>>> @@ -26,7 +25,7 @@ static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
>>> const struct rte_ipv4_hdr *src, uint16_t header_len,
>>> uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
>>> {
>>> - rte_memcpy(dst, src, header_len);
>>> + memcpy(dst, src, header_len);
>>
>>
>> I am fine with replacements in test and inside the lib, for cases
>> where 'len' parameter is constant value.
>> Though as I said before, here 'header_len' is not a constant value.
>> Are you sure it will not introduce any performance regression?
>
> Do you have any performance tests. The ip header options are very small.
From my experience - usually it is not about how big or small amount
we need to copy. It is about can compiler evaluate 'size' parameter
for memcpy() at compilation time or not.
If it can, great - it will most likely replace memcpy()
with some really well optimized code.
If not it has to generate a proper call to actual
memcpy() function. Which again, can be well optimized, but the
overhead of the function call itself can still be noticeable,
specially for small copies.
Anyway, as I can see, David already integrated these changes anyway.
So now, we'll have to wait and see would anyone complain or not.
About performance testing, the only one I am aware about:
examples/ip_fragmentation
Konstantin
On Fri, Jun 24, 2022 at 06:25:10PM +0100, Konstantin Ananyev wrote:
> 23/06/2022 03:35, Stephen Hemminger пишет:
> > On Wed, 22 Jun 2022 23:49:39 +0100
> > Konstantin Ananyev <konstantin.v.ananyev@yandex.ru> wrote:
> >
> > > > @@ -26,7 +25,7 @@ static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
> > > > const struct rte_ipv4_hdr *src, uint16_t header_len,
> > > > uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
> > > > {
> > > > - rte_memcpy(dst, src, header_len);
> > > > + memcpy(dst, src, header_len);
> > >
> > >
> > > I am fine with replacements in test and inside the lib, for cases
> > > where 'len' parameter is constant value.
> > > Though as I said before, here 'header_len' is not a constant value.
> > > Are you sure it will not introduce any performance regression?
> >
> > Do you have any performance tests. The ip header options are very small.
>
>
> From my experience - usually it is not about how big or small amount
> we need to copy. It is about can compiler evaluate 'size' parameter
> for memcpy() at compilation time or not.
> If it can, great - it will most likely replace memcpy()
> with some really well optimized code.
> If not it has to generate a proper call to actual
> memcpy() function. Which again, can be well optimized, but the
> overhead of the function call itself can still be noticeable,
> specially for small copies.
> Anyway, as I can see, David already integrated these changes anyway.
> So now, we'll have to wait and see would anyone complain or not.
> About performance testing, the only one I am aware about:
> examples/ip_fragmentation
>
> Konstantin
>
>
For some small(<1k) size, " rep movsb" is very fast. much faster than I
expected. I just noticed this in another application.
@@ -23,7 +23,6 @@
#include <rte_ip_frag.h>
#include <rte_mbuf.h>
-#include <rte_memcpy.h>
#include <rte_random.h>
#define NUM_MBUFS 128
@@ -147,13 +146,13 @@ static void ut_teardown(void)
if (opt_copied) {
expected_opt->len =
sizeof(expected_first_frag_ipv4_opts_copied);
- rte_memcpy(expected_opt->data,
+ memcpy(expected_opt->data,
expected_first_frag_ipv4_opts_copied,
sizeof(expected_first_frag_ipv4_opts_copied));
} else {
expected_opt->len =
sizeof(expected_first_frag_ipv4_opts_nocopied);
- rte_memcpy(expected_opt->data,
+ memcpy(expected_opt->data,
expected_first_frag_ipv4_opts_nocopied,
sizeof(expected_first_frag_ipv4_opts_nocopied));
}
@@ -161,13 +160,13 @@ static void ut_teardown(void)
if (opt_copied) {
expected_opt->len =
sizeof(expected_sub_frag_ipv4_opts_copied);
- rte_memcpy(expected_opt->data,
+ memcpy(expected_opt->data,
expected_sub_frag_ipv4_opts_copied,
sizeof(expected_sub_frag_ipv4_opts_copied));
} else {
expected_opt->len =
sizeof(expected_sub_frag_ipv4_opts_nocopied);
- rte_memcpy(expected_opt->data,
+ memcpy(expected_opt->data,
expected_sub_frag_ipv4_opts_nocopied,
sizeof(expected_sub_frag_ipv4_opts_nocopied));
}
@@ -227,7 +226,7 @@ static void ut_teardown(void)
hdr->src_addr = rte_cpu_to_be_32(0x8080808);
hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
- rte_memcpy(hdr + 1, opt.data, opt.len);
+ memcpy(hdr + 1, opt.data, opt.len);
}
static void
@@ -312,7 +311,7 @@ static void ut_teardown(void)
char *iph_opt = rte_pktmbuf_mtod_offset(mb[i],
char *, sizeof(struct rte_ipv4_hdr));
opt->len = opt_len;
- rte_memcpy(opt->data, iph_opt, opt_len);
+ memcpy(opt->data, iph_opt, opt_len);
} else {
opt->len = RTE_IPV4_HDR_OPT_MAX_LEN;
memset(opt->data, RTE_IPV4_HDR_OPT_EOL,
@@ -5,7 +5,6 @@
#include <stddef.h>
#include <errno.h>
-#include <rte_memcpy.h>
#include <rte_ether.h>
#include "ip_frag_common.h"
@@ -26,7 +25,7 @@ static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
const struct rte_ipv4_hdr *src, uint16_t header_len,
uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
{
- rte_memcpy(dst, src, header_len);
+ memcpy(dst, src, header_len);
fofs = (uint16_t)(fofs + (dofs >> RTE_IPV4_HDR_FO_SHIFT));
fofs = (uint16_t)(fofs | mf << RTE_IPV4_HDR_MF_SHIFT);
dst->fragment_offset = rte_cpu_to_be_16(fofs);
@@ -48,7 +47,7 @@ static inline uint16_t __create_ipopt_frag_hdr(uint8_t *iph,
struct rte_ipv4_hdr *iph_opt = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
ipopt_len = 0;
- rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
+ memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
ipopt_frag_hdr += sizeof(struct rte_ipv4_hdr);
uint8_t *p_opt = iph + sizeof(struct rte_ipv4_hdr);
@@ -65,7 +64,7 @@ static inline uint16_t __create_ipopt_frag_hdr(uint8_t *iph,
break;
if (RTE_IPV4_HDR_OPT_COPIED(*p_opt)) {
- rte_memcpy(ipopt_frag_hdr + ipopt_len,
+ memcpy(ipopt_frag_hdr + ipopt_len,
p_opt, p_opt[1]);
ipopt_len += p_opt[1];
}