Re: [bpf-next v4 1/2] bpf: hash map, avoid deadlock with suitable hash mask

[Martin KaFai Lau <martin.lau@linux.dev>]

On 1/9/23 6:25 PM, Tonghao Zhang wrote:
> 
> 
>> On Jan 10, 2023, at 9:52 AM, Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 1/5/23 1:26 AM, tong@infragraf.org wrote:
>>> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
>>> index 5aa2b5525f79..974f104f47a0 100644
>>> --- a/kernel/bpf/hashtab.c
>>> +++ b/kernel/bpf/hashtab.c
>>> @@ -152,7 +152,7 @@ static inline int htab_lock_bucket(const struct bpf_htab *htab,
>>>   {
>>>   	unsigned long flags;
>>>   -	hash = hash & HASHTAB_MAP_LOCK_MASK;
>>> +	hash = hash & min_t(u32, HASHTAB_MAP_LOCK_MASK, htab->n_buckets -1);
>>>     	preempt_disable();
>>>   	if (unlikely(__this_cpu_inc_return(*(htab->map_locked[hash])) != 1)) {
>>> @@ -171,7 +171,7 @@ static inline void htab_unlock_bucket(const struct bpf_htab *htab,
>>>   				      struct bucket *b, u32 hash,
>>>   				      unsigned long flags)
>>>   {
>>> -	hash = hash & HASHTAB_MAP_LOCK_MASK;
>>> +	hash = hash & min_t(u32, HASHTAB_MAP_LOCK_MASK, htab->n_buckets -1);
>>
>> Please run checkpatch.pl.  patchwork also reports the same thing:
>> https://patchwork.kernel.org/project/netdevbpf/patch/20230105092637.35069-1-tong@infragraf.org/
>>
>> CHECK: spaces preferred around that '-' (ctx:WxV)
>> #46: FILE: kernel/bpf/hashtab.c:155:
>> +	hash = hash & min_t(u32, HASHTAB_MAP_LOCK_MASK, htab->n_buckets -1);
>> 	                                                                ^
>>
>> CHECK: spaces preferred around that '-' (ctx:WxV)
>> #55: FILE: kernel/bpf/hashtab.c:174:
>> +	hash = hash & min_t(u32, HASHTAB_MAP_LOCK_MASK, htab->n_buckets -1);
>>
>> btw, instead of doing this min_t and -1 repeatedly, ensuring n_buckets is at least HASHTAB_MAP_LOCK_COUNT during map_alloc should be as good?  htab having 2 or 4 max_entries should be pretty uncommon.
>>
> I think we should not limit the max_entries, while it’s not common use case. But for performance, we can introduce htab->n_buckets_mask = HASHTAB_MAP_LOCK_COUNT & (htab->n_buckets -1) ?

To be clear, I didn't mean limiting max_entries... I meant lower bound the 
n_buckets to HASHTAB_MAP_LOCK_COUNT.

imo, adding another n_buckets_mask to htab is even worse for this uncommon case. 
eg. the future code needs to be more careful when to use which one.

It was a suggestion, if you insist on min_t during htab_(un)lock_bucket is fine.