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 > > Signed-off-by: Stephen Hemminger <step...@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 >
