Skip to contentSkip to conversation

net: Do synchronize_rcu() in ip6mr_sk_done() only if this is needed

PATCH RESEND In review Kirill Tkhai 4 participants 11 messages Mar 6, 2018 – Mar 7, 2018

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH RESEND 1/1 net: Do synchronize_rcu() in ip6mr_sk_done() only if this is needed

After unshare test kworker hangs for ages:

    $ while :; do unshare -n true; done &
    $ perf report <kworker>
    -   88,82%     0,00%  kworker/u16:0  [kernel.vmlinux]  [k] cleanup_net
         cleanup_net
       - ops_exit_list.isra.9
          - 85,68% igmp6_net_exit
             - 53,31% sock_release
                - inet_release
                   - 25,33% rawv6_close
                      - ip6mr_sk_done
                         + 23,38% synchronize_rcu

Keep in mind, this perf report shows the time a function was executing, and it does not show the time, it was sleeping. But it's easy to imagine, how much it is sleeping, if synchronize_rcu() execution takes the most time. Top shows the kworker R time is less then 1%.

This happen, because of in ip6mr_sk_done() we do too many synchronize_rcu(), even for the sockets, that are not referenced in mr_table, and which are not need it. So, the progress of kworker becomes very slow.

The patch introduces apparent solution, and it makes ip6mr_sk_done() to skip synchronize_rcu() for sockets, that are not need that. After the patch, kworker becomes able to warm the cpu up as expected.

---
 net/ipv6/ip6mr.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
net/ipv6/ip6mr.c +3 -1
index 2a38f9de45d3..290a8d0d5eac 100644
@@ -1485,7 +1485,9 @@ int ip6mr_sk_done(struct sock *sk)
 		}
 	}
 	rtnl_unlock();
-	synchronize_rcu();
+
+	if (!err)
+		synchronize_rcu();
 
 	return err;
 }
#1 lore raw
Quoted 53 lines · Kirill Tkhai

After unshare test kworker hangs for ages:

    $ while :; do unshare -n true; done &
    $ perf report <kworker>
    -   88,82%     0,00%  kworker/u16:0  [kernel.vmlinux]  [k]
cleanup_net
         cleanup_net
       - ops_exit_list.isra.9
          - 85,68% igmp6_net_exit
             - 53,31% sock_release
                - inet_release
                   - 25,33% rawv6_close
                      - ip6mr_sk_done
                         + 23,38% synchronize_rcu

Keep in mind, this perf report shows the time a function was executing, and
it does not show the time, it was sleeping. But it's easy to imagine, how
much it is sleeping, if synchronize_rcu() execution takes the most time.
Top shows the kworker R time is less then 1%.

This happen, because of in ip6mr_sk_done() we do too many synchronize_rcu(),
even for the sockets, that are not referenced in mr_table, and which are not
need it. So, the progress of kworker becomes very slow.

The patch introduces apparent solution, and it makes ip6mr_sk_done() to skip
synchronize_rcu() for sockets, that are not need that. After the patch,
kworker becomes able to warm the cpu up as expected.

---
 net/ipv6/ip6mr.c |    4 +++-

 1 file changed, 3 insertions(+), 1 deletion(-)
net/ipv6/ip6mr.c +3 -1
index 2a38f9de45d3..290a8d0d5eac 100644
@@ -1485,7 +1485,9 @@ int ip6mr_sk_done(struct sock *sk)
  		}
  	}
  	rtnl_unlock();
-	synchronize_rcu();
+
+	if (!err)
+		synchronize_rcu();
Quoted 1 line

But... what is this synchronize_rcu() doing exactly ?

This was added in 8571ab479a6e1ef46ead5ebee567e128a422767c

("ip6mr: Make mroute_sk rcu-based")

Typically on a delete, the synchronize_rcu() would be needed before freeing the deleted object.

But nowadays we have better way : SOCK_RCU_FREE

Reviewed-by
#2 lore raw
Quoted 11 lines · Eric Dumazet

But... what is this synchronize_rcu() doing exactly ?

