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

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

Hi Yipeng,

On 09/04/2018 03:51 PM, Wang, Yipeng1 wrote:
> Hmm, I guess my comment is for code readability. If we don’t need the extra state that would be great.

    Notice that applications only see the public, opaque state. And the 
private versions are scoped to the C file where they are needed.

> I think "rte_hash" is defined as an internal data structure but expose the type to the public header. Would this work?

    Exposing the private fields would bind the interface with the 
current implementation of the hash table. In the way we are proposing, 
one should be able to replace the underlying algorithm and not touching 
the header files that applications use. But, yes, your solution would 
enable applications to allocate iterator states as local variables as well.

> I propose to malloc inside function mostly because I think it is cleaner to the user. But your argument is
> valid. Depending on use case I think it is OK.

    I don't know how other applications will use this iterator, but we 
use it when our application is already overloaded. So avoiding an 
expensive operation like malloc() is a win.

> Another comment is you put the total_entry in the state, is it for performance of the rte_hash_iterate?

    We are saving one integer multiplication per call of 
rte_hash_iterate(). It's not a big deal, but since there's room in the 
state variable, we thought this would be a good idea because it grows 
with the size of the table. We didn't actually measure the effect of 
this decision.

> If you use it to iterate conflict entries, especially If you reuse same "state" struct and init it again and again for different keys,
> would this slow down the performance for your specific use case?

    Notice that the field total_entry only exists for 
rte_hash_iterate(). But even if total_entry were in the state of 
rte_hash_iterate_conflict_entries(), it would still save on the 
multiplication as long as rte_hash_iterate_conflict_entries() is called 
at least twice. Calling rte_hash_iterate_conflict_entries() once evens 
out, and calling rte_hash_iterate_conflict_entries() more times adds 
further savings. As a side note. in our application, whenever an 
iterator of conflicting entries is initialized, we call 
rte_hash_iterate_conflict_entries() at least once.

> Also iterate_conflic_entry may need reader lock protection.

    We are going to add the reader lock protection. Thanks.

[ ]'s
Michel Machado