Re: [PATCH net-next v4 3/5] net/ipv6: Remove expired routes with a separated list of routes.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 2/7/24 07:58, David Ahern wrote:
> On 2/5/24 2:40 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> FIB6 GC walks trees of fib6_tables to remove expired routes. Walking a tree
>> can be expensive if the number of routes in a table is big, even if most of
>> them are permanent. Checking routes in a separated list of routes having
>> expiration will avoid this potential issue.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   include/net/ip6_fib.h | 47 ++++++++++++++++++++++++++++++++-
>>   net/ipv6/addrconf.c   | 41 ++++++++++++++++++++++++-----
>>   net/ipv6/ip6_fib.c    | 60 +++++++++++++++++++++++++++++++++++++++----
>>   net/ipv6/ndisc.c      | 10 +++++++-
>>   net/ipv6/route.c      | 13 ++++++++--
>>   5 files changed, 155 insertions(+), 16 deletions(-)
>>
> 
> one nit below, but otherwise
> 
> Reviewed-by: David Ahern <dsahern@kernel.org>
> 
> 
>> @@ -498,6 +510,39 @@ void fib6_gc_cleanup(void);
>>   
>>   int fib6_init(void);
>>   
>> +/* Add the route to the gc list if it is not already there
>> + *
>> + * The callers should hold f6i->fib6_table->tb6_lock and make sure the
>> + * route is on a table.
> 
> The last comment is not correct given the fib6_node check below.
> 


Right! I will correct it.

>> + */
>> +static inline void fib6_add_gc_list(struct fib6_info *f6i)
>> +{
>> +	/* If fib6_node is null, the f6i is not in (or removed from) the
>> +	 * table.
>> +	 *
>> +	 * There is a gap between finding the f6i from the table and
>> +	 * calling this function without the protection of the tb6_lock.
>> +	 * This check makes sure the f6i is not added to the gc list when
>> +	 * it is not on the table.
>> +	 */
>> +	if (!rcu_dereference_protected(f6i->fib6_node,
>> +				       lockdep_is_held(&f6i->fib6_table->tb6_lock)))
>> +		return;
>> +
>> +	if (hlist_unhashed(&f6i->gc_link))
>> +		hlist_add_head(&f6i->gc_link, &f6i->fib6_table->tb6_gc_hlist);
>> +}
>> +
>