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

[Konstantin Khorenko]

Hi guys,

did you pick a brand new super name here? :)

i'm eagerly anticipating seeing it. :)

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 07.06.2024 06:34, Pavel Tikhomirov wrote:
> 
> 
> 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.
> 
>>
>>
>