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

2012-11-28 Thread David Woodhouse 
On Wed, 2012-11-28 at 10:58 +0100, Krzysztof Mazur wrote:
> ok, I think that we should just drop that patch, with test_bit()
> I think it's no longer an optimization.

After another cup of tea, it now uses test_and_clear_bit()... and
doesn't break the carefully crafted handling of the BLOCKED bit in the
normal flow control case, by setting it at the nospace: label!

From b2cf6a466697ecf19061cb11b8f4ec5bb381550a Mon Sep 17 00:00:00 2001
From: David Woodhouse 
Date: Wed, 28 Nov 2012 10:15:05 +
Subject: [PATCH] pppoatm: optimise PPP channel wakeups after 
sock_owned_by_user()

We don't need to schedule the wakeup tasklet on *every* unlock; only if we
actually blocked the channel in the first place.

Signed-off-by: David Woodhouse 
---
 net/atm/pppoatm.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 446a7f0..172e44e 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -113,7 +113,17 @@ static void pppoatm_release_cb(struct atm_vcc *atmvcc)
 {
struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
 
-   tasklet_schedule(>wakeup_tasklet);
+   /*
+* As in pppoatm_pop(), it's safe to clear the BLOCKED bit here because
+* the wakeup *can't* race with pppoatm_send(). They both hold the PPP
+* channel's ->downl lock. And the potential race with *setting* it,
+* which leads to the double-check dance in pppoatm_may_send(), doesn't
+* exist here. In the sock_owned_by_user() case in pppoatm_send(), we
+* set the BLOCKED bit while the socket is still locked. We know that
+* ->release_cb() can't be called until that's done.
+*/
+   if (test_and_clear_bit(BLOCKED, >blocked))
+   tasklet_schedule(>wakeup_tasklet);
if (pvcc->old_release_cb)
pvcc->old_release_cb(atmvcc);
 }
@@ -292,8 +302,15 @@ 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)))
+   if (sock_owned_by_user(sk_atm(vcc))) {
+   /*
+* 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, >blocked);
goto nospace;
+   }
if (test_bit(ATM_VF_RELEASED, >flags)
|| test_bit(ATM_VF_CLOSE, >flags)
|| !test_bit(ATM_VF_READY, >flags)) {
-- 
1.8.0



-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-28 Thread Krzysztof Mazur 
On Wed, Nov 28, 2012 at 09:24:09AM +, David Woodhouse wrote:
> On Wed, 2012-11-28 at 09:12 +0100, Krzysztof Mazur wrote:
> > On Wed, Nov 28, 2012 at 12:48:17AM +, 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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-28 Thread David Woodhouse 
On Wed, 2012-11-28 at 09:12 +0100, Krzysztof Mazur wrote:
> On Wed, Nov 28, 2012 at 12:48:17AM +, 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(>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, >blocked))
+   tasklet_schedule(>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, >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 

Ta.
-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-28 Thread Krzysztof Mazur 
On Wed, Nov 28, 2012 at 12:48:17AM +, 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".

> 
>  http://git.infradead.org/users/dwmw2/atm.git
>   git://git.infradead.org/users/dwmw2/atm.git
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-28 Thread Krzysztof Mazur 
On Wed, Nov 28, 2012 at 12:48:17AM +, 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.

 
  http://git.infradead.org/users/dwmw2/atm.git
   git://git.infradead.org/users/dwmw2/atm.git
 
 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 krzys...@podlesie.net

Krzysiek
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-28 Thread David Woodhouse 
On Wed, 2012-11-28 at 09:12 +0100, Krzysztof Mazur wrote:
 On Wed, Nov 28, 2012 at 12:48:17AM +, 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 krzys...@podlesie.net

Ta.
-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-28 Thread Krzysztof Mazur 
On Wed, Nov 28, 2012 at 09:24:09AM +, David Woodhouse wrote:
 On Wed, 2012-11-28 at 09:12 +0100, Krzysztof Mazur wrote:
  On Wed, Nov 28, 2012 at 12:48:17AM +, 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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-28 Thread David Woodhouse 
On Wed, 2012-11-28 at 10:58 +0100, Krzysztof Mazur wrote:
 ok, I think that we should just drop that patch, with test_bit()
 I think it's no longer an optimization.

After another cup of tea, it now uses test_and_clear_bit()... and
doesn't break the carefully crafted handling of the BLOCKED bit in the
normal flow control case, by setting it at the nospace: label!

From b2cf6a466697ecf19061cb11b8f4ec5bb381550a Mon Sep 17 00:00:00 2001
From: David Woodhouse david.woodho...@intel.com
Date: Wed, 28 Nov 2012 10:15:05 +
Subject: [PATCH] pppoatm: optimise PPP channel wakeups after 
sock_owned_by_user()

We don't need to schedule the wakeup tasklet on *every* unlock; only if we
actually blocked the channel in the first place.

Signed-off-by: David Woodhouse david.woodho...@intel.com
---
 net/atm/pppoatm.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 446a7f0..172e44e 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -113,7 +113,17 @@ static void pppoatm_release_cb(struct atm_vcc *atmvcc)
 {
struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
 
-   tasklet_schedule(pvcc-wakeup_tasklet);
+   /*
+* As in pppoatm_pop(), it's safe to clear the BLOCKED bit here because
+* the wakeup *can't* race with pppoatm_send(). They both hold the PPP
+* channel's -downl lock. And the potential race with *setting* it,
+* which leads to the double-check dance in pppoatm_may_send(), doesn't
+* exist here. In the sock_owned_by_user() case in pppoatm_send(), we
+* set the BLOCKED bit while the socket is still locked. We know that
+* -release_cb() can't be called until that's done.
+*/
+   if (test_and_clear_bit(BLOCKED, pvcc-blocked))
+   tasklet_schedule(pvcc-wakeup_tasklet);
if (pvcc-old_release_cb)
pvcc-old_release_cb(atmvcc);
 }
