Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 2788 bytes --]

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.

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


> > David Woodhouse (5):
> >       atm: Add release_cb() callback to vcc
> >       pppoatm: fix missing wakeup in pppoatm_send()
> >       br2684: fix module_put() race
> 
> for the three patches above:
> 
> Acked-by: Krzysztof Mazur <krzysiek@podlesie.net>

Ta.
-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]