| Message ID | 1354094649.21562.34.camel@shinybook.infradead.org |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Nov 28, 2012 at 09:24:09AM +0000, David Woodhouse wrote: > On Wed, 2012-11-28 at 09:12 +0100, Krzysztof Mazur wrote: > > On Wed, Nov 28, 2012 at 12:48:17AM +0000, David Woodhouse wrote: > > > On Tue, 2012-11-27 at 10:23 -0500, chas williams - CONTRACTOR wrote: > > > > yes, but dont call it 8/7 since that doesnt make sense. > > > > > > It made enough sense when it was a single patch appended to a thread of > > > 7 other patches from Krzysztof. But now it's all got a little more > > > complex, so I've tried to collect together the latest version of > > > everything we've discussed: > > > > There was also discussion about patch 9/7 "pppoatm: wakeup after ATM > > unlock only when it's needed". > > True. Is that really necessary? How often is the lock actually taken? Is > it once per packet that PPP sends (which is mostly just LCP > echo/response during an active connection)? And does that really warrant > the optimisation? > > This is a tasklet that we used to run after absolutely *every* packet, > remember. Optimising *that* made sense, but I'm less sure it's worth the > added complexity for this case. As I have a vague recollection that we > decided we couldn't use the existing BLOCKED bit for it... or can we? > > Can this work? Feel free to replace that test_bit() and the > corresponding comment, with a test_and_clear_bit() and a new comment > explaining *why* it's safe... while I go make another cup of tea. > ok, I think that we should just drop that patch, with test_bit() I think it's no longer an optimization. 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
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c index 446a7f0..da58863 100644 --- a/net/atm/pppoatm.c +++ b/net/atm/pppoatm.c @@ -113,7 +113,13 @@ static void pppoatm_release_cb(struct atm_vcc *atmvcc) { struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc); - tasklet_schedule(&pvcc->wakeup_tasklet); + /* + * We can't clear it here because I haven't had enough caffeine + * this morning to deal with the concurrency issues. Just leave + * it set, and let pppoatm_pop() clear it later. + */ + if (test_bit(BLOCKED, &pvcc->blocked)) + tasklet_schedule(&pvcc->wakeup_tasklet); if (pvcc->old_release_cb) pvcc->old_release_cb(atmvcc); } @@ -342,6 +348,12 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb) bh_unlock_sock(sk_atm(vcc)); return ret; nospace: + /* + * Needs to happen (and be flushed, hence test_and_) before we unlock + * the socket. It needs to be seen by the time our ->release_cb gets + * called. + */ + test_and_set_bit(BLOCKED, &pvcc->blocked); bh_unlock_sock(sk_atm(vcc)); /* * We don't have space to send this SKB now, but we might have