@@ -292,8 +302,15 @@ 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)))
+   if (sock_owned_by_user(sk_atm(vcc))) {
+   /*
+* 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);
goto nospace;
+   }
if (test_bit(ATM_VF_RELEASED, vcc-flags)
|| test_bit(ATM_VF_CLOSE, vcc-flags)
|| !test_bit(ATM_VF_READY, vcc-flags)) {
-- 
1.8.0



-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-27 Thread David Woodhouse 
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:

 http://git.infradead.org/users/dwmw2/atm.git
  git://git.infradead.org/users/dwmw2/atm.git

David Woodhouse (5):
  solos-pci: Wait for pending TX to complete when releasing vcc
  br2684: don't send frames on not-ready vcc
  atm: Add release_cb() callback to vcc
  pppoatm: fix missing wakeup in pppoatm_send()
  br2684: fix module_put() race

Krzysztof Mazur (6):
  atm: add owner of push() callback to atmvcc
  pppoatm: allow assign only on a connected socket
  pppoatm: fix module_put() race
  pppoatm: take ATM socket lock in pppoatm_send()
  pppoatm: drop frames to not-ready vcc
  pppoatm: do not inline pppoatm_may_send()


-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-27 Thread chas williams - CONTRACTOR 
On Tue, 27 Nov 2012 13:27:47 +
David Woodhouse  wrote:

> > i really would prefer not to use a strange name since it might confuse
> > larger group of people who are more familiar with the traditional meaning
> > of this function.  vcc_release() isnt exported so we could rename it if
> > things get too confusing.
> > 
> > i have to look at this a bit more but we might be able to use release_cb
> > to get rid of the null push to detach the underlying protocol.  that would
> > be somewhat nice.
> 
> In the meantime, should I resend this patch with the name 'release_cb'
> instead of 'unlock_cb'? I'll just put a comment in to make sure it isn't
> confused with vcc_release(), and if we need to change vcc_release()
> later we can.
> 

yes, but dont call it 8/7 since that doesnt make sense.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-27 Thread David Woodhouse 
On Sun, 2012-11-11 at 17:57 -0500, Chas Williams (CONTRACTOR) wrote:
> In message <1352667081.9449.135.ca...@shinybook.infradead.org>,David 
> Woodhouse writes:
> >Acked-by: David Woodhouse  for your new
> >version of patch #6 (returning DROP_PACKET for !VF_READY), and your
> >followup to my patch #8, adding the 'need_wakeup' flag. Which we might
> >as well merge into (the pppoatm part of) my patch.
> >
> >Chas, are you happy with the generic ATM part of that? And the
> >nomenclature? I didn't want to call it 'release_cb' like the core socket
> >code does, because we use 'release' to mean something different in ATM.
> >So I called it 'unlock_cb' instead...
> 
> i really would prefer not to use a strange name since it might confuse
> larger group of people who are more familiar with the traditional meaning
> of this function.  vcc_release() isnt exported so we could rename it if
> things get too confusing.
> 
> i have to look at this a bit more but we might be able to use release_cb
> to get rid of the null push to detach the underlying protocol.  that would
> be somewhat nice.

In the meantime, should I resend this patch with the name 'release_cb'
instead of 'unlock_cb'? I'll just put a comment in to make sure it isn't
confused with vcc_release(), and if we need to change vcc_release()
later we can.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-27 Thread David Woodhouse 
On Sun, 2012-11-11 at 17:57 -0500, Chas Williams (CONTRACTOR) wrote:
 In message 1352667081.9449.135.ca...@shinybook.infradead.org,David 
 Woodhouse writes:
 Acked-by: David Woodhouse david.woodho...@intel.com for your new
 version of patch #6 (returning DROP_PACKET for !VF_READY), and your
 followup to my patch #8, adding the 'need_wakeup' flag. Which we might
 as well merge into (the pppoatm part of) my patch.
 
 Chas, are you happy with the generic ATM part of that? And the
 nomenclature? I didn't want to call it 'release_cb' like the core socket
 code does, because we use 'release' to mean something different in ATM.
 So I called it 'unlock_cb' instead...
 
 i really would prefer not to use a strange name since it might confuse
 larger group of people who are more familiar with the traditional meaning
 of this function.  vcc_release() isnt exported so we could rename it if
 things get too confusing.
 
 i have to look at this a bit more but we might be able to use release_cb
 to get rid of the null push to detach the underlying protocol.  that would
 be somewhat nice.

In the meantime, should I resend this patch with the name 'release_cb'
instead of 'unlock_cb'? I'll just put a comment in to make sure it isn't
confused with vcc_release(), and if we need to change vcc_release()
later we can.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-27 Thread chas williams - CONTRACTOR 
On Tue, 27 Nov 2012 13:27:47 +
David Woodhouse dw...@infradead.org wrote:

  i really would prefer not to use a strange name since it might confuse
  larger group of people who are more familiar with the traditional meaning
  of this function.  vcc_release() isnt exported so we could rename it if
  things get too confusing.
  
  i have to look at this a bit more but we might be able to use release_cb
  to get rid of the null push to detach the underlying protocol.  that would
  be somewhat nice.
 
 In the meantime, should I resend this patch with the name 'release_cb'
 instead of 'unlock_cb'? I'll just put a comment in to make sure it isn't
 confused with vcc_release(), and if we need to change vcc_release()
 later we can.
 

yes, but dont call it 8/7 since that doesnt make sense.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-27 Thread David Woodhouse 
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:

 http://git.infradead.org/users/dwmw2/atm.git
  git://git.infradead.org/users/dwmw2/atm.git

David Woodhouse (5):
  solos-pci: Wait for pending TX to complete when releasing vcc
  br2684: don't send frames on not-ready vcc
  atm: Add release_cb() callback to vcc
  pppoatm: fix missing wakeup in pppoatm_send()
  br2684: fix module_put() race

Krzysztof Mazur (6):
  atm: add owner of push() callback to atmvcc
  pppoatm: allow assign only on a connected socket
  pppoatm: fix module_put() race
  pppoatm: take ATM socket lock in pppoatm_send()
  pppoatm: drop frames to not-ready vcc
  pppoatm: do not inline pppoatm_may_send()


-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-11 Thread Chas Williams (CONTRACTOR) 
In message <1352667081.9449.135.ca...@shinybook.infradead.org>,David Woodhouse 
writes:
>Acked-by: David Woodhouse  for your new
>version of patch #6 (returning DROP_PACKET for !VF_READY), and your
>followup to my patch #8, adding the 'need_wakeup' flag. Which we might
>as well merge into (the pppoatm part of) my patch.
>
>Chas, are you happy with the generic ATM part of that? And the
>nomenclature? I didn't want to call it 'release_cb' like the core socket
>code does, because we use 'release' to mean something different in ATM.
>So I called it 'unlock_cb' instead...

i really would prefer not to use a strange name since it might confuse
larger group of people who are more familiar with the traditional meaning
of this function.  vcc_release() isnt exported so we could rename it if
things get too confusing.

i have to look at this a bit more but we might be able to use release_cb
to get rid of the null push to detach the underlying protocol.  that would
be somewhat nice.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-11 Thread Chas Williams (CONTRACTOR) 
In message <2012110437.ga25...@shrek.podlesie.net>,Krzysztof Mazur writes:
>Any race with testing vcc flags is probably not really important
>because:
>   - vcc_release_async() does not take any lock so this check
> will be always racy so we are probably allowed to send
> new frames until vcc is really closed,

this locking/synchronization is handled in the atm device drivers.
the send and close routines are responsbile for synchronization (yes, i
believe that leads to duplicated code but that is the way it currently is)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-11 Thread David Woodhouse 
On Sun, 2012-11-11 at 19:49 +0100, Krzysztof Mazur wrote:
> 
> In pppoatm_devppp_ioctl() we also don't have sk->sk_lock.slock lock.
> In original patch synchronization was trivial because callback
> from socket lock is used.
> 
> I also though about sharing word with encaps enum - encaps needs only 2 bits,
> but it's ugly.

Yeah, fair enough. It's not the end of the world having it in a separate
word. I was just trying to avoid bloating the structure more than we
needed to.

Acked-by: David Woodhouse  for your new
version of patch #6 (returning DROP_PACKET for !VF_READY), and your
followup to my patch #8, adding the 'need_wakeup' flag. Which we might
as well merge into (the pppoatm part of) my patch.

Chas, are you happy with the generic ATM part of that? And the
nomenclature? I didn't want to call it 'release_cb' like the core socket
code does, because we use 'release' to mean something different in ATM.
So I called it 'unlock_cb' instead...

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-11 Thread Krzysztof Mazur 
On Sun, Nov 11, 2012 at 05:03:21PM +, David Woodhouse wrote:
> On Sun, 2012-11-11 at 17:12 +0100, Krzysztof Mazur wrote:
> > It would require using atomic ops because also pppoatm_pop() can
> > modify this word. I think it's better to add additional word instead
> > of using atomic ops.
> 
> Or use the existing flags word, perhaps. Only one bit of which is
> actually used already. We'd have to filter the new bit out in
> pppoatm_devppp_ioctl().
> 

In pppoatm_devppp_ioctl() we also don't have sk->sk_lock.slock lock.
In original patch synchronization was trivial because callback
from socket lock is used.

I also though about sharing word with encaps enum - encaps needs only 2 bits,
but it's ugly.

Krzysiek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-11 Thread David Woodhouse 
On Sun, 2012-11-11 at 17:12 +0100, Krzysztof Mazur wrote:
> It would require using atomic ops because also pppoatm_pop() can
> modify this word. I think it's better to add additional word instead
> of using atomic ops.

Or use the existing flags word, perhaps. Only one bit of which is
actually used already. We'd have to filter the new bit out in
pppoatm_devppp_ioctl().

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-11 Thread Krzysztof Mazur 
On Sun, Nov 11, 2012 at 03:26:41PM +, David Woodhouse wrote:
> On Sun, 2012-11-11 at 14:50 +0100, Krzysztof Mazur wrote:
> > Looks and works ok after:
> > +   atmvcc->unlock_cb = pppoatm_unlock_cb;
> 
> Heh, yeah. That would probably help :)
> 
> Not sure if it's really necessary to optimise out the unneeded wakeups ???
> I don't think that code path gets exercised very hard for normal passing
> of packets. Maybe only LCP echo and responses, on a live connection?
> 
> But yeah, the locking *is* that simple, isn't it ??? and not the painful
> stuff I had to do for the BLOCKED flag, which is why I deferred that
> question to concentrate on the basic concept of using ->release_cb().
> 
> So it's silly *not* to do the 'need_wakeup'. But could it also live in
> the 'blocked' word rather than expanding the structure further? Or just
> *use* the BLOCKED bit, for that matter?
> 

It would require using atomic ops because also pppoatm_pop() can modify this
word. I think it's better to add additional word instead of using
atomic ops.

Krzysiek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-11 Thread David Woodhouse 
On Sun, 2012-11-11 at 14:50 +0100, Krzysztof Mazur wrote:
> Looks and works ok after:
> +   atmvcc->unlock_cb = pppoatm_unlock_cb;

Heh, yeah. That would probably help :)

Not sure if it's really necessary to optimise out the unneeded wakeups —
I don't think that code path gets exercised very hard for normal passing
of packets. Maybe only LCP echo and responses, on a live connection?

But yeah, the locking *is* that simple, isn't it — and not the painful
stuff I had to do for the BLOCKED flag, which is why I deferred that
question to concentrate on the basic concept of using ->release_cb().

So it's silly *not* to do the 'need_wakeup'. But could it also live in
the 'blocked' word rather than expanding the structure further? Or just
*use* the BLOCKED bit, for that matter?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-11 Thread Krzysztof Mazur 
On Sun, Nov 11, 2012 at 11:39:53AM +, David Woodhouse wrote:
> 
> Right. Something like this then, instead of my previous patch 8/7?
> 
> Only addresses the sock_owned_by_user() case and not ATM_VF_RELEASED,
> ATM_VF_CLOSE or !ATM_VF_READY, but your amended patch 6 fixes that I
> think.
> 

Looks and works ok after:

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 7b8dafe..87e792c 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -414,6 +414,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void 
__user *arg)
atmvcc->user_back = pvcc;
atmvcc->push = pppoatm_push;
atmvcc->pop = pppoatm_pop;
+   atmvcc->unlock_cb = pppoatm_unlock_cb;
__module_get(THIS_MODULE);
atmvcc->owner = THIS_MODULE;

With this change:
Acked-by: Krzysztof Mazur 

This patch should be also acked by Chas, at least changes in generic ATM
code (maybe as separate patch).

I need also an ack for new version of patch 6 (pppoatm: drop frames to
not-ready vcc).

Maybe we should schedule tasklet in pppoatm_unlock_cb() only when it's needed.


Thanks,

Krzysiek

-- >8 --
Subject: [PATCH] pppoatm: wakeup after ATM unlock only when it's needed

We need to schedule wakeup tasklet only when ATM socket locked
was previously locked. The locking is provided by the sk->sk_lock.slock
spinlock.

Signed-off-by: Krzysztof Mazur 
---
 net/atm/pppoatm.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 87e792c..841b9f8 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -66,6 +66,7 @@ struct pppoatm_vcc {
enum pppoatm_encaps encaps;
atomic_t inflight;
unsigned long blocked;
+   int need_wakeup;
int flags;  /* SC_COMP_PROT - compress protocol */
struct ppp_channel chan;/* interface to generic ppp layer */
struct tasklet_struct wakeup_tasklet;
@@ -113,7 +114,10 @@ static void pppoatm_unlock_cb(struct atm_vcc *atmvcc)
 {
struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
 
-   tasklet_schedule(>wakeup_tasklet);
+   if (pvcc->need_wakeup) {
+   pvcc->need_wakeup = 0;
+   tasklet_schedule(>wakeup_tasklet);
+   }
if (pvcc->old_unlock_cb)
pvcc->old_unlock_cb(atmvcc);
 }
@@ -292,8 +296,10 @@ 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)))
+   if (sock_owned_by_user(sk_atm(vcc))) {
+   pvcc->need_wakeup = 1;
goto nospace;
+   }
if (test_bit(ATM_VF_RELEASED, >flags)
|| test_bit(ATM_VF_CLOSE, >flags)
|| !test_bit(ATM_VF_READY, >flags)) {
-- 
1.8.0.268.g9d5ca2e

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-11 Thread David Woodhouse 
On Sun, 2012-11-11 at 12:04 +0100, Krzysztof Mazur wrote:
> Yes, but socket can be also locked for a long time, vcc_sendmsg() sleeps
> owning socket lock waiting for memory or atm_may_send().

Right. Something like this then, instead of my previous patch 8/7?

Only addresses the sock_owned_by_user() case and not ATM_VF_RELEASED,
ATM_VF_CLOSE or !ATM_VF_READY, but your amended patch 6 fixes that I
think.

diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
index 31c16c6..59532ed 100644
--- a/include/linux/atmdev.h
+++ b/include/linux/atmdev.h
@@ -308,6 +308,7 @@ struct atm_vcc {
struct atm_dev  *dev;   /* device back pointer */
struct atm_qos  qos;/* QOS */
struct atm_sap  sap;/* SAP */
+   void (*unlock_cb)(struct atm_vcc *vcc); /* release_sock callback */
void (*push)(struct atm_vcc *vcc,struct sk_buff *skb);
void (*pop)(struct atm_vcc *vcc,struct sk_buff *skb); /* optional */
int (*push_oam)(struct atm_vcc *vcc,void *cell);
diff --git a/net/atm/common.c b/net/atm/common.c
index ea952b2..c212016 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -126,10 +126,19 @@ static void vcc_write_space(struct sock *sk)
rcu_read_unlock();
 }
 
+static void vcc_unlock_cb(struct sock *sk)
+{
+   struct atm_vcc *vcc = atm_sk(sk);
+
+   if (vcc->unlock_cb)
+   vcc->unlock_cb(vcc);
+}
+
 static struct proto vcc_proto = {
.name = "VCC",
.owner= THIS_MODULE,
.obj_size = sizeof(struct atm_vcc),
+   .release_cb = vcc_unlock_cb,
 };
 
 int vcc_create(struct net *net, struct socket *sock, int protocol, int family)
@@ -158,6 +167,7 @@ int vcc_create(struct net *net, struct socket *sock, int 
protocol, int family)
vcc->pop = NULL;
vcc->owner = NULL;
vcc->push_oam = NULL;
+   vcc->unlock_cb = NULL;
vcc->vpi = vcc->vci = 0; /* no VCI/VPI yet */
vcc->atm_options = vcc->aal_options = 0;
sk->sk_destruct = vcc_sock_destruct;
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 7507c20..751569a 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -60,6 +60,7 @@ struct pppoatm_vcc {
struct atm_vcc  *atmvcc;/* VCC descriptor */
void (*old_push)(struct atm_vcc *, struct sk_buff *);
void (*old_pop)(struct atm_vcc *, struct sk_buff *);
+   void (*old_unlock_cb)(struct atm_vcc *);
struct module *old_owner;
/* keep old push/pop for detaching */
enum pppoatm_encaps encaps;
@@ -108,6 +109,14 @@ static void pppoatm_wakeup_sender(unsigned long arg)
ppp_output_wakeup((struct ppp_channel *) arg);
 }
 
+static void pppoatm_unlock_cb(struct atm_vcc *atmvcc)
+{
+   struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
+
+   tasklet_schedule(>wakeup_tasklet);
+   if (pvcc->old_unlock_cb)
+   pvcc->old_unlock_cb(atmvcc);
+}
 /*
  * This gets called every time the ATM card has finished sending our
  * skb.  The ->old_pop will take care up normal atm flow control,
@@ -152,6 +161,7 @@ static void pppoatm_unassign_vcc(struct atm_vcc *atmvcc)
pvcc = atmvcc_to_pvcc(atmvcc);
atmvcc->push = pvcc->old_push;
atmvcc->pop = pvcc->old_pop;
+   atmvcc->unlock_cb = pvcc->old_unlock_cb;
tasklet_kill(>wakeup_tasklet);
ppp_unregister_channel(>chan);
atmvcc->user_back = NULL;
@@ -385,6 +395,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void 
__user *arg)
pvcc->old_push = atmvcc->push;
pvcc->old_pop = atmvcc->pop;
pvcc->old_owner = atmvcc->owner;
+   pvcc->old_unlock_cb = atmvcc->unlock_cb;
pvcc->encaps = (enum pppoatm_encaps) be.encaps;
pvcc->chan.private = pvcc;
pvcc->chan.ops = _ops;


-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-11 Thread Krzysztof Mazur 
On Sun, Nov 11, 2012 at 07:28:53AM +, David Woodhouse wrote:
> 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, but socket can be also locked for a long time, vcc_sendmsg() sleeps
owning socket lock waiting for memory or atm_may_send().

pppd was spinning in tasklet_kill() (at least when I did sysrq+t).

> 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

I think we can just drop frame if vcc flags indicate not-ready link
and in this case we not need a wakeup event. I already sent you
an updated patch 3 that drops frame instead of returning zero.

> 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?
> 

When vcc_sendmsg() waits for memory it can be a problem.

I think we can use release_cb callback that is called when socket lock
is released (we will need small wrapper in ATM layer that will call
new vcc->release_cb callback), but maybe we shouldn't use ATM socket
lock and use some new vcc->send_lock instead that will protect
vcc->send() and us against atm_may_send()/atomic_add(skb->truesize,
>sk_wmem_alloc)/ race with vcc_sendmsg().


Any race with testing vcc flags is probably not really important
because:
- vcc_release_async() does not take any lock so this check
  will be always racy so we are probably allowed to send
  new frames until vcc is really closed,
- we are already protected against vcc_destroy_socket()
- the only case that such test is really important
  is openned, but not ready vcc, and we avoid any race
  by not doing send.

Krzysiek

-- >8 --
release_cb wrapper for ATM - just to precisely show the first idea,
with this patch we can just implement vcc->release_cb() and call
tasklet_schedule() there (if needed).

diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
index 31c16c6..a6f9d4b 100644
--- a/include/linux/atmdev.h
+++ b/include/linux/atmdev.h
@@ -310,6 +310,7 @@ struct atm_vcc {
struct atm_sap  sap;/* SAP */
void (*push)(struct atm_vcc *vcc,struct sk_buff *skb);
void (*pop)(struct atm_vcc *vcc,struct sk_buff *skb); /* optional */
+   void (*release_cb)(struct atm_vcc *vcc);
int (*push_oam)(struct atm_vcc *vcc,void *cell);
int (*send)(struct atm_vcc *vcc,struct sk_buff *skb);
void*dev_data;  /* per-device data */
diff --git a/net/atm/common.c b/net/atm/common.c
index ea952b2..4e7f305 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -126,10 +126,19 @@ static void vcc_write_space(struct sock *sk)
rcu_read_unlock();
 }
 
