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

[David Ahern <dsahern@kernel.org>]

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.

> + */
> +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);
> +}
> +