| Message ID | 1354292626.21562.298.camel@shinybook.infradead.org |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Nov 30, 2012 at 04:23:46PM +0000, David Woodhouse wrote: > > +static void br2684_release_cb(struct atm_vcc *atmvcc) > +{ > + struct br2684_vcc *brvcc = BR2684_VCC(atmvcc); > + > + /* > + * A race with br2684_xmit_vcc() might cause a spurious wakeup just > + * after that function *stops* the queue, and qspace might actually > + * go negative before the queue stops again. We cope with that. > + */ We cannot race with br2684_xmit_vcc() because both br2684_xmit_vcc() and br2684_release_cb() are called with locked sk->sk_lock.slock. > + if (atomic_read(&brvcc->qspace) > 0) > + netif_wake_queue(brvcc->device); > + > + if (brvcc->old_release_cb) > + brvcc->old_release_cb(atmvcc); > +} Except that comment, the patch looks good: 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
On Fri, 30 Nov 2012 16:23:46 +0000 David Woodhouse <dwmw2@infradead.org> wrote: > On Fri, 2012-11-30 at 12:10 +0000, David Woodhouse wrote: > > In that case I think we're fine. I'll just do the same thing in > > br2684_push(), fix up the comment you just corrected, and we're all > > good. > > OK, here's an update to me my patch 8/17 'br2684: don't send frames on > not-ready vcc'. It takes the socket lock and does fairly much the same > thing as your pppoatm version. It returns NETDEV_TX_BUSY and stops the > queue if the socket is locked, and it gets woken from the ->release_cb > callback. > > I've dropped your Acked-By: since it's mostly new, but feel free to give > me a fresh one. With this I think we're done. > > Unless Chas has any objections, I'll ask Dave to pull it... no objections. i think this deals with my concerns. as for splitting the close functions, from one of your previous messages: >Really, what we're saying is that *one* of the driver or protocol close >functions needs to be split, and we need to do DPD or PDP. Since the >device driver *can* abort/flush the TX queue and also any pending RX >being handled by a tasklet, I think it makes most sense to keep it in >the middle, with the protocol being handled first and last... which is >the current order, as long as we consider setting ATM_VF_CLOSE to be the >first part. i believe this is essentially already done with the release_cb() implementation right? that is splitting the protocol detach/shutdown into two parts. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 30, 2012 at 12:12:56PM -0500, chas williams - CONTRACTOR wrote: > >Really, what we're saying is that *one* of the driver or protocol close > >functions needs to be split, and we need to do DPD or PDP. Since the > >device driver *can* abort/flush the TX queue and also any pending RX > >being handled by a tasklet, I think it makes most sense to keep it in > >the middle, with the protocol being handled first and last... which is > >the current order, as long as we consider setting ATM_VF_CLOSE to be the > >first part. > > i believe this is essentially already done with the release_cb() > implementation right? that is splitting the protocol detach/shutdown > into two parts. partially, release_cb() is about ATM socket locking. To avoid some races we need to take the ATM socket lock in protocol send function (br2684_start_xmit, pppoatm_send, ...). That functions are executed in bh context and we cannot sleep and wait for releasing the ATM socket lock, so we just block sending and when the ATM socket is unlocked release_cb() is called and we re-enabling sending. Currently the first part of detach is just: lock_sock(sk) (without latest "br2684: don't send frames on not-ready vcc" the first part was set_bit(ATM_VF_CLOSE, &vcc->flags); clear_bit(ATM_VF_READY, &vcc->flags); for br2684) After that the protocol stops sending new packets so the vcc may be fully closed by ATM driver. The protocol is still ready to process received packets. After vcc is closed the protocol can safely detach knowing that no new packets will be received. 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 Fri, 2012-11-30 at 18:00 +0100, Krzysztof Mazur wrote: > On Fri, Nov 30, 2012 at 04:23:46PM +0000, David Woodhouse wrote: > > > > +static void br2684_release_cb(struct atm_vcc *atmvcc) > > +{ > > + struct br2684_vcc *brvcc = BR2684_VCC(atmvcc); > > + > > + /* > > + * A race with br2684_xmit_vcc() might cause a spurious wakeup just > > + * after that function *stops* the queue, and qspace might actually > > + * go negative before the queue stops again. We cope with that. > > + */ > > We cannot race with br2684_xmit_vcc() because both br2684_xmit_vcc() > and br2684_release_cb() are called with locked sk->sk_lock.slock. Ah, right. For some reason I thought the lock was already dropped when ->release_cb() was called. In that case I'll remove the comment. Thanks.
diff --git a/net/atm/br2684.c b/net/atm/br2684.c index 8eb6fbe..5ff145f 100644 --- a/net/atm/br2684.c +++ b/net/atm/br2684.c @@ -68,6 +68,7 @@ struct br2684_vcc { /* keep old push, pop functions for chaining */ void (*old_push)(struct atm_vcc *vcc, struct sk_buff *skb); void (*old_pop)(struct atm_vcc *vcc, struct sk_buff *skb); + void (*old_release_cb)(struct atm_vcc *vcc); enum br2684_encaps encaps; struct list_head brvccs; #ifdef CONFIG_ATM_BR2684_IPFILTER @@ -269,6 +270,22 @@ static int br2684_xmit_vcc(struct sk_buff *skb, struct net_device *dev, return !atmvcc->send(atmvcc, skb); } +static void br2684_release_cb(struct atm_vcc *atmvcc) +{ + struct br2684_vcc *brvcc = BR2684_VCC(atmvcc); + + /* + * A race with br2684_xmit_vcc() might cause a spurious wakeup just + * after that function *stops* the queue, and qspace might actually + * go negative before the queue stops again. We cope with that. + */ + if (atomic_read(&brvcc->qspace) > 0) + netif_wake_queue(brvcc->device); + + if (brvcc->old_release_cb) + brvcc->old_release_cb(atmvcc); +} + static inline struct br2684_vcc *pick_outgoing_vcc(const struct sk_buff *skb, const struct br2684_dev *brdev) { @@ -280,6 +297,8 @@ static netdev_tx_t br2684_start_xmit(struct sk_buff *skb, { struct br2684_dev *brdev = BRPRIV(dev); struct br2684_vcc *brvcc; + struct atm_vcc *atmvcc; + netdev_tx_t ret = NETDEV_TX_OK; pr_debug("skb_dst(skb)=%p\n", skb_dst(skb)); read_lock(&devs_lock); @@ -290,9 +309,26 @@ static netdev_tx_t br2684_start_xmit(struct sk_buff *skb, dev->stats.tx_carrier_errors++; /* netif_stop_queue(dev); */ dev_kfree_skb(skb); - read_unlock(&devs_lock); - return NETDEV_TX_OK; + goto out_devs; + } + atmvcc = brvcc->atmvcc; + + bh_lock_sock(sk_atm(atmvcc)); + + if (test_bit(ATM_VF_RELEASED, &atmvcc->flags) || + test_bit(ATM_VF_CLOSE, &atmvcc->flags) || + !test_bit(ATM_VF_READY, &atmvcc->flags)) { + dev->stats.tx_dropped++; + dev_kfree_skb(skb); + goto out; } + + if (sock_owned_by_user(sk_atm(atmvcc))) { + netif_stop_queue(brvcc->device); + ret = NETDEV_TX_BUSY; + goto out; + } + if (!br2684_xmit_vcc(skb, dev, brvcc)) { /* * We should probably use netif_*_queue() here, but that @@ -304,8 +340,11 @@ static netdev_tx_t br2684_start_xmit(struct sk_buff *skb, dev->stats.tx_errors++; dev->stats.tx_fifo_errors++; } + out: + bh_unlock_sock(sk_atm(atmvcc)); + out_devs: read_unlock(&devs_lock); - return NETDEV_TX_OK; + return ret; } /* @@ -378,6 +417,7 @@ static void br2684_close_vcc(struct br2684_vcc *brvcc) list_del(&brvcc->brvccs); write_unlock_irq(&devs_lock); brvcc->atmvcc->user_back = NULL; /* what about vcc->recvq ??? */ + brvcc->atmvcc->release_cb = brvcc->old_release_cb; brvcc->old_push(brvcc->atmvcc, NULL); /* pass on the bad news */ kfree(brvcc); module_put(THIS_MODULE); @@ -554,9 +594,11 @@ static int br2684_regvcc(struct atm_vcc *atmvcc, void __user * arg) brvcc->encaps = (enum br2684_encaps)be.encaps; brvcc->old_push = atmvcc->push; brvcc->old_pop = atmvcc->pop; + brvcc->old_release_cb = atmvcc->release_cb; barrier(); atmvcc->push = br2684_push; atmvcc->pop = br2684_pop; + atmvcc->release_cb = br2684_release_cb; /* initialize netdev carrier state */ if (atmvcc->dev->signal == ATM_PHY_SIG_LOST)