All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kuniyuki Iwashima <kuniyu@amazon.com>
To: <stfomichev@gmail.com>
Cc: <davem@davemloft.net>, <dsahern@kernel.org>,
	<edumazet@google.com>, <horms@kernel.org>, <kuba@kernel.org>,
	<kuni1840@gmail.com>, <kuniyu@amazon.com>,
	<netdev@vger.kernel.org>, <pabeni@redhat.com>
Subject: Re: [PATCH v1 net-next 00/13] ipv6: No RTNL for IPv6 routing table.
Date: Fri, 21 Mar 2025 09:50:40 -0700	[thread overview]
Message-ID: <20250321165154.17497-1-kuniyu@amazon.com> (raw)
In-Reply-To: <Z91yk90LZy9yJexG@mini-arch>

From: Stanislav Fomichev <stfomichev@gmail.com>
Date: Fri, 21 Mar 2025 07:07:15 -0700
> On 03/20, Kuniyuki Iwashima wrote:
> > IPv6 routing tables are protected by each table's lock and work in
> > the interrupt context, which means we basically don't need RTNL to
> > modify an IPv6 routing table itself.
> > 
> > Currently, the control paths require RTNL because we may need to
> > perform device and nexthop lookups; we must prevent dev/nexthop from
> > going away from the netns.
> > 
> > This, however, can be achieved by RCU as well.
> > 
> > If we are in the RCU critical section while adding an IPv6 route,
> > synchronize_net() in netif_change_net_namespace() and
> > unregister_netdevice_many_notify() guarantee that the dev will not be
> > moved to another netns or removed.
> > 
> > Also, nexthop is guaranteed not to be freed during the RCU grace period.
> > 
> > If we care about a race between nexthop removal and IPv6 route addition,
> > we can get rid of RTNL from the control paths.
> > 
> > Patch 1 moves a validation for RTA_MULTIPATH earlier.
> > Patch 2 removes RTNL for SIOCDELRT and RTM_DELROUTE.
> > Patch 3 ~ 10 move validation and memory allocation earlier.
> > Patch 11 prevents a race between two requests for the same table.
> > Patch 12 prevents the race mentioned above.
> > Patch 13 removes RTNL for SIOCADDRT and RTM_NEWROUTE.
> > 
> > 
> > Test:
> > 
> > The script [0] lets each CPU-X create 100000 routes on table-X in a
> > batch.
> > 
> > On c7a.metal-48xl EC2 instance with 192 CPUs,
> > 
> > With this series:
> > 
> >   $ sudo ./route_test.sh
> >   start adding routes
> >   added 19200000 routes (100000 routes * 192 tables).
> >   Time elapsed: 189154 milliseconds.
> > 
> > Without series:
> > 
> >   $ sudo ./route_test.sh
> >   start adding routes
> >   added 19200000 routes (100000 routes * 192 tables).
> >   Time elapsed: 62531 milliseconds.
> > 
> > I changed the number of routes (1000 ~ 100000 per CPU/table) and
> > constantly saw it complete 3x faster with this series.
> > 
> > 
> > [0]
> > #!/bin/bash
> > 
> > mkdir tmp
> > 
> > NS="test"
> > ip netns add $NS
> > ip -n $NS link add veth0 type veth peer veth1
> > ip -n $NS link set veth0 up
> > ip -n $NS link set veth1 up
> > 
> > TABLES=()
> > for i in $(seq $(nproc)); do
> >     TABLES+=("$i")
> > done
> > 
> > ROUTES=()
> > for i in {1..100}; do
> >     for j in {1..1000}; do
> > 	ROUTES+=("2001:$i:$j::/64")
> >     done
> > done
> > 
> > for TABLE in "${TABLES[@]}"; do
> >     FILE="./tmp/batch-table-$TABLE.txt"
> >     > $FILE
> >     for ROUTE in "${ROUTES[@]}"; do
> >         echo "route add $ROUTE dev veth0 table $TABLE" >> $FILE
> >     done
> > done
> > 
> > echo "start adding routes"
> > 
> > START_TIME=$(date +%s%3N)
> > for TABLE in "${TABLES[@]}"; do
> >     ip -n $NS -6 -batch "./tmp/batch-table-$TABLE.txt" &
> > done
> > 
> > wait
> > END_TIME=$(date +%s%3N)
> > ELAPSED_TIME=$((END_TIME - START_TIME))
> > 
> > echo "added $((${#ROUTES[@]} * ${#TABLES[@]})) routes (${#ROUTES[@]} routes * ${#TABLES[@]} tables)."
> > echo "Time elapsed: ${ELAPSED_TIME} milliseconds."
> > echo $(ip -n $NS -6 route show table all | wc -l)  # Just for debug
> > 
> > ip netns del $NS
> > rm -fr ./tmp/
> 
> Lockdep is not supper happy about some patch:
> https://netdev-3.bots.linux.dev/vmksft-forwarding-dbg/results/42463/38-gre-multipath-nh-res-sh/stderr