This was added in 8571ab479a6e1ef46ead5ebee567e128a422767c

("ip6mr: Make mroute_sk rcu-based")

Typically on a delete, the synchronize_rcu() would be needed before freeing the deleted object.

But nowadays we have better way : SOCK_RCU_FREE

To be clear, your patch is fine Kirill,

I am only sad seeing one can add a synchronize_rcu() in hot path without anyone complaining during code review.

#3 lore raw
Quoted 5 lines · Eric Dumazet

To be clear, your patch is fine Kirill,

I am only sad seeing one can add a synchronize_rcu() in hot path without anyone complaining during code review.

lpaa2:~# time for i in {1..1000}; do unshare -n /bin/false;done

real 7m18.911s
user 0m0.185s
sys 0m2.314s

Instead of less than 10 seconds not a long time ago :/

#4 lore raw
In reply to Eric Dumazet
Quoted 59 lines · Eric Dumazet
Quoted 53 lines · Kirill Tkhai

After unshare test kworker hangs for ages:

    $ while :; do unshare -n true; done &
    $ perf report <kworker>
    -   88,82%     0,00%  kworker/u16:0  [kernel.vmlinux]  [k]
cleanup_net
         cleanup_net
       - ops_exit_list.isra.9
          - 85,68% igmp6_net_exit
             - 53,31% sock_release
                - inet_release
                   - 25,33% rawv6_close
                      - ip6mr_sk_done
                         + 23,38% synchronize_rcu

Keep in mind, this perf report shows the time a function was executing, and
it does not show the time, it was sleeping. But it's easy to imagine, how
much it is sleeping, if synchronize_rcu() execution takes the most time.
Top shows the kworker R time is less then 1%.

This happen, because of in ip6mr_sk_done() we do too many synchronize_rcu(),
even for the sockets, that are not referenced in mr_table, and which are not
need it. So, the progress of kworker becomes very slow.

The patch introduces apparent solution, and it makes ip6mr_sk_done() to skip
synchronize_rcu() for sockets, that are not need that. After the patch,
kworker becomes able to warm the cpu up as expected.

---
 net/ipv6/ip6mr.c |    4 +++-

 1 file changed, 3 insertions(+), 1 deletion(-)
net/ipv6/ip6mr.c +3 -1
index 2a38f9de45d3..290a8d0d5eac 100644
@@ -1485,7 +1485,9 @@ int ip6mr_sk_done(struct sock *sk)
  		}
  	}
  	rtnl_unlock();
-	synchronize_rcu();
+
+	if (!err)
+		synchronize_rcu();
Quoted 1 line

But... what is this synchronize_rcu() doing exactly ?

Quoted 8 lines

This was added in 8571ab479a6e1ef46ead5ebee567e128a422767c

("ip6mr: Make mroute_sk rcu-based")

Typically on a delete, the synchronize_rcu() would be needed before freeing the deleted object.

But nowadays we have better way : SOCK_RCU_FREE

Hm. I'm agree with you. This is hot path, and mroute sockets created from userspace will delay userspace tasks close() and exit(). Since there may be many such sockets, we may get a zombie task, which can't be reaped for ages. This slows down the system directly.

Fix for pernet_operations works, but we need generic solution instead.

The commit "8571ab479a6e1ef46ead5ebee567e128a422767c" says:

ip6mr: Make mroute_sk rcu-based

In ipmr the mr_table socket is handled under RCU. Introduce the same for ip6mr.

There is no pointing to improvements it invents, or to the problem it solves. The description looks like a cleanup. It's too expensive cleanup, if it worsens the performance a hundred times.

Can't we simply revert it?!

Yuval, do you have ideas to fix that (maybe, via SOCK_RCU_FREE suggested by Eric)?

We actually use rcu_dereference() in ip6mr_cache_report() only. The only user of dereference is sock_queue_rcv_skb(). Superficial analysis shows we purge the queue in inet_sock_destruct().

Thanks,
Kirill

