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

[Pavel Tikhomirov]

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

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.

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

> +	if (GFP_IN_SAFEZONE(nht->hash_shift) && atomic_read(&tbl->entries) > (1 << nht->hash_shift))
>   		nht = neigh_hash_grow(tbl, nht->hash_shift + 1);
>   
>   	hash_val = tbl->hash(pkey, dev, nht->hash_rnd) >> (32 - nht->hash_shift);

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