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

On 2/7/24 07:58, David Ahern wrote:

On 2/5/24 2:40 PM, thinker.li@gmail.com wrote:

quoted

From: Kui-Feng Lee <redacted>

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

quoted

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

quoted

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