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

[Alexander Atanasov]

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.

> 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.


-- 
Regards,
Alexander Atanasov