[net] net/smc: Prevent smc_release() from long blocking

Commit Message

D. Wythe Dec. 15, 2021, 12:29 p.m. UTC

From: "D. Wythe" <[e-mail]>

In nginx/wrk benchmark, there's a hung problem with high probability
on case likes that: (client will last several minutes to exit)

server: smc_run nginx

client: smc_run wrk -c 10000 -t 1 http://server

Client hangs with the following backtrace:

0 [ffffa7ce8Of3bbf8] __schedule at ffffffff9f9eOd5f
1 [ffffa7ce8Of3bc88] schedule at ffffffff9f9eløe6
2 [ffffa7ce8Of3bcaO] schedule_timeout at ffffffff9f9e3f3c
3 [ffffa7ce8Of3bd2O] wait_for_common at ffffffff9f9el9de
4 [ffffa7ce8Of3bd8O] __flush_work at ffffffff9fOfeOl3
5 [ffffa7ce8øf3bdfO] smc_release at ffffffffcO697d24 [smc]
6 [ffffa7ce8Of3be2O] __sock_release at ffffffff9f8O2e2d
7 [ffffa7ce8Of3be4ø] sock_close at ffffffff9f8ø2ebl
8 [ffffa7ce8øf3be48] __fput at ffffffff9f334f93
9 [ffffa7ce8Of3be78] task_work_run at ffffffff9flOlff5
10 [ffffa7ce8Of3beaO] do_exit at ffffffff9fOe5Ol2
11 [ffffa7ce8Of3bflO] do_group_exit at ffffffff9fOe592a
12 [ffffa7ce8Of3bf38] __x64_sys_exit_group at ffffffff9fOe5994
13 [ffffa7ce8Of3bf4O] do_syscall_64 at ffffffff9f9d4373
14 [ffffa7ce8Of3bfsO] entry_SYSCALL_64_after_hwframe at ffffffff9fa0007c

This issue dues to flush_work(), which is used to wait for
smc_connect_work() to finish in smc_release(). Once lots of
smc_connect_work() was pending or all executing work dangling,
smc_release() has to block until one worker comes to free, which
is equivalent to wait another smc_connnect_work() to finish.

In order to fix this, There are two changes:

1. For those idle smc_connect_work(), cancel it from the workqueue; for
   executing smc_connect_work(), waiting for it to finish. For that
   purpose, replace flush_work() with cancel_work_sync().

2. Since smc_connect() hold a reference for passive closing, if
   smc_connect_work() has been cancelled, release the reference.

Fixes: 24ac3a08e658 ("smc: rebuild nonblocking connect")
Reported-by: Tony Lu <[e-mail]>
Tested-by: Dust Li <[e-mail]>
Reviewed-by: Dust Li <[e-mail]>
Reviewed-by: Tony Lu <[e-mail]> 
Signed-off-by: D. Wythe <[e-mail]>
---
 net/smc/af_smc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Karsten Graul Dec. 16, 2021, 10:08 a.m. UTC | #1

On 15/12/2021 13:29, D. Wythe wrote:
> From: "D. Wythe" <[e-mail]>
> 
> In nginx/wrk benchmark, there's a hung problem with high probability
> on case likes that: (client will last several minutes to exit)
> 
> server: smc_run nginx
> 
> client: smc_run wrk -c 10000 -t 1 http://server
> 
> Client hangs with the following backtrace:

Good finding, thank you!

Acked-by: Karsten Graul <[e-mail]>

Jakub Kicinski Dec. 16, 2021, 4:13 p.m. UTC | #2

On Thu, 16 Dec 2021 11:08:13 +0100 Karsten Graul wrote:
> On 15/12/2021 13:29, D. Wythe wrote:
> > From: "D. Wythe" <[e-mail]>
> > 
> > In nginx/wrk benchmark, there's a hung problem with high probability
> > on case likes that: (client will last several minutes to exit)
> > 
> > server: smc_run nginx
> > 
> > client: smc_run wrk -c 10000 -t 1 http://server
> > 
> > Client hangs with the following backtrace:  

In the future please make sure to leave the commit title in the Fixes
tag exactly as is (you seem to have removed a "net/" prefix).

> Good finding, thank you!
> 
> Acked-by: Karsten Graul <[e-mail]>

Applied, thanks.

D. Wythe Dec. 17, 2021, 6:32 a.m. UTC | #3

Sorry for this mistake, I'll pay more attention next time.

Thanks.

On Thu, Dec 16, 2021 at 08:13:31AM -0800, Jakub Kicinski wrote:
> On Thu, 16 Dec 2021 11:08:13 +0100 Karsten Graul wrote:
> > On 15/12/2021 13:29, D. Wythe wrote:
> > > From: "D. Wythe" <[e-mail]>
> > > 
> > > In nginx/wrk benchmark, there's a hung problem with high probability
> > > on case likes that: (client will last several minutes to exit)
> > > 
> > > server: smc_run nginx
> > > 
> > > client: smc_run wrk -c 10000 -t 1 http://server
> > > 
> > > Client hangs with the following backtrace:  
> 
> In the future please make sure to leave the commit title in the Fixes
> tag exactly as is (you seem to have removed a "net/" prefix).
> 
> > Good finding, thank you!
> > 
> > Acked-by: Karsten Graul <[e-mail]>
> 
> Applied, thanks.

Patch

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index b44cc4c..5d9911c 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -195,7 +195,9 @@  static int smc_release(struct socket *sock)
 	/* cleanup for a dangling non-blocking connect */
 	if (smc->connect_nonblock && sk->sk_state == SMC_INIT)
 		tcp_abort(smc->clcsock->sk, ECONNABORTED);
-	flush_work(&smc->connect_work);
+
+	if (cancel_work_sync(&smc->connect_work))
+		sock_put(&smc->sk); /* sock_hold in smc_connect for passive closing */
 
 	if (sk->sk_state == SMC_LISTEN)
 		/* smc_close_non_accepted() is called and acquires