+void vcc_release_cb(struct sock *sk)
+{
+   struct atm_vcc *vcc = atm_sk(sk);
+
+   if (vcc->release_cb)
+   vcc->release_cb(vcc);
+}
+
 static struct proto vcc_proto = {
.name = "VCC",
.owner= THIS_MODULE,
.obj_size = sizeof(struct atm_vcc),
+   .release_cb = vcc_release_cb,
 };
 
 int vcc_create(struct net *net, struct socket *sock, int protocol, int family)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-11 Thread Krzysztof Mazur 
On Sun, Nov 11, 2012 at 07:28:53AM +, David Woodhouse wrote:
 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, but socket can be also locked for a long time, vcc_sendmsg() sleeps
owning socket lock waiting for memory or atm_may_send().

pppd was spinning in tasklet_kill() (at least when I did sysrq+t).

 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

I think we can just drop frame if vcc flags indicate not-ready link
and in this case we not need a wakeup event. I already sent you
an updated patch 3 that drops frame instead of returning zero.

 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?
 

When vcc_sendmsg() waits for memory it can be a problem.

I think we can use release_cb callback that is called when socket lock
is released (we will need small wrapper in ATM layer that will call
new vcc-release_cb callback), but maybe we shouldn't use ATM socket
lock and use some new vcc-send_lock instead that will protect
vcc-send() and us against atm_may_send()/atomic_add(skb-truesize,
sk-sk_wmem_alloc)/ race with vcc_sendmsg().


