Re: [dpdk-dev] [PATCH v3] hash table: add an iterator over conflicting entries

[Michel Machado <michel@digirati.com.br>]

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