Looks like I need to extend the RCU critical section in
ip6_route_del() in patch 2:

---8<---
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index d1d60415d1aa..b6434532858f 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4104,9 +4104,9 @@ static int ip6_route_del(struct fib6_config *cfg,
 			if (rt->nh) {
 				if (!fib6_info_hold_safe(rt))
 					continue;
-				rcu_read_unlock();
 
-				return __ip6_del_rt(rt, &cfg->fc_nlinfo);
+				err =  __ip6_del_rt(rt, &cfg->fc_nlinfo);
+				break;
 			}
 			if (cfg->fc_nh_id)
 				continue;
@@ -4121,13 +4121,13 @@ static int ip6_route_del(struct fib6_config *cfg,
 				continue;
 			if (!fib6_info_hold_safe(rt))
 				continue;
-			rcu_read_unlock();
 
 			/* if gateway was specified only delete the one hop */
 			if (cfg->fc_flags & RTF_GATEWAY)
-				return __ip6_del_rt(rt, &cfg->fc_nlinfo);
-
-			return __ip6_del_rt_siblings(rt, cfg);
+				err = __ip6_del_rt(rt, &cfg->fc_nlinfo);
+			else
+				err = __ip6_del_rt_siblings(rt, cfg);
+			break;
 		}
 	}
 	rcu_read_unlock();
---8<---

Thanks!

      reply	other threads:[~2025-03-21 16:52 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-21  4:00 [PATCH v1 net-next 00/13] ipv6: No RTNL for IPv6 routing table Kuniyuki Iwashima
2025-03-21  4:00 ` [PATCH v1 net-next 01/13] ipv6: Validate RTA_GATEWAY of RTA_MULTIPATH in rtm_to_fib6_config() Kuniyuki Iwashima
2025-03-21  4:00 ` [PATCH v1 net-next 02/13] ipv6: Get rid of RTNL for SIOCDELRT and RTM_DELROUTE Kuniyuki Iwashima
2025-03-21  4:00 ` [PATCH v1 net-next 03/13] ipv6: Move some validation from ip6_route_info_create() to rtm_to_fib6_config() Kuniyuki Iwashima
2025-03-21  4:00 ` [PATCH v1 net-next 04/13] ipv6: Check GATEWAY in rtm_to_fib6_multipath_config() Kuniyuki Iwashima
2025-03-21  4:00 ` [PATCH v1 net-next 05/13] ipv6: Move nexthop_find_by_id() after fib6_info_alloc() Kuniyuki Iwashima
2025-03-21  4:00 ` [PATCH v1 net-next 06/13] ipv6: Split ip6_route_info_create() Kuniyuki Iwashima
2025-03-21  4:00 ` [PATCH v1 net-next 07/13] ipv6: Preallocate rt->fib6_nh->rt6i_pcpu in ip6_route_info_create() Kuniyuki Iwashima
2025-03-21  4:00 ` [PATCH v1 net-next 08/13] ipv6: Preallocate nhc_pcpu_rth_output " Kuniyuki Iwashima
2025-03-21  4:00 ` [PATCH v1 net-next 09/13] ipv6: Don't pass net to ip6_route_info_append() Kuniyuki Iwashima
2025-03-21  4:00 ` [PATCH v1 net-next 10/13] ipv6: Factorise ip6_route_multipath_add() Kuniyuki Iwashima
2025-03-21  4:00 ` [PATCH v1 net-next 11/13] ipv6: Protect fib6_link_table() with spinlock Kuniyuki Iwashima
2025-03-21  4:00 ` [PATCH v1 net-next 12/13] ipv6: Protect nh->f6i_list with spinlock and flag Kuniyuki Iwashima
2025-03-21  4:00 ` [PATCH v1 net-next 13/13] ipv6: Get rid of RTNL for SIOCADDRT and RTM_NEWROUTE Kuniyuki Iwashima
2025-03-21 14:07 ` [PATCH v1 net-next 00/13] ipv6: No RTNL for IPv6 routing table Stanislav Fomichev
2025-03-21 16:50   ` Kuniyuki Iwashima [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250321165154.17497-1-kuniyu@amazon.com \
    --to=kuniyu@amazon.com \
    --cc=davem@davemloft.net \
    --cc=dsahern@kernel.org \
    --cc=edumazet@google.com \
    --cc=horms@kernel.org \
    --cc=kuba@kernel.org \
    --cc=kuni1840@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=stfomichev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.