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

[Lai Jiangshan]

On 11/04/2011 09:53 PM, Mathieu Desnoyers wrote:
> * 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 ?
> 

Yes. I planned. But they don't be required for struct cds_lfht_node changes,
so they can be done later.