Any race with testing vcc flags is probably not really important
because:
- vcc_release_async() does not take any lock so this check
  will be always racy so we are probably allowed to send
  new frames until vcc is really closed,
- we are already protected against vcc_destroy_socket()
- the only case that such test is really important
  is openned, but not ready vcc, and we avoid any race
  by not doing send.

Krzysiek

-- 8 --
release_cb wrapper for ATM - just to precisely show the first idea,
with this patch we can just implement vcc-release_cb() and call
tasklet_schedule() there (if needed).

diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
index 31c16c6..a6f9d4b 100644
--- a/include/linux/atmdev.h
+++ b/include/linux/atmdev.h
@@ -310,6 +310,7 @@ struct atm_vcc {
struct atm_sap  sap;/* SAP */
void (*push)(struct atm_vcc *vcc,struct sk_buff *skb);
void (*pop)(struct atm_vcc *vcc,struct sk_buff *skb); /* optional */
+   void (*release_cb)(struct atm_vcc *vcc);
int (*push_oam)(struct atm_vcc *vcc,void *cell);
int (*send)(struct atm_vcc *vcc,struct sk_buff *skb);
void*dev_data;  /* per-device data */
diff --git a/net/atm/common.c b/net/atm/common.c
index ea952b2..4e7f305 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -126,10 +126,19 @@ static void vcc_write_space(struct sock *sk)
rcu_read_unlock();
 }
 
