From: Michel Machado <michel@digirati.com.br>
To: "Gaëtan Rivet" <gaetan.rivet@6wind.com>, "Qiaobin Fu" <qiaobinf@bu.edu>
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
Subject: Re: [dpdk-dev] [PATCH v3] hash table: add an iterator over conflicting entries
Date: Tue, 4 Sep 2018 14:46:35 -0400 [thread overview]
Message-ID: <6f83afbe-a033-8ff8-cb49-5f58090aafdd@digirati.com.br> (raw)
In-Reply-To: <20180831225318.dumiys3sxgadv6of@bidouze.vm.6wind.com>
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
next prev parent reply other threads:[~2018-09-04 18:46 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-31 16:51 Qiaobin Fu
2018-08-31 22:53 ` Gaëtan Rivet
2018-09-04 18:46 ` Michel Machado [this message]
2018-09-02 22:05 ` Honnappa Nagarahalli
2018-09-04 19:36 ` Michel Machado
2018-09-05 22:13 ` Honnappa Nagarahalli
2018-09-06 14:28 ` Michel Machado
2018-09-12 20:37 ` Honnappa Nagarahalli
2018-09-20 19:50 ` Michel Machado
2018-09-04 18:55 ` Wang, Yipeng1
2018-09-04 19:07 ` Michel Machado
2018-09-04 19:51 ` Wang, Yipeng1
2018-09-04 20:26 ` Michel Machado
2018-09-04 20:57 ` Wang, Yipeng1
2018-09-05 17:52 ` Michel Machado
2018-09-05 20:27 ` Wang, Yipeng1
2018-09-06 13:34 ` Michel Machado
2018-10-09 19:29 ` [dpdk-dev] [PATCH v4 1/2] hash table: fix a bug in rte_hash_iterate() Qiaobin Fu
2018-10-09 19:29 ` [dpdk-dev] [PATCH v4 2/2] hash table: add an iterator over conflicting entries Qiaobin Fu
2018-10-10 2:54 ` Wang, Yipeng1
2018-10-10 1:55 ` [dpdk-dev] [PATCH v4 1/2] hash table: fix a bug in rte_hash_iterate() Wang, Yipeng1
-- strict thread matches above, loose matches on Subject: below --
2018-08-30 15:56 [dpdk-dev] [PATCH v3] hash table: add an iterator over conflicting entries Qiaobin Fu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6f83afbe-a033-8ff8-cb49-5f58090aafdd@digirati.com.br \
--to=michel@digirati.com.br \
--cc=bruce.richardson@intel.com \
--cc=charlie.tai@intel.com \
--cc=dev@dpdk.org \
--cc=doucette@bu.edu \
--cc=gaetan.rivet@6wind.com \
--cc=honnappa.nagarahalli@arm.com \
--cc=keith.wiles@intel.com \
--cc=nd@arm.com \
--cc=pablo.de.lara.guarch@intel.com \
--cc=qiaobinf@bu.edu \
--cc=sameh.gobriel@intel.com \
--cc=stephen@networkplumber.org \
--cc=yipeng1.wang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).