From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mta112.f1.k8.com.br (mta112.f1.k8.com.br [187.73.32.184]) by dpdk.org (Postfix) with ESMTP id 315DD1DBE for ; Tue, 4 Sep 2018 20:46:49 +0200 (CEST) Received: from [192.168.1.4] (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 D589B800B1; Tue, 4 Sep 2018 18:46:38 +0000 (UTC) X-DKIM: OpenDKIM Filter v2.6.8 smtpz.f1.k8.com.br D589B800B1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=digirati.com.br; s=default; t=1536086805; bh=DvYbby6SH1Ycbzkn0E6CEEQwnB5yUZlDHdAc1xnGxV4=; h=Subject:To:From:Date:Feedback-ID; b=p/HKimUz/Y9klU5LHbsLdRac5KpLzfBJVWRxZ73EGNC1A11laYP4xuqrKEIwbi3lM ek7n0/OxqHaEjKGF9eZNFaRQ+aUdKKan7irHcfPX0gmdcXwPP71+u+lw3Ro7zOj/28 q7BYCRgBUkh0xxCf7fDibxnFgvXBHov/2/+ANX8E= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=k8.com.br; s=default; t=1536086805; bh=DvYbby6SH1Ycbzkn0E6CEEQwnB5yUZlDHdAc1xnGxV4=; h=Subject:To:From:Date:Feedback-ID; b=lf5IaTiQvtbpxl/tjTjaN8gw8kloM6TwuiakDAUXw8qFSA/EaVsN/T6cTWJjL5BLS fxTy1zyBN461qYp6dOJhi6Vby6iUnbPPyQJxKxa4/rRamr2ivo7/Dg8D35HPjZ7BWi 0ntByKIMiKYNxeZP3yt4O/3Wodd0qaOaNgkYCW+E= To: =?UTF-8?Q?Ga=c3=abtan_Rivet?= , Qiaobin Fu Cc: bruce.richardson@intel.com, pablo.de.lara.guarch@intel.com, dev@dpdk.org, doucette@bu.edu, keith.wiles@intel.com, sameh.gobriel@intel.com, charlie.tai@intel.com, stephen@networkplumber.org, nd@arm.com, honnappa.nagarahalli@arm.com, yipeng1.wang@intel.com References: <20180831165101.20026-1-qiaobinf@bu.edu> <20180831225318.dumiys3sxgadv6of@bidouze.vm.6wind.com> From: Michel Machado Organization: Digirati Internet LTDA. Message-ID: <6f83afbe-a033-8ff8-cb49-5f58090aafdd@digirati.com.br> Date: Tue, 4 Sep 2018 14:46:35 -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: <20180831225318.dumiys3sxgadv6of@bidouze.vm.6wind.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-HN-S: bWljaGVsQGRpZ2lyYXRpLmNvbS5icg== X-HN-R: eWlwZW5nMS53YW5nQGludGVsLmNvbSxob25uYXBwYS5uYWdhcmFoYWxsaUBhcm0uY29tLG5kQGFybS5jb20sc3RlcGhlbkBuZXR3b3JrcGx1bWJlci5vcmcsY2hhcmxpZS50YWlAaW50ZWwuY29tLHNhbWVoLmdvYnJpZWxAaW50ZWwuY29tLGtlaXRoLndpbGVzQGludGVsLmNvbSxkb3VjZXR0ZUBidS5lZHUsZGV2QGRwZGsub3JnLHBhYmxvLmRlLmxhcmEuZ3VhcmNoQGludGVsLmNvbSxicnVjZS5yaWNoYXJkc29uQGludGVsLmNvbSxxaWFvYmluZkBidS5lZHUsZ2FldGFuLnJpdmV0QDZ3aW5kLmNvbQ== Feedback-ID: MjAxODA5MDQ=:bWljaGVsQGRpZ2lyYXRpLmNvbS5icg==:ZGlnaXJhdGkuY29tLmJy:k8networks X-Mailman-Approved-At: Thu, 06 Sep 2018 15:08:09 +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: Tue, 04 Sep 2018 18:46:49 -0000 Hi Gaëtan, On 08/31/2018 06:53 PM, Gaëtan Rivet wrote: > Hi Qiaobin, > > This work seems interesting, but is difficult to follow because > the previous discussion is not referenced. > > You can find a how-to there: > > http://doc.dpdk.org/guides/contributing/patches.html#sending-patches > > --in-reply-to is useful to check which comments were already made and > understand the work previously done on a patchset. Thanks for bringing this to our attention. >> +/* istate stands for internal state. */ > > Is a name requiring a comment to explain a good name? > Maybe rte_hash_iterator_priv? > >> +struct rte_hash_iterator_istate { >> + const struct rte_hash *h; >> + uint32_t next; >> + uint32_t total_entries; >> +}; We agree that the suffix _priv is better. > You should check that your private structure does not grow beyond > the public one, using RTE_BUILD_BUG_ON(sizeof(priv) < sizeof(pub)) somewhere. We have overlooked the macro RTE_BUILD_BUG_ON(). We'll use it. > "rte_hash_iterator_[i]state" seems unnecessarily verbose. > The memory you are manipulating through this variable is already holding > the state of your iterator. It is useless to append "_state". > > struct rte_hash_iterator_priv *state; > > is also clear and reads better. > On the other hand "h" is maybe not verbose enough. Why not "hash"? We'll keep the parameter name "state" and rename the variable "__state" to "it" as you suggest in a comment later in your email. About the variable "h", we are following the coding convention in the library. You can find plenty of examples of using "h" for a hash table. > Also, please do not align field names in a structure. It forces > future changes to either break the pattern or edit the whole structure > when someone attempts to insert a field with a name that is too long. Your suggestion goes against the coding style of DPDK. See section "1.5.5. Structure Declarations" on the page: https://doc.dpdk.org/guides-18.08/contributing/coding_style.html >> + >> +int32_t >> +rte_hash_iterator_init(const struct rte_hash *h, >> + struct rte_hash_iterator_state *state) >> +{ >> + struct rte_hash_iterator_istate *__state; > > Please do not use the "__" prefix to convey that > you are using a private version of the structure. > > You could use "istate" or "it", the common shorthand for > iterator handles. We'll do it as explained before. >> int32_t >> -rte_hash_iterate(const struct rte_hash *h, const void **key, void **data, uint32_t *next) >> +rte_hash_iterate( >> + struct rte_hash_iterator_state *state, const void **key, void **data) > > Why an empty first line of parameters here? > > rte_hash_iterate(struct rte_hash_iterator_state *state, > const void **key, > void **data) > > reads better. Okay. >> + >> +/* istate stands for internal state. */ >> +struct rte_hash_iterator_conflict_entries_istate { > > I find "conflict_entries" awkward, how about > > rte_hash_dup_iterator > > instead? It is shorter and conveys that you will iterate duplicate > entries. Yipeng Wang suggested the expression "conflict_entries" in his review of the first version of this patch. You find his suggestion here: http://mails.dpdk.org/archives/dev/2018-August/109103.html I find the name "dup" misleading because it suggests that the returned entries have the same key or refer to the same object. For example, the file descriptor returned by dup(2) refers to the same file. >> + const struct rte_hash *h; >> + uint32_t vnext; >> + uint32_t primary_bidx; >> + uint32_t secondary_bidx; >> +}; >> + >> +int32_t __rte_experimental >> +rte_hash_iterator_conflict_entries_init_with_hash(const struct rte_hash *h, > > rte_hash_dup_iterator_init() maybe? > > Why is _with_hash mentioned here? Is it possible to initialize this kind > of iterator without a reference to compare against? That this reference > is an rte_hash is already given by the parameter list. > > In any case, 49 characters for a name is too long. Honnappa Nagarahalli suggested adding the suffix "_with_hash" during his review of the second version of this patch. His argument went as follows: "Let me elaborate. For the API 'rte_hash_lookup', there are multiple variations such as 'rte_hash_lookup_with_hash', 'rte_hash_lookup_data', 'rte_hash_lookup_with_hash_data' etc. We do not need to create similar variations for 'rte_hash_iterate_conflict_entries' API right now. But the naming of the API should be such that these variations can be created in the future." You find a copy of his original message here: https://www.mail-archive.com/dev@dpdk.org/msg109653.html >> +int32_t __rte_experimental >> +rte_hash_iterate_conflict_entries( >> + struct rte_hash_iterator_state *state, const void **key, void **data) > > How about "rte_hash_dup_next()"? > Also, please break the parameter list instead of having an empty first > line. We are preserving the name convention used in DPDK (e.g. rte_hash_iterate()). We'll break the parameter list to avoid the empty first line. >> diff --git a/lib/librte_hash/rte_hash_version.map b/lib/librte_hash/rte_hash_version.map >> index e216ac8e2..301d4638c 100644 >> --- a/lib/librte_hash/rte_hash_version.map >> +++ b/lib/librte_hash/rte_hash_version.map >> @@ -24,6 +24,7 @@ DPDK_2.1 { >> >> rte_hash_add_key_data; >> rte_hash_add_key_with_hash_data; >> + rte_hash_iterator_init; >> rte_hash_iterate; >> rte_hash_lookup_bulk_data; >> rte_hash_lookup_data; >> @@ -53,3 +54,10 @@ DPDK_18.08 { >> rte_hash_count; >> >> } DPDK_16.07; >> + >> +EXPERIMENTAL { >> + global: >> + >> + rte_hash_iterator_conflict_entries_init_with_hash; >> + rte_hash_iterate_conflict_entries; >> +}; > > A blank line may be missing here. Do you mean a blank line before "};"? Thank you for the careful review. [ ]'s Michel Machado