+void vcc_release_cb(struct sock *sk)
+{
+   struct atm_vcc *vcc = atm_sk(sk);
+
+   if (vcc-release_cb)
+   vcc-release_cb(vcc);
+}
+
 static struct proto vcc_proto = {
.name = VCC,
.owner= THIS_MODULE,
.obj_size = sizeof(struct atm_vcc),
+   .release_cb = vcc_release_cb,
 };
 
 int vcc_create(struct net *net, struct socket *sock, int protocol, int family)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-11 Thread David Woodhouse 
On Sun, 2012-11-11 at 12:04 +0100, Krzysztof Mazur wrote:
 Yes, but socket can be also locked for a long time, vcc_sendmsg() sleeps
 owning socket lock waiting for memory or atm_may_send().

Right. Something like this then, instead of my previous patch 8/7?

Only addresses the sock_owned_by_user() case and not ATM_VF_RELEASED,
ATM_VF_CLOSE or !ATM_VF_READY, but your amended patch 6 fixes that I
think.

diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
index 31c16c6..59532ed 100644
--- a/include/linux/atmdev.h
+++ b/include/linux/atmdev.h
@@ -308,6 +308,7 @@ struct atm_vcc {
struct atm_dev  *dev;   /* device back pointer */
struct atm_qos  qos;/* QOS */
struct atm_sap  sap;/* SAP */
+   void (*unlock_cb)(struct atm_vcc *vcc); /* release_sock callback */
void (*push)(struct atm_vcc *vcc,struct sk_buff *skb);
void (*pop)(struct atm_vcc *vcc,struct sk_buff *skb); /* optional */
int (*push_oam)(struct atm_vcc *vcc,void *cell);
diff --git a/net/atm/common.c b/net/atm/common.c
index ea952b2..c212016 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -126,10 +126,19 @@ static void vcc_write_space(struct sock *sk)
rcu_read_unlock();
 }
 
