* [Patch net-next] atm: remove an unnecessary loop
@ 2017-03-12 0:41 Chas Williams
2017-03-13 22:37 ` David Miller
0 siblings, 1 reply; 2+ messages in thread
From: Chas Williams @ 2017-03-12 0:41 UTC (permalink / raw)
To: netdev; +Cc: Francois Romieu, Chas Williams
From: Francois Romieu <romieu@fr.zoreil.com>
Andrey reported this kernel warning:
WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 __might_sleep+0x149/0x1a0
do not call blocking ops when !TASK_RUNNING; state=1 set at
[<ffffffff813fcb22>] prepare_to_wait+0x182/0x530
The deeply nested alloc_skb is a problem.
Diagnosis: nesting is wrong. It makes zero sense. Fix it and the
implicit task state change problem automagically goes away.
alloc_skb() does not need to be in the "while" loop.
alloc_skb() does not need to be in the {prepare_to_wait/add_wait_queue ...
finish_wait/remove_wait_queue} block.
I claim that:
- alloc_tx() should only perform the "wait_for_decent_tx_drain" part
- alloc_skb() ought to be done directly in vcc_sendmsg
- alloc_skb() failure can be handled gracefully in vcc_sendmsg
- alloc_skb() may use a (m->msg_flags & MSG_DONTWAIT) dependent
GFP_{KERNEL / ATOMIC} flag
Reported-by: Andrey Konovalov <andreyknvl@google.com>
Reviewed-and-Tested-by: Chas Williams <3chas3@gmail.com>
Signed-off-by: Chas Williams <3chas3@gmail.com>
---
net/atm/common.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/net/atm/common.c b/net/atm/common.c
index 9613381..f06422f 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -62,21 +62,16 @@ static void vcc_remove_socket(struct sock *sk)
write_unlock_irq(&vcc_sklist_lock);
}
-static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
+static bool vcc_tx_ready(struct atm_vcc *vcc, unsigned int size)
{
- struct sk_buff *skb;
struct sock *sk = sk_atm(vcc);
if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {
pr_debug("Sorry: wmem_alloc = %d, size = %d, sndbuf = %d\n",
sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
- return NULL;
+ return false;
}
- while (!(skb = alloc_skb(size, GFP_KERNEL)))
- schedule();
- pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
- atomic_add(skb->truesize, &sk->sk_wmem_alloc);
- return skb;
+ return true;
}
static void vcc_sock_destruct(struct sock *sk)
@@ -606,7 +601,7 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
eff = (size+3) & ~3; /* align to word boundary */
prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
error = 0;
- while (!(skb = alloc_tx(vcc, eff))) {
+ while (!vcc_tx_ready(vcc, eff)) {
if (m->msg_flags & MSG_DONTWAIT) {
error = -EAGAIN;
break;
@@ -628,6 +623,15 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
finish_wait(sk_sleep(sk), &wait);
if (error)
goto out;
+
+ skb = alloc_skb(eff, GFP_KERNEL);
+ if (!skb) {
+ error = -ENOMEM;
+ goto out;
+ }
+ pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
+ atomic_add(skb->truesize, &sk->sk_wmem_alloc);
+
skb->dev = NULL; /* for paths shared with net_device interfaces */
ATM_SKB(skb)->atm_options = vcc->atm_options;
if (!copy_from_iter_full(skb_put(skb, size), size, &m->msg_iter)) {
--
2.7.4
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Patch net-next] atm: remove an unnecessary loop
2017-03-12 0:41 [Patch net-next] atm: remove an unnecessary loop Chas Williams
@ 2017-03-13 22:37 ` David Miller
0 siblings, 0 replies; 2+ messages in thread
From: David Miller @ 2017-03-13 22:37 UTC (permalink / raw)
To: 3chas3; +Cc: netdev, romieu
From: Chas Williams <3chas3@gmail.com>
Date: Sat, 11 Mar 2017 19:41:36 -0500
> From: Francois Romieu <romieu@fr.zoreil.com>
>
> Andrey reported this kernel warning:
>
> WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 __might_sleep+0x149/0x1a0
> do not call blocking ops when !TASK_RUNNING; state=1 set at
> [<ffffffff813fcb22>] prepare_to_wait+0x182/0x530
>
> The deeply nested alloc_skb is a problem.
>
> Diagnosis: nesting is wrong. It makes zero sense. Fix it and the
> implicit task state change problem automagically goes away.
>
> alloc_skb() does not need to be in the "while" loop.
>
> alloc_skb() does not need to be in the {prepare_to_wait/add_wait_queue ...
> finish_wait/remove_wait_queue} block.
>
> I claim that:
> - alloc_tx() should only perform the "wait_for_decent_tx_drain" part
> - alloc_skb() ought to be done directly in vcc_sendmsg
> - alloc_skb() failure can be handled gracefully in vcc_sendmsg
> - alloc_skb() may use a (m->msg_flags & MSG_DONTWAIT) dependent
> GFP_{KERNEL / ATOMIC} flag
>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Reviewed-and-Tested-by: Chas Williams <3chas3@gmail.com>
> Signed-off-by: Chas Williams <3chas3@gmail.com>
Applied, thanks a lot Chas.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-03-13 22:37 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-12 0:41 [Patch net-next] atm: remove an unnecessary loop Chas Williams
2017-03-13 22:37 ` 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).