| Message ID | 20180620080737.50323-1-ubraun@linux.ibm.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,1/1] net/smc: coordinate wait queues for nonblocking connect | expand |
On Wed, Jun 20, 2018 at 1:07 AM, Ursula Braun <ubraun@linux.ibm.com> wrote: > diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c > index da7f02edcd37..21c84b924ffb 100644 > --- a/net/smc/af_smc.c > +++ b/net/smc/af_smc.c > @@ -605,6 +605,8 @@ static int smc_connect(struct socket *sock, struct sockaddr *addr, > > smc_copy_sock_settings_to_clc(smc); > tcp_sk(smc->clcsock->sk)->syn_smc = 1; > + if (flags & O_NONBLOCK) > + sock->sk->sk_wq = smc->clcsock->sk->sk_wq; > rc = kernel_connect(smc->clcsock, addr, alen, flags); > if (rc) > goto out; > @@ -1285,12 +1287,9 @@ static __poll_t smc_poll_mask(struct socket *sock, __poll_t events) > > smc = smc_sk(sock->sk); > sock_hold(sk); > - lock_sock(sk); > if ((sk->sk_state == SMC_INIT) || smc->use_fallback) { > /* delegate to CLC child sock */ > - release_sock(sk); > mask = smc->clcsock->ops->poll_mask(smc->clcsock, events); > - lock_sock(sk); > sk->sk_err = smc->clcsock->sk->sk_err; > if (sk->sk_err) { > mask |= EPOLLERR; > @@ -1299,7 +1298,10 @@ static __poll_t smc_poll_mask(struct socket *sock, __poll_t events) > if (sk->sk_state == SMC_INIT && > mask & EPOLLOUT && > smc->clcsock->sk->sk_state != TCP_CLOSE) { > + sock->sk->sk_wq = smc->smcwq; > + lock_sock(sk); I think you need to use proper RCU API to protect these assignment of sk->sk_wq.
Hi Ursula,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net/master]
url: https://github.com/0day-ci/linux/commits/Ursula-Braun/net-smc-coordinate-wait-queues-for-nonblocking-connect/20180620-180901
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
>> net/smc/af_smc.c:1301:49: sparse: incorrect type in assignment (different address spaces) @@ expected struct socket_wq [noderef] <asn:4>*sk_wq @@ got [noderef] <asn:4>*sk_wq @@
net/smc/af_smc.c:1301:49: expected struct socket_wq [noderef] <asn:4>*sk_wq
net/smc/af_smc.c:1301:49: got struct socket_wq *smcwq
net/smc/smc_cdc.h:143:24: sparse: expression using sizeof(void)
net/smc/smc_cdc.h:146:16: sparse: expression using sizeof(void)
net/smc/smc_cdc.h:143:24: sparse: expression using sizeof(void)
net/smc/smc_cdc.h:146:16: sparse: expression using sizeof(void)
>> net/smc/af_smc.c:1667:20: sparse: incorrect type in assignment (different address spaces) @@ expected struct socket_wq *smcwq @@ got struct socket_wq struct socket_wq *smcwq @@
net/smc/af_smc.c:1667:20: expected struct socket_wq *smcwq
net/smc/af_smc.c:1667:20: got struct socket_wq [noderef] <asn:4>*sk_wq
net/smc/af_smc.c:1668:29: sparse: expression using sizeof(void)
net/smc/af_smc.c:1669:29: sparse: expression using sizeof(void)
vim +1301 net/smc/af_smc.c
1277
1278 static __poll_t smc_poll_mask(struct socket *sock, __poll_t events)
1279 {
1280 struct sock *sk = sock->sk;
1281 __poll_t mask = 0;
1282 struct smc_sock *smc;
1283 int rc;
1284
1285 if (!sk)
1286 return EPOLLNVAL;
1287
1288 smc = smc_sk(sock->sk);
1289 sock_hold(sk);
1290 if ((sk->sk_state == SMC_INIT) || smc->use_fallback) {
1291 /* delegate to CLC child sock */
1292 mask = smc->clcsock->ops->poll_mask(smc->clcsock, events);
1293 sk->sk_err = smc->clcsock->sk->sk_err;
1294 if (sk->sk_err) {
1295 mask |= EPOLLERR;
1296 } else {
1297 /* if non-blocking connect finished ... */
1298 if (sk->sk_state == SMC_INIT &&
1299 mask & EPOLLOUT &&
1300 smc->clcsock->sk->sk_state != TCP_CLOSE) {
> 1301 sock->sk->sk_wq = smc->smcwq;
1302 lock_sock(sk);
1303 rc = __smc_connect(smc);
1304 release_sock(sk);
1305 if (rc < 0)
1306 mask |= EPOLLERR;
1307 /* success cases including fallback */
1308 mask |= EPOLLOUT | EPOLLWRNORM;
1309 }
1310 }
1311 } else {
1312 if (sk->sk_err)
1313 mask |= EPOLLERR;
1314 if ((sk->sk_shutdown == SHUTDOWN_MASK) ||
1315 (sk->sk_state == SMC_CLOSED))
1316 mask |= EPOLLHUP;
1317 if (sk->sk_state == SMC_LISTEN) {
1318 /* woken up by sk_data_ready in smc_listen_work() */
1319 mask = smc_accept_poll(sk);
1320 } else {
1321 if (atomic_read(&smc->conn.sndbuf_space) ||
1322 sk->sk_shutdown & SEND_SHUTDOWN) {
1323 mask |= EPOLLOUT | EPOLLWRNORM;
1324 } else {
1325 sk_set_bit(SOCKWQ_ASYNC_NOSPACE, sk);
1326 set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
1327 }
1328 if (atomic_read(&smc->conn.bytes_to_rcv))
1329 mask |= EPOLLIN | EPOLLRDNORM;
1330 if (sk->sk_shutdown & RCV_SHUTDOWN)
1331 mask |= EPOLLIN | EPOLLRDNORM | EPOLLRDHUP;
1332 if (sk->sk_state == SMC_APPCLOSEWAIT1)
1333 mask |= EPOLLIN;
1334 }
1335 if (smc->conn.urg_state == SMC_URG_VALID)
1336 mask |= EPOLLPRI;
1337
1338 }
1339 sock_put(sk);
1340
1341 return mask;
1342 }
1343
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c index da7f02edcd37..21c84b924ffb 100644 --- a/net/smc/af_smc.c +++ b/net/smc/af_smc.c @@ -605,6 +605,8 @@ static int smc_connect(struct socket *sock, struct sockaddr *addr, smc_copy_sock_settings_to_clc(smc); tcp_sk(smc->clcsock->sk)->syn_smc = 1; + if (flags & O_NONBLOCK) + sock->sk->sk_wq = smc->clcsock->sk->sk_wq; rc = kernel_connect(smc->clcsock, addr, alen, flags); if (rc) goto out; @@ -1285,12 +1287,9 @@ static __poll_t smc_poll_mask(struct socket *sock, __poll_t events) smc = smc_sk(sock->sk); sock_hold(sk); - lock_sock(sk); if ((sk->sk_state == SMC_INIT) || smc->use_fallback) { /* delegate to CLC child sock */ - release_sock(sk); mask = smc->clcsock->ops->poll_mask(smc->clcsock, events); - lock_sock(sk); sk->sk_err = smc->clcsock->sk->sk_err; if (sk->sk_err) { mask |= EPOLLERR; @@ -1299,7 +1298,10 @@ static __poll_t smc_poll_mask(struct socket *sock, __poll_t events) if (sk->sk_state == SMC_INIT && mask & EPOLLOUT && smc->clcsock->sk->sk_state != TCP_CLOSE) { + sock->sk->sk_wq = smc->smcwq; + lock_sock(sk); rc = __smc_connect(smc); + release_sock(sk); if (rc < 0) mask |= EPOLLERR; /* success cases including fallback */ @@ -1334,7 +1336,6 @@ static __poll_t smc_poll_mask(struct socket *sock, __poll_t events) mask |= EPOLLPRI; } - release_sock(sk); sock_put(sk); return mask; @@ -1663,6 +1664,7 @@ static int smc_create(struct net *net, struct socket *sock, int protocol, sk_common_release(sk); goto out; } + smc->smcwq = sk->sk_wq; smc->sk.sk_sndbuf = max(smc->clcsock->sk->sk_sndbuf, SMC_BUF_MIN_SIZE); smc->sk.sk_rcvbuf = max(smc->clcsock->sk->sk_rcvbuf, SMC_BUF_MIN_SIZE); diff --git a/net/smc/smc.h b/net/smc/smc.h index 51ae1f10d81a..89d6d7ef973f 100644 --- a/net/smc/smc.h +++ b/net/smc/smc.h @@ -190,6 +190,7 @@ struct smc_connection { struct smc_sock { /* smc sock container */ struct sock sk; struct socket *clcsock; /* internal tcp socket */ + struct socket_wq *smcwq; /* original smcsock wq */ struct smc_connection conn; /* smc connection */ struct smc_sock *listen_smc; /* listen parent */ struct work_struct tcp_listen_work;/* handle tcp socket accepts */
The recent poll change may lead to stalls for non-blocking connecting SMC sockets, since sock_poll_wait is no longer performed on the internal CLC socket, but on the outer SMC socket. kernel_connect() on the internal CLC socket returns with -EINPROGRESS, but the wake up logic does not work in all cases. If the internal CLC socket is still in state TCP_SYN_SENT when polled, sock_poll_wait() from sock_poll() does not sleep. It is supposed to sleep till the state of the internal CLC socket switches to TCP_ESTABLISHED. This patch temporarily propagates the wait queue from the internal CLC sock to the SMC sock, till the non-blocking connect() is finished. In addition locking is reduced due to the removed poll waits. Fixes: c0129a061442 ("smc: convert to ->poll_mask") Signed-off-by: Ursula Braun <ubraun@linux.ibm.com> --- net/smc/af_smc.c | 10 ++++++---- net/smc/smc.h | 1 + 2 files changed, 7 insertions(+), 4 deletions(-)