Re: [PATCH 1/3] rhashtable: require max_shift definition

[Daniel Borkmann <daniel@iogearbox.net>]

On 02/10/2015 04:56 PM, Josh Hunt wrote:
> On 02/10/2015 02:30 AM, Daniel Borkmann wrote:
>> On 02/10/2015 01:58 AM, Thomas Graf wrote:
>>> On 02/09/15 at 07:48pm, Josh Hunt wrote:
>>>>       if ((params->key_len && !params->hashfn) ||
>>>> -        (!params->key_len && !params->obj_hashfn))
>>>> +        (!params->key_len && !params->obj_hashfn) ||
>>>> +        (!params->max_shift))
>>>>           return -EINVAL;
>>>
>>> You can drop the parenthesis around the new max_shift check.
>>
>> Also, I think the test should be expanded to check if there's
>> a grow_decision given and only in that case require max_shift
>> to be non-zero, otherwise we would require users who don't
>> want to expand their table to give a upper expansion limit.
>
> This is a good point. I'll make this change.
>
> max_shift restricts the # of buckets, but should there be an optional parameter, maxelems, to set a ceiling on the # of elements in a table also? If not, I believe users will be able to add an "unlimited" # of entries to the existing buckets, whether or not a grow_decision fn is defined.

Hm, given that min_shift/max_shift are parameters that directly
concern internals of rhashtable i.e. are tightly coupled to expand
and shrink functionality, I'd say that depending on the use case,
a maxelem limit should rather be handled outside of it, if it's
truly an issue/concern.