diff mbox series

[net-next] net: nexthop: reduce rcu synchronizations when replacing resilient groups

Message ID 20211129120924.461545-1-razor@blackwall.org (mailing list archive)
State Accepted
Commit 7709efa62c4fd2a79d154579ea19be34f9fa9a31
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: nexthop: reduce rcu synchronizations when replacing resilient groups | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 14 this patch: 14
netdev/cc_maintainers warning 2 maintainers not CCed: yoshfuji@linux-ipv6.org dsahern@kernel.org
netdev/build_clang success Errors and warnings before: 20 this patch: 20
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 5 this patch: 5
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 21 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Nikolay Aleksandrov Nov. 29, 2021, 12:09 p.m. UTC 
From: Nikolay Aleksandrov <nikolay@nvidia.com>

We can optimize resilient nexthop group replaces by reducing the number of
synchronize_net calls. After commit 1005f19b9357 ("net: nexthop: release
IPv6 per-cpu dsts when replacing a nexthop group") we always do a
synchronize_net because we must ensure no new dsts can be created for the
replaced group's removed nexthops, but we already did that when replacing
resilient groups, so if we always call synchronize_net after any group
type replacement we'll take care of both cases and reduce synchronize_net
calls for resilient groups.

Suggested-by: Ido Schimmel <idosch@idosch.org>
Signed-off-by: Nikolay Aleksandrov <nikolay@nvidia.com>
---
 net/ipv4/nexthop.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Ido Schimmel Nov. 29, 2021, 2:29 p.m. UTC | #1 
On Mon, Nov 29, 2021 at 02:09:24PM +0200, Nikolay Aleksandrov wrote:
> From: Nikolay Aleksandrov <nikolay@nvidia.com>
> 
> We can optimize resilient nexthop group replaces by reducing the number of
> synchronize_net calls. After commit 1005f19b9357 ("net: nexthop: release
> IPv6 per-cpu dsts when replacing a nexthop group") we always do a
> synchronize_net because we must ensure no new dsts can be created for the
> replaced group's removed nexthops, but we already did that when replacing
> resilient groups, so if we always call synchronize_net after any group
> type replacement we'll take care of both cases and reduce synchronize_net
> calls for resilient groups.
> 
> Suggested-by: Ido Schimmel <idosch@idosch.org>
> Signed-off-by: Nikolay Aleksandrov <nikolay@nvidia.com>

I ran fib_nexthops.sh that used to trigger the bug fixed by commit
563f23b00253 ("nexthop: Fix division by zero while replacing a resilient
group") and it looks good.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Tested-by: Ido Schimmel <idosch@nvidia.com>

Thanks!
David Ahern Nov. 29, 2021, 3:32 p.m. UTC | #2 
On 11/29/21 5:09 AM, Nikolay Aleksandrov wrote:
> From: Nikolay Aleksandrov <nikolay@nvidia.com>
> 
> We can optimize resilient nexthop group replaces by reducing the number of
> synchronize_net calls. After commit 1005f19b9357 ("net: nexthop: release
> IPv6 per-cpu dsts when replacing a nexthop group") we always do a
> synchronize_net because we must ensure no new dsts can be created for the
> replaced group's removed nexthops, but we already did that when replacing
> resilient groups, so if we always call synchronize_net after any group
> type replacement we'll take care of both cases and reduce synchronize_net
> calls for resilient groups.
> 
> Suggested-by: Ido Schimmel <idosch@idosch.org>
> Signed-off-by: Nikolay Aleksandrov <nikolay@nvidia.com>
> ---
>  net/ipv4/nexthop.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>
patchwork-bot+netdevbpf@kernel.org Nov. 30, 2021, 12:10 p.m. UTC | #3 
Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Mon, 29 Nov 2021 14:09:24 +0200 you wrote:
> From: Nikolay Aleksandrov <nikolay@nvidia.com>
> 
> We can optimize resilient nexthop group replaces by reducing the number of
> synchronize_net calls. After commit 1005f19b9357 ("net: nexthop: release
> IPv6 per-cpu dsts when replacing a nexthop group") we always do a
> synchronize_net because we must ensure no new dsts can be created for the
> replaced group's removed nexthops, but we already did that when replacing
> resilient groups, so if we always call synchronize_net after any group
> type replacement we'll take care of both cases and reduce synchronize_net
> calls for resilient groups.
> 
> [...]

Here is the summary with links:
  - [net-next] net: nexthop: reduce rcu synchronizations when replacing resilient groups
    https://git.kernel.org/netdev/net-next/c/7709efa62c4f

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 5dbd4b5505eb..1319d093cdda 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -1918,9 +1918,6 @@  static void nh_rt_cache_flush(struct net *net, struct nexthop *nh,
 	if (!replaced_nh->is_group)
 		return;
 
-	/* new dsts must use only the new nexthop group */
-	synchronize_net();
-
 	nhg = rtnl_dereference(replaced_nh->nh_grp);
 	for (i = 0; i < nhg->num_nh; i++) {
 		struct nh_grp_entry *nhge = &nhg->nh_entries[i];
@@ -2002,9 +1999,10 @@  static int replace_nexthop_grp(struct net *net, struct nexthop *old,
 
 	rcu_assign_pointer(old->nh_grp, newg);
 
+	/* Make sure concurrent readers are not using 'oldg' anymore. */
+	synchronize_net();
+
 	if (newg->resilient) {
-		/* Make sure concurrent readers are not using 'oldg' anymore. */
-		synchronize_net();
 		rcu_assign_pointer(oldg->res_table, tmp_table);
 		rcu_assign_pointer(oldg->spare->res_table, tmp_table);
 	}