[ltt-dev] [PATCH 3/7] rculfhash: make cds_lfht_lookup() more generic

[Mathieu Desnoyers]

* Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> On 11/04/2011 06:08 PM, Mathieu Desnoyers wrote:
> > * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> >> On 11/03/2011 01:23 AM, Mathieu Desnoyers wrote:
> >>> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> >>>> We use hash value to determine the range of the object
> >>>> and use cds_lfht_lookup_fct to lookup them.
> >>>>
> >>>> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> >>>> ---
> >>>>  rculfhash.c            |    8 ++++----
> >>>>  tests/test_urcu_hash.c |   25 ++++++++++++++++++++++---
> >>>>  urcu/rculfhash.h       |    4 +++-
> >>>>  3 files changed, 29 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/rculfhash.c b/rculfhash.c
> >>>> index d1a1766..29054cd 100644
> >>>> --- a/rculfhash.c
> >>>> +++ b/rculfhash.c
> >>>> @@ -1391,14 +1391,14 @@ struct cds_lfht *_cds_lfht_new(cds_lfht_hash_fct hash_fct,
> >>>>  	return ht;
> >>>>  }
> >>>>  
> >>>> -void cds_lfht_lookup(struct cds_lfht *ht, void *key, size_t key_len,
> >>>> +void cds_lfht_lookup(struct cds_lfht *ht, unsigned long hash,
> >>>> +		cds_lfht_lookup_fct match, void *arg,
> >>>
> >>> arg -> key ?
> >>>
> >>> for "match", our approach is to provide this function as parameter to
> >>> cds_lfht_new rather than pass it as parameter each time a lookup is
> >>> performed. What is the reason why we should pass it here as a parameter?
> >>>
> >>
> >> Allow the user define it.
> >> In the test, I pass match = a function always returns true when need,
> >> thus cds_lfht_lookup() returns the first node of same-hash-value-chain.
> > 
> > This looks like an interesting debug and testing feature, but I don't
> > really see why we should complicate the API to include this. Is there a
> > real-life use-case you have in mind besides testing ?
> > 
> > If we only need this for testing, we can use a static variable in the
> > test program to influence the behavior of cds_lfht_test_lookup.
> > 
> > The other patches look fine, I'm just concerned about this one because
> > it adds complexity to the API which does not seem useful to end-users.
> > 
> 
> I think a generic API is OK.
> 
> Sometimes the caller don't have the key or the keys which are big
> are saved separated. Example: file server
> Or key comparation is too slow.
> Or caller just need approximate lookup.
> Or caller want to compare keys with different strategies for performance.

These arguments are appealing. However, this modification seems
incomplete: we would need to also modify cds_lfht_next_duplicate, and
*cds_lfht_add*, so they also can specify their own compare_fct strategy,
and remove the compare_fct from the lfht_new arguments, right ?

Thanks,

Mathieu


-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com