From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1645A4559B; Fri, 5 Jul 2024 19:43:52 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F20F442F8F; Fri, 5 Jul 2024 19:43:51 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id C20F142F7F for ; Fri, 5 Jul 2024 19:43:50 +0200 (CEST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 12FF1DA7; Fri, 5 Jul 2024 10:44:15 -0700 (PDT) Received: from [10.57.45.238] (unknown [10.57.45.238]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 01FCE3F73B; Fri, 5 Jul 2024 10:43:47 -0700 (PDT) Message-ID: <38b2f96b-f8fc-4dc4-a3e4-f5a79dc4f4b4@arm.com> Date: Fri, 5 Jul 2024 18:43:40 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v10 1/4] hash: pack the hitmask for hash in bulk lookup To: David Marchand Cc: Thomas Monjalon , Yipeng Wang , Sameh Gobriel , Bruce Richardson , Vladimir Medvedkin , dev@dpdk.org, nd@arm.com, Ruifeng Wang , Nathan Brown References: <20231020165159.1649282-1-yoan.picchi@arm.com> <20240703171315.1470547-1-yoan.picchi@arm.com> <20240703171315.1470547-2-yoan.picchi@arm.com> Content-Language: en-US From: Yoan Picchi In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org I'll push a v11 tonight. There is a couple of comments I disagree with tough: On 7/4/24 21:31, David Marchand wrote: > Hello Yoan, > > On Wed, Jul 3, 2024 at 7:13 PM Yoan Picchi wrote: >> >> 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 >> The default non SIMD path now use this dense mask. >> >> Signed-off-by: Yoan Picchi >> Reviewed-by: Ruifeng Wang >> Reviewed-by: Nathan Brown > > This patch does too many things at the same time. > There is code movement and behavior modifications all mixed in. > > As there was still no review from the lib maintainer... I am going a > bit more in depth this time. > Please split this patch to make it less hard to understand. > > I can see the need for at least one patch for isolating the change on > sig_cmp_fn from the exposed API, then one patch for moving the code to > per arch headers with *no behavior change*, and one patch for > introducing/switching to "dense hitmask". > > More comments below. > > >> --- >> .mailmap | 1 + >> lib/hash/compare_signatures_arm_pvt.h | 60 +++++++ >> lib/hash/compare_signatures_generic_pvt.h | 37 +++++ >> lib/hash/compare_signatures_x86_pvt.h | 49 ++++++ >> lib/hash/hash_sig_cmp_func_pvt.h | 20 +++ >> lib/hash/rte_cuckoo_hash.c | 190 +++++++++++----------- >> lib/hash/rte_cuckoo_hash.h | 10 +- >> 7 files changed, 267 insertions(+), 100 deletions(-) >> create mode 100644 lib/hash/compare_signatures_arm_pvt.h >> create mode 100644 lib/hash/compare_signatures_generic_pvt.h >> create mode 100644 lib/hash/compare_signatures_x86_pvt.h >> create mode 100644 lib/hash/hash_sig_cmp_func_pvt.h >> >> diff --git a/.mailmap b/.mailmap >> index f76037213d..ec525981fe 100644 >> --- a/.mailmap >> +++ b/.mailmap >> @@ -1661,6 +1661,7 @@ Yixue Wang >> Yi Yang >> Yi Zhang >> Yoann Desmouceaux >> +Yoan Picchi >> Yogesh Jangra >> Yogev Chaimovich >> Yongjie Gu >> diff --git a/lib/hash/compare_signatures_arm_pvt.h b/lib/hash/compare_signatures_arm_pvt.h >> new file mode 100644 >> index 0000000000..e83bae9912 >> --- /dev/null >> +++ b/lib/hash/compare_signatures_arm_pvt.h > > I guess pvt stands for private. > No need for such suffix, this header won't be exported in any case. pvt do stand for private, yes. I had a look at the other lib and what they used to state a header as private. Several (rcu, ring and stack) use _pvt so it looks like that's might be the standard? If no, then how am I supposed to differentiate a public and a private header? > > >> @@ -0,0 +1,60 @@ >> +/* 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. >> + */ > > Please put a header guard. > > #ifndef _H > #define _H > >> + >> +#include >> +#include >> +#include >> + >> +#include "rte_cuckoo_hash.h" >> +#include "hash_sig_cmp_func_pvt.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), >> + "hitmask_buffer must be wide enough to fit a dense hitmask"); >> + >> + /* 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; >> + } >> + } >> +} > > IIRC, this code is copied in all three headers. > It is a common scalar version, so the ARM code could simply call the > "generic" implementation rather than copy/paste. Out of the three files, only two versions are the same: generic and arm. Intel's version do have some padding added (given it's sparse). I prefer to keep a scalar version in the arm implementation because that's what match the legacy implementation. We used to be able to choose (at runtime) to use the scalar path even if we had neon. In practice the choice ends up being made from #defines, but as far as this function goes, it is a runtime decision. [snip]