+static void vcc_unlock_cb(struct sock *sk)
+{
+   struct atm_vcc *vcc = atm_sk(sk);
+
+   if (vcc-unlock_cb)
+   vcc-unlock_cb(vcc);
+}
+
 static struct proto vcc_proto = {
.name = VCC,
.owner= THIS_MODULE,
.obj_size = sizeof(struct atm_vcc),
+   .release_cb = vcc_unlock_cb,
 };
 
 int vcc_create(struct net *net, struct socket *sock, int protocol, int family)
@@ -158,6 +167,7 @@ int vcc_create(struct net *net, struct socket *sock, int 
protocol, int family)
vcc-pop = NULL;
vcc-owner = NULL;
vcc-push_oam = NULL;
+   vcc-unlock_cb = NULL;
vcc-vpi = vcc-vci = 0; /* no VCI/VPI yet */
vcc-atm_options = vcc-aal_options = 0;
sk-sk_destruct = vcc_sock_destruct;
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 7507c20..751569a 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -60,6 +60,7 @@ struct pppoatm_vcc {
struct atm_vcc  *atmvcc;/* VCC descriptor */
void (*old_push)(struct atm_vcc *, struct sk_buff *);
void (*old_pop)(struct atm_vcc *, struct sk_buff *);
+   void (*old_unlock_cb)(struct atm_vcc *);
struct module *old_owner;
/* keep old push/pop for detaching */
enum pppoatm_encaps encaps;
@@ -108,6 +109,14 @@ static void pppoatm_wakeup_sender(unsigned long arg)
ppp_output_wakeup((struct ppp_channel *) arg);
 }
 
+static void pppoatm_unlock_cb(struct atm_vcc *atmvcc)
+{
+   struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
+
+   tasklet_schedule(pvcc-wakeup_tasklet);
+   if (pvcc-old_unlock_cb)
+   pvcc-old_unlock_cb(atmvcc);
+}
 /*
  * This gets called every time the ATM card has finished sending our
  * skb.  The -old_pop will take care up normal atm flow control,
@@ -152,6 +161,7 @@ static void pppoatm_unassign_vcc(struct atm_vcc *atmvcc)
pvcc = atmvcc_to_pvcc(atmvcc);
atmvcc-push = pvcc-old_push;
atmvcc-pop = pvcc-old_pop;
+   atmvcc-unlock_cb = pvcc-old_unlock_cb;
tasklet_kill(pvcc-wakeup_tasklet);
ppp_unregister_channel(pvcc-chan);
atmvcc-user_back = NULL;
@@ -385,6 +395,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void 
__user *arg)
pvcc-old_push = atmvcc-push;
pvcc-old_pop = atmvcc-pop;
pvcc-old_owner = atmvcc-owner;
+   pvcc-old_unlock_cb = atmvcc-unlock_cb;
pvcc-encaps = (enum pppoatm_encaps) be.encaps;
pvcc-chan.private = pvcc;
pvcc-chan.ops = pppoatm_ops;


-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-11 Thread Krzysztof Mazur 
On Sun, Nov 11, 2012 at 11:39:53AM +, David Woodhouse wrote:
 
 Right. Something like this then, instead of my previous patch 8/7?
 
 Only addresses the sock_owned_by_user() case and not ATM_VF_RELEASED,
 ATM_VF_CLOSE or !ATM_VF_READY, but your amended patch 6 fixes that I
 think.
 

Looks and works ok after:

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 7b8dafe..87e792c 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -414,6 +414,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void 
__user *arg)
atmvcc-user_back = pvcc;
atmvcc-push = pppoatm_push;
atmvcc-pop = pppoatm_pop;
+   atmvcc-unlock_cb = pppoatm_unlock_cb;
__module_get(THIS_MODULE);
atmvcc-owner = THIS_MODULE;

With this change:
Acked-by: Krzysztof Mazur krzys...@podlesie.net

This patch should be also acked by Chas, at least changes in generic ATM
code (maybe as separate patch).

I need also an ack for new version of patch 6 (pppoatm: drop frames to
not-ready vcc).

Maybe we should schedule tasklet in pppoatm_unlock_cb() only when it's needed.


Thanks,

Krzysiek

-- 8 --
Subject: [PATCH] pppoatm: wakeup after ATM unlock only when it's needed

We need to schedule wakeup tasklet only when ATM socket locked
was previously locked. The locking is provided by the sk-sk_lock.slock
spinlock.

Signed-off-by: Krzysztof Mazur krzys...@podlesie.net
---
 net/atm/pppoatm.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 87e792c..841b9f8 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -66,6 +66,7 @@ struct pppoatm_vcc {
enum pppoatm_encaps encaps;
atomic_t inflight;
unsigned long blocked;
+   int need_wakeup;
int flags;  /* SC_COMP_PROT - compress protocol */
struct ppp_channel chan;/* interface to generic ppp layer */
struct tasklet_struct wakeup_tasklet;
@@ -113,7 +114,10 @@ static void pppoatm_unlock_cb(struct atm_vcc *atmvcc)
 {
struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
 
-   tasklet_schedule(pvcc-wakeup_tasklet);
+   if (pvcc-need_wakeup) {
+   pvcc-need_wakeup = 0;
+   tasklet_schedule(pvcc-wakeup_tasklet);
+   }
if (pvcc-old_unlock_cb)
pvcc-old_unlock_cb(atmvcc);
 }
