* [PATCH net 0/5] net/smc: fixes 2018-09-18
@ 2018-09-18 13:46 Ursula Braun
2018-09-18 13:46 ` [PATCH net 1/5] net/smc: fix non-blocking connect problem Ursula Braun
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Ursula Braun @ 2018-09-18 13:46 UTC (permalink / raw)
To: davem
Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, linux-kernel
Dave,
here are some fixes in different areas of the smc code for the net
tree.
Thanks, Ursula
Karsten Graul (1):
net/smc: no urgent data check for listen sockets
Ursula Braun (3):
net/smc: fix non-blocking connect problem
net/smc: remove duplicate mutex_unlock
net/smc: enable fallback for connection abort in state INIT
YueHaibing (1):
net/smc: fix sizeof to int comparison
net/smc/af_smc.c | 26 ++++++++++++++++----------
net/smc/smc_clc.c | 14 ++++++--------
net/smc/smc_close.c | 14 +++++++-------
3 files changed, 29 insertions(+), 25 deletions(-)
--
2.16.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net 1/5] net/smc: fix non-blocking connect problem
2018-09-18 13:46 [PATCH net 0/5] net/smc: fixes 2018-09-18 Ursula Braun
@ 2018-09-18 13:46 ` Ursula Braun
2018-09-18 13:46 ` [PATCH net 2/5] net/smc: remove duplicate mutex_unlock Ursula Braun
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Ursula Braun @ 2018-09-18 13:46 UTC (permalink / raw)
To: davem
Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, linux-kernel
In state SMC_INIT smc_poll() delegates polling to the internal
CLC socket. This means, once the connect worker has finished
its kernel_connect() step, the poll wake-up may occur. This is not
intended. The wake-up should occur from the wake up call in
smc_connect_work() after __smc_connect() has finished.
Thus in state SMC_INIT this patch now calls sock_poll_wait() on the
main SMC socket.
Signed-off-by: Ursula Braun <ubraun@linux.ibm.com>
---
net/smc/af_smc.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 2d8a1e15e4f9..9c3976bcde46 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -742,7 +742,10 @@ static void smc_connect_work(struct work_struct *work)
smc->sk.sk_err = -rc;
out:
- smc->sk.sk_state_change(&smc->sk);
+ if (smc->sk.sk_err)
+ smc->sk.sk_state_change(&smc->sk);
+ else
+ smc->sk.sk_write_space(&smc->sk);
kfree(smc->connect_info);
smc->connect_info = NULL;
release_sock(&smc->sk);
@@ -1529,7 +1532,7 @@ static __poll_t smc_poll(struct file *file, struct socket *sock,
return EPOLLNVAL;
smc = smc_sk(sock->sk);
- if ((sk->sk_state == SMC_INIT) || smc->use_fallback) {
+ if (smc->use_fallback) {
/* delegate to CLC child sock */
mask = smc->clcsock->ops->poll(file, smc->clcsock, wait);
sk->sk_err = smc->clcsock->sk->sk_err;
--
2.16.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net 2/5] net/smc: remove duplicate mutex_unlock
2018-09-18 13:46 [PATCH net 0/5] net/smc: fixes 2018-09-18 Ursula Braun
2018-09-18 13:46 ` [PATCH net 1/5] net/smc: fix non-blocking connect problem Ursula Braun
@ 2018-09-18 13:46 ` Ursula Braun
2018-09-20 9:12 ` Dan Carpenter
2018-09-18 13:46 ` [PATCH net 3/5] net/smc: enable fallback for connection abort in state INIT Ursula Braun
` (3 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Ursula Braun @ 2018-09-18 13:46 UTC (permalink / raw)
To: davem
Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, linux-kernel
For a failing smc_listen_rdma_finish() smc_listen_decline() is
called. If fallback is possible, the new socket is already enqueued
to be accepted in smc_listen_decline(). Avoid enqueuing a second time
afterwards in this case, otherwise the smc_create_lgr_pending lock
is released twice:
[ 373.463976] WARNING: bad unlock balance detected!
[ 373.463978] 4.18.0-rc7+ #123 Tainted: G O
[ 373.463979] -------------------------------------
[ 373.463980] kworker/1:1/30 is trying to release lock (smc_create_lgr_pending) at:
[ 373.463990] [<000003ff801205fc>] smc_listen_work+0x22c/0x5d0 [smc]
[ 373.463991] but there are no more locks to release!
[ 373.463991]
other info that might help us debug this:
[ 373.463993] 2 locks held by kworker/1:1/30:
[ 373.463994] #0: 00000000772cbaed ((wq_completion)"events"){+.+.}, at: process_one_work+0x1ec/0x6b0
[ 373.464000] #1: 000000003ad0894a ((work_completion)(&new_smc->smc_listen_work)){+.+.}, at: process_one_work+0x1ec/0x6b0
[ 373.464003]
stack backtrace:
[ 373.464005] CPU: 1 PID: 30 Comm: kworker/1:1 Kdump: loaded Tainted: G O 4.18.0-rc7uschi+ #123
[ 373.464007] Hardware name: IBM 2827 H43 738 (LPAR)
[ 373.464010] Workqueue: events smc_listen_work [smc]
[ 373.464011] Call Trace:
[ 373.464015] ([<0000000000114100>] show_stack+0x60/0xd8)
[ 373.464019] [<0000000000a8c9bc>] dump_stack+0x9c/0xd8
[ 373.464021] [<00000000001dcaf8>] print_unlock_imbalance_bug+0xf8/0x108
[ 373.464022] [<00000000001e045c>] lock_release+0x114/0x4f8
[ 373.464025] [<0000000000aa87fa>] __mutex_unlock_slowpath+0x4a/0x300
[ 373.464027] [<000003ff801205fc>] smc_listen_work+0x22c/0x5d0 [smc]
[ 373.464029] [<0000000000197a68>] process_one_work+0x2a8/0x6b0
[ 373.464030] [<0000000000197ec2>] worker_thread+0x52/0x410
[ 373.464033] [<000000000019fd0e>] kthread+0x15e/0x178
[ 373.464035] [<0000000000aaf58a>] kernel_thread_starter+0x6/0xc
[ 373.464052] [<0000000000aaf584>] kernel_thread_starter+0x0/0xc
[ 373.464054] INFO: lockdep is turned off.
Signed-off-by: Ursula Braun <ubraun@linux.ibm.com>
---
net/smc/af_smc.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 9c3976bcde46..5c6d30eb4a71 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1153,9 +1153,9 @@ static int smc_listen_rdma_reg(struct smc_sock *new_smc, int local_contact)
}
/* listen worker: finish RDMA setup */
-static void smc_listen_rdma_finish(struct smc_sock *new_smc,
- struct smc_clc_msg_accept_confirm *cclc,
- int local_contact)
+static int smc_listen_rdma_finish(struct smc_sock *new_smc,
+ struct smc_clc_msg_accept_confirm *cclc,
+ int local_contact)
{
struct smc_link *link = &new_smc->conn.lgr->lnk[SMC_SINGLE_LINK];
int reason_code = 0;
@@ -1178,11 +1178,12 @@ static void smc_listen_rdma_finish(struct smc_sock *new_smc,
if (reason_code)
goto decline;
}
- return;
+ return 0;
decline:
mutex_unlock(&smc_create_lgr_pending);
smc_listen_decline(new_smc, reason_code, local_contact);
+ return reason_code;
}
/* setup for RDMA connection of server */
@@ -1279,8 +1280,10 @@ static void smc_listen_work(struct work_struct *work)
}
/* finish worker */
- if (!ism_supported)
- smc_listen_rdma_finish(new_smc, &cclc, local_contact);
+ if (!ism_supported) {
+ if (smc_listen_rdma_finish(new_smc, &cclc, local_contact))
+ return;
+ }
smc_conn_save_peer_info(new_smc, &cclc);
mutex_unlock(&smc_create_lgr_pending);
smc_listen_out_connected(new_smc);
--
2.16.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net 3/5] net/smc: enable fallback for connection abort in state INIT
2018-09-18 13:46 [PATCH net 0/5] net/smc: fixes 2018-09-18 Ursula Braun
2018-09-18 13:46 ` [PATCH net 1/5] net/smc: fix non-blocking connect problem Ursula Braun
2018-09-18 13:46 ` [PATCH net 2/5] net/smc: remove duplicate mutex_unlock Ursula Braun
@ 2018-09-18 13:46 ` Ursula Braun
2018-09-18 13:46 ` [PATCH net 4/5] net/smc: no urgent data check for listen sockets Ursula Braun
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Ursula Braun @ 2018-09-18 13:46 UTC (permalink / raw)
To: davem
Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, linux-kernel
If a linkgroup is terminated abnormally already due to failing
LLC CONFIRM LINK or LLC ADD LINK, fallback to TCP is still possible.
In this case do not switch to state SMC_PEERABORTWAIT and do not set
sk_err.
Signed-off-by: Ursula Braun <ubraun@linux.ibm.com>
---
net/smc/smc_close.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
index ac961dfb1ea1..ea2b87f29469 100644
--- a/net/smc/smc_close.c
+++ b/net/smc/smc_close.c
@@ -100,15 +100,14 @@ static void smc_close_active_abort(struct smc_sock *smc)
struct smc_cdc_conn_state_flags *txflags =
&smc->conn.local_tx_ctrl.conn_state_flags;
- sk->sk_err = ECONNABORTED;
- if (smc->clcsock && smc->clcsock->sk) {
- smc->clcsock->sk->sk_err = ECONNABORTED;
- smc->clcsock->sk->sk_state_change(smc->clcsock->sk);
+ if (sk->sk_state != SMC_INIT && smc->clcsock && smc->clcsock->sk) {
+ sk->sk_err = ECONNABORTED;
+ if (smc->clcsock && smc->clcsock->sk) {
+ smc->clcsock->sk->sk_err = ECONNABORTED;
+ smc->clcsock->sk->sk_state_change(smc->clcsock->sk);
+ }
}
switch (sk->sk_state) {
- case SMC_INIT:
- sk->sk_state = SMC_PEERABORTWAIT;
- break;
case SMC_ACTIVE:
sk->sk_state = SMC_PEERABORTWAIT;
release_sock(sk);
@@ -143,6 +142,7 @@ static void smc_close_active_abort(struct smc_sock *smc)
case SMC_PEERFINCLOSEWAIT:
sock_put(sk); /* passive closing */
break;
+ case SMC_INIT:
case SMC_PEERABORTWAIT:
case SMC_CLOSED:
break;
--
2.16.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net 4/5] net/smc: no urgent data check for listen sockets
2018-09-18 13:46 [PATCH net 0/5] net/smc: fixes 2018-09-18 Ursula Braun
` (2 preceding siblings ...)
2018-09-18 13:46 ` [PATCH net 3/5] net/smc: enable fallback for connection abort in state INIT Ursula Braun
@ 2018-09-18 13:46 ` Ursula Braun
2018-09-18 13:46 ` [PATCH net 5/5] net/smc: fix sizeof to int comparison Ursula Braun
2018-09-19 3:12 ` [PATCH net 0/5] net/smc: fixes 2018-09-18 David Miller
5 siblings, 0 replies; 10+ messages in thread
From: Ursula Braun @ 2018-09-18 13:46 UTC (permalink / raw)
To: davem
Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, linux-kernel
From: Karsten Graul <kgraul@linux.ibm.com>
Don't check a listen socket for pending urgent data in smc_poll().
Signed-off-by: Karsten Graul <kgraul@linux.ibm.com>
Signed-off-by: Ursula Braun <ubraun@linux.ibm.com>
---
net/smc/af_smc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 5c6d30eb4a71..015231789ed2 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1566,9 +1566,9 @@ static __poll_t smc_poll(struct file *file, struct socket *sock,
mask |= EPOLLIN | EPOLLRDNORM | EPOLLRDHUP;
if (sk->sk_state == SMC_APPCLOSEWAIT1)
mask |= EPOLLIN;
+ if (smc->conn.urg_state == SMC_URG_VALID)
+ mask |= EPOLLPRI;
}
- if (smc->conn.urg_state == SMC_URG_VALID)
- mask |= EPOLLPRI;
}
return mask;
--
2.16.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net 5/5] net/smc: fix sizeof to int comparison
2018-09-18 13:46 [PATCH net 0/5] net/smc: fixes 2018-09-18 Ursula Braun
` (3 preceding siblings ...)
2018-09-18 13:46 ` [PATCH net 4/5] net/smc: no urgent data check for listen sockets Ursula Braun
@ 2018-09-18 13:46 ` Ursula Braun
2018-09-19 3:12 ` [PATCH net 0/5] net/smc: fixes 2018-09-18 David Miller
5 siblings, 0 replies; 10+ messages in thread
From: Ursula Braun @ 2018-09-18 13:46 UTC (permalink / raw)
To: davem
Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, linux-kernel
From: YueHaibing <yuehaibing@huawei.com>
Comparing an int to a size, which is unsigned, causes the int to become
unsigned, giving the wrong result. kernel_sendmsg can return a negative
error code.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Signed-off-by: Ursula Braun <ubraun@linux.ibm.com>
---
net/smc/smc_clc.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index 83aba9ade060..52241d679cc9 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -446,14 +446,12 @@ int smc_clc_send_proposal(struct smc_sock *smc, int smc_type,
vec[i++].iov_len = sizeof(trl);
/* due to the few bytes needed for clc-handshake this cannot block */
len = kernel_sendmsg(smc->clcsock, &msg, vec, i, plen);
- if (len < sizeof(pclc)) {
- if (len >= 0) {
- reason_code = -ENETUNREACH;
- smc->sk.sk_err = -reason_code;
- } else {
- smc->sk.sk_err = smc->clcsock->sk->sk_err;
- reason_code = -smc->sk.sk_err;
- }
+ if (len < 0) {
+ smc->sk.sk_err = smc->clcsock->sk->sk_err;
+ reason_code = -smc->sk.sk_err;
+ } else if (len < (int)sizeof(pclc)) {
+ reason_code = -ENETUNREACH;
+ smc->sk.sk_err = -reason_code;
}
return reason_code;
--
2.16.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net 0/5] net/smc: fixes 2018-09-18
2018-09-18 13:46 [PATCH net 0/5] net/smc: fixes 2018-09-18 Ursula Braun
` (4 preceding siblings ...)
2018-09-18 13:46 ` [PATCH net 5/5] net/smc: fix sizeof to int comparison Ursula Braun
@ 2018-09-19 3:12 ` David Miller
5 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2018-09-19 3:12 UTC (permalink / raw)
To: ubraun
Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, linux-kernel
From: Ursula Braun <ubraun@linux.ibm.com>
Date: Tue, 18 Sep 2018 15:46:33 +0200
> here are some fixes in different areas of the smc code for the net
> tree.
Series applied, thanks.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 2/5] net/smc: remove duplicate mutex_unlock
2018-09-18 13:46 ` [PATCH net 2/5] net/smc: remove duplicate mutex_unlock Ursula Braun
@ 2018-09-20 9:12 ` Dan Carpenter
2018-10-01 15:22 ` Ursula Braun
0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2018-09-20 9:12 UTC (permalink / raw)
To: kbuild, Ursula Braun
Cc: kbuild-all, davem, netdev, linux-s390, schwidefsky,
heiko.carstens, raspl, linux-kernel
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-fixes-2018-09-18/20180919-080857
smatch warnings:
net/smc/af_smc.c:1289 smc_listen_work() warn: inconsistent returns 'mutex:&smc_create_lgr_pending'.
Locked on: line 1285
Unlocked on: line 1209
# https://github.com/0day-ci/linux/commit/c69342ef9becfe90f3778db1c386abdd80b786d1
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout c69342ef9becfe90f3778db1c386abdd80b786d1
vim +1289 net/smc/af_smc.c
3b2dec260 Hans Wippel 2018-05-18 1231 /* IPSec connections opt out of SMC-R optimizations */
3b2dec260 Hans Wippel 2018-05-18 1232 if (using_ipsec(new_smc)) {
3b2dec260 Hans Wippel 2018-05-18 1233 smc_listen_decline(new_smc, SMC_CLC_DECL_IPSEC, 0);
3b2dec260 Hans Wippel 2018-05-18 1234 return;
3b2dec260 Hans Wippel 2018-05-18 1235 }
3b2dec260 Hans Wippel 2018-05-18 1236
3b2dec260 Hans Wippel 2018-05-18 1237 mutex_lock(&smc_create_lgr_pending);
3b2dec260 Hans Wippel 2018-05-18 1238 smc_close_init(new_smc);
3b2dec260 Hans Wippel 2018-05-18 1239 smc_rx_init(new_smc);
3b2dec260 Hans Wippel 2018-05-18 1240 smc_tx_init(new_smc);
3b2dec260 Hans Wippel 2018-05-18 1241
413498440 Hans Wippel 2018-06-28 1242 /* check if ISM is available */
413498440 Hans Wippel 2018-06-28 1243 if ((pclc->hdr.path == SMC_TYPE_D || pclc->hdr.path == SMC_TYPE_B) &&
413498440 Hans Wippel 2018-06-28 1244 !smc_check_ism(new_smc, &ismdev) &&
413498440 Hans Wippel 2018-06-28 1245 !smc_listen_ism_init(new_smc, pclc, ismdev, &local_contact)) {
413498440 Hans Wippel 2018-06-28 1246 ism_supported = true;
413498440 Hans Wippel 2018-06-28 1247 }
413498440 Hans Wippel 2018-06-28 1248
3b2dec260 Hans Wippel 2018-05-18 1249 /* check if RDMA is available */
413498440 Hans Wippel 2018-06-28 1250 if (!ism_supported &&
413498440 Hans Wippel 2018-06-28 1251 ((pclc->hdr.path != SMC_TYPE_R && pclc->hdr.path != SMC_TYPE_B) ||
7005ada68 Ursula Braun 2018-07-25 1252 smc_vlan_by_tcpsk(new_smc->clcsock, &vlan) ||
7005ada68 Ursula Braun 2018-07-25 1253 smc_check_rdma(new_smc, &ibdev, &ibport, vlan, NULL) ||
3b2dec260 Hans Wippel 2018-05-18 1254 smc_listen_rdma_check(new_smc, pclc) ||
3b2dec260 Hans Wippel 2018-05-18 1255 smc_listen_rdma_init(new_smc, pclc, ibdev, ibport,
3b2dec260 Hans Wippel 2018-05-18 1256 &local_contact) ||
413498440 Hans Wippel 2018-06-28 1257 smc_listen_rdma_reg(new_smc, local_contact))) {
3b2dec260 Hans Wippel 2018-05-18 1258 /* SMC not supported, decline */
145686baa Ursula Braun 2017-10-25 1259 mutex_unlock(&smc_create_lgr_pending);
603cc1498 Karsten Graul 2018-07-25 1260 smc_listen_decline(new_smc, SMC_CLC_DECL_MODEUNSUPP,
603cc1498 Karsten Graul 2018-07-25 1261 local_contact);
3b2dec260 Hans Wippel 2018-05-18 1262 return;
a046d57da Ursula Braun 2017-01-09 1263 }
a046d57da Ursula Braun 2017-01-09 1264
3b2dec260 Hans Wippel 2018-05-18 1265 /* send SMC Accept CLC message */
3b2dec260 Hans Wippel 2018-05-18 1266 rc = smc_clc_send_accept(new_smc, local_contact);
3b2dec260 Hans Wippel 2018-05-18 1267 if (rc) {
145686baa Ursula Braun 2017-10-25 1268 mutex_unlock(&smc_create_lgr_pending);
3b2dec260 Hans Wippel 2018-05-18 1269 smc_listen_decline(new_smc, rc, local_contact);
3b2dec260 Hans Wippel 2018-05-18 1270 return;
3b2dec260 Hans Wippel 2018-05-18 1271 }
3b2dec260 Hans Wippel 2018-05-18 1272
3b2dec260 Hans Wippel 2018-05-18 1273 /* receive SMC Confirm CLC message */
3b2dec260 Hans Wippel 2018-05-18 1274 reason_code = smc_clc_wait_msg(new_smc, &cclc, sizeof(cclc),
3b2dec260 Hans Wippel 2018-05-18 1275 SMC_CLC_CONFIRM);
3b2dec260 Hans Wippel 2018-05-18 1276 if (reason_code) {
3b2dec260 Hans Wippel 2018-05-18 1277 mutex_unlock(&smc_create_lgr_pending);
3b2dec260 Hans Wippel 2018-05-18 1278 smc_listen_decline(new_smc, reason_code, local_contact);
3b2dec260 Hans Wippel 2018-05-18 1279 return;
3b2dec260 Hans Wippel 2018-05-18 1280 }
3b2dec260 Hans Wippel 2018-05-18 1281
3b2dec260 Hans Wippel 2018-05-18 1282 /* finish worker */
c69342ef9 Ursula Braun 2018-09-18 1283 if (!ism_supported) {
c69342ef9 Ursula Braun 2018-09-18 1284 if (smc_listen_rdma_finish(new_smc, &cclc, local_contact))
c69342ef9 Ursula Braun 2018-09-18 1285 return;
^^^^^^
We need to mutex_unlock(&smc_create_lgr_pending); before the return.
c69342ef9 Ursula Braun 2018-09-18 1286 }
3b2dec260 Hans Wippel 2018-05-18 1287 smc_conn_save_peer_info(new_smc, &cclc);
3b2dec260 Hans Wippel 2018-05-18 1288 mutex_unlock(&smc_create_lgr_pending);
3b2dec260 Hans Wippel 2018-05-18 @1289 smc_listen_out_connected(new_smc);
a046d57da Ursula Braun 2017-01-09 1290 }
a046d57da Ursula Braun 2017-01-09 1291
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 2/5] net/smc: remove duplicate mutex_unlock
2018-09-20 9:12 ` Dan Carpenter
@ 2018-10-01 15:22 ` Ursula Braun
2018-10-02 6:46 ` Heiko Carstens
0 siblings, 1 reply; 10+ messages in thread
From: Ursula Braun @ 2018-10-01 15:22 UTC (permalink / raw)
To: Dan Carpenter, kbuild
Cc: kbuild-all, davem, netdev, linux-s390, schwidefsky,
heiko.carstens, raspl, linux-kernel
On 09/20/2018 11:12 AM, Dan Carpenter wrote:
> 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-fixes-2018-09-18/20180919-080857
>
> smatch warnings:
> net/smc/af_smc.c:1289 smc_listen_work() warn: inconsistent returns 'mutex:&smc_create_lgr_pending'.
> Locked on: line 1285
> Unlocked on: line 1209
>
> # https://github.com/0day-ci/linux/commit/c69342ef9becfe90f3778db1c386abdd80b786d1
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout c69342ef9becfe90f3778db1c386abdd80b786d1
> vim +1289 net/smc/af_smc.c
>
> 3b2dec260 Hans Wippel 2018-05-18 1231 /* IPSec connections opt out of SMC-R optimizations */
> 3b2dec260 Hans Wippel 2018-05-18 1232 if (using_ipsec(new_smc)) {
> 3b2dec260 Hans Wippel 2018-05-18 1233 smc_listen_decline(new_smc, SMC_CLC_DECL_IPSEC, 0);
> 3b2dec260 Hans Wippel 2018-05-18 1234 return;
> 3b2dec260 Hans Wippel 2018-05-18 1235 }
> 3b2dec260 Hans Wippel 2018-05-18 1236
> 3b2dec260 Hans Wippel 2018-05-18 1237 mutex_lock(&smc_create_lgr_pending);
> 3b2dec260 Hans Wippel 2018-05-18 1238 smc_close_init(new_smc);
> 3b2dec260 Hans Wippel 2018-05-18 1239 smc_rx_init(new_smc);
> 3b2dec260 Hans Wippel 2018-05-18 1240 smc_tx_init(new_smc);
> 3b2dec260 Hans Wippel 2018-05-18 1241
> 413498440 Hans Wippel 2018-06-28 1242 /* check if ISM is available */
> 413498440 Hans Wippel 2018-06-28 1243 if ((pclc->hdr.path == SMC_TYPE_D || pclc->hdr.path == SMC_TYPE_B) &&
> 413498440 Hans Wippel 2018-06-28 1244 !smc_check_ism(new_smc, &ismdev) &&
> 413498440 Hans Wippel 2018-06-28 1245 !smc_listen_ism_init(new_smc, pclc, ismdev, &local_contact)) {
> 413498440 Hans Wippel 2018-06-28 1246 ism_supported = true;
> 413498440 Hans Wippel 2018-06-28 1247 }
> 413498440 Hans Wippel 2018-06-28 1248
> 3b2dec260 Hans Wippel 2018-05-18 1249 /* check if RDMA is available */
> 413498440 Hans Wippel 2018-06-28 1250 if (!ism_supported &&
> 413498440 Hans Wippel 2018-06-28 1251 ((pclc->hdr.path != SMC_TYPE_R && pclc->hdr.path != SMC_TYPE_B) ||
> 7005ada68 Ursula Braun 2018-07-25 1252 smc_vlan_by_tcpsk(new_smc->clcsock, &vlan) ||
> 7005ada68 Ursula Braun 2018-07-25 1253 smc_check_rdma(new_smc, &ibdev, &ibport, vlan, NULL) ||
> 3b2dec260 Hans Wippel 2018-05-18 1254 smc_listen_rdma_check(new_smc, pclc) ||
> 3b2dec260 Hans Wippel 2018-05-18 1255 smc_listen_rdma_init(new_smc, pclc, ibdev, ibport,
> 3b2dec260 Hans Wippel 2018-05-18 1256 &local_contact) ||
> 413498440 Hans Wippel 2018-06-28 1257 smc_listen_rdma_reg(new_smc, local_contact))) {
> 3b2dec260 Hans Wippel 2018-05-18 1258 /* SMC not supported, decline */
> 145686baa Ursula Braun 2017-10-25 1259 mutex_unlock(&smc_create_lgr_pending);
> 603cc1498 Karsten Graul 2018-07-25 1260 smc_listen_decline(new_smc, SMC_CLC_DECL_MODEUNSUPP,
> 603cc1498 Karsten Graul 2018-07-25 1261 local_contact);
> 3b2dec260 Hans Wippel 2018-05-18 1262 return;
> a046d57da Ursula Braun 2017-01-09 1263 }
> a046d57da Ursula Braun 2017-01-09 1264
> 3b2dec260 Hans Wippel 2018-05-18 1265 /* send SMC Accept CLC message */
> 3b2dec260 Hans Wippel 2018-05-18 1266 rc = smc_clc_send_accept(new_smc, local_contact);
> 3b2dec260 Hans Wippel 2018-05-18 1267 if (rc) {
> 145686baa Ursula Braun 2017-10-25 1268 mutex_unlock(&smc_create_lgr_pending);
> 3b2dec260 Hans Wippel 2018-05-18 1269 smc_listen_decline(new_smc, rc, local_contact);
> 3b2dec260 Hans Wippel 2018-05-18 1270 return;
> 3b2dec260 Hans Wippel 2018-05-18 1271 }
> 3b2dec260 Hans Wippel 2018-05-18 1272
> 3b2dec260 Hans Wippel 2018-05-18 1273 /* receive SMC Confirm CLC message */
> 3b2dec260 Hans Wippel 2018-05-18 1274 reason_code = smc_clc_wait_msg(new_smc, &cclc, sizeof(cclc),
> 3b2dec260 Hans Wippel 2018-05-18 1275 SMC_CLC_CONFIRM);
> 3b2dec260 Hans Wippel 2018-05-18 1276 if (reason_code) {
> 3b2dec260 Hans Wippel 2018-05-18 1277 mutex_unlock(&smc_create_lgr_pending);
> 3b2dec260 Hans Wippel 2018-05-18 1278 smc_listen_decline(new_smc, reason_code, local_contact);
> 3b2dec260 Hans Wippel 2018-05-18 1279 return;
> 3b2dec260 Hans Wippel 2018-05-18 1280 }
> 3b2dec260 Hans Wippel 2018-05-18 1281
> 3b2dec260 Hans Wippel 2018-05-18 1282 /* finish worker */
> c69342ef9 Ursula Braun 2018-09-18 1283 if (!ism_supported) {
> c69342ef9 Ursula Braun 2018-09-18 1284 if (smc_listen_rdma_finish(new_smc, &cclc, local_contact))
> c69342ef9 Ursula Braun 2018-09-18 1285 return;
> ^^^^^^
> We need to mutex_unlock(&smc_create_lgr_pending); before the return.
>
The smatch warning is not necessary, since the mutex_unlock(&smc_create_lgr_pending)
for this case is done within smc_listen_rdma_finish().
> c69342ef9 Ursula Braun 2018-09-18 1286 }
> 3b2dec260 Hans Wippel 2018-05-18 1287 smc_conn_save_peer_info(new_smc, &cclc);
> 3b2dec260 Hans Wippel 2018-05-18 1288 mutex_unlock(&smc_create_lgr_pending);
> 3b2dec260 Hans Wippel 2018-05-18 @1289 smc_listen_out_connected(new_smc);
> a046d57da Ursula Braun 2017-01-09 1290 }
> a046d57da Ursula Braun 2017-01-09 1291
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
But you could argue the code confuses smatch and other readers. Thus I could improve
readability with a patch like this:
---
net/smc/af_smc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1184,7 +1184,6 @@ static int smc_listen_rdma_finish(struct
return 0;
decline:
- mutex_unlock(&smc_create_lgr_pending);
smc_listen_decline(new_smc, reason_code, local_contact);
return reason_code;
}
@@ -1284,8 +1283,10 @@ static void smc_listen_work(struct work_
/* finish worker */
if (!ism_supported) {
- if (smc_listen_rdma_finish(new_smc, &cclc, local_contact))
+ if (smc_listen_rdma_finish(new_smc, &cclc, local_contact)) {
+ mutex_unlock(&smc_create_lgr_pending);
return;
+ }
}
smc_conn_save_peer_info(new_smc, &cclc);
mutex_unlock(&smc_create_lgr_pending);
---
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 2/5] net/smc: remove duplicate mutex_unlock
2018-10-01 15:22 ` Ursula Braun
@ 2018-10-02 6:46 ` Heiko Carstens
0 siblings, 0 replies; 10+ messages in thread
From: Heiko Carstens @ 2018-10-02 6:46 UTC (permalink / raw)
To: Ursula Braun
Cc: Dan Carpenter, kbuild, kbuild-all, davem, netdev, linux-s390,
schwidefsky, raspl, linux-kernel
On Mon, Oct 01, 2018 at 05:22:22PM +0200, Ursula Braun wrote:
>
> > 3b2dec260 Hans Wippel 2018-05-18 1282 /* finish worker */
> > c69342ef9 Ursula Braun 2018-09-18 1283 if (!ism_supported) {
> > c69342ef9 Ursula Braun 2018-09-18 1284 if (smc_listen_rdma_finish(new_smc, &cclc, local_contact))
> > c69342ef9 Ursula Braun 2018-09-18 1285 return;
> > ^^^^^^
> > We need to mutex_unlock(&smc_create_lgr_pending); before the return.
> >
>
> The smatch warning is not necessary, since the mutex_unlock(&smc_create_lgr_pending)
> for this case is done within smc_listen_rdma_finish().
>
> > c69342ef9 Ursula Braun 2018-09-18 1286 }
> > 3b2dec260 Hans Wippel 2018-05-18 1287 smc_conn_save_peer_info(new_smc, &cclc);
> > 3b2dec260 Hans Wippel 2018-05-18 1288 mutex_unlock(&smc_create_lgr_pending);
> > 3b2dec260 Hans Wippel 2018-05-18 @1289 smc_listen_out_connected(new_smc);
> > a046d57da Ursula Braun 2017-01-09 1290 }
> > a046d57da Ursula Braun 2017-01-09 1291
> >
> > ---
> > 0-DAY kernel test infrastructure Open Source Technology Center
> > https://lists.01.org/pipermail/kbuild-all Intel Corporation
> >
>
> But you could argue the code confuses smatch and other readers. Thus I could improve
> readability with a patch like this:
Yes, please.
> ---
> net/smc/af_smc.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1184,7 +1184,6 @@ static int smc_listen_rdma_finish(struct
> return 0;
>
> decline:
> - mutex_unlock(&smc_create_lgr_pending);
> smc_listen_decline(new_smc, reason_code, local_contact);
> return reason_code;
> }
The lonely mutex_unlock() also _looks_ like a bug ;)
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-10-02 6:46 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-18 13:46 [PATCH net 0/5] net/smc: fixes 2018-09-18 Ursula Braun
2018-09-18 13:46 ` [PATCH net 1/5] net/smc: fix non-blocking connect problem Ursula Braun
2018-09-18 13:46 ` [PATCH net 2/5] net/smc: remove duplicate mutex_unlock Ursula Braun
2018-09-20 9:12 ` Dan Carpenter
2018-10-01 15:22 ` Ursula Braun
2018-10-02 6:46 ` Heiko Carstens
2018-09-18 13:46 ` [PATCH net 3/5] net/smc: enable fallback for connection abort in state INIT Ursula Braun
2018-09-18 13:46 ` [PATCH net 4/5] net/smc: no urgent data check for listen sockets Ursula Braun
2018-09-18 13:46 ` [PATCH net 5/5] net/smc: fix sizeof to int comparison Ursula Braun
2018-09-19 3:12 ` [PATCH net 0/5] net/smc: fixes 2018-09-18 David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).