[RFC 0/8] Gcc-12 warning fixes

[RFC 0/8] Gcc-12 warning fixes

[Stephen Hemminger]

Ths is early proposal on how to address some of the gcc-12
warnings. Most of these are related to rte_memcpy which can
simply be addressed by using regular memcpy instead.

Stephen Hemminger (8):
  net/ena: fix warnings related to rte_memcpy and gcc-12
  net/qede: fix gcc-12 rte_memcpy warnings
  net/ice: fix rte_memcpy warnings with gcc-12
  test/ipfrag: fix gcc-12 warnings
  test/ipsec: fix gcc-12 rte_memcpy warnings
  net/enetfc: fix array out of bounds warning
  vhost: replace rte_memcpy to fix warning
  ip_frag: fix gcc-12 warnings

 app/test/test_ipfrag.c               | 13 ++++++-----
 app/test/test_ipsec.c                | 13 ++++++-----
 drivers/net/ena/base/ena_plat_dpdk.h | 10 +--------
 drivers/net/ena/ena_ethdev.c         |  8 +++----
 drivers/net/ena/ena_rss.c            |  2 +-
 drivers/net/enetfec/enet_ethdev.c    |  5 +++++
 drivers/net/ice/base/ice_osdep.h     |  5 ++---
 drivers/net/ice/ice_dcf.c            | 12 +++++------
 drivers/net/ice/ice_dcf_ethdev.c     | 10 ++++-----
 drivers/net/ice/ice_dcf_parent.c     |  2 +-
 drivers/net/ice/ice_dcf_sched.c      | 11 +++++-----
 drivers/net/ice/ice_ethdev.c         | 13 ++++++-----
 drivers/net/ice/ice_fdir_filter.c    | 32 ++++++++++++++--------------
 drivers/net/ice/ice_tm.c             |  6 +++---
 drivers/net/qede/base/bcm_osal.h     |  3 +--
 drivers/net/qede/qede_ethdev.c       |  2 +-
 drivers/net/qede/qede_filter.c       | 16 ++++++--------
 drivers/net/qede/qede_main.c         | 13 ++++++-----
 drivers/net/qede/qede_sriov.c        |  6 +++---
 lib/ip_frag/rte_ipv4_fragmentation.c |  7 +++---
 lib/vhost/vhost_crypto.c             |  2 +-
 21 files changed, 88 insertions(+), 103 deletions(-)

-- 
2.35.1

[RFC 1/8] net/ena: fix warnings related to rte_memcpy and gcc-12

[Stephen Hemminger]

Rte_memcpy is not needed for small objects only used on control
path. Regular memcpy is as fast or faster and there is more
robust since static analysis etc knows what it does.

In this driver it was redefining all memcpy as rte_memcpy
which is even worse.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/ena/base/ena_plat_dpdk.h | 10 +---------
 drivers/net/ena/ena_ethdev.c         |  8 ++++----
 drivers/net/ena/ena_rss.c            |  2 +-
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ena/base/ena_plat_dpdk.h b/drivers/net/ena/base/ena_plat_dpdk.h
index 8f2b3a87c2ab..caea763e3eca 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;
@@ -67,14 +66,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
diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index 68768cab7077..5f87429606e6 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -481,7 +481,7 @@ ENA_PROXY_DESC(ena_com_get_dev_basic_stats, ENA_MP_DEV_STATS_GET,
 	ENA_TOUCH(rsp);
 	ENA_TOUCH(ena_dev);
 	if (stats != &adapter->basic_stats)
-		rte_memcpy(stats, &adapter->basic_stats, sizeof(*stats));
+		memcpy(stats, &adapter->basic_stats, sizeof(*stats));
 }),
 	struct ena_com_dev *ena_dev, struct ena_admin_basic_stats *stats);
 
@@ -496,7 +496,7 @@ ENA_PROXY_DESC(ena_com_get_eni_stats, ENA_MP_ENI_STATS_GET,
 	ENA_TOUCH(rsp);
 	ENA_TOUCH(ena_dev);
 	if (stats != (struct ena_admin_eni_stats *)&adapter->eni_stats)
-		rte_memcpy(stats, &adapter->eni_stats, sizeof(*stats));
+		memcpy(stats, &adapter->eni_stats, sizeof(*stats));
 }),
 	struct ena_com_dev *ena_dev, struct ena_admin_eni_stats *stats);
 
@@ -538,8 +538,8 @@ ENA_PROXY_DESC(ena_com_indirect_table_get, ENA_MP_IND_TBL_GET,
 	ENA_TOUCH(rsp);
 	ENA_TOUCH(ena_dev);
 	if (ind_tbl != adapter->indirect_table)
-		rte_memcpy(ind_tbl, adapter->indirect_table,
-			   sizeof(adapter->indirect_table));
+		memcpy(ind_tbl, adapter->indirect_table,
+		       sizeof(adapter->indirect_table));
 }),
 	struct ena_com_dev *ena_dev, u32 *ind_tbl);
 
diff --git a/drivers/net/ena/ena_rss.c b/drivers/net/ena/ena_rss.c
index b6c4f76e3820..c723d3f5fca1 100644
--- a/drivers/net/ena/ena_rss.c
+++ b/drivers/net/ena/ena_rss.c
@@ -59,7 +59,7 @@ void ena_rss_key_fill(void *key, size_t size)
 		key_generated = true;
 	}
 
-	rte_memcpy(key, default_key, size);
+	memcpy(key, default_key, size);
 }
 
 int ena_rss_reta_update(struct rte_eth_dev *dev,
-- 
2.35.1

Re: [RFC 1/8] net/ena: fix warnings related to rte_memcpy and gcc-12

[Michał Krawczyk]

wt., 7 cze 2022 o 19:17 Stephen Hemminger <stephen@networkplumber.org>
napisał(a):
>
> Rte_memcpy is not needed for small objects only used on control
> path. Regular memcpy is as fast or faster and there is more
> robust since static analysis etc knows what it does.
>
> In this driver it was redefining all memcpy as rte_memcpy
> which is even worse.

Hi Stephen,

I would like to shed some light on why we're redefining all the memcpy
as rte_memcpy. The ENA HAL is unmodifiable, as it's shared across many
platforms and we cannot simply adjust it for the DPDK. We can use the
ena_plat_dpdk.h to change the ena_com (HAL) definitions, and that's
what we're doing with memcpy. It's being used on the data path for the
Tx, to copy the bounce buffers. Following the recommendations in [1]
plus the results from [2], we wanted to make use of the optimized
memcpy on the ENA's data path as well to reduce the CPU time spent in
the PMD. I'm worried that removing rte_memcpy from the ena_plat_dpdk.h
will result in some performance degradation for the ENA data path.
However I understand your concerns for the control path and I'm ok
with it.

[1] https://doc.dpdk.org/guides/prog_guide/writing_efficient_code.html#memory
[2] https://www.intel.com/content/www/us/en/developer/articles/technical/performance-optimization-of-memcpy-in-dpdk.html

Thanks,
Michal

>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/ena/base/ena_plat_dpdk.h | 10 +---------
>  drivers/net/ena/ena_ethdev.c         |  8 ++++----
>  drivers/net/ena/ena_rss.c            |  2 +-
>  3 files changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ena/base/ena_plat_dpdk.h b/drivers/net/ena/base/ena_plat_dpdk.h
> index 8f2b3a87c2ab..caea763e3eca 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;
> @@ -67,14 +66,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
> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> index 68768cab7077..5f87429606e6 100644
> --- a/drivers/net/ena/ena_ethdev.c
> +++ b/drivers/net/ena/ena_ethdev.c
> @@ -481,7 +481,7 @@ ENA_PROXY_DESC(ena_com_get_dev_basic_stats, ENA_MP_DEV_STATS_GET,
>         ENA_TOUCH(rsp);
>         ENA_TOUCH(ena_dev);
>         if (stats != &adapter->basic_stats)
> -               rte_memcpy(stats, &adapter->basic_stats, sizeof(*stats));
> +               memcpy(stats, &adapter->basic_stats, sizeof(*stats));
>  }),
>         struct ena_com_dev *ena_dev, struct ena_admin_basic_stats *stats);
>
> @@ -496,7 +496,7 @@ ENA_PROXY_DESC(ena_com_get_eni_stats, ENA_MP_ENI_STATS_GET,
>         ENA_TOUCH(rsp);
>         ENA_TOUCH(ena_dev);
>         if (stats != (struct ena_admin_eni_stats *)&adapter->eni_stats)
> -               rte_memcpy(stats, &adapter->eni_stats, sizeof(*stats));
> +               memcpy(stats, &adapter->eni_stats, sizeof(*stats));
>  }),
>         struct ena_com_dev *ena_dev, struct ena_admin_eni_stats *stats);
>
> @@ -538,8 +538,8 @@ ENA_PROXY_DESC(ena_com_indirect_table_get, ENA_MP_IND_TBL_GET,
>         ENA_TOUCH(rsp);
>         ENA_TOUCH(ena_dev);
>         if (ind_tbl != adapter->indirect_table)
> -               rte_memcpy(ind_tbl, adapter->indirect_table,
> -                          sizeof(adapter->indirect_table));
> +               memcpy(ind_tbl, adapter->indirect_table,
> +                      sizeof(adapter->indirect_table));
>  }),
>         struct ena_com_dev *ena_dev, u32 *ind_tbl);
>
> diff --git a/drivers/net/ena/ena_rss.c b/drivers/net/ena/ena_rss.c
> index b6c4f76e3820..c723d3f5fca1 100644
> --- a/drivers/net/ena/ena_rss.c
> +++ b/drivers/net/ena/ena_rss.c
> @@ -59,7 +59,7 @@ void ena_rss_key_fill(void *key, size_t size)
>                 key_generated = true;
>         }
>
> -       rte_memcpy(key, default_key, size);
> +       memcpy(key, default_key, size);
>  }
>
>  int ena_rss_reta_update(struct rte_eth_dev *dev,
> --
> 2.35.1
>

Re: [RFC 1/8] net/ena: fix warnings related to rte_memcpy and gcc-12

[Stephen Hemminger]

On Wed, 8 Jun 2022 14:29:58 +0200
Michał Krawczyk <mk@semihalf.com> wrote:

> wt., 7 cze 2022 o 19:17 Stephen Hemminger <stephen@networkplumber.org>
> napisał(a):
> >
> > Rte_memcpy is not needed for small objects only used on control
> > path. Regular memcpy is as fast or faster and there is more
> > robust since static analysis etc knows what it does.
> >
> > In this driver it was redefining all memcpy as rte_memcpy
> > which is even worse.  
> 
> Hi Stephen,
> 
> I would like to shed some light on why we're redefining all the memcpy
> as rte_memcpy. The ENA HAL is unmodifiable, as it's shared across many
> platforms and we cannot simply adjust it for the DPDK. We can use the
> ena_plat_dpdk.h to change the ena_com (HAL) definitions, and that's
> what we're doing with memcpy. It's being used on the data path for the
> Tx, to copy the bounce buffers. Following the recommendations in [1]
> plus the results from [2], we wanted to make use of the optimized
> memcpy on the ENA's data path as well to reduce the CPU time spent in
> the PMD. I'm worried that removing rte_memcpy from the ena_plat_dpdk.h
> will result in some performance degradation for the ENA data path.
> However I understand your concerns for the control path and I'm ok
> with it.
> 
> [1] https://doc.dpdk.org/guides/prog_guide/writing_efficient_code.html#memory
> [2] https://www.intel.com/content/www/us/en/developer/articles/technical/performance-optimization-of-memcpy-in-dpdk.html
> 
> Thanks,
> Michal
> 


I admit to having little sympathy unfixable for base/ style code.
You could have just replaced memcpy() in their with an abstraction layer
like other drivers.

The full gcc-12 warnings are:

913/2989] Compiling C object drivers/libtmp_rte_net_ena.a.p/net_ena_ena_rss.c.o
In file included from /usr/lib/gcc/x86_64-linux-gnu/12/include/immintrin.h:43,
                 from /usr/lib/gcc/x86_64-linux-gnu/12/include/x86intrin.h:32,
                 from ../lib/eal/x86/include/rte_vect.h:31,
                 from ../lib/eal/x86/include/rte_memcpy.h:17,
                 from ../lib/mempool/rte_mempool.h:46,
                 from ../lib/mbuf/rte_mbuf.h:38,
                 from ../lib/net/rte_ether.h:22,
                 from ../drivers/net/ena/ena_ethdev.h:10,
                 from ../drivers/net/ena/ena_rss.c:6:
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_mov128’ at ../lib/eal/x86/include/rte_memcpy.h:369:2,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:445:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘ena_rss_key_fill’ at ../drivers/net/ena/ena_rss.c:62:2:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:929:10: warning: array subscript ‘__m256i_u[1]’ is partly outside array bounds of ‘uint8_t[40]’ {aka ‘unsigned char[40]’} [-Warray-bounds]
  929 |   return *__P;
      |          ^~~~
../drivers/net/ena/ena_rss.c: In function ‘ena_rss_key_fill’:
../drivers/net/ena/ena_rss.c:51:24: note: at offset 32 into object ‘default_key’ of size 40
   51 |         static uint8_t default_key[ENA_HASH_KEY_SIZE];
      |                        ^~~~~~~~~~~
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_mov128’ at ../lib/eal/x86/include/rte_memcpy.h:370:2,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:445:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘ena_rss_key_fill’ at ../drivers/net/ena/ena_rss.c:62:2:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:929:10: warning: array subscript 2 is outside array bounds of ‘uint8_t[40]’ {aka ‘unsigned char[40]’} [-Warray-bounds]
  929 |   return *__P;
      |          ^~~~
../drivers/net/ena/ena_rss.c: In function ‘ena_rss_key_fill’:
../drivers/net/ena/ena_rss.c:51:24: note: at offset 64 into object ‘default_key’ of size 40
   51 |         static uint8_t default_key[ENA_HASH_KEY_SIZE];
      |                        ^~~~~~~~~~~
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_mov128’ at ../lib/eal/x86/include/rte_memcpy.h:371:2,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:445:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘ena_rss_key_fill’ at ../drivers/net/ena/ena_rss.c:62:2:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:929:10: warning: array subscript 3 is outside array bounds of ‘uint8_t[40]’ {aka ‘unsigned char[40]’} [-Warray-bounds]
  929 |   return *__P;
      |          ^~~~
../drivers/net/ena/ena_rss.c: In function ‘ena_rss_key_fill’:
../drivers/net/ena/ena_rss.c:51:24: note: at offset 96 into object ‘default_key’ of size 40
   51 |         static uint8_t default_key[ENA_HASH_KEY_SIZE];
      |                        ^~~~~~~~~~~
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_mov64’ at ../lib/eal/x86/include/rte_memcpy.h:358:2,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:452:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘ena_rss_key_fill’ at ../drivers/net/ena/ena_rss.c:62:2:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:929:10: warning: array subscript ‘__m256i_u[1]’ is partly outside array bounds of ‘const void[40]’ [-Warray-bounds]
  929 |   return *__P;
      |          ^~~~
../drivers/net/ena/ena_rss.c: In function ‘ena_rss_key_fill’:
../drivers/net/ena/ena_rss.c:51:24: note: at offset 32 into object ‘default_key’ of size 40
   51 |         static uint8_t default_key[ENA_HASH_KEY_SIZE];
      |                        ^~~~~~~~~~~
../drivers/net/ena/ena_rss.c:51:24: note: at offset [33, 40] into object ‘default_key’ of size 40
../drivers/net/ena/ena_rss.c:51:24: note: at offset 160 into object ‘default_key’ of size 40
../drivers/net/ena/ena_rss.c:51:24: note: at offset 32 into object ‘default_key’ of size 40
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:457:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘ena_rss_key_fill’ at ../drivers/net/ena/ena_rss.c:62:2:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:929:10: warning: array subscript [2, 288230376151711745] is outside array bounds of ‘const void[40]’ [-Warray-bounds]
  929 |   return *__P;
      |          ^~~~
../drivers/net/ena/ena_rss.c: In function ‘ena_rss_key_fill’:
../drivers/net/ena/ena_rss.c:51:24: note: object ‘default_key’ of size 40
   51 |         static uint8_t default_key[ENA_HASH_KEY_SIZE];
      |                        ^~~~~~~~~~~
../drivers/net/ena/ena_rss.c:51:24: note: at offset [1, 40] into object ‘default_key’ of size 40
../drivers/net/ena/ena_rss.c:51:24: note: at offset [128, 192] into object ‘default_key’ of size 40
../drivers/net/ena/ena_rss.c:51:24: note: object ‘default_key’ of size 40
../drivers/net/ena/ena_rss.c:51:24: note: at offset [1, 40] into object ‘default_key’ of size 40
../drivers/net/ena/ena_rss.c:51:24: note: at offset [128, 192] into object ‘default_key’ of size 40
../drivers/net/ena/ena_rss.c:51:24: note: object ‘default_key’ of size 40
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:458:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘ena_rss_key_fill’ at ../drivers/net/ena/ena_rss.c:62:2:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:929:10: warning: array subscript [2, 288230376151711746] is outside array bounds of ‘const void[40]’ [-Warray-bounds]
  929 |   return *__P;
      |          ^~~~