@@ -292,8 +296,10 @@ 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)))
+   if (sock_owned_by_user(sk_atm(vcc))) {
+   pvcc-need_wakeup = 1;
goto nospace;
+   }
if (test_bit(ATM_VF_RELEASED, vcc-flags)
|| test_bit(ATM_VF_CLOSE, vcc-flags)
|| !test_bit(ATM_VF_READY, vcc-flags)) {
-- 
1.8.0.268.g9d5ca2e

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-11 Thread David Woodhouse 
On Sun, 2012-11-11 at 14:50 +0100, Krzysztof Mazur wrote:
 Looks and works ok after:
 +   atmvcc-unlock_cb = pppoatm_unlock_cb;

Heh, yeah. That would probably help :)

Not sure if it's really necessary to optimise out the unneeded wakeups —
I don't think that code path gets exercised very hard for normal passing
of packets. Maybe only LCP echo and responses, on a live connection?

But yeah, the locking *is* that simple, isn't it — and not the painful
stuff I had to do for the BLOCKED flag, which is why I deferred that
question to concentrate on the basic concept of using -release_cb().

So it's silly *not* to do the 'need_wakeup'. But could it also live in
the 'blocked' word rather than expanding the structure further? Or just
*use* the BLOCKED bit, for that matter?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-11 Thread Krzysztof Mazur 
On Sun, Nov 11, 2012 at 03:26:41PM +, David Woodhouse wrote:
 On Sun, 2012-11-11 at 14:50 +0100, Krzysztof Mazur wrote:
  Looks and works ok after:
  +   atmvcc-unlock_cb = pppoatm_unlock_cb;
 
 Heh, yeah. That would probably help :)
 
 Not sure if it's really necessary to optimise out the unneeded wakeups ???
 I don't think that code path gets exercised very hard for normal passing
 of packets. Maybe only LCP echo and responses, on a live connection?
 
 But yeah, the locking *is* that simple, isn't it ??? and not the painful
 stuff I had to do for the BLOCKED flag, which is why I deferred that
 question to concentrate on the basic concept of using -release_cb().
 
 So it's silly *not* to do the 'need_wakeup'. But could it also live in
 the 'blocked' word rather than expanding the structure further? Or just
 *use* the BLOCKED bit, for that matter?
 

It would require using atomic ops because also pppoatm_pop() can modify this
word. I think it's better to add additional word instead of using
atomic ops.

Krzysiek
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-11 Thread David Woodhouse 
On Sun, 2012-11-11 at 17:12 +0100, Krzysztof Mazur wrote:
 It would require using atomic ops because also pppoatm_pop() can
 modify this word. I think it's better to add additional word instead
 of using atomic ops.

Or use the existing flags word, perhaps. Only one bit of which is
actually used already. We'd have to filter the new bit out in
pppoatm_devppp_ioctl().

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-11 Thread Krzysztof Mazur 
On Sun, Nov 11, 2012 at 05:03:21PM +, David Woodhouse wrote:
 On Sun, 2012-11-11 at 17:12 +0100, Krzysztof Mazur wrote:
  It would require using atomic ops because also pppoatm_pop() can
  modify this word. I think it's better to add additional word instead
  of using atomic ops.
 
 Or use the existing flags word, perhaps. Only one bit of which is
 actually used already. We'd have to filter the new bit out in
 pppoatm_devppp_ioctl().
 

In pppoatm_devppp_ioctl() we also don't have sk-sk_lock.slock lock.
In original patch synchronization was trivial because callback
from socket lock is used.

I also though about sharing word with encaps enum - encaps needs only 2 bits,
but it's ugly.

Krzysiek
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-11 Thread David Woodhouse 
On Sun, 2012-11-11 at 19:49 +0100, Krzysztof Mazur wrote:
 
 In pppoatm_devppp_ioctl() we also don't have sk-sk_lock.slock lock.
 In original patch synchronization was trivial because callback
 from socket lock is used.
 
 I also though about sharing word with encaps enum - encaps needs only 2 bits,
 but it's ugly.

Yeah, fair enough. It's not the end of the world having it in a separate
word. I was just trying to avoid bloating the structure more than we
needed to.

Acked-by: David Woodhouse david.woodho...@intel.com for your new
version of patch #6 (returning DROP_PACKET for !VF_READY), and your
followup to my patch #8, adding the 'need_wakeup' flag. Which we might
as well merge into (the pppoatm part of) my patch.

Chas, are you happy with the generic ATM part of that? And the
nomenclature? I didn't want to call it 'release_cb' like the core socket
code does, because we use 'release' to mean something different in ATM.
So I called it 'unlock_cb' instead...

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-11 Thread Chas Williams (CONTRACTOR) 
In message 2012110437.ga25...@shrek.podlesie.net,Krzysztof Mazur writes:
Any race with testing vcc flags is probably not really important
because:
   - vcc_release_async() does not take any lock so this check
 will be always racy so we are probably allowed to send
 new frames until vcc is really closed,

this locking/synchronization is handled in the atm device drivers.
the send and close routines are responsbile for synchronization (yes, i
believe that leads to duplicated code but that is the way it currently is)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-11 Thread Chas Williams (CONTRACTOR) 
In message 1352667081.9449.135.ca...@shinybook.infradead.org,David Woodhouse 
writes:
Acked-by: David Woodhouse david.woodho...@intel.com for your new
version of patch #6 (returning DROP_PACKET for !VF_READY), and your
followup to my patch #8, adding the 'need_wakeup' flag. Which we might
as well merge into (the pppoatm part of) my patch.

Chas, are you happy with the generic ATM part of that? And the
nomenclature? I didn't want to call it 'release_cb' like the core socket
code does, because we use 'release' to mean something different in ATM.
So I called it 'unlock_cb' instead...

i really would prefer not to use a strange name since it might confuse
larger group of people who are more familiar with the traditional meaning
of this function.  vcc_release() isnt exported so we could rename it if
things get too confusing.

