netdev.vger.kernel.org archive mirror
 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).