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.atana...@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.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
