> > > > Hi, > > > >> Current hitmask includes padding due to Intel's SIMD > >> implementation detail. This patch allows non Intel SIMD > >> implementations to benefit from a dense hitmask. > >> In addition, the new dense hitmask interweave the primary > >> and secondary matches which allow a better cache usage and > >> enable future improvements for the SIMD implementations > >> > >> Signed-off-by: Yoan Picchi <yoan.picchi@arm.com> > >> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> > >> Reviewed-by: Nathan Brown <nathan.brown@arm.com> > >> --- > >> .mailmap | 2 + > >> lib/hash/arch/arm/compare_signatures.h | 61 +++++++ > >> lib/hash/arch/common/compare_signatures.h | 38 +++++ > >> lib/hash/arch/x86/compare_signatures.h | 53 ++++++ > >> lib/hash/rte_cuckoo_hash.c | 192 ++++++++++++---------- > >> 5 files changed, 255 insertions(+), 91 deletions(-) > >> create mode 100644 lib/hash/arch/arm/compare_signatures.h > >> create mode 100644 lib/hash/arch/common/compare_signatures.h > >> create mode 100644 lib/hash/arch/x86/compare_signatures.h > >> > >> diff --git a/.mailmap b/.mailmap > >> index 66ebc20666..00b50414d3 100644 > >> --- a/.mailmap > >> +++ b/.mailmap > >> @@ -494,6 +494,7 @@ Hari Kumar Vemula <hari.kumarx.vemula@intel.com> > >> Harini Ramakrishnan <harini.ramakrishnan@microsoft.com> > >> Hariprasad Govindharajan <hariprasad.govindharajan@intel.com> > >> Harish Patil <harish.patil@cavium.com> <harish.patil@qlogic.com> > >> +Harjot Singh <harjot.singh@arm.com> > >> Harman Kalra <hkalra@marvell.com> > >> Harneet Singh <harneet.singh@intel.com> > >> Harold Huang <baymaxhuang@gmail.com> > >> @@ -1633,6 +1634,7 @@ Yixue Wang <yixue.wang@intel.com> > >> Yi Yang <yangyi01@inspur.com> <yi.y.yang@intel.com> > >> Yi Zhang <zhang.yi75@zte.com.cn> > >> Yoann Desmouceaux <ydesmouc@cisco.com> > >> +Yoan Picchi <yoan.picchi@arm.com> > >> Yogesh Jangra <yogesh.jangra@intel.com> > >> Yogev Chaimovich <yogev@cgstowernetworks.com> > >> Yongjie Gu <yongjiex.gu@intel.com> > >> diff --git a/lib/hash/arch/arm/compare_signatures.h b/lib/hash/arch/arm/compare_signatures.h > >> new file mode 100644 > >> index 0000000000..1af6ba8190 > >> --- /dev/null > >> +++ b/lib/hash/arch/arm/compare_signatures.h > >> @@ -0,0 +1,61 @@ > >> +/* SPDX-License-Identifier: BSD-3-Clause > >> + * Copyright(c) 2010-2016 Intel Corporation > >> + * Copyright(c) 2018-2024 Arm Limited > >> + */ > >> + > >> +/* > >> + * Arm's version uses a densely packed hitmask buffer: > >> + * Every bit is in use. > >> + */ > >> + > >> +#include <inttypes.h> > >> +#include <rte_common.h> > >> +#include <rte_vect.h> > >> +#include "rte_cuckoo_hash.h" > >> + > >> +#define DENSE_HASH_BULK_LOOKUP 1 > >> + > >> +static inline void > >> +compare_signatures_dense(uint16_t *hitmask_buffer, > >> + const uint16_t *prim_bucket_sigs, > >> + const uint16_t *sec_bucket_sigs, > >> + uint16_t sig, > >> + enum rte_hash_sig_compare_function sig_cmp_fn) > >> +{ > >> + > >> + static_assert(sizeof(*hitmask_buffer) >= 2*(RTE_HASH_BUCKET_ENTRIES/8), > >> + "The hitmask must be exactly wide enough to accept the whole hitmask if it is dense"); > >> + > >> + /* For match mask every bits indicates the match */ > >> + switch (sig_cmp_fn) { > >> +#if RTE_HASH_BUCKET_ENTRIES <= 8 > >> + case RTE_HASH_COMPARE_NEON: { > >> + uint16x8_t vmat, vsig, x; > >> + int16x8_t shift = {0, 1, 2, 3, 4, 5, 6, 7}; > >> + uint16_t low, high; > >> + > >> + vsig = vld1q_dup_u16((uint16_t const *)&sig); > >> + /* Compare all signatures in the primary bucket */ > >> + vmat = vceqq_u16(vsig, > >> + vld1q_u16((uint16_t const *)prim_bucket_sigs)); > >> + x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift); > >> + low = (uint16_t)(vaddvq_u16(x)); > >> + /* Compare all signatures in the secondary bucket */ > >> + vmat = vceqq_u16(vsig, > >> + vld1q_u16((uint16_t const *)sec_bucket_sigs)); > >> + x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift); > >> + high = (uint16_t)(vaddvq_u16(x)); > >> + *hitmask_buffer = low | high << RTE_HASH_BUCKET_ENTRIES; > >> + > >> + } > >> + break; > >> +#endif > >> + default: > >> + for (unsigned int i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) { > >> + *hitmask_buffer |= > >> + ((sig == prim_bucket_sigs[i]) << i); > >> + *hitmask_buffer |= > >> + ((sig == sec_bucket_sigs[i]) << i) << RTE_HASH_BUCKET_ENTRIES; > >> + } > >> + } > >> +} > >> diff --git a/lib/hash/arch/common/compare_signatures.h b/lib/hash/arch/common/compare_signatures.h > >> new file mode 100644 > >> index 0000000000..dcf9444032 > >> --- /dev/null > >> +++ b/lib/hash/arch/common/compare_signatures.h > >> @@ -0,0 +1,38 @@ > >> +/* SPDX-License-Identifier: BSD-3-Clause > >> + * Copyright(c) 2010-2016 Intel Corporation > >> + * Copyright(c) 2018-2024 Arm Limited > >> + */ > >> + > >> +/* > >> + * The generic version could use either a dense or sparsely packed hitmask buffer, > >> + * but the dense one is slightly faster. > >> + */ > >> + > >> +#include <inttypes.h> > >> +#include <rte_common.h> > >> +#include <rte_vect.h> > >> +#include "rte_cuckoo_hash.h" > >> + > >> +#define DENSE_HASH_BULK_LOOKUP 1 > >> + > >> +static inline void > >> +compare_signatures_dense(uint16_t *hitmask_buffer, > >> + const uint16_t *prim_bucket_sigs, > >> + const uint16_t *sec_bucket_sigs, > >> + uint16_t sig, > >> + enum rte_hash_sig_compare_function sig_cmp_fn) > >> +{ > >> + (void) sig_cmp_fn; > >> + > >> + static_assert(sizeof(*hitmask_buffer) >= 2*(RTE_HASH_BUCKET_ENTRIES/8), > >> + "The hitmask must be exactly wide enough to accept the whole hitmask if it is dense"); > >> + > >> + /* For match mask every bits indicates the match */ > >> + for (unsigned int i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) { > >> + *hitmask_buffer |= > >> + ((sig == prim_bucket_sigs[i]) << i); > >> + *hitmask_buffer |= > >> + ((sig == sec_bucket_sigs[i]) << i) << RTE_HASH_BUCKET_ENTRIES; > >> + } > >> + > >> +} > > > > Thanks for re-factoring compare_signatures_...() code, it looks much cleaner that way. > > One question I have - does it mean that now for x86 we always use 'sparse' while for all other > > ARM and non-ARM platforms we switch to 'dense'? > > Yes it does. x86 support only the sparse method (the legacy one). Arm > and generic code could support both dense and sparse. The reason I made > them use the dense method is because it was slightly faster in my tests. Ok, but before that, a 'generic' one (non-x86 and non-ARM) used 'sparse' one, correct? If so, then probably need to outline it a bit more in patch comments and might be even release notes. At least that would be my expectations, probably hash lib maintainers need to say what is the best way here. The code refactoring itself - LGTM. > (no need to add padding and shifts amongst other benefit.) > > > > >> diff --git a/lib/hash/arch/x86/compare_signatures.h b/lib/hash/arch/x86/compare_signatures.h > >> new file mode 100644 > >> index 0000000000..7eec499e1f > >> --- /dev/null > >> +++ b/lib/hash/arch/x86/compare_signatures.h > >> @@ -0,0 +1,53 @@ > >> +/* SPDX-License-Identifier: BSD-3-Clause > >> + * Copyright(c) 2010-2016 Intel Corporation > >> + * Copyright(c) 2018-2024 Arm Limited > >> + */ > >> + > >> +/* > >> + * x86's version uses a sparsely packed hitmask buffer: > >> + * Every other bit is padding. > >> + */ > >> + > >> +#include <inttypes.h> > >> +#include <rte_common.h> > >> +#include <rte_vect.h> > >> +#include "rte_cuckoo_hash.h" > >> + > >> +#define DENSE_HASH_BULK_LOOKUP 0 > >> + > >> +static inline void > >> +compare_signatures_sparse(uint32_t *prim_hash_matches, uint32_t *sec_hash_matches, > >> + const struct rte_hash_bucket *prim_bkt, > >> + const struct rte_hash_bucket *sec_bkt, > >> + uint16_t sig, > >> + enum rte_hash_sig_compare_function sig_cmp_fn) > >> +{ > >> + /* For match mask the first bit of every two bits indicates the match */ > >> + switch (sig_cmp_fn) { > >> +#if defined(__SSE2__) && RTE_HASH_BUCKET_ENTRIES <= 8 > >> + case RTE_HASH_COMPARE_SSE: > >> + /* Compare all signatures in the bucket */ > >> + *prim_hash_matches = _mm_movemask_epi8(_mm_cmpeq_epi16( > >> + _mm_load_si128( > >> + (__m128i const *)prim_bkt->sig_current), > >> + _mm_set1_epi16(sig))); > >> + /* Extract the even-index bits only */ > >> + *prim_hash_matches &= 0x5555; > >> + /* Compare all signatures in the bucket */ > >> + *sec_hash_matches = _mm_movemask_epi8(_mm_cmpeq_epi16( > >> + _mm_load_si128( > >> + (__m128i const *)sec_bkt->sig_current), > >> + _mm_set1_epi16(sig))); > >> + /* Extract the even-index bits only */ > >> + *sec_hash_matches &= 0x5555; > >> + break; > >> +#endif /* defined(__SSE2__) */ > >> + default: > >> + for (unsigned int i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) { > >> + *prim_hash_matches |= > >> + ((sig == prim_bkt->sig_current[i]) << (i << 1)); > >> + *sec_hash_matches |= > >> + ((sig == sec_bkt->sig_current[i]) << (i << 1)); > >> + } > >> + } > >> +}
On 3/19/24 10:41, Konstantin Ananyev wrote: > > Hi, > >> Current hitmask includes padding due to Intel's SIMD >> implementation detail. This patch allows non Intel SIMD >> implementations to benefit from a dense hitmask. >> In addition, the new dense hitmask interweave the primary >> and secondary matches which allow a better cache usage and >> enable future improvements for the SIMD implementations >> >> Signed-off-by: Yoan Picchi <yoan.picchi@arm.com> >> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> >> Reviewed-by: Nathan Brown <nathan.brown@arm.com> >> --- >> .mailmap | 2 + >> lib/hash/arch/arm/compare_signatures.h | 61 +++++++ >> lib/hash/arch/common/compare_signatures.h | 38 +++++ >> lib/hash/arch/x86/compare_signatures.h | 53 ++++++ >> lib/hash/rte_cuckoo_hash.c | 192 ++++++++++++---------- >> 5 files changed, 255 insertions(+), 91 deletions(-) >> create mode 100644 lib/hash/arch/arm/compare_signatures.h >> create mode 100644 lib/hash/arch/common/compare_signatures.h >> create mode 100644 lib/hash/arch/x86/compare_signatures.h >> >> diff --git a/.mailmap b/.mailmap >> index 66ebc20666..00b50414d3 100644 >> --- a/.mailmap >> +++ b/.mailmap >> @@ -494,6 +494,7 @@ Hari Kumar Vemula <hari.kumarx.vemula@intel.com> >> Harini Ramakrishnan <harini.ramakrishnan@microsoft.com> >> Hariprasad Govindharajan <hariprasad.govindharajan@intel.com> >> Harish Patil <harish.patil@cavium.com> <harish.patil@qlogic.com> >> +Harjot Singh <harjot.singh@arm.com> >> Harman Kalra <hkalra@marvell.com> >> Harneet Singh <harneet.singh@intel.com> >> Harold Huang <baymaxhuang@gmail.com> >> @@ -1633,6 +1634,7 @@ Yixue Wang <yixue.wang@intel.com> >> Yi Yang <yangyi01@inspur.com> <yi.y.yang@intel.com> >> Yi Zhang <zhang.yi75@zte.com.cn> >> Yoann Desmouceaux <ydesmouc@cisco.com> >> +Yoan Picchi <yoan.picchi@arm.com> >> Yogesh Jangra <yogesh.jangra@intel.com> >> Yogev Chaimovich <yogev@cgstowernetworks.com> >> Yongjie Gu <yongjiex.gu@intel.com> >> diff --git a/lib/hash/arch/arm/compare_signatures.h b/lib/hash/arch/arm/compare_signatures.h >> new file mode 100644 >> index 0000000000..1af6ba8190 >> --- /dev/null >> +++ b/lib/hash/arch/arm/compare_signatures.h >> @@ -0,0 +1,61 @@ >> +/* SPDX-License-Identifier: BSD-3-Clause >> + * Copyright(c) 2010-2016 Intel Corporation >> + * Copyright(c) 2018-2024 Arm Limited >> + */ >> + >> +/* >> + * Arm's version uses a densely packed hitmask buffer: >> + * Every bit is in use. >> + */ >> + >> +#include <inttypes.h> >> +#include <rte_common.h> >> +#include <rte_vect.h> >> +#include "rte_cuckoo_hash.h" >> + >> +#define DENSE_HASH_BULK_LOOKUP 1 >> + >> +static inline void >> +compare_signatures_dense(uint16_t *hitmask_buffer, >> + const uint16_t *prim_bucket_sigs, >> + const uint16_t *sec_bucket_sigs, >> + uint16_t sig, >> + enum rte_hash_sig_compare_function sig_cmp_fn) >> +{ >> + >> + static_assert(sizeof(*hitmask_buffer) >= 2*(RTE_HASH_BUCKET_ENTRIES/8), >> + "The hitmask must be exactly wide enough to accept the whole hitmask if it is dense"); >> + >> + /* For match mask every bits indicates the match */ >> + switch (sig_cmp_fn) { >> +#if RTE_HASH_BUCKET_ENTRIES <= 8 >> + case RTE_HASH_COMPARE_NEON: { >> + uint16x8_t vmat, vsig, x; >> + int16x8_t shift = {0, 1, 2, 3, 4, 5, 6, 7}; >> + uint16_t low, high; >> + >> + vsig = vld1q_dup_u16((uint16_t const *)&sig); >> + /* Compare all signatures in the primary bucket */ >> + vmat = vceqq_u16(vsig, >> + vld1q_u16((uint16_t const *)prim_bucket_sigs)); >> + x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift); >> + low = (uint16_t)(vaddvq_u16(x)); >> + /* Compare all signatures in the secondary bucket */ >> + vmat = vceqq_u16(vsig, >> + vld1q_u16((uint16_t const *)sec_bucket_sigs)); >> + x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift); >> + high = (uint16_t)(vaddvq_u16(x)); >> + *hitmask_buffer = low | high << RTE_HASH_BUCKET_ENTRIES; >> + >> + } >> + break; >> +#endif >> + default: >> + for (unsigned int i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) { >> + *hitmask_buffer |= >> + ((sig == prim_bucket_sigs[i]) << i); >> + *hitmask_buffer |= >> + ((sig == sec_bucket_sigs[i]) << i) << RTE_HASH_BUCKET_ENTRIES; >> + } >> + } >> +} >> diff --git a/lib/hash/arch/common/compare_signatures.h b/lib/hash/arch/common/compare_signatures.h >> new file mode 100644 >> index 0000000000..dcf9444032 >> --- /dev/null >> +++ b/lib/hash/arch/common/compare_signatures.h >> @@ -0,0 +1,38 @@ >> +/* SPDX-License-Identifier: BSD-3-Clause >> + * Copyright(c) 2010-2016 Intel Corporation >> + * Copyright(c) 2018-2024 Arm Limited >> + */ >> + >> +/* >> + * The generic version could use either a dense or sparsely packed hitmask buffer, >> + * but the dense one is slightly faster. >> + */ >> + >> +#include <inttypes.h> >> +#include <rte_common.h> >> +#include <rte_vect.h> >> +#include "rte_cuckoo_hash.h" >> + >> +#define DENSE_HASH_BULK_LOOKUP 1 >> + >> +static inline void >> +compare_signatures_dense(uint16_t *hitmask_buffer, >> + const uint16_t *prim_bucket_sigs, >> + const uint16_t *sec_bucket_sigs, >> + uint16_t sig, >> + enum rte_hash_sig_compare_function sig_cmp_fn) >> +{ >> + (void) sig_cmp_fn; >> + >> + static_assert(sizeof(*hitmask_buffer) >= 2*(RTE_HASH_BUCKET_ENTRIES/8), >> + "The hitmask must be exactly wide enough to accept the whole hitmask if it is dense"); >> + >> + /* For match mask every bits indicates the match */ >> + for (unsigned int i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) { >> + *hitmask_buffer |= >> + ((sig == prim_bucket_sigs[i]) << i); >> + *hitmask_buffer |= >> + ((sig == sec_bucket_sigs[i]) << i) << RTE_HASH_BUCKET_ENTRIES; >> + } >> + >> +} > > Thanks for re-factoring compare_signatures_...() code, it looks much cleaner that way. > One question I have - does it mean that now for x86 we always use 'sparse' while for all other > ARM and non-ARM platforms we switch to 'dense'? Yes it does. x86 support only the sparse method (the legacy one). Arm and generic code could support both dense and sparse. The reason I made them use the dense method is because it was slightly faster in my tests. (no need to add padding and shifts amongst other benefit.) > >> diff --git a/lib/hash/arch/x86/compare_signatures.h b/lib/hash/arch/x86/compare_signatures.h >> new file mode 100644 >> index 0000000000..7eec499e1f >> --- /dev/null >> +++ b/lib/hash/arch/x86/compare_signatures.h >> @@ -0,0 +1,53 @@ >> +/* SPDX-License-Identifier: BSD-3-Clause >> + * Copyright(c) 2010-2016 Intel Corporation >> + * Copyright(c) 2018-2024 Arm Limited >> + */ >> + >> +/* >> + * x86's version uses a sparsely packed hitmask buffer: >> + * Every other bit is padding. >> + */ >> + >> +#include <inttypes.h> >> +#include <rte_common.h> >> +#include <rte_vect.h> >> +#include "rte_cuckoo_hash.h" >> + >> +#define DENSE_HASH_BULK_LOOKUP 0 >> + >> +static inline void >> +compare_signatures_sparse(uint32_t *prim_hash_matches, uint32_t *sec_hash_matches, >> + const struct rte_hash_bucket *prim_bkt, >> + const struct rte_hash_bucket *sec_bkt, >> + uint16_t sig, >> + enum rte_hash_sig_compare_function sig_cmp_fn) >> +{ >> + /* For match mask the first bit of every two bits indicates the match */ >> + switch (sig_cmp_fn) { >> +#if defined(__SSE2__) && RTE_HASH_BUCKET_ENTRIES <= 8 >> + case RTE_HASH_COMPARE_SSE: >> + /* Compare all signatures in the bucket */ >> + *prim_hash_matches = _mm_movemask_epi8(_mm_cmpeq_epi16( >> + _mm_load_si128( >> + (__m128i const *)prim_bkt->sig_current), >> + _mm_set1_epi16(sig))); >> + /* Extract the even-index bits only */ >> + *prim_hash_matches &= 0x5555; >> + /* Compare all signatures in the bucket */ >> + *sec_hash_matches = _mm_movemask_epi8(_mm_cmpeq_epi16( >> + _mm_load_si128( >> + (__m128i const *)sec_bkt->sig_current), >> + _mm_set1_epi16(sig))); >> + /* Extract the even-index bits only */ >> + *sec_hash_matches &= 0x5555; >> + break; >> +#endif /* defined(__SSE2__) */ >> + default: >> + for (unsigned int i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) { >> + *prim_hash_matches |= >> + ((sig == prim_bkt->sig_current[i]) << (i << 1)); >> + *sec_hash_matches |= >> + ((sig == sec_bkt->sig_current[i]) << (i << 1)); >> + } >> + } >> +}
Hi Mattias, > Introduce DPDK per-lcore id variables, or lcore variables for short. > > An lcore variable has one value for every current and future lcore > id-equipped thread. > > The primary <rte_lcore_var.h> use case is for statically allocating > small chunks of often-used data, which is related logically, but where > there are performance benefits to reap from having updates being local > to an lcore. > > Lcore variables are similar to thread-local storage (TLS, e.g., C11 > _Thread_local), but decoupling the values' life time with that of the > threads. > > Lcore variables are also similar in terms of functionality provided by > FreeBSD kernel's DPCPU_*() family of macros and the associated > build-time machinery. DPCPU uses linker scripts, which effectively > prevents the reuse of its, otherwise seemingly viable, approach. > > The currently-prevailing way to solve the same problem as lcore > variables is to keep a module's per-lcore data as RTE_MAX_LCORE-sized > array of cache-aligned, RTE_CACHE_GUARDed structs. The benefit of > lcore variables over this approach is that data related to the same > lcore now is close (spatially, in memory), rather than data used by > the same module, which in turn avoid excessive use of padding, > polluting caches with unused data. Thanks for the RFC, very interesting one. Few comments/questions below. > RFC v5: > * In Doxygen, consistenly use @<cmd> (and not \<cmd>). > * The RTE_LCORE_VAR_GET() and SET() convience access macros > covered an uncommon use case, where the lcore value is of a > primitive type, rather than a struct, and is thus eliminated > from the API. (Morten Brørup) > * In the wake up GET()/SET() removeal, rename RTE_LCORE_VAR_PTR() > RTE_LCORE_VAR_VALUE(). > * The underscores are removed from __rte_lcore_var_lcore_ptr() to > signal that this function is a part of the public API. > * Macro arguments are documented. > > RFV v4: > * Replace large static array with libc heap-allocated memory. One > implication of this change is there no longer exists a fixed upper > bound for the total amount of memory used by lcore variables. > RTE_MAX_LCORE_VAR has changed meaning, and now represent the > maximum size of any individual lcore variable value. > * Fix issues in example. (Morten Brørup) > * Improve access macro type checking. (Morten Brørup) > * Refer to the lcore variable handle as "handle" and not "name" in > various macros. > * Document lack of thread safety in rte_lcore_var_alloc(). > * Provide API-level assurance the lcore variable handle is > always non-NULL, to all applications to use NULL to mean > "not yet allocated". > * Note zero-sized allocations are not allowed. > * Give API-level guarantee the lcore variable values are zeroed. > > RFC v3: > * Replace use of GCC-specific alignof(<expression>) with alignof(<type>). > * Update example to reflect FOREACH macro name change (in RFC v2). > > RFC v2: > * Use alignof to derive alignment requirements. (Morten Brørup) > * Change name of FOREACH to make it distinct from <rte_lcore.h>'s > *per-EAL-thread* RTE_LCORE_FOREACH(). (Morten Brørup) > * Allow user-specified alignment, but limit max to cache line size. > > Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com> > Acked-by: Morten Brørup <mb@smartsharesystems.com> > --- > config/rte_config.h | 1 + > doc/api/doxy-api-index.md | 1 + > lib/eal/common/eal_common_lcore_var.c | 68 +++++ > lib/eal/common/meson.build | 1 + > lib/eal/include/meson.build | 1 + > lib/eal/include/rte_lcore_var.h | 368 ++++++++++++++++++++++++++ > lib/eal/version.map | 4 + > 7 files changed, 444 insertions(+) > create mode 100644 lib/eal/common/eal_common_lcore_var.c > create mode 100644 lib/eal/include/rte_lcore_var.h > > diff --git a/config/rte_config.h b/config/rte_config.h > index d743a5c3d3..0dac33d3b9 100644 > --- a/config/rte_config.h > +++ b/config/rte_config.h > @@ -41,6 +41,7 @@ > /* EAL defines */ > #define RTE_CACHE_GUARD_LINES 1 > #define RTE_MAX_HEAPS 32 > +#define RTE_MAX_LCORE_VAR 1048576 > #define RTE_MAX_MEMSEG_LISTS 128 > #define RTE_MAX_MEMSEG_PER_LIST 8192 > #define RTE_MAX_MEM_MB_PER_LIST 32768 > diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md > index 8c1eb8fafa..a3b8391570 100644 > --- a/doc/api/doxy-api-index.md > +++ b/doc/api/doxy-api-index.md > @@ -99,6 +99,7 @@ The public API headers are grouped by topics: > [interrupts](@ref rte_interrupts.h), > [launch](@ref rte_launch.h), > [lcore](@ref rte_lcore.h), > + [lcore-varible](@ref rte_lcore_var.h), > [per-lcore](@ref rte_per_lcore.h), > [service cores](@ref rte_service.h), > [keepalive](@ref rte_keepalive.h), > diff --git a/lib/eal/common/eal_common_lcore_var.c b/lib/eal/common/eal_common_lcore_var.c > new file mode 100644 > index 0000000000..5c353ebd46 > --- /dev/null > +++ b/lib/eal/common/eal_common_lcore_var.c > @@ -0,0 +1,68 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2024 Ericsson AB > + */ > + > +#include <inttypes.h> > + > +#include <rte_common.h> > +#include <rte_debug.h> > +#include <rte_log.h> > + > +#include <rte_lcore_var.h> > + > +#include "eal_private.h" > + > +#define LCORE_BUFFER_SIZE (RTE_MAX_LCORE_VAR * RTE_MAX_LCORE) > + > +static void *lcore_buffer; > +static size_t offset = RTE_MAX_LCORE_VAR; > + > +static void * > +lcore_var_alloc(size_t size, size_t align) > +{ > + void *handle; > + void *value; > + > + offset = RTE_ALIGN_CEIL(offset, align); > + > + if (offset + size > RTE_MAX_LCORE_VAR) { > + lcore_buffer = aligned_alloc(RTE_CACHE_LINE_SIZE, > + LCORE_BUFFER_SIZE); Hmm... do I get it right: if offset is <= then RTE_MAX_LCORE_VAR, and offset + size > RTE_MAX_LCORE_VAR we simply overwrite lcore_buffer with newly allocated buffer of the same size? I understand that you expect it just never to happen (total size of all lcore vars never exceed 1MB), but still I think we need to handle it in a some better way then just ignoring such possibility... Might be RTE_VERIFY() at least? As a more generic question - do we need to support LCORE_VAR for dlopen()s that could happen after rte_eal_init() is called and LCORE threads were created? Because, if no, then we probably can make this construction much more flexible: one buffer per LCORE, allocate on demand, etc. > + RTE_VERIFY(lcore_buffer != NULL); > + > + offset = 0; > + } > + > + handle = RTE_PTR_ADD(lcore_buffer, offset); > + > + offset += size; > + > + RTE_LCORE_VAR_FOREACH_VALUE(value, handle) > + memset(value, 0, size); > + > + EAL_LOG(DEBUG, "Allocated %"PRIuPTR" bytes of per-lcore data with a " > + "%"PRIuPTR"-byte alignment", size, align); > + > + return handle; > +} > + > +void * > +rte_lcore_var_alloc(size_t size, size_t align) > +{ > + /* Having the per-lcore buffer size aligned on cache lines > + * assures as well as having the base pointer aligned on cache > + * size assures that aligned offsets also translate to alipgned > + * pointers across all values. > + */ > + RTE_BUILD_BUG_ON(RTE_MAX_LCORE_VAR % RTE_CACHE_LINE_SIZE != 0); > + RTE_ASSERT(align <= RTE_CACHE_LINE_SIZE); > + RTE_ASSERT(size <= RTE_MAX_LCORE_VAR); > + > + /* '0' means asking for worst-case alignment requirements */ > + if (align == 0) > + align = alignof(max_align_t); > + > + RTE_ASSERT(rte_is_power_of_2(align)); > + > + return lcore_var_alloc(size, align); > +}
On Wed, Mar 13, 2024 at 03:43:35PM +0000, Anatoly Burakov wrote:
> When configuring a port, the configured MTU will
> not include VLAN tag size, but the physical
> function driver will add it automatically if the
> port has VLAN filtering configured, which may
> result in seemingly valid MTU to be rejected by
> the PF.
>
> Document the limitation.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
On 3/19/2024 8:55 AM, Chaoyong He wrote:
> CI found in the logic of 'nfp_aesgcm_iv_update()', the variable
> 'cfg_iv' may used uninitialized in some case.
>
> Coverity issue: 415808
> Fixes: 36361ca7fea2 ("net/nfp: fix data endianness problem")
> Cc: shihong.wang@corigine.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>
Applied to dpdk-next-net/main, thanks.
On 1/9/2024 2:10 PM, jerinj@marvell.com wrote: > From: Jerin Jacob <jerinj@marvell.com> > > Define qualification criteria for external library > based on a techboard meeting minutes [1] and past > learnings from mailing list discussion. > > [1] > http://mails.dpdk.org/archives/dev/2019-June/135847.html > https://mails.dpdk.org/archives/dev/2024-January/284849.html > > Signed-off-by: Jerin Jacob <jerinj@marvell.com> > Acked-by: Thomas Monjalon <thomas@monjalon.net> > --- > > v6: > - Address Morten's comments at https://mails.dpdk.org/archives/dev/2024-January/285029.html > > v5: > - Added "Dependency nature" section based on Stephen's input > > v4: > - Address Thomas comments from https://patches.dpdk.org/project/dpdk/patch/20240105121215.3950532-1-jerinj@marvell.com/ > > v3: > - Updated the content based on TB discussion which is documented at > https://mails.dpdk.org/archives/dev/2024-January/284849.html > > v2: > - Added "Meson build integration" and "Code readability" sections. > > doc/guides/contributing/index.rst | 1 + > .../contributing/library_dependency.rst | 53 +++++++++++++++++++ > 2 files changed, 54 insertions(+) > create mode 100644 doc/guides/contributing/library_dependency.rst > > diff --git a/doc/guides/contributing/index.rst b/doc/guides/contributing/index.rst > index dcb9b1fbf0..e5a8c2b0a3 100644 > --- a/doc/guides/contributing/index.rst > +++ b/doc/guides/contributing/index.rst > @@ -15,6 +15,7 @@ Contributor's Guidelines > documentation > unit_test > new_library > + library_dependency > patches > vulnerability > stable > diff --git a/doc/guides/contributing/library_dependency.rst b/doc/guides/contributing/library_dependency.rst > new file mode 100644 > index 0000000000..3b275f1c52 > --- /dev/null > +++ b/doc/guides/contributing/library_dependency.rst > @@ -0,0 +1,53 @@ > +.. SPDX-License-Identifier: BSD-3-Clause > + Copyright(c) 2024 Marvell. > + > +External Library dependency > +=========================== > + > +This document defines the qualification criteria for external libraries that may be > +used as dependencies in DPDK drivers or libraries. > +The final decision to accept or reject is at the discretion of the DPDK Project's Technical Board. > + > +#. **Documentation:** > + > + - Must have adequate documentation for the steps to build it. > + - Must have clear license documentation on distribution and usage aspects of external library. > + > +#. **Free availability:** > + > + - The library must be freely available to build in either source or binary form. > As binary form can't be built, just for language can we drop "to build": "The library must be freely available in either source or binary form." > + - It shall be downloadable from a direct link. There shall not be any requirement to explicitly > + login or sign a user agreement. > + > +#. **Usage License:** > + > + - Both permissive (e.g., BSD-3 or Apache) and non-permissive (e.g., GPLv3) licenses are acceptable. > Both above sample licenses are open source licenses, but as far as I can see proprietary licenses are accepted. Does it make sense to clarify it, like: "Both open-source and proprietary licenses are acceptable." I believe it is OK to have binary or proprietary dependencies for the device support (drivers) code, but this may have consequences for libraries, if specially a core library ends up having this kind of dependency. We don't have a guarantee that a proprietary licensed dependency won't be stopped distributing or changing its license conditions, right? Does it make sense to make this distinction, as driver and library code, for binary or proprietary dependencies? Or are we freely open to any kind of binary or proprietary dependency? > + - In the case of a permissive license, automatic inclusion in the build process is assumed. > + For non-permissive licenses, an additional build configuration option is required. > + > As this is about external dependency, what is about "inclusion in the build process", in build system we just detect the availability of the library, right? How it changes for different license type? What kind of 'additional build configuration option' mentioned, can it be possible to elaborate? > +#. **Distribution License:** > + > + - No specific constraints, but clear documentation on distribution usage aspects is required. > + > +#. **Compiler compatibility:** > + > + - The library must be able to compile with a DPDK supported compiler for the given target > + environment. > Item says 'must', but as there is an option to deliver as binary, this requirement is only for source distribution, although this is kind of obvious does it worth to mention it? > + For example, for Linux, the library must be able to compile with GCC and/or clang. > + - Library may be limited to a specific OS and/or specific hardware. > + > +#. **Meson build integration:** > + > + - The library must have standard method like ``pkg-config`` for seamless integration with > + DPDK's build environment. > + > +#. **Code readability:** > + > + - Optional dependencies should use stubs to minimize ``ifdef`` clutter, promoting improved > + code readability. > + > +#. **Dependency nature:** > + > + - The external library dependency must be optional. > + i.e Missing external library must not impact the core functionality of the DPDK, specific > + library and/or driver will not be built if dependencies are not met. > There is a possibility that a DPDK library is just a wrapper to external proprietary library, should we try to add clarification to prevent this kind of usage?
During throughput running, re-filling the test data will impact the performance test result. So for now, to run decrypt throughput testing is not supported since the test data is not filled. But if user requires OOP(out-of-place) mode, the test data from source mbuf will never be modified, and if the test data can be prepared out of the running loop, the decryption test should be fine. This commit adds the support of out-of-place decryption testing for throughput. [1]: http://mails.dpdk.org/archives/dev/2023-July/273328.html Signed-off-by: Suanming Mou <suanmingm@nvidia.com> --- app/test-crypto-perf/cperf_ops.c | 5 ++- app/test-crypto-perf/cperf_options_parsing.c | 8 +++++ app/test-crypto-perf/cperf_test_throughput.c | 34 +++++++++++++++++--- 3 files changed, 41 insertions(+), 6 deletions(-) diff --git a/app/test-crypto-perf/cperf_ops.c b/app/test-crypto-perf/cperf_ops.c index d3fd115bc0..714616c697 100644 --- a/app/test-crypto-perf/cperf_ops.c +++ b/app/test-crypto-perf/cperf_ops.c @@ -644,7 +644,10 @@ cperf_set_ops_aead(struct rte_crypto_op **ops, } if ((options->test == CPERF_TEST_TYPE_VERIFY) || - (options->test == CPERF_TEST_TYPE_LATENCY)) { + (options->test == CPERF_TEST_TYPE_LATENCY) || + (options->test == CPERF_TEST_TYPE_THROUGHPUT && + (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT || + options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT))) { for (i = 0; i < nb_ops; i++) { uint8_t *iv_ptr = rte_crypto_op_ctod_offset(ops[i], uint8_t *, iv_offset); diff --git a/app/test-crypto-perf/cperf_options_parsing.c b/app/test-crypto-perf/cperf_options_parsing.c index 8c20974273..90526e676f 100644 --- a/app/test-crypto-perf/cperf_options_parsing.c +++ b/app/test-crypto-perf/cperf_options_parsing.c @@ -1341,6 +1341,14 @@ cperf_options_check(struct cperf_options *options) } } + if (options->test == CPERF_TEST_TYPE_THROUGHPUT && + (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT || + options->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) && + !options->out_of_place) { + RTE_LOG(ERR, USER1, "Only out-of-place is allowed in throughput decryption.\n"); + return -EINVAL; + } + if (options->op_type == CPERF_CIPHER_ONLY || options->op_type == CPERF_CIPHER_THEN_AUTH || options->op_type == CPERF_AUTH_THEN_CIPHER) { diff --git a/app/test-crypto-perf/cperf_test_throughput.c b/app/test-crypto-perf/cperf_test_throughput.c index e3d266d7a4..b347baa913 100644 --- a/app/test-crypto-perf/cperf_test_throughput.c +++ b/app/test-crypto-perf/cperf_test_throughput.c @@ -99,6 +99,26 @@ cperf_throughput_test_constructor(struct rte_mempool *sess_mp, return NULL; } +static void +cperf_verify_init_ops(struct rte_mempool *mp __rte_unused, + void *opaque_arg, + void *obj, + __rte_unused unsigned int i) +{ + uint16_t iv_offset = sizeof(struct rte_crypto_op) + + sizeof(struct rte_crypto_sym_op); + uint32_t imix_idx = 0; + struct cperf_throughput_ctx *ctx = opaque_arg; + struct rte_crypto_op *op = obj; + + (ctx->populate_ops)(&op, ctx->src_buf_offset, + ctx->dst_buf_offset, + 1, ctx->sess, ctx->options, + ctx->test_vector, iv_offset, &imix_idx, NULL); + + cperf_mbuf_set(op->sym->m_src, ctx->options, ctx->test_vector); +} + int cperf_throughput_test_runner(void *test_ctx) { @@ -144,6 +164,9 @@ cperf_throughput_test_runner(void *test_ctx) uint16_t iv_offset = sizeof(struct rte_crypto_op) + sizeof(struct rte_crypto_sym_op); + if (ctx->options->out_of_place) + rte_mempool_obj_iter(ctx->pool, cperf_verify_init_ops, (void *)ctx); + while (test_burst_size <= ctx->options->max_burst_size) { uint64_t ops_enqd = 0, ops_enqd_total = 0, ops_enqd_failed = 0; uint64_t ops_deqd = 0, ops_deqd_total = 0, ops_deqd_failed = 0; @@ -176,11 +199,12 @@ cperf_throughput_test_runner(void *test_ctx) } /* Setup crypto op, attach mbuf etc */ - (ctx->populate_ops)(ops, ctx->src_buf_offset, - ctx->dst_buf_offset, - ops_needed, ctx->sess, - ctx->options, ctx->test_vector, - iv_offset, &imix_idx, &tsc_start); + if (!ctx->options->out_of_place) + (ctx->populate_ops)(ops, ctx->src_buf_offset, + ctx->dst_buf_offset, + ops_needed, ctx->sess, + ctx->options, ctx->test_vector, + iv_offset, &imix_idx, &tsc_start); /** * When ops_needed is smaller than ops_enqd, the -- 2.34.1
> -----Original Message----- > From: Akhil Goyal <gakhil@marvell.com> > Sent: Tuesday, March 19, 2024 5:32 PM > To: Suanming Mou <suanmingm@nvidia.com>; Anoob Joseph > <anoobj@marvell.com>; ciara.power@intel.com > Cc: dev@dpdk.org > Subject: RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption > > > > Subject: RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP > > decryption > > > > > > > > > + if (options->test == CPERF_TEST_TYPE_THROUGHPUT && > > > > > > + (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT || > > > > > > + options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT) > > && > > > > > > + !options->out_of_place) { > > > > > > + RTE_LOG(ERR, USER1, "Only out-of-place is allowed in > > > > > > throughput decryption.\n"); > > > > > > + return -EINVAL; > > > > > > + } > > > > > > > > > > This check is blocking cipher_only decryption which should pass > > > > > irrespective of inplace/oop and Data correct/incorrect. > > > > > > > > Sorry, in that case I will remove "options->cipher_op == > > > > RTE_CRYPTO_CIPHER_OP_DECRYPT" and only kept " options->aead_op == > > > > RTE_CRYPTO_AEAD_OP_DECRYPT ", what do you think? > > > > > > I would suggest to check for "auth_op == RTE_CRYPTO_AUTH_OP_VERIFY" > > > Instead of cipher_op. > > > > I'm not sure. Since in AEAD OP, auth_op will always be > > RTE_CRYPTO_AUTH_OP_VERIFY, in that case even in place encrypt will be > > rejected. > > If the combination here is too complicated, what about just remove > > that limits and let user to decide? If the input is not correct, PMD will reject it as > well. > > The problematic cases are where auth data (ICV) is not correct. > i.e. AEAD, AUTH_ONLY and CIPHER_AUTH. > > Hence following check should be ok. > if (options->test == CPERF_TEST_TYPE_THROUGHPUT && > (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT || > options->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) && > !options->out_of_place) { OK, that make sense. Will update, thanks. > > Yes PMD will report error if the input data is not correct, but we cannot just fail in > that case just because the app is intentionally not filling the data. > It should report unsupported case. > > > > > > > > Ciara, What do you suggest? You were also seeing some issues in this patch.
PMD implements sync METER flow action as async. Queue selected for sync operations is `MLX5_HW_INV_QUEUE`. That dummy queue value is translated into `CTRL_QUEUE_ID(priv)`. Async job allocation converts INV queue into the real value, but job release does not. This patch fixes the queue value provided to `flow_hw_job_put()`. This patch also removes dead code found in METER_MARK destroy handler. Coverity issue: 415806 Coverity issue: 415804 Fixes: 4359d9d1f76b ("net/mlx5: fix sync meter processing in HWS") Signed-off-by: Gregory Etelson <getelson@nvidia.com> Acked-by: Dariusz Sosnowski <dsosnowski@nvidia.com> --- v2: Fixed Coverity tag. --- drivers/net/mlx5/mlx5_flow_hw.c | 5 +---- drivers/net/mlx5/mlx5_flow_meter.c | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/net/mlx5/mlx5_flow_hw.c b/drivers/net/mlx5/mlx5_flow_hw.c index 35f1ed7a03..9ebbe664d1 100644 --- a/drivers/net/mlx5/mlx5_flow_hw.c +++ b/drivers/net/mlx5/mlx5_flow_hw.c @@ -11494,10 +11494,7 @@ flow_hw_action_handle_destroy(struct rte_eth_dev *dev, uint32_t queue, NULL, "Unable to wait for ASO meter CQE"); break; } - if (!job) - mlx5_ipool_free(pool->idx_pool, idx); - else - aso = true; + aso = true; break; case MLX5_INDIRECT_ACTION_TYPE_RSS: ret = flow_dv_action_destroy(dev, handle, error); diff --git a/drivers/net/mlx5/mlx5_flow_meter.c b/drivers/net/mlx5/mlx5_flow_meter.c index 4045c4c249..ca361f7efa 100644 --- a/drivers/net/mlx5/mlx5_flow_meter.c +++ b/drivers/net/mlx5/mlx5_flow_meter.c @@ -2265,7 +2265,7 @@ mlx5_flow_meter_hws_create(struct rte_eth_dev *dev, uint32_t meter_id, ret = mlx5_aso_meter_update_by_wqe(priv, MLX5_HW_INV_QUEUE, aso_mtr, &priv->mtr_bulk, job, true); if (ret) { - flow_hw_job_put(priv, job, MLX5_HW_INV_QUEUE); + flow_hw_job_put(priv, job, CTRL_QUEUE_ID(priv)); return -rte_mtr_error_set(error, ENOTSUP, RTE_MTR_ERROR_TYPE_UNSPECIFIED, NULL, "Failed to create devx meter."); -- 2.39.2
> On 3/19/2024 7:07 AM, Chaoyong He wrote:
> > From: Long Wu <long.wu@corigine.com>
> >
> > The PF representor port's queue is different from the VF/physical
> > representor port. So the release process in close port should be
> > different too.
> >
> > Fixes: 39b3951 ("net/nfp: fix resource leak for exit of flower
> > firmware")
> > Cc: chaoyong.he@corigine.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Long Wu <long.wu@corigine.com>
> > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> > Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> >
>
> Hi Chaoyong,
>
> Can you please clarify the impact of the issue?
> As we are post -rc3, is this something we can consider for next release?
Okay, we can delay it for next release.
Thanks.
On 3/19/2024 7:07 AM, Chaoyong He wrote:
> From: Long Wu <long.wu@corigine.com>
>
> The PF representor port's queue is different from the VF/physical
> representor port. So the release process in close port should
> be different too.
>
> Fixes: 39b3951 ("net/nfp: fix resource leak for exit of flower firmware")
> Cc: chaoyong.he@corigine.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>
Hi Chaoyong,
Can you please clarify the impact of the issue?
As we are post -rc3, is this something we can consider for next release?
Hi, > Current hitmask includes padding due to Intel's SIMD > implementation detail. This patch allows non Intel SIMD > implementations to benefit from a dense hitmask. > In addition, the new dense hitmask interweave the primary > and secondary matches which allow a better cache usage and > enable future improvements for the SIMD implementations > > Signed-off-by: Yoan Picchi <yoan.picchi@arm.com> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> > Reviewed-by: Nathan Brown <nathan.brown@arm.com> > --- > .mailmap | 2 + > lib/hash/arch/arm/compare_signatures.h | 61 +++++++ > lib/hash/arch/common/compare_signatures.h | 38 +++++ > lib/hash/arch/x86/compare_signatures.h | 53 ++++++ > lib/hash/rte_cuckoo_hash.c | 192 ++++++++++++---------- > 5 files changed, 255 insertions(+), 91 deletions(-) > create mode 100644 lib/hash/arch/arm/compare_signatures.h > create mode 100644 lib/hash/arch/common/compare_signatures.h > create mode 100644 lib/hash/arch/x86/compare_signatures.h > > diff --git a/.mailmap b/.mailmap > index 66ebc20666..00b50414d3 100644 > --- a/.mailmap > +++ b/.mailmap > @@ -494,6 +494,7 @@ Hari Kumar Vemula <hari.kumarx.vemula@intel.com> > Harini Ramakrishnan <harini.ramakrishnan@microsoft.com> > Hariprasad Govindharajan <hariprasad.govindharajan@intel.com> > Harish Patil <harish.patil@cavium.com> <harish.patil@qlogic.com> > +Harjot Singh <harjot.singh@arm.com> > Harman Kalra <hkalra@marvell.com> > Harneet Singh <harneet.singh@intel.com> > Harold Huang <baymaxhuang@gmail.com> > @@ -1633,6 +1634,7 @@ Yixue Wang <yixue.wang@intel.com> > Yi Yang <yangyi01@inspur.com> <yi.y.yang@intel.com> > Yi Zhang <zhang.yi75@zte.com.cn> > Yoann Desmouceaux <ydesmouc@cisco.com> > +Yoan Picchi <yoan.picchi@arm.com> > Yogesh Jangra <yogesh.jangra@intel.com> > Yogev Chaimovich <yogev@cgstowernetworks.com> > Yongjie Gu <yongjiex.gu@intel.com> > diff --git a/lib/hash/arch/arm/compare_signatures.h b/lib/hash/arch/arm/compare_signatures.h > new file mode 100644 > index 0000000000..1af6ba8190 > --- /dev/null > +++ b/lib/hash/arch/arm/compare_signatures.h > @@ -0,0 +1,61 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2010-2016 Intel Corporation > + * Copyright(c) 2018-2024 Arm Limited > + */ > + > +/* > + * Arm's version uses a densely packed hitmask buffer: > + * Every bit is in use. > + */ > + > +#include <inttypes.h> > +#include <rte_common.h> > +#include <rte_vect.h> > +#include "rte_cuckoo_hash.h" > + > +#define DENSE_HASH_BULK_LOOKUP 1 > + > +static inline void > +compare_signatures_dense(uint16_t *hitmask_buffer, > + const uint16_t *prim_bucket_sigs, > + const uint16_t *sec_bucket_sigs, > + uint16_t sig, > + enum rte_hash_sig_compare_function sig_cmp_fn) > +{ > + > + static_assert(sizeof(*hitmask_buffer) >= 2*(RTE_HASH_BUCKET_ENTRIES/8), > + "The hitmask must be exactly wide enough to accept the whole hitmask if it is dense"); > + > + /* For match mask every bits indicates the match */ > + switch (sig_cmp_fn) { > +#if RTE_HASH_BUCKET_ENTRIES <= 8 > + case RTE_HASH_COMPARE_NEON: { > + uint16x8_t vmat, vsig, x; > + int16x8_t shift = {0, 1, 2, 3, 4, 5, 6, 7}; > + uint16_t low, high; > + > + vsig = vld1q_dup_u16((uint16_t const *)&sig); > + /* Compare all signatures in the primary bucket */ > + vmat = vceqq_u16(vsig, > + vld1q_u16((uint16_t const *)prim_bucket_sigs)); > + x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift); > + low = (uint16_t)(vaddvq_u16(x)); > + /* Compare all signatures in the secondary bucket */ > + vmat = vceqq_u16(vsig, > + vld1q_u16((uint16_t const *)sec_bucket_sigs)); > + x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)), shift); > + high = (uint16_t)(vaddvq_u16(x)); > + *hitmask_buffer = low | high << RTE_HASH_BUCKET_ENTRIES; > + > + } > + break; > +#endif > + default: > + for (unsigned int i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) { > + *hitmask_buffer |= > + ((sig == prim_bucket_sigs[i]) << i); > + *hitmask_buffer |= > + ((sig == sec_bucket_sigs[i]) << i) << RTE_HASH_BUCKET_ENTRIES; > + } > + } > +} > diff --git a/lib/hash/arch/common/compare_signatures.h b/lib/hash/arch/common/compare_signatures.h > new file mode 100644 > index 0000000000..dcf9444032 > --- /dev/null > +++ b/lib/hash/arch/common/compare_signatures.h > @@ -0,0 +1,38 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2010-2016 Intel Corporation > + * Copyright(c) 2018-2024 Arm Limited > + */ > + > +/* > + * The generic version could use either a dense or sparsely packed hitmask buffer, > + * but the dense one is slightly faster. > + */ > + > +#include <inttypes.h> > +#include <rte_common.h> > +#include <rte_vect.h> > +#include "rte_cuckoo_hash.h" > + > +#define DENSE_HASH_BULK_LOOKUP 1 > + > +static inline void > +compare_signatures_dense(uint16_t *hitmask_buffer, > + const uint16_t *prim_bucket_sigs, > + const uint16_t *sec_bucket_sigs, > + uint16_t sig, > + enum rte_hash_sig_compare_function sig_cmp_fn) > +{ > + (void) sig_cmp_fn; > + > + static_assert(sizeof(*hitmask_buffer) >= 2*(RTE_HASH_BUCKET_ENTRIES/8), > + "The hitmask must be exactly wide enough to accept the whole hitmask if it is dense"); > + > + /* For match mask every bits indicates the match */ > + for (unsigned int i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) { > + *hitmask_buffer |= > + ((sig == prim_bucket_sigs[i]) << i); > + *hitmask_buffer |= > + ((sig == sec_bucket_sigs[i]) << i) << RTE_HASH_BUCKET_ENTRIES; > + } > + > +} Thanks for re-factoring compare_signatures_...() code, it looks much cleaner that way. One question I have - does it mean that now for x86 we always use 'sparse' while for all other ARM and non-ARM platforms we switch to 'dense'? > diff --git a/lib/hash/arch/x86/compare_signatures.h b/lib/hash/arch/x86/compare_signatures.h > new file mode 100644 > index 0000000000..7eec499e1f > --- /dev/null > +++ b/lib/hash/arch/x86/compare_signatures.h > @@ -0,0 +1,53 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2010-2016 Intel Corporation > + * Copyright(c) 2018-2024 Arm Limited > + */ > + > +/* > + * x86's version uses a sparsely packed hitmask buffer: > + * Every other bit is padding. > + */ > + > +#include <inttypes.h> > +#include <rte_common.h> > +#include <rte_vect.h> > +#include "rte_cuckoo_hash.h" > + > +#define DENSE_HASH_BULK_LOOKUP 0 > + > +static inline void > +compare_signatures_sparse(uint32_t *prim_hash_matches, uint32_t *sec_hash_matches, > + const struct rte_hash_bucket *prim_bkt, > + const struct rte_hash_bucket *sec_bkt, > + uint16_t sig, > + enum rte_hash_sig_compare_function sig_cmp_fn) > +{ > + /* For match mask the first bit of every two bits indicates the match */ > + switch (sig_cmp_fn) { > +#if defined(__SSE2__) && RTE_HASH_BUCKET_ENTRIES <= 8 > + case RTE_HASH_COMPARE_SSE: > + /* Compare all signatures in the bucket */ > + *prim_hash_matches = _mm_movemask_epi8(_mm_cmpeq_epi16( > + _mm_load_si128( > + (__m128i const *)prim_bkt->sig_current), > + _mm_set1_epi16(sig))); > + /* Extract the even-index bits only */ > + *prim_hash_matches &= 0x5555; > + /* Compare all signatures in the bucket */ > + *sec_hash_matches = _mm_movemask_epi8(_mm_cmpeq_epi16( > + _mm_load_si128( > + (__m128i const *)sec_bkt->sig_current), > + _mm_set1_epi16(sig))); > + /* Extract the even-index bits only */ > + *sec_hash_matches &= 0x5555; > + break; > +#endif /* defined(__SSE2__) */ > + default: > + for (unsigned int i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) { > + *prim_hash_matches |= > + ((sig == prim_bkt->sig_current[i]) << (i << 1)); > + *sec_hash_matches |= > + ((sig == sec_bkt->sig_current[i]) << (i << 1)); > + } > + } > +}
> -----Original Message-----
> From: Kaiwen Deng <kaiwenx.deng@intel.com>
> Sent: Thursday, March 14, 2024 9:01 AM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>; Deng, KaiwenX
> <kaiwenx.deng@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Zeng,
> ZhichaoX <zhichaox.zeng@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: [PATCH] net/iavf: fix fail to reset vf when using dcf
>
> On the latest ice kernel driver, renegotiating VIRTCHNL_OP_GET_VF_RESOURCES
> will fail without hardware reset when using dcf.
>
> This commit will send VIRTCHNL_OP_RESET_VF to pf before dpdk resets vf.
>
> Fixes: 7a93cd3575eb ("net/iavf: add VF reset check")
> Cc: stable@dpdk.org
>
> Signed-off-by: Kaiwen Deng <kaiwenx.deng@intel.com>
Tested-by: Li, HongboX <hongbox.li@intel.com>
> -----Original Message-----
> From: Ma, WenwuX <wenwux.ma@intel.com>
> Sent: Friday, March 15, 2024 9:44 AM
> To: dev@dpdk.org; fengchengwen@huawei.com
> Cc: Jiale, SongX <songx.jiale@intel.com>; Ma, WenwuX
> <wenwux.ma@intel.com>; stable@dpdk.org
> Subject: [PATCH v2] dmadev: fix structure alignment
>
> The structure rte_dma_dev needs only 8 byte alignment.
> This patch replaces __rte_cache_aligned of rte_dma_dev with
> __rte_aligned(8).
>
> Fixes: b36970f2e13e ("dmadev: introduce DMA device library")
> Cc: stable@dpdk.org
>
> Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
> ---
Tested-by: Jiale Song <songx.jiale@intel.com>
> -----Original Message-----
> From: Mingjin Ye <mingjinx.ye@intel.com>
> Sent: Friday, March 15, 2024 6:24 PM
> To: dev@dpdk.org
> Cc: Ye, MingjinX <mingjinx.ye@intel.com>
> Subject: [PATCH 0/2] Tx path check mbuf sub-segment
>
> Add check mbuf sub-segment to Tx diagnostic path.
>
> Mingjin Ye (2):
> net/i40e: Tx path check mbuf sub-segment
> net/ice: Tx path check mbuf sub-segment
>
> drivers/net/i40e/i40e_rxtx.c | 2 +-
> drivers/net/ice/ice_rxtx.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> --
> 2.25.1
Tested-by: Li, HongboX <hongbox.li@intel.com>
The memory of the indirect action handles should be freed after being destroyed in the flush. The behavior needs to be consistent with the single handle destroy. Or else, there will be some unexpected error when the action handle is destroyed for the 2nd time, for example, the port needs to be closed again. Fixes: f7352c176bbf ("app/testpmd: fix use of indirect action after port close") Cc: dmitry.kozliuk@gmail.com Cc: stable@dpdk.org Signed-off-by: Bing Zhao <bingz@nvidia.com> Reviewed-by: Dariusz Sosnowski <dsosnowski@nvidia.com> --- app/test-pmd/config.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index ba1007ace6..f62ba90c87 100644 --- a/app/test-pmd/config.c +++ b/app/test-pmd/config.c @@ -1918,8 +1918,7 @@ port_action_handle_flush(portid_t port_id) /* Poisoning to make sure PMDs update it in case of error. */ memset(&error, 0x44, sizeof(error)); if (pia->handle != NULL) { - ret = pia->type == - RTE_FLOW_ACTION_TYPE_INDIRECT_LIST ? + ret = pia->type == RTE_FLOW_ACTION_TYPE_INDIRECT_LIST ? rte_flow_action_list_handle_destroy (port_id, pia->list_handle, &error) : rte_flow_action_handle_destroy @@ -1929,11 +1928,9 @@ port_action_handle_flush(portid_t port_id) pia->id); ret = port_flow_complain(&error); } - tmp = &pia->next; - } else { - *tmp = pia->next; - free(pia); } + *tmp = pia->next; + free(pia); } return ret; } -- 2.34.1
> > Subject: RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP > decryption > > > > > > > + if (options->test == CPERF_TEST_TYPE_THROUGHPUT && > > > > > + (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT || > > > > > + options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT) > && > > > > > + !options->out_of_place) { > > > > > + RTE_LOG(ERR, USER1, "Only out-of-place is allowed in > > > > > throughput decryption.\n"); > > > > > + return -EINVAL; > > > > > + } > > > > > > > > This check is blocking cipher_only decryption which should pass > > > > irrespective of inplace/oop and Data correct/incorrect. > > > > > > Sorry, in that case I will remove "options->cipher_op == > > > RTE_CRYPTO_CIPHER_OP_DECRYPT" and only kept " options->aead_op == > > > RTE_CRYPTO_AEAD_OP_DECRYPT ", what do you think? > > > > I would suggest to check for "auth_op == RTE_CRYPTO_AUTH_OP_VERIFY" > > Instead of cipher_op. > > I'm not sure. Since in AEAD OP, auth_op will always be > RTE_CRYPTO_AUTH_OP_VERIFY, in that case even in place encrypt will be > rejected. > If the combination here is too complicated, what about just remove that limits and > let user to decide? If the input is not correct, PMD will reject it as well. The problematic cases are where auth data (ICV) is not correct. i.e. AEAD, AUTH_ONLY and CIPHER_AUTH. Hence following check should be ok. if (options->test == CPERF_TEST_TYPE_THROUGHPUT && (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT || options->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) && !options->out_of_place) { Yes PMD will report error if the input data is not correct, but we cannot just fail in that case just because the app is intentionally not filling the data. It should report unsupported case. > > > > > Ciara, What do you suggest? You were also seeing some issues in this patch.
> -----Original Message----- > From: Akhil Goyal <gakhil@marvell.com> > Sent: Tuesday, March 19, 2024 4:23 PM > To: Suanming Mou <suanmingm@nvidia.com>; Anoob Joseph > <anoobj@marvell.com>; ciara.power@intel.com > Cc: dev@dpdk.org > Subject: RE: [EXT] [PATCH] app/test-crypto-perf: add throughput OOP decryption > > > > > + if (options->test == CPERF_TEST_TYPE_THROUGHPUT && > > > > + (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT || > > > > + options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT) && > > > > + !options->out_of_place) { > > > > + RTE_LOG(ERR, USER1, "Only out-of-place is allowed in > > > > throughput decryption.\n"); > > > > + return -EINVAL; > > > > + } > > > > > > This check is blocking cipher_only decryption which should pass > > > irrespective of inplace/oop and Data correct/incorrect. > > > > Sorry, in that case I will remove "options->cipher_op == > > RTE_CRYPTO_CIPHER_OP_DECRYPT" and only kept " options->aead_op == > > RTE_CRYPTO_AEAD_OP_DECRYPT ", what do you think? > > I would suggest to check for "auth_op == RTE_CRYPTO_AUTH_OP_VERIFY" > Instead of cipher_op. I'm not sure. Since in AEAD OP, auth_op will always be RTE_CRYPTO_AUTH_OP_VERIFY, in that case even in place encrypt will be rejected. If the combination here is too complicated, what about just remove that limits and let user to decide? If the input is not correct, PMD will reject it as well. > > Ciara, What do you suggest? You were also seeing some issues in this patch.
On Mon, Mar 18, 2024 at 6:17PM, Amit Prakash Shukla wrote: > Update dma perf test document with below support features: > 1. Memory-to-device and device-to-memory copy. > 2. Skip support. > 3. Scatter-gather support. > > Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com> > --- > doc/guides/tools/dmaperf.rst | 89 ++++++++++++++++++++++++++---------- > 1 file changed, 64 insertions(+), 25 deletions(-) > > diff --git a/doc/guides/tools/dmaperf.rst b/doc/guides/tools/dmaperf.rst > index 9e3e78a6b7..4a5702a628 100644 > --- a/doc/guides/tools/dmaperf.rst > +++ b/doc/guides/tools/dmaperf.rst > @@ -5,27 +5,23 @@ dpdk-test-dma-perf Application > ============================== > > The ``dpdk-test-dma-perf`` tool is a Data Plane Development Kit (DPDK) application > -that enables testing the performance of DMA (Direct Memory Access) devices available within DPDK. > -It provides a test framework to assess the performance of CPU and DMA devices > -under various scenarios, such as varying buffer lengths. > -Doing so provides insight into the potential performance > -when using these DMA devices for acceleration in DPDK applications. > +that evaluates the performance of DMA (Direct Memory Access) devices accessible in DPDK environment. > +It provides a benchmark framework to assess the performance of CPU and DMA devices > +under various combinations, such as varying buffer lengths, scatter-gather copy, copying in remote > +memory etc. It helps in evaluating performance of DMA device as hardware acceleration vehicle in > +DPDK application. > > -It supports memory copy performance tests for now, > -comparing the performance of CPU and DMA automatically in various conditions > -with the help of a pre-set configuration file. > +In addition, this tool supports memory-to-memory, memory-to-device and device-to-memory copy tests, > +to compare the performance of CPU and DMA capabilities under various conditions with the help of a > +pre-set configuration file. > > > Configuration > ------------- > > -This application uses inherent DPDK EAL command-line options > -as well as custom command-line options in the application. > -An example configuration file for the application is provided > -and gives the meanings for each parameter. > - > -Here is an extracted sample from the configuration file > -(the complete sample can be found in the application source directory): > +Along with EAL command-line arguments, this application supports various parameters for the > +benchmarking through a configuration file. An example configuration file is provided below along > +with the application to demonstrate all the parameters. > > .. code-block:: ini > > @@ -53,14 +49,35 @@ Here is an extracted sample from the configuration file > lcore = 3, 4 > eal_args=--in-memory --no-pci > > + [case3] > + skip=1 > + type=DMA_MEM_COPY > + direction=mem2dev > + vchan_dev=raddr=0x200000000,coreid=1,pfid=2,vfid=3 > + dma_src_sge=4 > + dma_dst_sge=1 > + mem_size=10 > + buf_size=64,8192,2,MUL > + dma_ring_size=1024 > + kick_batch=32 > + src_numa_node=0 > + dst_numa_node=0 > + cache_flush=0 > + test_seconds=2 > + lcore_dma=lcore10@0000:00:04.2, lcore11@0000:00:04.3 > + eal_args=--in-memory --file-prefix=test > + > The configuration file is divided into multiple sections, each section represents a test case. > -The four variables ``mem_size``, ``buf_size``, ``dma_ring_size``, and ``kick_batch`` > -can vary in each test case. > -The format for this is ``variable=first,last,increment,ADD|MUL``. > -This means that the first value of the variable is 'first', > -the last value is 'last', > -'increment' is the step size, > -and 'ADD|MUL' indicates whether the change is by addition or multiplication. > +The four mandatory variables ``mem_size``, ``buf_size``, ``dma_ring_size``, and ``kick_batch`` > +can vary in each test case. The format for this is ``variable=first,last,increment,ADD|MUL``. > +This means that the first value of the variable is 'first', the last value is 'last', > +'increment' is the step size, and 'ADD|MUL' indicates whether the change is by addition or > +multiplication. > + > +The variables for mem2dev and dev2mem copy are ``direction``, ``vchan_dev`` and can vary in each > +test case. If the direction is not configured, the default is mem2mem copy. > + > +For scatter-gather copy test ``dma_src_sge``, ``dma_dst_sge`` must be configured. > > Each case can only have one variable change, > and each change will generate a scenario, so each case can have multiple scenarios. > @@ -69,10 +86,32 @@ and each change will generate a scenario, so each case can have multiple scenari > Configuration Parameters > ~~~~~~~~~~~~~~~~~~~~~~~~ > > +``skip`` > + To skip a test-case, must be configured as ``1`` > + > ``type`` > The type of the test. > Currently supported types are ``DMA_MEM_COPY`` and ``CPU_MEM_COPY``. > > +``direction`` > + The direction of data transfer. > + Currently supported directions: > + > + * ``mem2mem`` - memory to memory copy > + > + * ``mem2dev`` - memory to device copy > + > + * ``dev2mem`` - device to memory copy > + > +``vchan_dev`` > + Comma separated bus related parameters for ``mem2dev`` and ``dev2mem`` copy. > + > +``dma_src_sge`` > + Number of source segments for scatter-gather. > + > +``dma_dst_sge`` > + Number of destination segments for scatter-gather. > + > ``mem_size`` > The size of the memory footprint. I'm curious about the success of applying the patch onto the DPDK main branch (commit ID: 80ecef6d1f) except the loongson lab. Because the commit of `d692cb6c8ce7` in the main branch had changed the above line to `The size of the memory footprint in megabytes (MB) for source and destination.`. That change will make the applying failure. > > @@ -131,6 +170,6 @@ with the same name as the configuration file with the addition of ``_result.csv` > Limitations > ----------- > > -Currently, this tool only supports memory copy performance tests. > -Additional enhancements are possible in the future > -to support more types of tests for DMA devices and CPUs. > +DMA copy to/from remote memory address has following limitations: > + > + * ``vchan_dev`` config will be same for all the configured DMA devices.
CI found in the logic of 'nfp_aesgcm_iv_update()', the variable 'cfg_iv' may used uninitialized in some case. Coverity issue: 415808 Fixes: 36361ca7fea2 ("net/nfp: fix data endianness problem") Cc: shihong.wang@corigine.com Cc: stable@dpdk.org Signed-off-by: Chaoyong He <chaoyong.he@corigine.com> Reviewed-by: Long Wu <long.wu@corigine.com> Reviewed-by: Peng Zhang <peng.zhang@corigine.com> --- drivers/net/nfp/nfp_ipsec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/nfp/nfp_ipsec.c b/drivers/net/nfp/nfp_ipsec.c index 205d1d594c..647bc2bb6d 100644 --- a/drivers/net/nfp/nfp_ipsec.c +++ b/drivers/net/nfp/nfp_ipsec.c @@ -526,7 +526,7 @@ nfp_aesgcm_iv_update(struct ipsec_add_sa *cfg, char *iv_b; char *iv_str; const rte_be32_t *iv_value; - uint8_t cfg_iv[NFP_ESP_IV_LENGTH]; + uint8_t cfg_iv[NFP_ESP_IV_LENGTH] = {}; iv_str = strdup(iv_string); if (iv_str == NULL) { -- 2.39.1
On Mon, Mar 18, 2024 at 3:59PM, Patrick Robb wrote:
> On Thu, Mar 7, 2024 at 12:06 PM Adam Hassick <ahassick@iol.unh.edu> wrote:
>>
>> I'm not opposed to having the contexts be a key-value pair argument
>> like the others, however that does break backwards compatibility with
>> our existing syntax. If we don't care very much about backwards
>> compatibility, then we could make this change.
>>
>> Instead of having a boolean and a string parameter for whether to
>> rebase and the branch to rebase on, we could have a single argument
>> specifying a branch. Then, labs rebase on the given branch and then
>> rerun all tests if the "rebase=<branch>" argument is present. This
>> would look like:
>>
>> Recheck-request: rebase=main, iol-sample-apps-testing,
>> iol-unit-amd64-testing, iol-broadcom-Performance
> I agree with this approach because it preserves backward
> compatibility, while still providing us with all the functionality we
> need. We will also be able to accept key value arguments in the future
> if further feature requests come in which require it.
>
>> I don't think the context should be required if the request includes
>> the rebase argument, because we do not want to mix valid and invalid
>> test results as Aaron said.
>> This would be a valid format if contexts are optional:
>>
>> Recheck-request: rebase=main
> Okay, I agree that contexts should not be considered by labs when we
> use rebase - but of course we will still store the contexts (if they
> are submitted) alongside the key value args. In the future there may
> be an application for this.
>
> Zhoumin, does this sound acceptable, or do you think there are any
> flaws? If it works, we will implement the updates and try to upstream
> this week. Thanks!
Thanks for your hard work.
I also agree with this approach. The meaning of the key value
`rebase=main` is sufficient, and loongson lab can support it.
One more thing I want to confirm is whether we should apply the patch
onto the branch commit which existed at the time when that patch was
submitted or onto the latest tip of branch if users request doing
rebase. Users probably request a recheck with `rebase` when the CI lab
chose a wrong branch onto which apply the patch. I worry we may
encounter conflicts when apply the patch onto the latest commit of the
target branch if that branch is just updated before the request.
Hi Vladimir, I have been using rte_fib for a while and stumbled upon a few quirks. I was wondering if you would answer some questions: 1) Is it OK/safe to share the same fib to perform route lookups from multiple lcores in parallel? So far my observations seem to validate that assumption but I would like your opinion :) 2) Is it OK/safe to modify a fib from a control thread (read/write) while it is used by data path threads (read only)? 3) There is no public API to list/walk all configured routes in a fib. Would that be possible/easy to implement? 4) In rte_fib, every IPv4 address (route *and* next hop) needs to be in host order. This is not consistent with fib6 where addresses are stored in network order. It took me quite a while to figure out what was wrong with my code. I assume this is because DIR24 needs host order integers and not TRIE. Why was this not hidden in the API? Could we add a flag to rte_fib_conf to change the behaviour? This would avoid error prone ntohl/htonl juggling. Thanks in advance for your replies :) -- Robin
> > > + if (options->test == CPERF_TEST_TYPE_THROUGHPUT &&
> > > + (options->aead_op == RTE_CRYPTO_AEAD_OP_DECRYPT ||
> > > + options->cipher_op == RTE_CRYPTO_CIPHER_OP_DECRYPT) &&
> > > + !options->out_of_place) {
> > > + RTE_LOG(ERR, USER1, "Only out-of-place is allowed in
> > > throughput decryption.\n");
> > > + return -EINVAL;
> > > + }
> >
> > This check is blocking cipher_only decryption which should pass irrespective of
> > inplace/oop and Data correct/incorrect.
>
> Sorry, in that case I will remove "options->cipher_op ==
> RTE_CRYPTO_CIPHER_OP_DECRYPT" and only kept " options->aead_op ==
> RTE_CRYPTO_AEAD_OP_DECRYPT ", what do you think?
I would suggest to check for "auth_op == RTE_CRYPTO_AUTH_OP_VERIFY"
Instead of cipher_op.
Ciara, What do you suggest? You were also seeing some issues in this patch.
> From: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Monday, 18 March 2024 23.03 > > When debugging driver or startup issues, it is useful to have > a timestamp on each message printed. The messages in syslog > already have a timestamp, but often syslog is not available > during testing. The timestamp format is chosen to look > like the default Linux dmesg timestamp. > > The first few lines are not timestamped because the flag is stored > in internal configuration which is stored in shared memory > which is not setup up until a little later in startup process. > > This logging skips the unnecessary step of going through stdio, > which makes it more robust against being called in interrupt > handlers etc. > > Example: > $ dpdk-testpmd --log-timestamp -- -i > EAL: Detected CPU lcores: 16 > EAL: Detected NUMA nodes: 1 > EAL: Detected static linkage of DPDK > EAL: Multi-process socket /var/run/dpdk/rte/mp_socket > EAL: Selected IOVA mode 'VA' > [ 0.112264] testpmd: No probed ethernet devices > Interactive-mode selected > [ 0.184573] testpmd: create a new mbuf pool <mb_pool_0>: n=163456, > size=2176, socket=0 > [ 0.184612] testpmd: preferred mempool ops selected: ring_mp_mc > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > --- [...] > static ssize_t > console_log_write(__rte_unused void *c, const char *buf, size_t size) > { > + struct timespec ts; > ssize_t ret; > > - /* write on stderr */ > - ret = fwrite(buf, 1, size, stderr); > + if (timestamp_enabled) { > + clock_gettime(CLOCK_MONOTONIC, &ts); > + ts.tv_sec -= log_started.tv_sec; > + ts.tv_nsec -= log_started.tv_nsec; Please log the absolute CLOCK_MONOTONIC instead of subtracting log_started, so timestamps can be easily compared with timestamps from other processes. > + if (ts.tv_nsec < 0) { > + --ts.tv_sec; > + ts.tv_nsec += 1000000000ul; > + } > + > + ret = fprintf(stderr, "[%8lu.%06lu] %.*s", > + ts.tv_sec, ts.tv_nsec / 1000u, > + (int) size, buf); With the above change, For the series, Acked-by: Morten Brørup <mb@smartsharesystems.com>
Hi,
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, March 18, 2024 4:49 PM
> To: Itamar Gozlan <igozlan@nvidia.com>; Erez Shitrit <erezsh@nvidia.com>;
> Hamdan Agbariya <hamdani@nvidia.com>; Yevgeny Kliteynik
> <kliteyn@nvidia.com>; Alex Vesker <valex@nvidia.com>; Raslan Darawsheh
> <rasland@nvidia.com>
> Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; Dariusz Sosnowski
> <dsosnowski@nvidia.com>; Ori Kam <orika@nvidia.com>; Suanming Mou
> <suanmingm@nvidia.com>; Matan Azrad <matan@nvidia.com>; Mark Bloch
> <mbloch@nvidia.com>; dev@dpdk.org; Maayan Kashani
> <mkashani@nvidia.com>
> Subject: Re: [PATCH 01/13] net/mlx5/hws: move warn into debug level when
> needed
>
> 18/03/2024 13:56, Raslan Darawsheh:
> > From: Itamar Gozlan <igozlan@nvidia.com>
> > > From: Erez Shitrit <erezsh@nvidia.com>
> > >
> > > When the user tries to create a matcher and if failed with specific
> > > errno
> > > (E2BIG) the message will be in debug level and not in warning.
> > > It is a part of a feature when the user re-try to insert a new
> > > matching depends on that errno, no need the annoying message.
> > >
> > > Fixes: c55c2bf3533 ("net/mlx5/hws: net/mlx5/hws: add definer layer")
> > >
> > > Signed-off-by: Erez Shitrit <erezsh@nvidia.com>
> > > Acked-by: Matan Azrad <matan@nvidia.com>
> > Fixed Cc stable on several patches on this series, and reworded the
> > commits Series applied to next-net-mlx,
>
> There is no cover letter for this series, so we are not able to understand how
> critical it is, and what is the general intent.
>
> Is it supposed to be integrated in the last week of 24.03 release cycle?
>
No, it's not critical for RC4 It's my fault, I'll drop it for now and we'll merge it in the next release cycle only.
Kindest regards
Raslan Darawsheh