../drivers/net/ena/ena_rss.c: In function ‘ena_rss_key_fill’:
../drivers/net/ena/ena_rss.c:51:24: note: at offset [1, 40] into object ‘default_key’ of size 40
   51 |         static uint8_t default_key[ENA_HASH_KEY_SIZE];
      |                        ^~~~~~~~~~~
../drivers/net/ena/ena_rss.c:51:24: note: at offset [2, 40] into object ‘default_key’ of size 40
../drivers/net/ena/ena_rss.c:51:24: note: at offset [129, 193] into object ‘default_key’ of size 40
../drivers/net/ena/ena_rss.c:51:24: note: at offset [1, 40] into object ‘default_key’ of size 40
../drivers/net/ena/ena_rss.c:51:24: note: at offset [2, 40] into object ‘default_key’ of size 40
../drivers/net/ena/ena_rss.c:51:24: note: at offset [129, 193] into object ‘default_key’ of size 40
../drivers/net/ena/ena_rss.c:51:24: note: at offset [1, 40] into object ‘default_key’ of size 40
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:438:3,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘ena_rss_key_fill’ at ../drivers/net/ena/ena_rss.c:62:2:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:929:10: warning: array subscript ‘__m256i_u[0]’ is partly outside array bounds of ‘uint8_t[40]’ {aka ‘unsigned char[40]’} [-Warray-bounds]
  929 |   return *__P;
      |          ^~~~
../drivers/net/ena/ena_rss.c: In function ‘ena_rss_key_fill’:
../drivers/net/ena/ena_rss.c:51:24: note: at offset [17, 32] into object ‘default_key’ of size 40
   51 |         static uint8_t default_key[ENA_HASH_KEY_SIZE];
      |                        ^~~~~~~~~~~

Re: [RFC 1/8] net/ena: fix warnings related to rte_memcpy and gcc-12

[Michał Krawczyk]

śr., 8 cze 2022 o 17:32 Stephen Hemminger <stephen@networkplumber.org>
napisał(a):
>
> On Wed, 8 Jun 2022 14:29:58 +0200
> Michał Krawczyk <mk@semihalf.com> wrote:
>
> > wt., 7 cze 2022 o 19:17 Stephen Hemminger <stephen@networkplumber.org>
> > napisał(a):
> > >
> > > Rte_memcpy is not needed for small objects only used on control
> > > path. Regular memcpy is as fast or faster and there is more
> > > robust since static analysis etc knows what it does.
> > >
> > > In this driver it was redefining all memcpy as rte_memcpy
> > > which is even worse.
> >
> > Hi Stephen,
> >
> > I would like to shed some light on why we're redefining all the memcpy
> > as rte_memcpy. The ENA HAL is unmodifiable, as it's shared across many
> > platforms and we cannot simply adjust it for the DPDK. We can use the
> > ena_plat_dpdk.h to change the ena_com (HAL) definitions, and that's
> > what we're doing with memcpy. It's being used on the data path for the
> > Tx, to copy the bounce buffers. Following the recommendations in [1]
> > plus the results from [2], we wanted to make use of the optimized
> > memcpy on the ENA's data path as well to reduce the CPU time spent in
> > the PMD. I'm worried that removing rte_memcpy from the ena_plat_dpdk.h
> > will result in some performance degradation for the ENA data path.
> > However I understand your concerns for the control path and I'm ok
> > with it.
> >
> > [1] https://doc.dpdk.org/guides/prog_guide/writing_efficient_code.html#memory
> > [2] https://www.intel.com/content/www/us/en/developer/articles/technical/performance-optimization-of-memcpy-in-dpdk.html
> >
> > Thanks,
> > Michal
> >
>
>
> I admit to having little sympathy unfixable for base/ style code.
> You could have just replaced memcpy() in their with an abstraction layer
> like other drivers.
>

We'll probably end up with the solution you're suggesting. For now
let's remove the memcpy redefinition at all to suppress the warnings.

Acked-by: Michal Krawczyk <mk@semiahalf.com>

Re: [RFC 1/8] net/ena: fix warnings related to rte_memcpy and gcc-12

[Stephen Hemminger]

On Wed, 8 Jun 2022 21:18:15 +0200
Michał Krawczyk <mk@semihalf.com> wrote:

> śr., 8 cze 2022 o 17:32 Stephen Hemminger <stephen@networkplumber.org>
> napisał(a):
> >
> > On Wed, 8 Jun 2022 14:29:58 +0200
> > Michał Krawczyk <mk@semihalf.com> wrote:
> >  
> > > wt., 7 cze 2022 o 19:17 Stephen Hemminger <stephen@networkplumber.org>
> > > napisał(a):  
> > > >
> > > > Rte_memcpy is not needed for small objects only used on control
> > > > path. Regular memcpy is as fast or faster and there is more
> > > > robust since static analysis etc knows what it does.
> > > >
> > > > In this driver it was redefining all memcpy as rte_memcpy
> > > > which is even worse.  
> > >
> > > Hi Stephen,
> > >
> > > I would like to shed some light on why we're redefining all the memcpy
> > > as rte_memcpy. The ENA HAL is unmodifiable, as it's shared across many
> > > platforms and we cannot simply adjust it for the DPDK. We can use the
> > > ena_plat_dpdk.h to change the ena_com (HAL) definitions, and that's
> > > what we're doing with memcpy. It's being used on the data path for the
> > > Tx, to copy the bounce buffers. Following the recommendations in [1]
> > > plus the results from [2], we wanted to make use of the optimized
> > > memcpy on the ENA's data path as well to reduce the CPU time spent in
> > > the PMD. I'm worried that removing rte_memcpy from the ena_plat_dpdk.h
> > > will result in some performance degradation for the ENA data path.
> > > However I understand your concerns for the control path and I'm ok
> > > with it.
> > >
> > > [1] https://doc.dpdk.org/guides/prog_guide/writing_efficient_code.html#memory
> > > [2] https://www.intel.com/content/www/us/en/developer/articles/technical/performance-optimization-of-memcpy-in-dpdk.html
> > >
> > > Thanks,
> > > Michal
> > >  
> >
> >
> > I admit to having little sympathy unfixable for base/ style code.
> > You could have just replaced memcpy() in their with an abstraction layer
> > like other drivers.
> >  
> 
> We'll probably end up with the solution you're suggesting. For now
> let's remove the memcpy redefinition at all to suppress the warnings.
> 
> Acked-by: Michal Krawczyk <mk@semiahalf.com>

Lets see if we can fix rte_memcpy() on x86 first.

It seems to me that rte_memcpy() should be an inline that only handles variable
size data, and use __builtin_memcpy() automatically for fixed size values.

[RFC 2/8] net/qede: fix gcc-12 rte_memcpy warnings

[Stephen Hemminger]

The x86 version of rte_memcpy can cause warnings. The driver does
not need to use rte_memcpy for everything. Standard memcpy is
just as fast and safer; the compiler and static analysis tools
treat memcpy specially.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/qede/base/bcm_osal.h |  3 +--
 drivers/net/qede/qede_ethdev.c   |  2 +-
 drivers/net/qede/qede_filter.c   | 16 ++++++----------
 drivers/net/qede/qede_main.c     | 13 ++++++-------
 drivers/net/qede/qede_sriov.c    |  6 +++---
 5 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/net/qede/base/bcm_osal.h b/drivers/net/qede/base/bcm_osal.h
index c5b53992822b..9ea579bfc849 100644
--- a/drivers/net/qede/base/bcm_osal.h
+++ b/drivers/net/qede/base/bcm_osal.h
@@ -14,7 +14,6 @@
 #include <rte_spinlock.h>
 #include <rte_malloc.h>
 #include <rte_atomic.h>
-#include <rte_memcpy.h>
 #include <rte_log.h>
 #include <rte_cycles.h>
 #include <rte_debug.h>
@@ -99,7 +98,7 @@ typedef intptr_t osal_int_ptr_t;
 	} while (0)
 #define OSAL_VFREE(dev, memory) OSAL_FREE(dev, memory)
 #define OSAL_MEM_ZERO(mem, size) bzero(mem, size)
-#define OSAL_MEMCPY(dst, src, size) rte_memcpy(dst, src, size)
+#define OSAL_MEMCPY(dst, src, size) memcpy(dst, src, size)
 #define OSAL_MEMCMP(s1, s2, size) memcmp(s1, s2, size)
 #define OSAL_MEMSET(dst, val, length) \
 	memset(dst, val, length)
diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
index ea6b71f09355..a4923670d6ba 100644
--- a/drivers/net/qede/qede_ethdev.c
+++ b/drivers/net/qede/qede_ethdev.c
@@ -358,7 +358,7 @@ qede_assign_rxtx_handlers(struct rte_eth_dev *dev, bool is_dummy)
 static void
 qede_alloc_etherdev(struct qede_dev *qdev, struct qed_dev_eth_info *info)
 {
-	rte_memcpy(&qdev->dev_info, info, sizeof(*info));
+	qdev->dev_info = *info;
 	qdev->ops = qed_ops;
 }
 
diff --git a/drivers/net/qede/qede_filter.c b/drivers/net/qede/qede_filter.c
index 440440423a32..ca3165d97210 100644
--- a/drivers/net/qede/qede_filter.c
+++ b/drivers/net/qede/qede_filter.c
@@ -388,10 +388,8 @@ qede_arfs_construct_pkt(struct rte_eth_dev *eth_dev,
 		ip6->vtc_flow =
 			rte_cpu_to_be_32(QEDE_FDIR_IPV6_DEFAULT_VTC_FLOW);
 
-		rte_memcpy(&ip6->src_addr, arfs->tuple.src_ipv6,
-			   IPV6_ADDR_LEN);
-		rte_memcpy(&ip6->dst_addr, arfs->tuple.dst_ipv6,
-			   IPV6_ADDR_LEN);
+		memcpy(&ip6->src_addr, arfs->tuple.src_ipv6, IPV6_ADDR_LEN);
+		memcpy(&ip6->dst_addr, arfs->tuple.dst_ipv6, IPV6_ADDR_LEN);
 		len += sizeof(struct rte_ipv6_hdr);
 		params->ipv6 = true;
 
@@ -821,12 +819,10 @@ qede_flow_parse_pattern(__rte_unused struct rte_eth_dev *dev,
 				const struct rte_flow_item_ipv6 *spec;
 
 				spec = pattern->spec;
-				rte_memcpy(flow->entry.tuple.src_ipv6,
-					   spec->hdr.src_addr,
-					   IPV6_ADDR_LEN);
-				rte_memcpy(flow->entry.tuple.dst_ipv6,
-					   spec->hdr.dst_addr,
-					   IPV6_ADDR_LEN);
+				memcpy(flow->entry.tuple.src_ipv6,
+				       spec->hdr.src_addr, IPV6_ADDR_LEN);
+				memcpy(flow->entry.tuple.dst_ipv6,
+				       spec->hdr.dst_addr, IPV6_ADDR_LEN);
 				flow->entry.tuple.eth_proto =
 					RTE_ETHER_TYPE_IPV6;
 			}
diff --git a/drivers/net/qede/qede_main.c b/drivers/net/qede/qede_main.c
index ad101194d613..03039038ad3d 100644
--- a/drivers/net/qede/qede_main.c
+++ b/drivers/net/qede/qede_main.c
@@ -372,7 +372,7 @@ qed_fill_dev_info(struct ecore_dev *edev, struct qed_dev_info *dev_info)
 	dev_info->mtu = ECORE_LEADING_HWFN(edev)->hw_info.mtu;
 	dev_info->dev_type = edev->type;
 
-	rte_memcpy(&dev_info->hw_mac, &edev->hwfns[0].hw_info.hw_mac_addr,
+	memcpy(&dev_info->hw_mac, &edev->hwfns[0].hw_info.hw_mac_addr,
 	       RTE_ETHER_ADDR_LEN);
 
 	dev_info->fw_major = FW_MAJOR_VERSION;
@@ -440,7 +440,7 @@ qed_fill_eth_dev_info(struct ecore_dev *edev, struct qed_dev_eth_info *info)
 		info->num_vlan_filters = RESC_NUM(&edev->hwfns[0], ECORE_VLAN) -
 					 max_vf_vlan_filters;
 
-		rte_memcpy(&info->port_mac, &edev->hwfns[0].hw_info.hw_mac_addr,
+		memcpy(&info->port_mac, &edev->hwfns[0].hw_info.hw_mac_addr,
 			   RTE_ETHER_ADDR_LEN);
 	} else {
 		ecore_vf_get_num_rxqs(ECORE_LEADING_HWFN(edev),
@@ -471,7 +471,7 @@ static void qed_set_name(struct ecore_dev *edev, char name[NAME_SIZE])
 {
 	int i;
 
-	rte_memcpy(edev->name, name, NAME_SIZE);
+	memcpy(edev->name, name, NAME_SIZE);
 	for_each_hwfn(edev, i) {
 		snprintf(edev->hwfns[i].name, NAME_SIZE, "%s-%d", name, i);
 	}
@@ -513,10 +513,9 @@ static void qed_fill_link(struct ecore_hwfn *hwfn,
 
 	/* Prepare source inputs */
 	if (IS_PF(hwfn->p_dev)) {
-		rte_memcpy(&params, ecore_mcp_get_link_params(hwfn),
-		       sizeof(params));
-		rte_memcpy(&link, ecore_mcp_get_link_state(hwfn), sizeof(link));
-		rte_memcpy(&link_caps, ecore_mcp_get_link_capabilities(hwfn),
+		memcpy(&params, ecore_mcp_get_link_params(hwfn), sizeof(params));
+		memcpy(&link, ecore_mcp_get_link_state(hwfn), sizeof(link));
+		memcpy(&link_caps, ecore_mcp_get_link_capabilities(hwfn),
 		       sizeof(link_caps));
 	} else {
 		ecore_vf_read_bulletin(hwfn, &change);
diff --git a/drivers/net/qede/qede_sriov.c b/drivers/net/qede/qede_sriov.c
index 0b99a8d6fe9c..937d339fb802 100644
--- a/drivers/net/qede/qede_sriov.c
+++ b/drivers/net/qede/qede_sriov.c
@@ -203,10 +203,10 @@ void qed_inform_vf_link_state(struct ecore_hwfn *hwfn)
 	if (!hwfn->pf_iov_info)
 		return;
 
-	rte_memcpy(&params, ecore_mcp_get_link_params(lead_hwfn),
+	memcpy(&params, ecore_mcp_get_link_params(lead_hwfn),
 		   sizeof(params));
-	rte_memcpy(&link, ecore_mcp_get_link_state(lead_hwfn), sizeof(link));
-	rte_memcpy(&caps, ecore_mcp_get_link_capabilities(lead_hwfn),
+	memcpy(&link, ecore_mcp_get_link_state(lead_hwfn), sizeof(link));
+	memcpy(&caps, ecore_mcp_get_link_capabilities(lead_hwfn),
 		   sizeof(caps));
 
 	/* Update bulletin of all future possible VFs with link configuration */
-- 
2.35.1

Re: [RFC 2/8] net/qede: fix gcc-12 rte_memcpy warnings

[David Marchand]

On Tue, Jun 7, 2022 at 7:18 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> The x86 version of rte_memcpy can cause warnings. The driver does
> not need to use rte_memcpy for everything. Standard memcpy is
> just as fast and safer; the compiler and static analysis tools
> treat memcpy specially.

Cc: stable@dpdk.org

>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

We want to have gcc 12 enabled for rc2.
No news from the maintainers, so I'll go ahead with the only clean
enough proposal we have.
If you think this patch is wrong, please provide an alternative for rc3.
Thanks.

Patch applied in main directly, thanks Stephen.


-- 
David Marchand

[RFC 3/8] net/ice: fix rte_memcpy warnings with gcc-12

[Stephen Hemminger]

There is no good reason to always use rte_memcpy.
Regular memcpy is as fast or faster for small values and safer
since it won't reference past end of input. Also static analysis
tools know about regular memcpy and can analyze better.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/ice/base/ice_osdep.h  |  5 ++---
 drivers/net/ice/ice_dcf.c         | 12 ++++++------
 drivers/net/ice/ice_dcf_ethdev.c  | 10 +++++-----
 drivers/net/ice/ice_dcf_parent.c  |  2 +-
 drivers/net/ice/ice_dcf_sched.c   | 11 +++++------
 drivers/net/ice/ice_ethdev.c      | 13 ++++++-------
 drivers/net/ice/ice_fdir_filter.c | 32 +++++++++++++++----------------
 drivers/net/ice/ice_tm.c          |  6 +++---
 8 files changed, 44 insertions(+), 47 deletions(-)

diff --git a/drivers/net/ice/base/ice_osdep.h b/drivers/net/ice/base/ice_osdep.h
index 8160eb68eeda..191646711088 100644
--- a/drivers/net/ice/base/ice_osdep.h
+++ b/drivers/net/ice/base/ice_osdep.h
@@ -14,7 +14,6 @@
 #include <stdbool.h>
 
 #include <rte_common.h>
-#include <rte_memcpy.h>
 #include <rte_malloc.h>
 #include <rte_memzone.h>
 #include <rte_byteorder.h>
@@ -204,7 +203,7 @@ struct ice_virt_mem {
 #define ice_free(h, m)         rte_free(m)
 
 #define ice_memset(a, b, c, d) memset((a), (b), (c))
-#define ice_memcpy(a, b, c, d) rte_memcpy((a), (b), (c))
+#define ice_memcpy(a, b, c, d) memcpy((a), (b), (c))
 
 /* SW spinlock */
 struct ice_lock {
@@ -244,7 +243,7 @@ ice_memdup(__rte_unused struct ice_hw *hw, const void *src, size_t size,
 
 	p = ice_malloc(hw, size);
 	if (p)
-		rte_memcpy(p, src, size);
+		memcpy(p, src, size);
 
 	return p;
 }
diff --git a/drivers/net/ice/ice_dcf.c b/drivers/net/ice/ice_dcf.c
index 885d58c0f4c5..842a88299bab 100644
--- a/drivers/net/ice/ice_dcf.c
+++ b/drivers/net/ice/ice_dcf.c
@@ -328,7 +328,7 @@ ice_dcf_get_vf_vsi_map(struct ice_dcf_hw *hw)
 		return 1;
 	}
 
-	rte_memcpy(hw->vf_vsi_map, vsi_map->vf_vsi, len);
+	memcpy(hw->vf_vsi_map, vsi_map->vf_vsi, len);
 	return 0;
 }
 
@@ -606,7 +606,7 @@ dcf_get_vlan_offload_caps_v2(struct ice_dcf_hw *hw)
 		return ret;
 	}
 
-	rte_memcpy(&hw->vlan_v2_caps, &vlan_v2_caps, sizeof(vlan_v2_caps));
+	memcpy(&hw->vlan_v2_caps, &vlan_v2_caps, sizeof(vlan_v2_caps));
 	return 0;
 }
 
@@ -799,7 +799,7 @@ ice_dcf_configure_rss_key(struct ice_dcf_hw *hw)
 
 	rss_key->vsi_id = hw->vsi_res->vsi_id;
 	rss_key->key_len = hw->vf_res->rss_key_size;
-	rte_memcpy(rss_key->key, hw->rss_key, hw->vf_res->rss_key_size);
+	memcpy(rss_key->key, hw->rss_key, hw->vf_res->rss_key_size);
 
 	args.v_op = VIRTCHNL_OP_CONFIG_RSS_KEY;
 	args.req_msglen = len;
@@ -831,7 +831,7 @@ ice_dcf_configure_rss_lut(struct ice_dcf_hw *hw)
 
 	rss_lut->vsi_id = hw->vsi_res->vsi_id;
 	rss_lut->lut_entries = hw->vf_res->rss_lut_size;
-	rte_memcpy(rss_lut->lut, hw->rss_lut, hw->vf_res->rss_lut_size);
+	memcpy(rss_lut->lut, hw->rss_lut, hw->vf_res->rss_lut_size);
 
 	args.v_op = VIRTCHNL_OP_CONFIG_RSS_LUT;
 	args.req_msglen = len;
@@ -881,7 +881,7 @@ ice_dcf_init_rss(struct ice_dcf_hw *hw)
 		for (i = 0; i < hw->vf_res->rss_key_size; i++)
 			hw->rss_key[i] = (uint8_t)rte_rand();
 	else
-		rte_memcpy(hw->rss_key, rss_conf->rss_key,
+		memcpy(hw->rss_key, rss_conf->rss_key,
 			   RTE_MIN(rss_conf->rss_key_len,
 				   hw->vf_res->rss_key_size));
 
@@ -1141,7 +1141,7 @@ ice_dcf_add_del_all_mac_addr(struct ice_dcf_hw *hw,
 		return -ENOMEM;
 	}
 
-	rte_memcpy(list->list[0].addr, addr->addr_bytes,
+	memcpy(list->list[0].addr, addr->addr_bytes,
 			sizeof(addr->addr_bytes));
 
 	PMD_DRV_LOG(DEBUG, "add/rm mac:" RTE_ETHER_ADDR_PRT_FMT,
diff --git a/drivers/net/ice/ice_dcf_ethdev.c b/drivers/net/ice/ice_dcf_ethdev.c
index 0da267db1f97..ec28f8583863 100644
--- a/drivers/net/ice/ice_dcf_ethdev.c
+++ b/drivers/net/ice/ice_dcf_ethdev.c
@@ -1326,7 +1326,7 @@ ice_dcf_dev_rss_reta_update(struct rte_eth_dev *dev,
 		return -ENOMEM;
 	}
 	/* store the old lut table temporarily */
-	rte_memcpy(lut, hw->rss_lut, reta_size);
+	memcpy(lut, hw->rss_lut, reta_size);
 
 	for (i = 0; i < reta_size; i++) {
 		idx = i / RTE_ETH_RETA_GROUP_SIZE;
@@ -1335,11 +1335,11 @@ ice_dcf_dev_rss_reta_update(struct rte_eth_dev *dev,
 			lut[i] = reta_conf[idx].reta[shift];
 	}
 
-	rte_memcpy(hw->rss_lut, lut, reta_size);
+	memcpy(hw->rss_lut, lut, reta_size);
 	/* send virtchnnl ops to configure rss*/
 	ret = ice_dcf_configure_rss_lut(hw);
 	if (ret) /* revert back */
-		rte_memcpy(hw->rss_lut, lut, reta_size);
+		memcpy(hw->rss_lut, lut, reta_size);
 	rte_free(lut);
 
 	return ret;
@@ -1396,7 +1396,7 @@ ice_dcf_dev_rss_hash_update(struct rte_eth_dev *dev,
 		return -EINVAL;
 	}
 
-	rte_memcpy(hw->rss_key, rss_conf->rss_key, rss_conf->rss_key_len);
+	memcpy(hw->rss_key, rss_conf->rss_key, rss_conf->rss_key_len);
 
 	return ice_dcf_configure_rss_key(hw);
 }
@@ -1418,7 +1418,7 @@ ice_dcf_dev_rss_hash_conf_get(struct rte_eth_dev *dev,
 		return 0;
 
 	rss_conf->rss_key_len = hw->vf_res->rss_key_size;
-	rte_memcpy(rss_conf->rss_key, hw->rss_key, rss_conf->rss_key_len);
+	memcpy(rss_conf->rss_key, hw->rss_key, rss_conf->rss_key_len);
 
 	return 0;
 }
diff --git a/drivers/net/ice/ice_dcf_parent.c b/drivers/net/ice/ice_dcf_parent.c
index 2f96dedcce1c..415caad3ac88 100644
--- a/drivers/net/ice/ice_dcf_parent.c
+++ b/drivers/net/ice/ice_dcf_parent.c
@@ -402,7 +402,7 @@ ice_dcf_load_pkg(struct ice_adapter *adapter)
 
 	use_dsn = ice_dcf_execute_virtchnl_cmd(&dcf_adapter->real_hw, &vc_cmd) == 0;
 	if (use_dsn)
-		rte_memcpy(&dsn, pkg_info.dsn, sizeof(dsn));
+		memcpy(&dsn, pkg_info.dsn, sizeof(dsn));
 
 	return ice_load_pkg(adapter, use_dsn, dsn);
 }
diff --git a/drivers/net/ice/ice_dcf_sched.c b/drivers/net/ice/ice_dcf_sched.c
index a231c1e60b2b..805e389b6699 100644
--- a/drivers/net/ice/ice_dcf_sched.c
+++ b/drivers/net/ice/ice_dcf_sched.c
@@ -307,8 +307,8 @@ ice_dcf_node_add(struct rte_eth_dev *dev, uint32_t node_id,
 		tm_node->id = node_id;
 		tm_node->parent = NULL;
 		tm_node->reference_count = 0;
-		rte_memcpy(&tm_node->params, params,
-				 sizeof(struct rte_tm_node_params));
+		memcpy(&tm_node->params, params,
+		       sizeof(struct rte_tm_node_params));
 		hw->tm_conf.root = tm_node;
 
 		return 0;
@@ -372,8 +372,7 @@ ice_dcf_node_add(struct rte_eth_dev *dev, uint32_t node_id,
 	tm_node->shaper_profile = shaper_profile;
 	tm_node->reference_count = 0;
 	tm_node->parent = parent_node;
-	rte_memcpy(&tm_node->params, params,
-			 sizeof(struct rte_tm_node_params));
+	memcpy(&tm_node->params, params, sizeof(struct rte_tm_node_params));
 	if (parent_node_type == ICE_DCF_TM_NODE_TYPE_PORT) {
 		TAILQ_INSERT_TAIL(&hw->tm_conf.tc_list,
 				  tm_node, node);
@@ -518,8 +517,8 @@ ice_dcf_shaper_profile_add(struct rte_eth_dev *dev,
 	if (!shaper_profile)
 		return -ENOMEM;
 	shaper_profile->shaper_profile_id = shaper_profile_id;
-	rte_memcpy(&shaper_profile->profile, profile,
-			 sizeof(struct rte_tm_shaper_params));
+	memcpy(&shaper_profile->profile, profile,
+	       sizeof(struct rte_tm_shaper_params));
 	TAILQ_INSERT_TAIL(&hw->tm_conf.shaper_profile_list,
 			  shaper_profile, node);
 
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 35ab542e61d4..04c70a7a7837 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -3224,7 +3224,7 @@ ice_get_default_rss_key(uint8_t *rss_key, uint32_t rss_key_size)
 			key[i] = (uint8_t)rte_rand();
 		default_key_done = true;
 	}
-	rte_memcpy(rss_key, key, RTE_MIN(rss_key_size, sizeof(default_key)));
+	memcpy(rss_key, key, RTE_MIN(rss_key_size, sizeof(default_key)));
 }
 
 static int ice_init_rss(struct ice_pf *pf)
@@ -3278,11 +3278,10 @@ static int ice_init_rss(struct ice_pf *pf)
 	if (!rss_conf->rss_key)
 		ice_get_default_rss_key(vsi->rss_key, vsi->rss_key_size);
 	else
-		rte_memcpy(vsi->rss_key, rss_conf->rss_key,
-			   RTE_MIN(rss_conf->rss_key_len,
-				   vsi->rss_key_size));
+		memcpy(vsi->rss_key, rss_conf->rss_key,
+		       RTE_MIN(rss_conf->rss_key_len, vsi->rss_key_size));
 
-	rte_memcpy(key.standard_rss_key, vsi->rss_key, vsi->rss_key_size);
+	memcpy(key.standard_rss_key, vsi->rss_key, vsi->rss_key_size);
 	ret = ice_aq_set_rss_key(hw, vsi->idx, &key);
 	if (ret)
 		goto out;
@@ -4244,7 +4243,7 @@ ice_vsi_config_vlan_filter(struct ice_vsi *vsi, bool on)
 		vsi->info.sw_flags2 &= ~sw_flags2;
 
 	vsi->info.sw_id = hw->port_info->sw_id;
-	(void)rte_memcpy(&ctxt.info, &vsi->info, sizeof(vsi->info));
+	ctxt.info = vsi->info;
 	ctxt.info.valid_sections =
 		rte_cpu_to_le_16(ICE_AQ_VSI_PROP_SW_VALID |
 				 ICE_AQ_VSI_PROP_SECURITY_VALID);
@@ -4895,7 +4894,7 @@ ice_vsi_vlan_pvid_set(struct ice_vsi *vsi, struct ice_vsi_vlan_pvid_info *info)
 				  ICE_AQ_VSI_INNER_VLAN_EMODE_M);
 	vsi->info.inner_vlan_flags |= vlan_flags;
 	memset(&ctxt, 0, sizeof(ctxt));
-	rte_memcpy(&ctxt.info, &vsi->info, sizeof(vsi->info));
+	memcpy(&ctxt.info, &vsi->info, sizeof(vsi->info));
 	ctxt.info.valid_sections =
 		rte_cpu_to_le_16(ICE_AQ_VSI_PROP_VLAN_VALID);
 	ctxt.vsi_num = vsi->vsi_id;
diff --git a/drivers/net/ice/ice_fdir_filter.c b/drivers/net/ice/ice_fdir_filter.c
index 7914ba940731..2ce1cc82927c 100644
--- a/drivers/net/ice/ice_fdir_filter.c
+++ b/drivers/net/ice/ice_fdir_filter.c
@@ -1195,7 +1195,7 @@ ice_fdir_add_del_raw(struct ice_pf *pf,
 	struct ice_hw *hw = ICE_PF_TO_HW(pf);
 
 	unsigned char *pkt = (unsigned char *)pf->fdir.prg_pkt;
-	rte_memcpy(pkt, filter->pkt_buf, filter->pkt_len);
+	memcpy(pkt, filter->pkt_buf, filter->pkt_len);
 
 	struct ice_fltr_desc desc;
 	memset(&desc, 0, sizeof(desc));
@@ -1242,13 +1242,13 @@ ice_fdir_extract_fltr_key(struct ice_fdir_fltr_pattern *key,
 	memset(key, 0, sizeof(*key));
 
 	key->flow_type = input->flow_type;
-	rte_memcpy(&key->ip, &input->ip, sizeof(key->ip));
-	rte_memcpy(&key->mask, &input->mask, sizeof(key->mask));
-	rte_memcpy(&key->ext_data, &input->ext_data, sizeof(key->ext_data));
-	rte_memcpy(&key->ext_mask, &input->ext_mask, sizeof(key->ext_mask));
+	memcpy(&key->ip, &input->ip, sizeof(key->ip));
+	memcpy(&key->mask, &input->mask, sizeof(key->mask));
+	memcpy(&key->ext_data, &input->ext_data, sizeof(key->ext_data));
+	memcpy(&key->ext_mask, &input->ext_mask, sizeof(key->ext_mask));
 
-	rte_memcpy(&key->gtpu_data, &input->gtpu_data, sizeof(key->gtpu_data));
-	rte_memcpy(&key->gtpu_mask, &input->gtpu_mask, sizeof(key->gtpu_mask));
+	memcpy(&key->gtpu_data, &input->gtpu_data, sizeof(key->gtpu_data));
+	memcpy(&key->gtpu_mask, &input->gtpu_mask, sizeof(key->gtpu_mask));
 
 	key->tunnel_type = filter->tunnel_type;
 }
@@ -1376,7 +1376,7 @@ ice_fdir_create_filter(struct ice_adapter *ad,
 		if (!entry)
 			goto error;
 
-		rte_memcpy(entry, filter, sizeof(*filter));
+		memcpy(entry, filter, sizeof(*filter));
 
 		flow->rule = entry;
 
@@ -1437,7 +1437,7 @@ ice_fdir_create_filter(struct ice_adapter *ad,
 	if (filter->mark_flag == 1)
 		ice_fdir_rx_parsing_enable(ad, 1);
 
-	rte_memcpy(entry, filter, sizeof(*entry));
+	memcpy(entry, filter, sizeof(*entry));
 	ret = ice_fdir_entry_insert(pf, entry, &key);
 	if (ret) {
 		rte_flow_error_set(error, -ret,
@@ -1738,7 +1738,7 @@ ice_fdir_parse_action(struct ice_adapter *ad,
 
 			act_count = actions->conf;
 			filter->input.cnt_ena = ICE_FXD_FLTR_QW0_STAT_ENA_PKTS;
-			rte_memcpy(&filter->act_count, act_count,
+			memcpy(&filter->act_count, act_count,
 						sizeof(filter->act_count));
 
 			break;
@@ -1951,7 +1951,7 @@ ice_fdir_parse_pattern(__rte_unused struct ice_adapter *ad,
 			u8 *pkt_buf = (u8 *)ice_malloc(&ad->hw, pkt_len + 1);
 			if (!pkt_buf)
 				return -ENOMEM;
-			rte_memcpy(pkt_buf, tmp_spec, pkt_len);
+			memcpy(pkt_buf, tmp_spec, pkt_len);
 			filter->pkt_buf = pkt_buf;
 
 			filter->pkt_len = pkt_len;
@@ -1996,11 +1996,11 @@ ice_fdir_parse_pattern(__rte_unused struct ice_adapter *ad,
 			p_ext_data = (tunnel_type && is_outer) ?
 				     &filter->input.ext_data_outer :
 				     &filter->input.ext_data;
-			rte_memcpy(&p_ext_data->src_mac,
+			memcpy(&p_ext_data->src_mac,
 				   &eth_spec->src, RTE_ETHER_ADDR_LEN);
-			rte_memcpy(&p_ext_data->dst_mac,
+			memcpy(&p_ext_data->dst_mac,
 				   &eth_spec->dst, RTE_ETHER_ADDR_LEN);
-			rte_memcpy(&p_ext_data->ether_type,
+			memcpy(&p_ext_data->ether_type,
 				   &eth_spec->type, sizeof(eth_spec->type));
 			break;
 		case RTE_FLOW_ITEM_TYPE_IPV4:
@@ -2126,8 +2126,8 @@ ice_fdir_parse_pattern(__rte_unused struct ice_adapter *ad,
 			if (ipv6_mask->hdr.hop_limits == UINT8_MAX)
 				*input_set |= ICE_INSET_IPV6_HOP_LIMIT;
 
-			rte_memcpy(&p_v6->dst_ip, ipv6_spec->hdr.dst_addr, 16);
-			rte_memcpy(&p_v6->src_ip, ipv6_spec->hdr.src_addr, 16);
+			memcpy(&p_v6->dst_ip, ipv6_spec->hdr.dst_addr, 16);
+			memcpy(&p_v6->src_ip, ipv6_spec->hdr.src_addr, 16);
 			vtc_flow_cpu = rte_be_to_cpu_32(ipv6_spec->hdr.vtc_flow);
 			p_v6->tc = (uint8_t)(vtc_flow_cpu >> ICE_FDIR_IPV6_TC_OFFSET);
 			p_v6->proto = ipv6_spec->hdr.proto;
diff --git a/drivers/net/ice/ice_tm.c b/drivers/net/ice/ice_tm.c
index 34a0bfcff875..82ae2a740728 100644
--- a/drivers/net/ice/ice_tm.c
+++ b/drivers/net/ice/ice_tm.c
@@ -330,7 +330,7 @@ ice_shaper_profile_add(struct rte_eth_dev *dev,
 	if (!shaper_profile)
 		return -ENOMEM;
 	shaper_profile->shaper_profile_id = shaper_profile_id;
-	rte_memcpy(&shaper_profile->profile, profile,
+	memcpy(&shaper_profile->profile, profile,
 			 sizeof(struct rte_tm_shaper_params));
 	TAILQ_INSERT_TAIL(&pf->tm_conf.shaper_profile_list,
 			  shaper_profile, node);
@@ -448,8 +448,8 @@ ice_tm_node_add(struct rte_eth_dev *dev, uint32_t node_id,
 		tm_node->reference_count = 0;
 		tm_node->children = (struct ice_tm_node **)
 			rte_calloc(NULL, 256, (sizeof(struct ice_tm_node *)), 0);
-		rte_memcpy(&tm_node->params, params,
-				 sizeof(struct rte_tm_node_params));
+		memcpy(&tm_node->params, params,
+		       sizeof(struct rte_tm_node_params));
 		pf->tm_conf.root = tm_node;
 		return 0;
 	}
-- 
2.35.1

[RFC 4/8] test/ipfrag: fix gcc-12 warnings

[Stephen Hemminger]

Using rte_memcpy is not necessary here. It also causes warnings
from derefencing past end of input.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_ipfrag.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
index dc62b0e5475b..ba0ffd080604 100644
--- a/app/test/test_ipfrag.c
+++ b/app/test/test_ipfrag.c
@@ -23,7 +23,6 @@ test_ipfrag(void)
 
 #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 @@ test_get_ipv4_opt(bool is_first_frag, bool opt_copied,
 		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 @@ test_get_ipv4_opt(bool is_first_frag, bool opt_copied,
 		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 @@ v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s,
 	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 @@ test_get_frag_opt(struct rte_mbuf **mb, int32_t num,
 				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,
-- 
2.35.1

[RFC 5/8] test/ipsec: fix gcc-12 rte_memcpy warnings

[Stephen Hemminger]

It is not necessary to use rte_memcpy here, and it can cause
warnings when gcc-12 detects that rte_memcpy will derference past
input data.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_ipsec.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/app/test/test_ipsec.c b/app/test/test_ipsec.c
index 8da025bf6621..4aa9b346646d 100644
--- a/app/test/test_ipsec.c
+++ b/app/test/test_ipsec.c
@@ -10,7 +10,6 @@
 #include <rte_hexdump.h>
 #include <rte_mbuf.h>
 #include <rte_malloc.h>
-#include <rte_memcpy.h>
 #include <rte_cycles.h>
 #include <rte_bus_vdev.h>
 #include <rte_ip.h>
@@ -569,7 +568,7 @@ setup_test_string(struct rte_mempool *mpool,
 			return NULL;
 		}
 		if (string != NULL)
-			rte_memcpy(dst, string, t_len);
+			memcpy(dst, string, t_len);
 		else
 			memset(dst, 0, t_len);
 	}
@@ -614,21 +613,21 @@ setup_test_string_tunneled(struct rte_mempool *mpool, const char *string,
 	/* copy outer IP and ESP header */
 	ipv4_outer.total_length = rte_cpu_to_be_16(t_len);
 	ipv4_outer.packet_id = rte_cpu_to_be_16(seq);
-	rte_memcpy(dst, &ipv4_outer, sizeof(ipv4_outer));
+	memcpy(dst, &ipv4_outer, sizeof(ipv4_outer));
 	dst += sizeof(ipv4_outer);
 	m->l3_len = sizeof(ipv4_outer);
-	rte_memcpy(dst, &esph, sizeof(esph));
+	memcpy(dst, &esph, sizeof(esph));
 	dst += sizeof(esph);
 
 	if (string != NULL) {
 		/* copy payload */
-		rte_memcpy(dst, string, len);
+		memcpy(dst, string, len);
 		dst += len;
 		/* copy pad bytes */
-		rte_memcpy(dst, esp_pad_bytes, padlen);
+		memcpy(dst, esp_pad_bytes, padlen);
 		dst += padlen;
 		/* copy ESP tail header */
-		rte_memcpy(dst, &espt, sizeof(espt));
+		memcpy(dst, &espt, sizeof(espt));
 	} else
 		memset(dst, 0, t_len);
 
-- 
2.35.1

[RFC 6/8] net/enetfc: fix array out of bounds warning

[Stephen Hemminger]

With gcc-12 it detects that this function could be passed a
queue > 1 which would cause out of bounds access.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/enetfec/enet_ethdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/enetfec/enet_ethdev.c b/drivers/net/enetfec/enet_ethdev.c
index 714f8ac7eccc..4956235a10a6 100644
--- a/drivers/net/enetfec/enet_ethdev.c
+++ b/drivers/net/enetfec/enet_ethdev.c
@@ -454,6 +454,11 @@ enetfec_rx_queue_setup(struct rte_eth_dev *dev,
 		return -EINVAL;
 	}
 
+	if (queue_idx >= ENETFEC_MAX_Q) {
+		ENETFEC_PMD_ERR("Only %u receive queues supported", ENETFEC_MAX_Q);
+		return -EINVAL;
+	}
+
 	/* allocate receive queue */
 	rxq = rte_zmalloc(NULL, sizeof(*rxq), RTE_CACHE_LINE_SIZE);
 	if (rxq == NULL) {
-- 
2.35.1

[RFC 7/8] vhost: replace rte_memcpy to fix warning

[Stephen Hemminger]

Using rte_memcpy is not needed here. Just use memcpy() which
is safer and just as fast for this non-critical place.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/vhost/vhost_crypto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vhost/vhost_crypto.c b/lib/vhost/vhost_crypto.c
index b1c0eb6a0f97..e8727f076fdd 100644
--- a/lib/vhost/vhost_crypto.c
+++ b/lib/vhost/vhost_crypto.c
@@ -585,7 +585,7 @@ copy_data(void *dst_data, struct vhost_crypto_data_req *vc_req,
 	if (unlikely(!src || !dlen))
 		return -1;
 
-	rte_memcpy((uint8_t *)data, src, dlen);
+	memcpy(data, src, dlen);
 	data += dlen;
 
 	if (unlikely(dlen < to_copy)) {
-- 
2.35.1

[RFC 8/8] ip_frag: fix gcc-12 warnings

[Stephen Hemminger]

The function rte_memcpy can derference past source buffer which
will cause array out of bounds warnings. But there is no good reason
to use rte_memcpy instead of memcpy in this code. Memcpy is just
as fast for these small inputs, and compiler will optimize.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/ip_frag/rte_ipv4_fragmentation.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
index a19f6fda6408..27a8ad224dec 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];
 		}
-- 
2.35.1

Re: [RFC 8/8] ip_frag: fix gcc-12 warnings

[Konstantin Ananyev]

07/06/2022 18:17, Stephen Hemminger пишет:
> The function rte_memcpy can derference past source buffer which
> will cause array out of bounds warnings. But there is no good reason
> to use rte_memcpy instead of memcpy in this code. Memcpy is just
> as fast for these small inputs, and compiler will optimize.


AFAIK, rte_memcpy() will outperform memcpy() when _size_ parameter
is a variable. Unfortunately that's exactly the case here.
So not sure it is a good change, at least without extensive perf testing.
BTW, if rte_memcpy() really access src buffer beyond it's boundaries,
I think that's definitely a bug that needs to be fixed.


> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   lib/ip_frag/rte_ipv4_fragmentation.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
> index a19f6fda6408..27a8ad224dec 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];
>   		}

Re: [RFC 8/8] ip_frag: fix gcc-12 warnings

[Stephen Hemminger]

On Wed, 8 Jun 2022 09:19:20 +0100
Konstantin Ananyev <konstantin.v.ananyev@yandex.ru> wrote:

> 07/06/2022 18:17, Stephen Hemminger пишет:
> > The function rte_memcpy can derference past source buffer which
> > will cause array out of bounds warnings. But there is no good reason
> > to use rte_memcpy instead of memcpy in this code. Memcpy is just
> > as fast for these small inputs, and compiler will optimize.  
> 
> 
> AFAIK, rte_memcpy() will outperform memcpy() when _size_ parameter
> is a variable. Unfortunately that's exactly the case here.
> So not sure it is a good change, at least without extensive perf testing.
> BTW, if rte_memcpy() really access src buffer beyond it's boundaries,
> I think that's definitely a bug that needs to be fixed.

Yes and no.
IMHO DPDK should not in the C library business, and glibc etc should be
more optimized if necessary.


The ip_frag warning with rte_memcpy in full is:

[296/3606] Compiling C object lib/libr...a.p/ip_frag_rte_ipv4_fragmentation.c.o
In file included from /usr/lib/gcc/x86_64-linux-gnu/12/include/immintrin.h:43,
                 from /usr/lib/gcc/x86_64-linux-gnu/12/include/x86intrin.h:32,
                 from ../lib/eal/x86/include/rte_vect.h:31,
                 from ../lib/eal/x86/include/rte_memcpy.h:17,
                 from ../lib/ip_frag/rte_ipv4_fragmentation.c:8:
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_mov128’ at ../lib/eal/x86/include/rte_memcpy.h:369:2,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:445:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘__create_ipopt_frag_hdr’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:935:8: warning: array subscript ‘__m256i_u[1]’ is partly outside array bounds of ‘uint8_t[60]’ {aka ‘unsigned char[60]’} [-Warray-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [52, 60] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
      |                 ^~~~~~~~~~~~~~
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_mov128’ at ../lib/eal/x86/include/rte_memcpy.h:370:2,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:445:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘__create_ipopt_frag_hdr’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:935:8: warning: array subscript [2, 3] is outside array bounds of ‘uint8_t[60]’ {aka ‘unsigned char[60]’} [-Warray-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [84, 124] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
      |                 ^~~~~~~~~~~~~~
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_mov128’ at ../lib/eal/x86/include/rte_memcpy.h:371:2,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:445:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘__create_ipopt_frag_hdr’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:935:8: warning: array subscript [3, 4] is outside array bounds of ‘uint8_t[60]’ {aka ‘unsigned char[60]’} [-Warray-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [116, 156] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
      |                 ^~~~~~~~~~~~~~
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_mov64’ at ../lib/eal/x86/include/rte_memcpy.h:358:2,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:452:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘__create_ipopt_frag_hdr’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:935:8: warning: array subscript ‘__m256i_u[1]’ is partly outside array bounds of ‘void[60]’ [-Warray-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [180, 240] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
      |                 ^~~~~~~~~~~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [52, 60] into object ‘ipopt_frag_hdr’ of size 60
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:457:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘__create_ipopt_frag_hdr’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:935:8: warning: array subscript [2, 7] is outside array bounds of ‘void[60]’ [-Warray-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [148, 272] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
      |                 ^~~~~~~~~~~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [148, 272] into object ‘ipopt_frag_hdr’ of size 60
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [20, 60] into object ‘ipopt_frag_hdr’ of size 60
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:458:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘__create_ipopt_frag_hdr’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:935:8: warning: array subscript [2, 8] is outside array bounds of ‘void[60]’ [-Warray-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [149, 273] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
      |                 ^~~~~~~~~~~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [149, 273] into object ‘ipopt_frag_hdr’ of size 60
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [21, 60] into object ‘ipopt_frag_hdr’ of size 60
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_memcpy_generic’ at ../lib/eal/x86/include/rte_memcpy.h:438:3,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:853:10,
    inlined from ‘__create_ipopt_frag_hdr’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at ../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-linux-gnu/12/include/avxintrin.h:935:8: warning: array subscript ‘__m256i_u[1]’ is partly outside array bounds of ‘uint8_t[60]’ {aka ‘unsigned char[60]’} [-Warray-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [37, 60] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];

RE: [RFC 8/8] ip_frag: fix gcc-12 warnings

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, 8 June 2022 17.27
> 
> On Wed, 8 Jun 2022 09:19:20 +0100
> Konstantin Ananyev <konstantin.v.ananyev@yandex.ru> wrote:
> 
> > 07/06/2022 18:17, Stephen Hemminger пишет:
> > > The function rte_memcpy can derference past source buffer which
> > > will cause array out of bounds warnings. But there is no good
> reason
> > > to use rte_memcpy instead of memcpy in this code. Memcpy is just
> > > as fast for these small inputs, and compiler will optimize.
> >
> >
> > AFAIK, rte_memcpy() will outperform memcpy() when _size_ parameter
> > is a variable. Unfortunately that's exactly the case here.
> > So not sure it is a good change, at least without extensive perf
> testing.
> > BTW, if rte_memcpy() really access src buffer beyond it's boundaries,
> > I think that's definitely a bug that needs to be fixed.
> 
> Yes and no.
> IMHO DPDK should not in the C library business, and glibc etc should be
> more optimized if necessary.

A very big +1 to that!

DPDK contains a lot of optimizations that really belong in the compiler and/or C library, but weren't back then, so the clever DPDK developers put them inside DPDK instead.

Over time, the compilers and C libraries have improved, and many of these manually implemented optimizations have become obsolete. They should be cleaned up and replaced by simpler code, and the documentation about optimizing code should be updated accordingly.

Until that happens, we have to expect contributors to use rte_memcpy() and other obsolete optimizations - they are only doing what the DPDK documentation and reference code tells them. Just like application developers are using KNI, because it is so heavily promoted in DPDK documentation.

The DPDK community has a very high focus on the risk of performance regressions when touching DPDK Core libraries, so a general cleaning is probably not going to happen. Luckily, there are exceptions to every rule, such as Georg Sauthoff's patch removing the manual loop unroll in __rte_raw_cksum() [1], which allowed the compiler to generate something better.

I guess that "if it isn't broken, don't fix it" applies to DPDK Core libraries too. ;-)


PS: A funny example of an exotic optimization is the use of Duff's Device in rte_pktmbuf_alloc_bulk() [2]; a C implementation of an optimization for assembler code.

[1] http://inbox.dpdk.org/dev/20211017203718.801998-2-mail@gms.tf/
[2] https://elixir.bootlin.com/dpdk/latest/source/lib/mbuf/rte_mbuf.h#L893

Re: [RFC 8/8] ip_frag: fix gcc-12 warnings

[Thomas Monjalon]

09/06/2022 09:09, Morten Brørup:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Wednesday, 8 June 2022 17.27
> > 
> > On Wed, 8 Jun 2022 09:19:20 +0100
> > Konstantin Ananyev <konstantin.v.ananyev@yandex.ru> wrote:
> > 
> > > 07/06/2022 18:17, Stephen Hemminger пишет:
> > > > The function rte_memcpy can derference past source buffer which
> > > > will cause array out of bounds warnings. But there is no good
> > reason
> > > > to use rte_memcpy instead of memcpy in this code. Memcpy is just
> > > > as fast for these small inputs, and compiler will optimize.
> > >
> > >
> > > AFAIK, rte_memcpy() will outperform memcpy() when _size_ parameter
> > > is a variable. Unfortunately that's exactly the case here.
> > > So not sure it is a good change, at least without extensive perf
> > testing.
> > > BTW, if rte_memcpy() really access src buffer beyond it's boundaries,
> > > I think that's definitely a bug that needs to be fixed.
> > 
> > Yes and no.
> > IMHO DPDK should not in the C library business, and glibc etc should be
> > more optimized if necessary.
> 
> A very big +1 to that!
> 
> DPDK contains a lot of optimizations that really belong in the compiler and/or C library, but weren't back then, so the clever DPDK developers put them inside DPDK instead.
> 
> Over time, the compilers and C libraries have improved, and many of these manually implemented optimizations have become obsolete. They should be cleaned up and replaced by simpler code, and the documentation about optimizing code should be updated accordingly.
> 
> Until that happens, we have to expect contributors to use rte_memcpy() and other obsolete optimizations - they are only doing what the DPDK documentation and reference code tells them. Just like application developers are using KNI, because it is so heavily promoted in DPDK documentation.
> 
> The DPDK community has a very high focus on the risk of performance regressions when touching DPDK Core libraries, so a general cleaning is probably not going to happen. Luckily, there are exceptions to every rule, such as Georg Sauthoff's patch removing the manual loop unroll in __rte_raw_cksum() [1], which allowed the compiler to generate something better.
> 
> I guess that "if it isn't broken, don't fix it" applies to DPDK Core libraries too. ;-)

No it doesn't apply, the only limitation is the number of contributions.
Feel free to propose cleanups.