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

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

On 08/17/2018 03:41 PM, Honnappa Nagarahalli wrote:
>> Do we also need to have 'rte_hash_iterate_conflict_entries_with_hash' API?
> 
>      I may have not understood the question. We are already working with the hash (i.e. sig). Did you mean something else?
> 
> 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.

    So you mean that we should actually name 
rte_hash_iterator_conflict_entries_init() as 
rte_hash_iterator_conflict_entries_init_with_hash()? I'd be fine with this.

>> diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h
>> index f71ca9fbf..7ecb6a7eb 100644
>> --- a/lib/librte_hash/rte_hash.h
>> +++ b/lib/librte_hash/rte_hash.h
>> @@ -61,6 +61,11 @@ struct rte_hash_parameters {
>>    /** @internal A hash table structure. */  struct rte_hash;
>>    
>> +/** @internal A hash table conflict iterator state structure. */
>> +struct rte_conflict_iterator_state {
>> +	uint8_t space[64];
>> +};
>> +
> Needs aligning to cache line.

    Ok.

>> The size depends on the current size of the state, which is subject to change with the algorithm used.
> 
>      We chose a size that should be robust for any future underlying algorithm. Do you have a suggestion on how to go about it? We chose to have a simple struct to enable applications to allocate a state as a local variable and avoid a memory allocation.
> 
> This looks fine after your explanation. The structure name can be changed to 'rte_iterator_state' so that it can be used in other iterator APIs too.

    I like this suggestion. What about the name 
"rte_hash_iterator_state" to make it specific to the hash table?

[ ]'s
Michel Machado