#5 lore raw
Quoted 87 lines · Kirill Tkhai
Quoted 59 lines · Eric Dumazet
Quoted 53 lines · Kirill Tkhai

After unshare test kworker hangs for ages:

    $ while :; do unshare -n true; done &
    $ perf report <kworker>
    -   88,82%     0,00%  kworker/u16:0  [kernel.vmlinux]  [k]
cleanup_net
         cleanup_net
       - ops_exit_list.isra.9
          - 85,68% igmp6_net_exit
             - 53,31% sock_release
                - inet_release
                   - 25,33% rawv6_close
                      - ip6mr_sk_done
                         + 23,38% synchronize_rcu

Keep in mind, this perf report shows the time a function was executing, and
it does not show the time, it was sleeping. But it's easy to imagine, how
much it is sleeping, if synchronize_rcu() execution takes the most time.
Top shows the kworker R time is less then 1%.

This happen, because of in ip6mr_sk_done() we do too many synchronize_rcu(),
even for the sockets, that are not referenced in mr_table, and which are not
need it. So, the progress of kworker becomes very slow.

The patch introduces apparent solution, and it makes ip6mr_sk_done() to skip
synchronize_rcu() for sockets, that are not need that. After the patch,
kworker becomes able to warm the cpu up as expected.

---
 net/ipv6/ip6mr.c |    4 +++-

 1 file changed, 3 insertions(+), 1 deletion(-)
net/ipv6/ip6mr.c +3 -1
index 2a38f9de45d3..290a8d0d5eac 100644
@@ -1485,7 +1485,9 @@ int ip6mr_sk_done(struct sock *sk)
  		}
  	}
  	rtnl_unlock();
-	synchronize_rcu();
+
+	if (!err)
+		synchronize_rcu();
Quoted 1 line

But... what is this synchronize_rcu() doing exactly ?

Quoted 8 lines

This was added in 8571ab479a6e1ef46ead5ebee567e128a422767c

("ip6mr: Make mroute_sk rcu-based")

Typically on a delete, the synchronize_rcu() would be needed before freeing the deleted object.

But nowadays we have better way : SOCK_RCU_FREE

Hm. I'm agree with you. This is hot path, and mroute sockets created from userspace will delay userspace tasks close() and exit(). Since there may be many such sockets, we may get a zombie task, which can't be reaped for ages. This slows down the system directly.

Fix for pernet_operations works, but we need generic solution instead.

The commit "8571ab479a6e1ef46ead5ebee567e128a422767c" says:

ip6mr: Make mroute_sk rcu-based

In ipmr the mr_table socket is handled under RCU. Introduce the same for ip6mr.

There is no pointing to improvements it invents, or to the problem it solves. The description looks like a cleanup. It's too expensive cleanup, if it worsens the performance a hundred times.

Quoted 6 lines

Can't we simply revert it?!

Yuval, do you have ideas to fix that (maybe, via SOCK_RCU_FREE suggested by Eric)?

We actually use rcu_dereference() in ip6mr_cache_report() only. The only user of dereference is sock_queue_rcv_skb(). Superficial analysis shows we purge the queue in inet_sock_destruct().

+ So this change should be safe.

Quoted 3 lines

Thanks,
Kirill

#6 lore raw
Quoted 86 lines
Quoted 60 lines
Quoted 60 lines
Quoted 46 lines

After unshare test kworker hangs for ages:

    $ while :; do unshare -n true; done &
    $ perf report <kworker>
    -   88,82%     0,00%  kworker/u16:0  [kernel.vmlinux]  [k]
cleanup_net
         cleanup_net
       - ops_exit_list.isra.9
          - 85,68% igmp6_net_exit
             - 53,31% sock_release
                - inet_release
                   - 25,33% rawv6_close
                      - ip6mr_sk_done
                         + 23,38% synchronize_rcu

Keep in mind, this perf report shows the time a function was executing, and
it does not show the time, it was sleeping. But it's easy to imagine, how
much it is sleeping, if synchronize_rcu() execution takes the most time.
Top shows the kworker R time is less then 1%.

