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

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

-----Original Message-----
From: Michel Machado <michel@digirati.com.br> 
Sent: Saturday, August 18, 2018 6:08 PM
To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Fu, Qiaobin <qiaobinf@bu.edu>; Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
Cc: dev@dpdk.org; Doucette, Cody, Joseph <doucette@bu.edu>; Wang, Yipeng1 <yipeng1.wang@intel.com>; Wiles, Keith <keith.wiles@intel.com>; Gobriel, Sameh <sameh.gobriel@intel.com>; Tai, Charlie <charlie.tai@intel.com>; Stephen Hemminger <stephen@networkplumber.org>; nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH v2] hash table: add an iterator over conflicting entries

On 08/17/2018 03:41 PM, Honnappa Nagarahalli wrote:
> Can you elaborate more on using ' struct rte_conflict_iterator_state' as the argument for the API?
> 
> If the API signature is changed to: rte_hash_iterate_conflict_entries (const struct rte_hash *h, void **key, void **data, const hash_sig_t sig, struct rte_conflict_iterator_state *state) - it will be inline with the existing APIs. Contents of 'state' must be initialized to 0 for the first call. This will also avoid creating 'rte_hash_iterator_conflict_entries_init' API.

    Testing `state' every time rte_hash_iterate_conflict_entries() is called to find out if it's the first call of the iterator will possibly add some small, but unnecessary, overhead on
rte_hash_iterate_conflict_entries() and constraints on struct rte_conflict_iterator_state. Moreover,
rte_hash_iterator_conflict_entries_init() enables one to easily add variations of the init function to initialize the state (e.g. using a key instead of a sig) and still use the exactly same iterator.

IMO, I think, this over-head will be trivial. Looking at the function 'rte_hash_iterate_conflict_entries' the check for '(__state->vnext < RTE_HASH_BUCKET_ENTRIES * 2)' already exists. If the primary/secondary bucket indices are calculated as well in 'rte_hash_iterate_conflict_entries' API ('rte_hash_iterate' API does such calculations), storing them in the state can be avoided. I am wondering if it makes sense to benchmark with these changes and then take a decision?
 
[ ]'s
Michel Machado