| Message ID | 1352292734.7340.35.camel@shinybook.infradead.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: David Woodhouse <dwmw2@infradead.org> Date: Wed, 07 Nov 2012 12:52:14 +0000 > Now that we can return zero from pppoatm_send() for reasons *other* than > the queue being full, that means we can't depend on a subsequent call to > pppoatm_pop() waking the queue, and we might leave it stalled > indefinitely. > > Fix this by immediately scheduling the wakeup tasklet. As documented > already elsewhere, the PPP channel's ->downl lock protects against the > wakeup happening too soon and effectively being missed. > > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> > ---- > Untested. > With this sorted, Acked-By: David Woodhouse <David.Woodhouse@intel.com< > to the other seven. Thanks. I don't know what to do with this patch because I don't have any context whatsoever. So I'm tossing it, please resubmit it when it's meant to be applied, and with some context. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2012-11-09 at 16:30 -0500, David Miller wrote: > I don't know what to do with this patch because I don't have any > context whatsoever. I sent two replies to Krzysztof's series starting with [PATCH v3 0/7] in Message-Id: <1352240222-363-1-git-send-email-krzysiek@podlesie.net> The first was pointing out a problem; the second was a [PATCH v3 8/7] which fixed the problem I'd pointed out. It wasn't clear to me that more context would be needed. In particular, [PATCH v3 8/7] was a reply to 0/7, just as the other patches ##1-7 had been. > So I'm tossing it, please resubmit it when it's meant to be > applied, and with some context. That's OK. I was hoping for an ack from Chas and/or Krzysztof, especially as I hadn't tested my patch. So hopefully there'll be a v4 series of 8 patches, including this one... and all from the same person, which makes it slightly easier to follow :)
From: David Woodhouse <dwmw2@infradead.org> Date: Sat, 10 Nov 2012 07:36:13 +0000 > I was hoping for an ack from Chas and/or Krzysztof, especially as I > hadn't tested my patch. So hopefully there'll be a v4 series of 8 > patches, including this one... and all from the same person, which > makes it slightly easier to follow :) Ok, great. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 07, 2012 at 12:52:14PM +0000, David Woodhouse wrote: > > diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c > index 7507c20..56ad541 100644 > --- a/net/atm/pppoatm.c > +++ b/net/atm/pppoatm.c > @@ -283,11 +283,11 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb) > vcc = ATM_SKB(skb)->vcc; > bh_lock_sock(sk_atm(vcc)); > if (sock_owned_by_user(sk_atm(vcc))) > - goto nospace; > + goto nospace_sched_wakeup; > if (test_bit(ATM_VF_RELEASED, &vcc->flags) > - || test_bit(ATM_VF_CLOSE, &vcc->flags) > - || !test_bit(ATM_VF_READY, &vcc->flags)) > - goto nospace; > + || test_bit(ATM_VF_CLOSE, &vcc->flags) > + || !test_bit(ATM_VF_READY, &vcc->flags)) > + goto nospace_sched_wakeup; > > switch (pvcc->encaps) { /* LLC encapsulation needed */ > case e_llc: > @@ -328,7 +328,17 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb) > ? DROP_PACKET : 1; > bh_unlock_sock(sk_atm(vcc)); > return ret; > -nospace: > + nospace_sched_wakeup: > + /* If we're returning zero for reasons *other* than the queue > + * being full, then we need to ensure that a wakeup will > + * happen and not just leave the channel stalled for ever. > + * Just schedule the wakeup tasklet directly. As observed in > + * pppoatm_pop(), it'll take the channel's ->downl lock which > + * is also held by our caller, so it can't happen "too soon" > + * and cause us to effectively miss a wakeup. > + */ > + tasklet_schedule(&pvcc->wakeup_tasklet); With this tasklet_schedule() we implement a "spin_lock" here, but in this case both conditions (vcc not ready and socket locked) can be true for a long time and we can spin here for a long time. I confirmed it by reverting patch 1 (atm: detach protocol before closing vcc) and now I have 50% of CPU used by ksoftirqd and 50% by pppd (UP system). Krzysiek -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2012-11-10 at 21:23 +0100, Krzysztof Mazur wrote: > With this tasklet_schedule() we implement a "spin_lock" here, but in > this case both conditions (vcc not ready and socket locked) can be > true for a long time and we can spin here for a long time. I confirmed > it by reverting patch 1 (atm: detach protocol before closing vcc) and > now I have 50% of CPU used by ksoftirqd and 50% by pppd (UP system). Ah, thanks. Can we take the lock in the tasklet, so we wait for it instead of spinning?
On Sat, 2012-11-10 at 21:23 +0100, Krzysztof Mazur wrote: > With this tasklet_schedule() we implement a "spin_lock" here, but in > this case both conditions (vcc not ready and socket locked) can be > true for a long time and we can spin here for a long time. Reading this more carefully this morning... I hadn't realised it was these conditions, and not the sock_owned_by_user(), which had triggered. Yes, perhaps we should just return zero in that case and find another wakeup trigger... if indeed a wakeup is ever required in the VF_RELEASED and VF_CLOSE case. And if we've fixed things so that !VF_READY can never happen (have we?).... perhaps this one doesn't matter at all? It was the sock_owned_by_user() case I was most interested in, and I was expecting that lock would generally be held briefly enough that the tasklet would be fine. Was that not so?
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c index 7507c20..56ad541 100644 --- a/net/atm/pppoatm.c +++ b/net/atm/pppoatm.c @@ -283,11 +283,11 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb) vcc = ATM_SKB(skb)->vcc; bh_lock_sock(sk_atm(vcc)); if (sock_owned_by_user(sk_atm(vcc))) - goto nospace; + goto nospace_sched_wakeup; if (test_bit(ATM_VF_RELEASED, &vcc->flags) - || test_bit(ATM_VF_CLOSE, &vcc->flags) - || !test_bit(ATM_VF_READY, &vcc->flags)) - goto nospace; + || test_bit(ATM_VF_CLOSE, &vcc->flags) + || !test_bit(ATM_VF_READY, &vcc->flags)) + goto nospace_sched_wakeup; switch (pvcc->encaps) { /* LLC encapsulation needed */ case e_llc: @@ -328,7 +328,17 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb) ? DROP_PACKET : 1; bh_unlock_sock(sk_atm(vcc)); return ret; -nospace: + nospace_sched_wakeup: + /* If we're returning zero for reasons *other* than the queue + * being full, then we need to ensure that a wakeup will + * happen and not just leave the channel stalled for ever. + * Just schedule the wakeup tasklet directly. As observed in + * pppoatm_pop(), it'll take the channel's ->downl lock which + * is also held by our caller, so it can't happen "too soon" + * and cause us to effectively miss a wakeup. + */ + tasklet_schedule(&pvcc->wakeup_tasklet); + nospace: bh_unlock_sock(sk_atm(vcc)); /* * We don't have space to send this SKB now, but we might have
Now that we can return zero from pppoatm_send() for reasons *other* than the queue being full, that means we can't depend on a subsequent call to pppoatm_pop() waking the queue, and we might leave it stalled indefinitely. Fix this by immediately scheduling the wakeup tasklet. As documented already elsewhere, the PPP channel's ->downl lock protects against the wakeup happening too soon and effectively being missed. Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> ---- Untested. With this sorted, Acked-By: David Woodhouse <David.Woodhouse@intel.com< to the other seven. Thanks.