On Wed, 8 Jun 2022 14:29:58 +0200 Michał Krawczyk <m...@semihalf.com> wrote:
> wt., 7 cze 2022 o 19:17 Stephen Hemminger <step...@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]; | ^~~~~~~~~~~
