| Message ID | 20121111135002.GA32390@shrek.podlesie.net |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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?
On Sun, Nov 11, 2012 at 03:26:41PM +0000, 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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().
On Sun, Nov 11, 2012 at 05:03:21PM +0000, 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.Woodhouse@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...
In message <1352667081.9449.135.camel@shinybook.infradead.org>,David Woodhouse writes: >Acked-by: David Woodhouse <David.Woodhouse@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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2012-11-11 at 17:57 -0500, Chas Williams (CONTRACTOR) wrote: > In message <1352667081.9449.135.camel@shinybook.infradead.org>,David Woodhouse writes: > >Acked-by: David Woodhouse <David.Woodhouse@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.
On Tue, 27 Nov 2012 13:27:47 +0000 David Woodhouse <dwmw2@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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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()
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". > > 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@podlesie.net> 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 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 <krzysiek@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 <krzysiek@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)) {