From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mta113.f1.k8.com.br (mta113.f1.k8.com.br [187.73.32.185]) by dpdk.org (Postfix) with ESMTP id 330541B10A for ; Thu, 20 Sep 2018 21:50:13 +0200 (CEST) Received: from [192.168.1.10] (pool-173-48-214-200.bstnma.fios.verizon.net [173.48.214.200]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtpz.f1.k8.com.br (Postfix) with ESMTPSA id DC77E801B5; Thu, 20 Sep 2018 19:50:03 +0000 (UTC) X-DKIM: OpenDKIM Filter v2.6.8 smtpz.f1.k8.com.br DC77E801B5 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=digirati.com.br; s=default; t=1537473010; bh=jvV0QkPdeI663wtwfYcq2Eq3OC66D9twxjEMhaJv8tI=; h=Subject:To:From:Date:Feedback-ID; b=O7JHhKVdaxodIrYJddUqCtq4Op3kqCgyjtIaqcayOPmMa37Y3BMJDk/vXv1NIJ6LS sIQME0i5dd9CvTuAO10j1RRhljDzwlsC+ig2uJKGXRBERI++z89QqqXK+cUmjQZ0qU kInxZaEzkEouQkkzcpsyIU6xnkgk9HRUs2UADEBs= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=k8.com.br; s=default; t=1537473010; bh=jvV0QkPdeI663wtwfYcq2Eq3OC66D9twxjEMhaJv8tI=; h=Subject:To:From:Date:Feedback-ID; b=SJ92/CICiucLB9yH+e+ectv8Q4ITgdFuW7eXUqKf+p43LN04bAI+4OuT/LjycyCUS POjKAw5S0NmXDPbBXHwAHeHFcSkEZqe+GPMU48uEI2pmXCEl4loj1fbd110AGwcaDn 2v/YkCeJlxySsbcTyA4WKKwNJLRtvko+dHTAgzno= To: Honnappa Nagarahalli , Qiaobin Fu , "bruce.richardson@intel.com" , "pablo.de.lara.guarch@intel.com" Cc: "dev@dpdk.org" , "doucette@bu.edu" , "keith.wiles@intel.com" , "sameh.gobriel@intel.com" , "charlie.tai@intel.com" , "stephen@networkplumber.org" , nd , "yipeng1.wang@intel.com" References: <20180831165101.20026-1-qiaobinf@bu.edu> <8ff2d6be-df5b-51cb-95e9-f227127b7d45@digirati.com.br> From: Michel Machado Organization: Digirati Internet LTDA. Message-ID: Date: Thu, 20 Sep 2018 15:50:00 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-HN-S: bWljaGVsQGRpZ2lyYXRpLmNvbS5icg== X-HN-R: eWlwZW5nMS53YW5nQGludGVsLmNvbQogIG5kQGFybS5jb20KICBzdGVwaGVuQG5ldHdvcmtwbHVtYmVyLm9yZwogIGNoYXJsaWUudGFpQGludGVsLmNvbQogIHNhbWVoLmdvYnJpZWxAaW50ZWwuY29tCiAga2VpdGgud2lsZXNAaW50ZWwuY29tCiAgZG91Y2V0dGVAYnUuZWR1CiAgZGV2QGRwZGsub3JnCiAgcGFibG8uZGUubGFyYS5ndWFyY2hAaW50ZWwuY29tCiAgYnJ1Y2UucmljaGFyZHNvbkBpbnRlbC5jb20KICBxaWFvYmluZkBidS5lZHUKICBIb25uYXBwYS5OYWdhcmFoYWxsaUBhcm0uY29t Feedback-ID: MjAxODA5MjA=:bWljaGVsQGRpZ2lyYXRpLmNvbS5icg==:ZGlnaXJhdGkuY29tLmJy:k8networks X-Mailman-Approved-At: Fri, 21 Sep 2018 12:52:40 +0200 Subject: Re: [dpdk-dev] [PATCH v3] hash table: add an iterator over conflicting entries X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Sep 2018 19:50:13 -0000 On 09/12/2018 04:37 PM, Honnappa Nagarahalli wrote: >>> +int32_t >>> +rte_hash_iterator_init(const struct rte_hash *h, >>> + struct rte_hash_iterator_state *state) { >>> + struct rte_hash_iterator_istate *__state; >>> '__state' can be replaced by 's'. >>> >>> + >>> + RETURN_IF_TRUE(((h == NULL) || (state == NULL)), -EINVAL); >>> + >>> + __state = (struct rte_hash_iterator_istate *)state; >>> + __state->h = h; >>> + __state->next = 0; >>> + __state->total_entries = h->num_buckets * RTE_HASH_BUCKET_ENTRIES; >>> + >>> + return 0; >>> +} >>> IMO, creating this API can be avoided if the initialization is handled in 'rte_hash_iterate' function. The cost of doing this is very trivial (one extra 'if' statement) in 'rte_hash_iterate' function. It will help keep the number of APIs to minimal. >> >> Applications would have to initialize struct rte_hash_iterator_state *state before calling rte_hash_iterate() anyway. Why not initializing the fields of a state only once? >> >> My concern is about creating another API for every iterator API. You have a valid point on saving cycles as this API applies for data plane. Have you done any performance benchmarking with and without this API? May be we can guide our decision based on that. > > It's not just about creating one init function for each iterator because an iterator may have a couple of init functions. For example, someone may eventually find useful to add another init function for the conflicting-entry iterator that we are advocating in this patch. A possibility would be for this new init function to use the key of the new entry instead of its signature to initialize the state. Similar to what is already done in rte_hash_lookup*() functions. In spite of possibly having multiple init functions, there will be a single iterator function. > > About the performance benchmarking, the current API only requites applications to initialize a single 32-bit integer. But with the adoption of a struct for the state, the initialization will grow to 64 bytes. > > As my tests showed, I do not see any impact of this. Ok, we are going to eliminate the init functions in v4. >>> diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h >>> index 9e7d9315f..fdb01023e 100644 >>> --- a/lib/librte_hash/rte_hash.h >>> +++ b/lib/librte_hash/rte_hash.h >>> @@ -14,6 +14,8 @@ >>> #include >>> #include >>> >>> +#include >>> + >>> #ifdef __cplusplus >>> extern "C" { >>> #endif >>> @@ -64,6 +66,16 @@ struct rte_hash_parameters { >>> /** @internal A hash table structure. */ struct rte_hash; >>> >>> +/** >>> + * @warning >>> + * @b EXPERIMENTAL: this API may change without prior notice. >>> + * >>> + * @internal A hash table iterator state structure. >>> + */ >>> +struct rte_hash_iterator_state { >>> + uint8_t space[64]; >>> I would call this 'state'. 64 can be replaced by 'RTE_CACHE_LINE_SIZE'. >> >> Okay. > > I think we should not replace 64 with RTE_CACHE_LINE_SIZE because the ABI would change based on the architecture for which it's compiled. > > Ok. May be have a #define for 64? Ok. [ ]'s Michel Machado