[Devel] [PATCH vz7] neighbour: restore hashtable size limit

[Pavel Tikhomirov]

On 07/06/2024 12:24, Alexander Atanasov wrote:
> On 7.06.24 5:34, Pavel Tikhomirov wrote:
>>
>>
>> On 06/06/2024 19:50, Alexander Atanasov wrote:
>>> With counters fro neigh entries per VE introduced in
>>> https://virtuozzo.atlassian.net/browse/PSBM-87155
>>> tbl->entries, which served as limit of hashtable size,
>>> become unlimited, so the table can grow very large.
>>>
>>> Table is allocated via __get_free_pages which allocates
>>> continuous regions of phys mem and large order allocations
>>> are very likely to fail - which was observed in the issue.
>>>
>>> To address this limit the allocation order to 5.
>>>
>>> Fixes: 019712d0d37d (ve/net/neighbour: per-ct limit for neighbour 
>>> entries)
>>> https://virtuozzo.atlassian.net/browse/PSBM-153199
>>> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
>>> ---
>>>   net/core/neighbour.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> v1->v2: fix spelling type, add defines to explain checks
>>>
>>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>>> index 9e53bb3d1c81..63cadf2d022b 100644
>>> --- a/net/core/neighbour.c
>>> +++ b/net/core/neighbour.c
>>> @@ -641,7 +641,14 @@ struct neighbour *__neigh_create(struct 
>>> neigh_table *tbl, const void *pkey,
>>>       nht = rcu_dereference_protected(tbl->nht,
>>>                       lockdep_is_held(&tbl->lock));
>>> -    if (atomic_read(&tbl->entries) > (1 << nht->hash_shift))
>>> +    /* Since entries can grow unlimited we limit the size of the 
>>> hash table
>>> +     * here. __get_free_pages allocates continuous regions of phys mem
>>> +     * and orders above 10 are very hard to satisfy. We limit the 
>>> size to 5
>>> +     * as it is the middle ground > +     */
>>> +    #define GFP_SAFEZONE_LIMIT 5
>>> +    #define GFP_IN_SAFEZONE(x) ((x) < GFP_SAFEZONE_LIMIT)
>>
>> Sorry in advance for picking on words...
> 
> Nothing to be sorry of, words are important here if we want to define it 
> properly - picking them right is hard and leaving the 5 as a number was 
> due to this exact reason.
> 
>>
>> I'd personally call it NEIGH_HASH_SHIFT_MAX / 
>> NEIGH_HASH_(SHIFT_)GROW_LIMIT. GFP_SAFEZONE_LIMIT is to general (can 
>> intersect with something elsewhere) and does not represent it's real 
>> cause here.
> 
> It is a limit coming from GFP not from NEIGH, so naming it 
> NEIGH_*_MAX/LIMIT is misleading for me.

Limit is due to GFP can't handle >11 order, yes, but it limits neigh 
hash table growth to 5-th order. I prefer naming the limit for WHAT it 
limits and not for WHY it limits it.

> 
>> Also GFP_IN_SAFEZONE() is not perfect too, as it returns true for 
>> {0,1,2,3,4} but not 5, though 5 is actually allowed as we give 
>> hash_shift+1 to neigh_hash_grow.
>>
>> Maybe just:
>>
>> if (nht->hash_shift < NEIGH_HASH_SHIFT_MAX && 
>> atomic_read(&tbl->entries)  > (1 << nht->hash_shift))
>>
>> or:
>>
>> #define NEIGH_HASH_SHIFT_MAX 5
>> #define NEIGH_HASH_GROW(x) ((x) < NEIGH_HASH_SHIFT_MAX)
>>      if (NEIGH_HASH_GROW(nht->hash_shift)....
> 
> This adds more indirection and makes it harder to read - does not ease 
> the reader with the answer of WHAT is checked and WHY?
> 
> GFP_SAFE_THRESHOLD may be , i will think of a better name since neither 
> is good enough.

As I said, WHY it limits should not be important for limit naming, if we 
have some limit we name it for WHAT it limits and put WHY in comments.

> 
> 

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.