i have to look at this a bit more but we might be able to use release_cb
to get rid of the null push to detach the underlying protocol.  that would
be somewhat nice.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-10 Thread David Woodhouse 
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?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-10 Thread Krzysztof Mazur 
On Sat, Nov 10, 2012 at 09:02:02PM +, David Woodhouse wrote:
> 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?
> 

I don't think so, we cannot sleep in tasklet.

I think we should use sk_add_backlog() or release_cb (introduced by:
46d3ceabd8d98ed0ad10f20c595ca784e34786c5 tcp: TCP Small Queues).
The release_cb callback is almost exactly what we need except that
it works on protocol level, not on socket. The same race with
vcc_sendmsg() exists also in other ATM protocols, so maybe we should
add wrapper in ATM layer that calls vcc->sock_release_cb().

But what about socket flags? Maybe we should just drop that frame?
When ppp is used on serial links "not-ready link" usually does that.
I'm sending an updated patch 6.

Krzysiek
-- >8 --
Subject: [PATCH] pppoatm: drop frames to not-ready vcc

Patches "atm: detach protocol before closing vcc"
and "pppoatm: allow assign only on a connected socket" fixed
common cases where the pppoatm_send() crashes while sending
frame to not-ready vcc. However there are still some other cases
where we can send frames to vcc, which is flagged as ATM_VF_CLOSE
(for instance after vcc_release_async()) or it's opened but not
ready yet.

Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
indicate that vcc is not ready. If the vcc is not ready we just
drop frame. Queueing frames is much more complicated because we
don't have callbacks that inform us about vcc flags changes.

Signed-off-by: Krzysztof Mazur 
---
 net/atm/pppoatm.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index c4a57bc..63541a3 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -284,6 +284,13 @@ static int pppoatm_send(struct ppp_channel *chan, struct 
sk_buff *skb)
bh_lock_sock(sk_atm(vcc));
if (sock_owned_by_user(sk_atm(vcc)))
goto nospace;
+   if (test_bit(ATM_VF_RELEASED, >flags)
+   || test_bit(ATM_VF_CLOSE, >flags)
+   || !test_bit(ATM_VF_READY, >flags)) {
+   bh_unlock_sock(sk_atm(vcc));
+   kfree_skb(skb);
+   return DROP_PACKET;
+   }
 
switch (pvcc->encaps) { /* LLC encapsulation needed */
case e_llc:
-- 
1.8.0.268.g9d5ca2e

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-10 Thread David Woodhouse 
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?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-10 Thread Krzysztof Mazur 
On Wed, Nov 07, 2012 at 12:52:14PM +, 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, >flags)
> - || test_bit(ATM_VF_CLOSE, >flags)
> - || !test_bit(ATM_VF_READY, >flags))
> - goto nospace;
> + || test_bit(ATM_VF_CLOSE, >flags)
> + || !test_bit(ATM_VF_READY, >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(>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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-10 Thread David Miller 
From: David Woodhouse 
Date: Sat, 10 Nov 2012 07:36:13 +

> 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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-10 Thread David Miller 
From: David Woodhouse dw...@infradead.org
Date: Sat, 10 Nov 2012 07:36:13 +

 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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-10 Thread Krzysztof Mazur 
On Wed, Nov 07, 2012 at 12:52:14PM +, 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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-10 Thread David Woodhouse 
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?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-10 Thread Krzysztof Mazur 
On Sat, Nov 10, 2012 at 09:02:02PM +, David Woodhouse wrote:
 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?
 

I don't think so, we cannot sleep in tasklet.

I think we should use sk_add_backlog() or release_cb (introduced by:
46d3ceabd8d98ed0ad10f20c595ca784e34786c5 tcp: TCP Small Queues).
The release_cb callback is almost exactly what we need except that
it works on protocol level, not on socket. The same race with
vcc_sendmsg() exists also in other ATM protocols, so maybe we should
add wrapper in ATM layer that calls vcc-sock_release_cb().

But what about socket flags? Maybe we should just drop that frame?
When ppp is used on serial links not-ready link usually does that.
I'm sending an updated patch 6.

Krzysiek
-- 8 --
Subject: [PATCH] pppoatm: drop frames to not-ready vcc

Patches atm: detach protocol before closing vcc
and pppoatm: allow assign only on a connected socket fixed
common cases where the pppoatm_send() crashes while sending
frame to not-ready vcc. However there are still some other cases
where we can send frames to vcc, which is flagged as ATM_VF_CLOSE
(for instance after vcc_release_async()) or it's opened but not
ready yet.

Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
indicate that vcc is not ready. If the vcc is not ready we just
drop frame. Queueing frames is much more complicated because we
don't have callbacks that inform us about vcc flags changes.

Signed-off-by: Krzysztof Mazur krzys...@podlesie.net
---
 net/atm/pppoatm.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index c4a57bc..63541a3 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -284,6 +284,13 @@ static int pppoatm_send(struct ppp_channel *chan, struct 
sk_buff *skb)
bh_lock_sock(sk_atm(vcc));
if (sock_owned_by_user(sk_atm(vcc)))
goto nospace;
+   if (test_bit(ATM_VF_RELEASED, vcc-flags)
+   || test_bit(ATM_VF_CLOSE, vcc-flags)
+   || !test_bit(ATM_VF_READY, vcc-flags)) {
+   bh_unlock_sock(sk_atm(vcc));
+   kfree_skb(skb);
+   return DROP_PACKET;
+   }
 
switch (pvcc-encaps) { /* LLC encapsulation needed */
case e_llc:
-- 
1.8.0.268.g9d5ca2e

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-10 Thread David Woodhouse 
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?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-09 Thread David Woodhouse 
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-krzys...@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 :)

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

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

2012-11-09 Thread David Miller 
From: David Woodhouse 
Date: Wed, 07 Nov 2012 12:52:14 +

> 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 
> 
> Untested.
> With this sorted, Acked-By: David Woodhouse  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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-09 Thread David Miller 
From: David Woodhouse dw...@infradead.org
Date: Wed, 07 Nov 2012 12:52:14 +

 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.woodho...@intel.com
 
 Untested.
 With this sorted, Acked-By: David Woodhouse david.woodho...@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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-11-09 Thread David Woodhouse 
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-krzys...@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 :)

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature