When application sending data more than sndbuf_space, there have chances
application will sleep in epoll_wait, and will never be wakeup again. This
is caused by a race between smc_poll and smc_cdc_tx_handler.
application tasklet
smc_tx_sendmsg(len > sndbuf_space) |
epoll_wait for EPOLL_OUT,timeout=0 |
smc_poll |
if (!smc->conn.sndbuf_space) |
| smc_cdc_tx_handler
| atomic_add sndbuf_space
| smc_tx_sndbuf_nonfull
| if (!test_bit SOCK_NOSPACE)
| do not sk_write_space;
set_bit SOCK_NOSPACE; |
return mask=0; |
Application will sleep in epoll_wait as smc_poll returns 0. And
smc_cdc_tx_handler will not call sk_write_space because the SOCK_NOSPACE
has not be set. If there is no inflight cdc msg, sk_write_space will not be
called any more, and application will sleep in epoll_wait forever.
So set SOCK_NOSPACE when send_remaining but no sndbuf_space left in
smc_tx_sendmsg, to ensure call sk_write_space in smc_cdc_tx_handler
even when the above race happens.
Fixes: 6889b36da78a ("net/smc: don't wait for send buffer space when data was already sent")
Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
Reviewed-by: Wen Gu <guwen@linux.alibaba.com>
Reviewed-by: D. Wythe <alibuda@linux.alibaba.com>
---
net/smc/smc_tx.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
index 214ac3cbcf9a..60cfec8eb255 100644
--- a/net/smc/smc_tx.c
+++ b/net/smc/smc_tx.c
@@ -222,8 +222,11 @@ int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len)
conn->local_tx_ctrl.prod_flags.urg_data_pending = 1;
if (!atomic_read(&conn->sndbuf_space) || conn->urg_tx_pend) {
- if (send_done)
+ if (send_done) {
+ sk_set_bit(SOCKWQ_ASYNC_NOSPACE, sk);
+ set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
return send_done;
+ }
rc = smc_tx_wait(smc, msg->msg_flags);
if (rc)
goto out_err;
--
2.24.3 (Apple Git-128)On 11/28/24 13:14, Guangguan Wang wrote: > When application sending data more than sndbuf_space, there have chances > application will sleep in epoll_wait, and will never be wakeup again. This > is caused by a race between smc_poll and smc_cdc_tx_handler. > > application tasklet > smc_tx_sendmsg(len > sndbuf_space) | > epoll_wait for EPOLL_OUT,timeout=0 | > smc_poll | > if (!smc->conn.sndbuf_space) | > | smc_cdc_tx_handler > | atomic_add sndbuf_space > | smc_tx_sndbuf_nonfull > | if (!test_bit SOCK_NOSPACE) > | do not sk_write_space; > set_bit SOCK_NOSPACE; | > return mask=0; | > > Application will sleep in epoll_wait as smc_poll returns 0. And > smc_cdc_tx_handler will not call sk_write_space because the SOCK_NOSPACE > has not be set. If there is no inflight cdc msg, sk_write_space will not be > called any more, and application will sleep in epoll_wait forever. > So set SOCK_NOSPACE when send_remaining but no sndbuf_space left in > smc_tx_sendmsg, to ensure call sk_write_space in smc_cdc_tx_handler > even when the above race happens. I think it should be preferable to address the mentioned race the same way as tcp_poll(). i.e. checking again smc->conn.sndbuf_space after setting the NOSPACE bit with appropriate barrier, see: https://elixir.bootlin.com/linux/v6.12.1/source/net/ipv4/tcp.c#L590 that will avoid additional, possibly unneeded atomic operation in the tx path (the application could do the next sendmsg()/poll() call after that the send buf has been freed) and will avoid some code duplication. Cheers, Paolo
On 2024/12/3 18:04, Paolo Abeni wrote: > > > On 11/28/24 13:14, Guangguan Wang wrote: >> When application sending data more than sndbuf_space, there have chances >> application will sleep in epoll_wait, and will never be wakeup again. This >> is caused by a race between smc_poll and smc_cdc_tx_handler. >> >> application tasklet >> smc_tx_sendmsg(len > sndbuf_space) | >> epoll_wait for EPOLL_OUT,timeout=0 | >> smc_poll | >> if (!smc->conn.sndbuf_space) | >> | smc_cdc_tx_handler >> | atomic_add sndbuf_space >> | smc_tx_sndbuf_nonfull >> | if (!test_bit SOCK_NOSPACE) >> | do not sk_write_space; >> set_bit SOCK_NOSPACE; | >> return mask=0; | >> >> Application will sleep in epoll_wait as smc_poll returns 0. And >> smc_cdc_tx_handler will not call sk_write_space because the SOCK_NOSPACE >> has not be set. If there is no inflight cdc msg, sk_write_space will not be >> called any more, and application will sleep in epoll_wait forever. >> So set SOCK_NOSPACE when send_remaining but no sndbuf_space left in >> smc_tx_sendmsg, to ensure call sk_write_space in smc_cdc_tx_handler >> even when the above race happens. > > I think it should be preferable to address the mentioned race the same > way as tcp_poll(). i.e. checking again smc->conn.sndbuf_space after > setting the NOSPACE bit with appropriate barrier, see: > > https://elixir.bootlin.com/linux/v6.12.1/source/net/ipv4/tcp.c#L590 > > that will avoid additional, possibly unneeded atomic operation in the tx > path (the application could do the next sendmsg()/poll() call after that > the send buf has been freed) and will avoid some code duplication. > > Cheers, > > Paolo Hi, Paolo Thanks for advice, and the way in tcp_poll() seems a better solution for this race. I will retest it, and resend a new version of patch if it works. Thanks, Guangguan Wang
© 2016 - 2026 Red Hat, Inc.