This happen, because of in ip6mr_sk_done() we do too many synchronize_rcu(),
even for the sockets, that are not referenced in mr_table, and which are not
need it. So, the progress of kworker becomes very slow.

The patch introduces apparent solution, and it makes ip6mr_sk_done() to skip
synchronize_rcu() for sockets, that are not need that. After the patch,
kworker becomes able to warm the cpu up as expected.

---
 net/ipv6/ip6mr.c |    4 +++-

 1 file changed, 3 insertions(+), 1 deletion(-)
net/ipv6/ip6mr.c +0 -0
index 2a38f9de45d3..290a8d0d5eac 100644

net/ipv6/ip6mr.c int ip6mr_sk_done(struct sock *sk) · line 1491

Quoted 8 lines
net/ipv6/ip6mr.c +3 -1
@@ -1485,7 +1485,9 @@ int ip6mr_sk_done(struct sock *sk)
  		}
  	}
  	rtnl_unlock();
-	synchronize_rcu();
+
+	if (!err)
+		synchronize_rcu();
 

But... what is this synchronize_rcu() doing exactly ?

This was added in 8571ab479a6e1ef46ead5ebee567e128a422767c

Quoted 8 lines
Quoted 6 lines

("ip6mr: Make mroute_sk rcu-based")

Typically on a delete, the synchronize_rcu() would be needed before freeing the deleted object.

But nowadays we have better way : SOCK_RCU_FREE

Hm. I'm agree with you. This is hot path, and mroute sockets created from

userspace

Quoted 1 line

will delay userspace tasks close() and exit(). Since there may be many such

sockets,

Quoted 1 line

we may get a zombie task, which can't be reaped for ages. This slows down

the system

Quoted 4 lines

directly.

Fix for pernet_operations works, but we need generic solution instead.

Quoted 8 lines

The commit "8571ab479a6e1ef46ead5ebee567e128a422767c" says:

ip6mr: Make mroute_sk rcu-based

In ipmr the mr_table socket is handled under RCU. Introduce the same for ip6mr.

There is no pointing to improvements it invents, or to the problem it solves.

The description

Quoted 8 lines
Quoted 1 line

looks like a cleanup. It's too expensive cleanup, if it worsens the

performance a hundred

Quoted 5 lines

times.

Can't we simply revert it?!

Yuval, do you have ideas to fix that (maybe, via SOCK_RCU_FREE suggested

by Eric)?

Sorry, failed to notice ip6mr_sk_done() is called unconditionally from rawv6_close(). But even with your suggested fix it should be ~resolved [as only sockets used for ip6mr would reach the sync].
Or are you claiming there's still some performance hit even with your suggested change?

Quoted 7 lines
Quoted 2 lines

We actually use rcu_dereference() in ip6mr_cache_report() only. The only

user of dereference

Quoted 1 line

is sock_queue_rcv_skb(). Superficial analysis shows we purge the queue in

inet_sock_destruct().

+ So this change should be safe.

I might have misunderstood the comment from
commit 4c9687098f24 ("ipmr: RCU conversion of mroute_sk") when writing this; Thought comment regarding ip_ra_destroy() meant that for the IPv6 case we DO have to make sure there's a grace-period before destroying the socket.

Quoted 4 lines
Quoted 4 lines

Thanks,
Kirill

#7 lore raw
Quoted 114 lines · Yuval Mintz
Quoted 114 lines
Quoted 68 lines
Quoted 68 lines
Quoted 52 lines

After unshare test kworker hangs for ages:

    $ while :; do unshare -n true; done &
    $ perf report <kworker>
    -   88,82%     0,00%  kworker/u16:0  [kernel.vmlinux]  [k
]
cleanup_net
         cleanup_net
       - ops_exit_list.isra.9
          - 85,68% igmp6_net_exit
             - 53,31% sock_release
                - inet_release
                   - 25,33% rawv6_close
                      - ip6mr_sk_done
                         + 23,38% synchronize_rcu

