| Message ID | CAM_iQpWaHX_orHbFjKcjBZL67b6LUAmOfvS1SQP13VPZDo-69w@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Jan 10, 2017 at 9:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Mon, Jan 9, 2017 at 9:20 AM, Andrey Konovalov <andreyknvl@google.com> wrote: > > The fix should be straight-forward. Mind to try the attached patch? You forgot to remove schedule() ? schedule(); + wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
On Tue, Jan 10, 2017 at 9:40 AM, Eric Dumazet <edumazet@google.com> wrote: > On Tue, Jan 10, 2017 at 9:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> On Mon, Jan 9, 2017 at 9:20 AM, Andrey Konovalov <andreyknvl@google.com> wrote: >> >> The fix should be straight-forward. Mind to try the attached patch? > > > You forgot to remove schedule() ? > > schedule(); > + wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT); Ah, of course, never even compile it... :-/
Eric Dumazet <edumazet@google.com> : > On Tue, Jan 10, 2017 at 9:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Mon, Jan 9, 2017 at 9:20 AM, Andrey Konovalov <andreyknvl@google.com> wrote: > > > > The fix should be straight-forward. Mind to try the attached patch? > > > You forgot to remove schedule() ? It may be clearer to split alloc_tx in two parts: only the unsleepable "if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {" part of it contributes to the inner "while (!(skb = alloc_tx(vcc, eff))) {" block. See net/atm/common.c [...] static struct sk_buff *alloc_tx(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; } 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; } The waiting stuff is related to vcc drain but the code makes it look as if it were also related to skb alloc (it isn't). It may be obvious for you but it took me a while to figure what the code is supposed to achieve.
On Tue, Jan 10, 2017 at 6:40 PM, Eric Dumazet <edumazet@google.com> wrote: > On Tue, Jan 10, 2017 at 9:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> On Mon, Jan 9, 2017 at 9:20 AM, Andrey Konovalov <andreyknvl@google.com> wrote: >> >> The fix should be straight-forward. Mind to try the attached patch? Hi Cong, Your patch with schedule() removed as suggested by Eric fixes the issue. Tested-by: Andrey Konovalov <andreyknvl@google.com> Thanks! > > > You forgot to remove schedule() ? > > schedule(); > + wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
On Tue, 2017-01-10 at 23:47 +0100, Francois Romieu wrote: > Eric Dumazet <edumazet@google.com> : > > On Tue, Jan 10, 2017 at 9:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > On Mon, Jan 9, 2017 at 9:20 AM, Andrey Konovalov <andreyknvl@google.com> wrote: > > > > > > The fix should be straight-forward. Mind to try the attached patch? > > > > > > You forgot to remove schedule() ? > > It may be clearer to split alloc_tx in two parts: only the unsleepable > "if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {" part of it > contributes to the inner "while (!(skb = alloc_tx(vcc, eff))) {" block. > > See net/atm/common.c > [...] > static struct sk_buff *alloc_tx(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; > } > while (!(skb = alloc_skb(size, GFP_KERNEL))) > schedule(); Yeah, this code looks quite wrong anyway. We can read it as an infinite loop in some stress conditions or memcg constraints. > The waiting stuff is related to vcc drain but the code makes it look as > if it were also related to skb alloc (it isn't). > > It may be obvious for you but it took me a while to figure what the > code is supposed to achieve. >
diff --git a/net/atm/common.c b/net/atm/common.c index a3ca922..b7d9661 100644 --- a/net/atm/common.c +++ b/net/atm/common.c @@ -571,8 +571,8 @@ int vcc_recvmsg(struct socket *sock, struct msghdr *msg, size_t size, int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size) { + DEFINE_WAIT_FUNC(wait, woken_wake_function); struct sock *sk = sock->sk; - DEFINE_WAIT(wait); struct atm_vcc *vcc; struct sk_buff *skb; int eff, error; @@ -604,7 +604,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); + add_wait_queue(sk_sleep(sk), &wait); error = 0; while (!(skb = alloc_tx(vcc, eff))) { if (m->msg_flags & MSG_DONTWAIT) { @@ -612,6 +612,7 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size) break; } schedule(); + wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT); if (signal_pending(current)) { error = -ERESTARTSYS; break; @@ -623,9 +624,8 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size) send_sig(SIGPIPE, current, 0); break; } - prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE); } - finish_wait(sk_sleep(sk), &wait); + remove_wait_queue(sk_sleep(sk), &wait); if (error) goto out; skb->dev = NULL; /* for paths shared with net_device interfaces */