Keep in mind, this perf report shows the time a function was executing, and
it does not show the time, it was sleeping. But it's easy to imagine,
how
much it is sleeping, if synchronize_rcu() execution takes the most
time.
Top shows the kworker R time is less then 1%.

This happen, because of in ip6mr_sk_done() we do too many synchronize_rcu(),
even for the sockets, that are not referenced in mr_table, and which
are not
need it. So, the progress of kworker becomes very slow.

The patch introduces apparent solution, and it makes ip6mr_sk_done()
to skip
synchronize_rcu() for sockets, that are not need that. After the
patch,
kworker becomes able to warm the cpu up as expected.

---
 net/ipv6/ip6mr.c |    4 +++-

 1 file changed, 3 insertions(+), 1 deletion(-)
net/ipv6/ip6mr.c +0 -0
index 2a38f9de45d3..290a8d0d5eac 100644

net/ipv6/ip6mr.c int ip6mr_sk_done(struct sock *sk) · line 1491

Quoted 8 lines
net/ipv6/ip6mr.c +3 -1
@@ -1485,7 +1485,9 @@ int ip6mr_sk_done(struct sock *sk)
  		}
  	}
  	rtnl_unlock();
-	synchronize_rcu();
+
+	if (!err)
+		synchronize_rcu();
 

But... what is this synchronize_rcu() doing exactly ?

This was added in 8571ab479a6e1ef46ead5ebee567e128a422767c

("ip6mr: Make mroute_sk rcu-based")

Quoted 8 lines
Quoted 5 lines

Typically on a delete, the synchronize_rcu() would be needed before
freeing the deleted object.

But nowadays we have better way : SOCK_RCU_FREE

Hm. I'm agree with you. This is hot path, and mroute sockets created from

userspace

Quoted 2 lines

will delay userspace tasks close() and exit(). Since there may be many such

sockets,

Quoted 2 lines

we may get a zombie task, which can't be reaped for ages. This slows down

the system

Quoted 7 lines

directly.

Fix for pernet_operations works, but we need generic solution instead.

The commit "8571ab479a6e1ef46ead5ebee567e128a422767c" says:

Quoted 8 lines

    ip6mr: Make mroute_sk rcu-based

    In ipmr the mr_table socket is handled under RCU. Introduce the same
    for ip6mr.

There is no pointing to improvements it invents, or to the problem it solves.

The description

Quoted 2 lines

looks like a cleanup. It's too expensive cleanup, if it worsens the

performance a hundred

Quoted 6 lines

times.

Can't we simply revert it?!

Yuval, do you have ideas to fix that (maybe, via SOCK_RCU_FREE suggested

Quoted 7 lines
Quoted 1 line

by Eric)?

Sorry, failed to notice ip6mr_sk_done() is called unconditionally from
rawv6_close(). But even with your suggested fix it should be ~resolved
[as only sockets used for ip6mr would reach the sync]

Yes but some user setups could still hit this synchronize_rcu() quite hard.

New synchronize_rcu() should not be added in possibly critical paths unless absolutely needed (if really there is no other ways)

We have to be very careful here, since this kind of mistake can have a 100x impact.

Quoted 16 lines

.
Or are you claiming there's still some performance hit even with your suggested change?

Quoted 11 lines
Quoted 3 lines

We actually use rcu_dereference() in ip6mr_cache_report() only. The only

user of dereference

Quoted 2 lines

is sock_queue_rcv_skb(). Superficial analysis shows we purge the queue in

inet_sock_destruct().

+ So this change should be safe.

Quoted 7 lines

I might have misunderstood the comment from
commit 4c9687098f24 ("ipmr: RCU conversion of mroute_sk") when writing
this; Thought comment regarding ip_ra_destroy() meant that for the IPv6 case
we DO have to make sure there's a grace-period before destroying the socket.

Yeah but since 2010 things have changed a lot in the kernel ;)

And we now have SOCK_RCU_FREE for the few sockets that need RCU grace period at dismantle.

Note that TCP sockets are RCU protected but do not request SOCK_RCU_FREE in general (only listeners use this)

SOCK_RCU_FREE will not require a synchronize_rcu() stall, and instead use the asynchronous RCU infrastructure ( call_rcu() )

So all we need now is to make sure these (RAW / ICMPV6) IPv6 sockets have this flag set, then we can remove this problematic synchronize_rcu() from ip6mr_sk_done()

I will test the following fix.

net/ipv6/ip6mr.c +5 -1
index 2a38f9de45d399fafc9f4bcc662b44be17279e51..7345bd6c4b7dda39c0d73d542e9ca9a5366542ff 100644
@@ -1443,6 +1443,7 @@ static int ip6mr_sk_init(struct mr_table *mrt, struct sock *sk)
 		err = -EADDRINUSE;
 	} else {
 		rcu_assign_pointer(mrt->mroute_sk, sk);
+		sock_set_flag(sk, SOCK_RCU_FREE);
 		net->ipv6.devconf_all->mc_forwarding++;
 	}
 	write_unlock_bh(&mrt_lock);
@@ -1472,6 +1473,10 @@ int ip6mr_sk_done(struct sock *sk)
 		if (sk == rtnl_dereference(mrt->mroute_sk)) {
 			write_lock_bh(&mrt_lock);
 			RCU_INIT_POINTER(mrt->mroute_sk, NULL);
+			/* Note that mroute_sk had SOCK_RCU_FREE set,
+			 * so the RCU grace period before sk freeing
+			 * is guaranteed by sk_destruct()
+			 */
 			net->ipv6.devconf_all->mc_forwarding--;
 			write_unlock_bh(&mrt_lock);
 			inet6_netconf_notify_devconf(net, RTM_NEWNETCONF,
@@ -1485,7 +1490,6 @@ int ip6mr_sk_done(struct sock *sk)
 		}
 	}
 	rtnl_unlock();
-	synchronize_rcu();
 
 	return err;
 }
#8 lore raw

PATCH 1/1 ip6mr: remove synchronize_rcu() in favor of SOCK_RCU_FREE

From: Eric Dumazet <[email protected]>

Kirill found that recently added synchronize_rcu() call in ip6mr_sk_done()
was slowing down netns dismantle and posted a patch to use it only if the socket
was found.

I instead suggested to get rid of this call, and use instead SOCK_RCU_FREE

We might later change IPv4 side to use the same technique and unify both stacks. IPv4 does not use synchronize_rcu() but has a call_rcu() that could be replaced by SOCK_RCU_FREE.

Tested:
time for i in {1..1000}; do unshare -n /bin/false;done

Before : real 7m18.911s
After : real 10.187s

Fixes 8571ab479a6e ("ip6mr: Make mroute_sk rcu-based")
Cc Yuval Mintz <[email protected]>

---
 net/ipv6/ip6mr.c |    6 +++++-

 1 file changed, 5 insertions(+), 1 deletion(-)
net/ipv6/ip6mr.c +5 -1
index 2a38f9de45d399fafc9f4bcc662b44be17279e51..7345bd6c4b7dda39c0d73d542e9ca9a5366542ff 100644
@@ -1443,6 +1443,7 @@ static int ip6mr_sk_init(struct mr_table *mrt, struct sock *sk)
 		err = -EADDRINUSE;
 	} else {
 		rcu_assign_pointer(mrt->mroute_sk, sk);
+		sock_set_flag(sk, SOCK_RCU_FREE);
 		net->ipv6.devconf_all->mc_forwarding++;
 	}
 	write_unlock_bh(&mrt_lock);
@@ -1472,6 +1473,10 @@ int ip6mr_sk_done(struct sock *sk)
 		if (sk == rtnl_dereference(mrt->mroute_sk)) {
 			write_lock_bh(&mrt_lock);
 			RCU_INIT_POINTER(mrt->mroute_sk, NULL);
+			/* Note that mroute_sk had SOCK_RCU_FREE set,
+			 * so the RCU grace period before sk freeing
+			 * is guaranteed by sk_destruct()
+			 */
 			net->ipv6.devconf_all->mc_forwarding--;
 			write_unlock_bh(&mrt_lock);
 			inet6_netconf_notify_devconf(net, RTM_NEWNETCONF,
@@ -1485,7 +1490,6 @@ int ip6mr_sk_done(struct sock *sk)
 		}
 	}
 	rtnl_unlock();
-	synchronize_rcu();
 
 	return err;
 }
author Reviewed-by on v1 1/1
#9 lore raw

Re: [PATCH net-next] ip6mr: remove synchronize_rcu() in favor of SOCK_RCU_FREE

Quoted 18 lines · Eric Dumazet

From: Eric Dumazet <[email protected]>

Kirill found that recently added synchronize_rcu() call in ip6mr_sk_done()
was slowing down netns dismantle and posted a patch to use it only if the socket
was found.

I instead suggested to get rid of this call, and use instead SOCK_RCU_FREE

We might later change IPv4 side to use the same technique and unify both stacks. IPv4 does not use synchronize_rcu() but has a call_rcu() that could be replaced by SOCK_RCU_FREE.

Tested:
time for i in {1..1000}; do unshare -n /bin/false;done

Quoted 7 lines

Before : real 7m18.911s
After : real 10.187s

Fixes 8571ab479a6e ("ip6mr: Make mroute_sk rcu-based")
Cc Yuval Mintz <[email protected]>
Quoted 36 lines

---
 net/ipv6/ip6mr.c |    6 +++++-

 1 file changed, 5 insertions(+), 1 deletion(-)
net/ipv6/ip6mr.c +5 -1
index 2a38f9de45d399fafc9f4bcc662b44be17279e51..7345bd6c4b7dda39c0d73d542e9ca9a5366542ff 100644
@@ -1443,6 +1443,7 @@ static int ip6mr_sk_init(struct mr_table *mrt, struct sock *sk)
 		err = -EADDRINUSE;
 	} else {
 		rcu_assign_pointer(mrt->mroute_sk, sk);
+		sock_set_flag(sk, SOCK_RCU_FREE);
 		net->ipv6.devconf_all->mc_forwarding++;
 	}
 	write_unlock_bh(&mrt_lock);
@@ -1472,6 +1473,10 @@ int ip6mr_sk_done(struct sock *sk)
 		if (sk == rtnl_dereference(mrt->mroute_sk)) {
 			write_lock_bh(&mrt_lock);
 			RCU_INIT_POINTER(mrt->mroute_sk, NULL);
+			/* Note that mroute_sk had SOCK_RCU_FREE set,
+			 * so the RCU grace period before sk freeing
+			 * is guaranteed by sk_destruct()
+			 */
 			net->ipv6.devconf_all->mc_forwarding--;
 			write_unlock_bh(&mrt_lock);
 			inet6_netconf_notify_devconf(net, RTM_NEWNETCONF,
@@ -1485,7 +1490,6 @@ int ip6mr_sk_done(struct sock *sk)
 		}
 	}
 	rtnl_unlock();
-	synchronize_rcu();
 
 	return err;
 }
maintainer
#10 lore raw

Re: [PATCH net-next] ip6mr: remove synchronize_rcu() in favor of SOCK_RCU_FREE

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Wed, 07 Mar 2018 08:43:19 -0800

Quoted 18 lines

From: Eric Dumazet <[email protected]>

Kirill found that recently added synchronize_rcu() call in ip6mr_sk_done()
was slowing down netns dismantle and posted a patch to use it only if the socket
was found.

I instead suggested to get rid of this call, and use instead SOCK_RCU_FREE

We might later change IPv4 side to use the same technique and unify both stacks. IPv4 does not use synchronize_rcu() but has a call_rcu() that could be replaced by SOCK_RCU_FREE.

Tested:
time for i in {1..1000}; do unshare -n /bin/false;done

Quoted 7 lines

Before : real 7m18.911s
After : real 10.187s

Fixes 8571ab479a6e ("ip6mr: Make mroute_sk rcu-based")
Cc Yuval Mintz <[email protected]>

Looks great, applied, thanks everyone.