Fix bufferbloat in PPPoATM TX queue
authorJo-Philipp Wich <jow@openwrt.org>
Mon, 16 Apr 2012 15:04:45 +0000 (15:04 +0000)
committerJo-Philipp Wich <jow@openwrt.org>
Mon, 16 Apr 2012 15:04:45 +0000 (15:04 +0000)
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
SVN-Revision: 31313

target/linux/generic/patches-3.2/130-pppoatm-queue-depth.patch [new file with mode: 0644]
target/linux/generic/patches-3.3/130-pppoatm-queue-depth.patch [new file with mode: 0644]

diff --git a/target/linux/generic/patches-3.2/130-pppoatm-queue-depth.patch b/target/linux/generic/patches-3.2/130-pppoatm-queue-depth.patch
new file mode 100644 (file)
index 0000000..d95ddf6
--- /dev/null
@@ -0,0 +1,193 @@
+From 9d02daf754238adac48fa075ee79e7edd3d79ed3 Mon Sep 17 00:00:00 2001
+From: David Woodhouse <dwmw2@infradead.org>
+Date: Sun, 8 Apr 2012 09:55:43 +0000
+Subject: [PATCH] pppoatm: Fix excessive queue bloat
+
+We discovered that PPPoATM has an excessively deep transmit queue. A
+queue the size of the default socket send buffer (wmem_default) is
+maintained between the PPP generic core and the ATM device.
+
+Fix it to queue a maximum of *two* packets. The one the ATM device is
+currently working on, and one more for the ATM driver to process
+immediately in its TX done interrupt handler. The PPP core is designed
+to feed packets to the channel with minimal latency, so that really
+ought to be enough to keep the ATM device busy.
+
+While we're at it, fix the fact that we were triggering the wakeup
+tasklet on *every* pppoatm_pop() call. The comment saying "this is
+inefficient, but doing it right is too hard" turns out to be overly
+pessimistic... I think :)
+
+On machines like the Traverse Geos, with a slow Geode CPU and two
+high-speed ADSL2+ interfaces, there were reports of extremely high CPU
+usage which could partly be attributed to the extra wakeups.
+
+(The wakeup handling could actually be made a whole lot easier if we
+ stop checking sk->sk_sndbuf altogether. Given that we now only queue
+ *two* packets ever, one wonders what the point is. As it is, you could
+ already deadlock the thing by setting the sk_sndbuf to a value lower
+ than the MTU of the device, and it'd just block for ever.)
+
+Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
+Signed-off-by: David S. Miller <davem@davemloft.net>
+---
+ net/atm/pppoatm.c |   95 +++++++++++++++++++++++++++++++++++++++++++++++-----
+ 1 files changed, 85 insertions(+), 10 deletions(-)
+
+diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
+index 614d3fc..ce1e59f 100644
+--- a/net/atm/pppoatm.c
++++ b/net/atm/pppoatm.c
+@@ -62,12 +62,25 @@ struct pppoatm_vcc {
+       void (*old_pop)(struct atm_vcc *, struct sk_buff *);
+                                       /* keep old push/pop for detaching */
+       enum pppoatm_encaps encaps;
++      atomic_t inflight;
++      unsigned long blocked;
+       int flags;                      /* SC_COMP_PROT - compress protocol */
+       struct ppp_channel chan;        /* interface to generic ppp layer */
+       struct tasklet_struct wakeup_tasklet;
+ };
+ /*
++ * We want to allow two packets in the queue. The one that's currently in
++ * flight, and *one* queued up ready for the ATM device to send immediately
++ * from its TX done IRQ. We want to be able to use atomic_inc_not_zero(), so
++ * inflight == -2 represents an empty queue, -1 one packet, and zero means
++ * there are two packets in the queue.
++ */
++#define NONE_INFLIGHT -2
++
++#define BLOCKED 0
++
++/*
+  * Header used for LLC Encapsulated PPP (4 bytes) followed by the LCP protocol
+  * ID (0xC021) used in autodetection
+  */
+@@ -102,16 +115,30 @@ static void pppoatm_wakeup_sender(unsigned long arg)
+ static void pppoatm_pop(struct atm_vcc *atmvcc, struct sk_buff *skb)
+ {
+       struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
++
+       pvcc->old_pop(atmvcc, skb);
++      atomic_dec(&pvcc->inflight);
++
+       /*
+-       * We don't really always want to do this since it's
+-       * really inefficient - it would be much better if we could
+-       * test if we had actually throttled the generic layer.
+-       * Unfortunately then there would be a nasty SMP race where
+-       * we could clear that flag just as we refuse another packet.
+-       * For now we do the safe thing.
++       * We always used to run the wakeup tasklet unconditionally here, for
++       * fear of race conditions where we clear the BLOCKED flag just as we
++       * refuse another packet in pppoatm_send(). This was quite inefficient.
++       *
++       * In fact it's OK. The PPP core will only ever call pppoatm_send()
++       * while holding the channel->downl lock. And ppp_output_wakeup() as
++       * called by the tasklet will *also* grab that lock. So even if another
++       * CPU is in pppoatm_send() right now, the tasklet isn't going to race
++       * with it. The wakeup *will* happen after the other CPU is safely out
++       * of pppoatm_send() again.
++       *
++       * So if the CPU in pppoatm_send() has already set the BLOCKED bit and
++       * it about to return, that's fine. We trigger a wakeup which will
++       * happen later. And if the CPU in pppoatm_send() *hasn't* set the
++       * BLOCKED bit yet, that's fine too because of the double check in
++       * pppoatm_may_send() which is commented there.
+        */
+-      tasklet_schedule(&pvcc->wakeup_tasklet);
++      if (test_and_clear_bit(BLOCKED, &pvcc->blocked))
++              tasklet_schedule(&pvcc->wakeup_tasklet);
+ }
+ /*
+@@ -184,6 +211,51 @@ error:
+       ppp_input_error(&pvcc->chan, 0);
+ }
++static inline int pppoatm_may_send(struct pppoatm_vcc *pvcc, int size)
++{
++      /*
++       * It's not clear that we need to bother with using atm_may_send()
++       * to check we don't exceed sk->sk_sndbuf. If userspace sets a
++       * value of sk_sndbuf which is lower than the MTU, we're going to
++       * block for ever. But the code always did that before we introduced
++       * the packet count limit, so...
++       */
++      if (atm_may_send(pvcc->atmvcc, size) &&
++          atomic_inc_not_zero_hint(&pvcc->inflight, NONE_INFLIGHT))
++              return 1;
++
++      /*
++       * We use test_and_set_bit() rather than set_bit() here because
++       * we need to ensure there's a memory barrier after it. The bit
++       * *must* be set before we do the atomic_inc() on pvcc->inflight.
++       * There's no smp_mb__after_set_bit(), so it's this or abuse
++       * smp_mb__after_clear_bit().
++       */
++      test_and_set_bit(BLOCKED, &pvcc->blocked);
++
++      /*
++       * We may have raced with pppoatm_pop(). If it ran for the
++       * last packet in the queue, *just* before we set the BLOCKED
++       * bit, then it might never run again and the channel could
++       * remain permanently blocked. Cope with that race by checking
++       * *again*. If it did run in that window, we'll have space on
++       * the queue now and can return success. It's harmless to leave
++       * the BLOCKED flag set, since it's only used as a trigger to
++       * run the wakeup tasklet. Another wakeup will never hurt.
++       * If pppoatm_pop() is running but hasn't got as far as making
++       * space on the queue yet, then it hasn't checked the BLOCKED
++       * flag yet either, so we're safe in that case too. It'll issue
++       * an "immediate" wakeup... where "immediate" actually involves
++       * taking the PPP channel's ->downl lock, which is held by the
++       * code path that calls pppoatm_send(), and is thus going to
++       * wait for us to finish.
++       */
++      if (atm_may_send(pvcc->atmvcc, size) &&
++          atomic_inc_not_zero(&pvcc->inflight))
++              return 1;
++
++      return 0;
++}
+ /*
+  * Called by the ppp_generic.c to send a packet - returns true if packet
+  * was accepted.  If we return false, then it's our job to call
+@@ -207,7 +279,7 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
+                       struct sk_buff *n;
+                       n = skb_realloc_headroom(skb, LLC_LEN);
+                       if (n != NULL &&
+-                          !atm_may_send(pvcc->atmvcc, n->truesize)) {
++                          !pppoatm_may_send(pvcc, n->truesize)) {
+                               kfree_skb(n);
+                               goto nospace;
+                       }
+@@ -215,12 +287,12 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
+                       skb = n;
+                       if (skb == NULL)
+                               return DROP_PACKET;
+-              } else if (!atm_may_send(pvcc->atmvcc, skb->truesize))
++              } else if (!pppoatm_may_send(pvcc, skb->truesize))
+                       goto nospace;
+               memcpy(skb_push(skb, LLC_LEN), pppllc, LLC_LEN);
+               break;
+       case e_vc:
+-              if (!atm_may_send(pvcc->atmvcc, skb->truesize))
++              if (!pppoatm_may_send(pvcc, skb->truesize))
+                       goto nospace;
+               break;
+       case e_autodetect:
+@@ -285,6 +357,9 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
+       if (pvcc == NULL)
+               return -ENOMEM;
+       pvcc->atmvcc = atmvcc;
++
++      /* Maximum is zero, so that we can use atomic_inc_not_zero() */
++      atomic_set(&pvcc->inflight, NONE_INFLIGHT);
+       pvcc->old_push = atmvcc->push;
+       pvcc->old_pop = atmvcc->pop;
+       pvcc->encaps = (enum pppoatm_encaps) be.encaps;
+-- 
+1.7.7.6
+
diff --git a/target/linux/generic/patches-3.3/130-pppoatm-queue-depth.patch b/target/linux/generic/patches-3.3/130-pppoatm-queue-depth.patch
new file mode 100644 (file)
index 0000000..d95ddf6
--- /dev/null
@@ -0,0 +1,193 @@
+From 9d02daf754238adac48fa075ee79e7edd3d79ed3 Mon Sep 17 00:00:00 2001
+From: David Woodhouse <dwmw2@infradead.org>
+Date: Sun, 8 Apr 2012 09:55:43 +0000
+Subject: [PATCH] pppoatm: Fix excessive queue bloat
+
+We discovered that PPPoATM has an excessively deep transmit queue. A
+queue the size of the default socket send buffer (wmem_default) is
+maintained between the PPP generic core and the ATM device.
+
+Fix it to queue a maximum of *two* packets. The one the ATM device is
+currently working on, and one more for the ATM driver to process
+immediately in its TX done interrupt handler. The PPP core is designed
+to feed packets to the channel with minimal latency, so that really
+ought to be enough to keep the ATM device busy.
+
+While we're at it, fix the fact that we were triggering the wakeup
+tasklet on *every* pppoatm_pop() call. The comment saying "this is
+inefficient, but doing it right is too hard" turns out to be overly
+pessimistic... I think :)
+
+On machines like the Traverse Geos, with a slow Geode CPU and two
+high-speed ADSL2+ interfaces, there were reports of extremely high CPU
+usage which could partly be attributed to the extra wakeups.
+
+(The wakeup handling could actually be made a whole lot easier if we
+ stop checking sk->sk_sndbuf altogether. Given that we now only queue
+ *two* packets ever, one wonders what the point is. As it is, you could
+ already deadlock the thing by setting the sk_sndbuf to a value lower
+ than the MTU of the device, and it'd just block for ever.)
+
+Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
+Signed-off-by: David S. Miller <davem@davemloft.net>
+---
+ net/atm/pppoatm.c |   95 +++++++++++++++++++++++++++++++++++++++++++++++-----
+ 1 files changed, 85 insertions(+), 10 deletions(-)
+
+diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
+index 614d3fc..ce1e59f 100644
+--- a/net/atm/pppoatm.c
++++ b/net/atm/pppoatm.c
+@@ -62,12 +62,25 @@ struct pppoatm_vcc {
+       void (*old_pop)(struct atm_vcc *, struct sk_buff *);
+                                       /* keep old push/pop for detaching */
+       enum pppoatm_encaps encaps;
++      atomic_t inflight;
++      unsigned long blocked;
+       int flags;                      /* SC_COMP_PROT - compress protocol */
+       struct ppp_channel chan;        /* interface to generic ppp layer */
+       struct tasklet_struct wakeup_tasklet;
+ };
+ /*
++ * We want to allow two packets in the queue. The one that's currently in
++ * flight, and *one* queued up ready for the ATM device to send immediately
++ * from its TX done IRQ. We want to be able to use atomic_inc_not_zero(), so
++ * inflight == -2 represents an empty queue, -1 one packet, and zero means
++ * there are two packets in the queue.
++ */
++#define NONE_INFLIGHT -2
++
++#define BLOCKED 0
++
++/*
+  * Header used for LLC Encapsulated PPP (4 bytes) followed by the LCP protocol
+  * ID (0xC021) used in autodetection
+  */
+@@ -102,16 +115,30 @@ static void pppoatm_wakeup_sender(unsigned long arg)
+ static void pppoatm_pop(struct atm_vcc *atmvcc, struct sk_buff *skb)
+ {
+       struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
++
+       pvcc->old_pop(atmvcc, skb);
++      atomic_dec(&pvcc->inflight);
++
+       /*
+-       * We don't really always want to do this since it's
+-       * really inefficient - it would be much better if we could
+-       * test if we had actually throttled the generic layer.
+-       * Unfortunately then there would be a nasty SMP race where
+-       * we could clear that flag just as we refuse another packet.
+-       * For now we do the safe thing.
++       * We always used to run the wakeup tasklet unconditionally here, for
++       * fear of race conditions where we clear the BLOCKED flag just as we
++       * refuse another packet in pppoatm_send(). This was quite inefficient.
++       *
++       * In fact it's OK. The PPP core will only ever call pppoatm_send()
++       * while holding the channel->downl lock. And ppp_output_wakeup() as
++       * called by the tasklet will *also* grab that lock. So even if another
++       * CPU is in pppoatm_send() right now, the tasklet isn't going to race
++       * with it. The wakeup *will* happen after the other CPU is safely out
++       * of pppoatm_send() again.
++       *
++       * So if the CPU in pppoatm_send() has already set the BLOCKED bit and
++       * it about to return, that's fine. We trigger a wakeup which will
++       * happen later. And if the CPU in pppoatm_send() *hasn't* set the
++       * BLOCKED bit yet, that's fine too because of the double check in
++       * pppoatm_may_send() which is commented there.
+        */
+-      tasklet_schedule(&pvcc->wakeup_tasklet);
++      if (test_and_clear_bit(BLOCKED, &pvcc->blocked))
++              tasklet_schedule(&pvcc->wakeup_tasklet);
+ }
+ /*
+@@ -184,6 +211,51 @@ error:
+       ppp_input_error(&pvcc->chan, 0);
+ }
++static inline int pppoatm_may_send(struct pppoatm_vcc *pvcc, int size)
++{
++      /*
++       * It's not clear that we need to bother with using atm_may_send()
++       * to check we don't exceed sk->sk_sndbuf. If userspace sets a
++       * value of sk_sndbuf which is lower than the MTU, we're going to
++       * block for ever. But the code always did that before we introduced
++       * the packet count limit, so...
++       */
++      if (atm_may_send(pvcc->atmvcc, size) &&
++          atomic_inc_not_zero_hint(&pvcc->inflight, NONE_INFLIGHT))
++              return 1;
++
++      /*
++       * We use test_and_set_bit() rather than set_bit() here because
++       * we need to ensure there's a memory barrier after it. The bit
++       * *must* be set before we do the atomic_inc() on pvcc->inflight.
++       * There's no smp_mb__after_set_bit(), so it's this or abuse
++       * smp_mb__after_clear_bit().
++       */
++      test_and_set_bit(BLOCKED, &pvcc->blocked);
++
++      /*
++       * We may have raced with pppoatm_pop(). If it ran for the
++       * last packet in the queue, *just* before we set the BLOCKED
++       * bit, then it might never run again and the channel could
++       * remain permanently blocked. Cope with that race by checking
++       * *again*. If it did run in that window, we'll have space on
++       * the queue now and can return success. It's harmless to leave
++       * the BLOCKED flag set, since it's only used as a trigger to
++       * run the wakeup tasklet. Another wakeup will never hurt.
++       * If pppoatm_pop() is running but hasn't got as far as making
++       * space on the queue yet, then it hasn't checked the BLOCKED
++       * flag yet either, so we're safe in that case too. It'll issue
++       * an "immediate" wakeup... where "immediate" actually involves
++       * taking the PPP channel's ->downl lock, which is held by the
++       * code path that calls pppoatm_send(), and is thus going to
++       * wait for us to finish.
++       */
++      if (atm_may_send(pvcc->atmvcc, size) &&
++          atomic_inc_not_zero(&pvcc->inflight))
++              return 1;
++
++      return 0;
++}
+ /*
+  * Called by the ppp_generic.c to send a packet - returns true if packet
+  * was accepted.  If we return false, then it's our job to call
+@@ -207,7 +279,7 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
+                       struct sk_buff *n;
+                       n = skb_realloc_headroom(skb, LLC_LEN);
+                       if (n != NULL &&
+-                          !atm_may_send(pvcc->atmvcc, n->truesize)) {
++                          !pppoatm_may_send(pvcc, n->truesize)) {
+                               kfree_skb(n);
+                               goto nospace;
+                       }
+@@ -215,12 +287,12 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
+                       skb = n;
+                       if (skb == NULL)
+                               return DROP_PACKET;
+-              } else if (!atm_may_send(pvcc->atmvcc, skb->truesize))
++              } else if (!pppoatm_may_send(pvcc, skb->truesize))
+                       goto nospace;
+               memcpy(skb_push(skb, LLC_LEN), pppllc, LLC_LEN);
+               break;
+       case e_vc:
+-              if (!atm_may_send(pvcc->atmvcc, skb->truesize))
++              if (!pppoatm_may_send(pvcc, skb->truesize))
+                       goto nospace;
+               break;
+       case e_autodetect:
+@@ -285,6 +357,9 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
+       if (pvcc == NULL)
+               return -ENOMEM;
+       pvcc->atmvcc = atmvcc;
++
++      /* Maximum is zero, so that we can use atomic_inc_not_zero() */
++      atomic_set(&pvcc->inflight, NONE_INFLIGHT);
+       pvcc->old_push = atmvcc->push;
+       pvcc->old_pop = atmvcc->pop;
+       pvcc->encaps = (enum pppoatm_encaps) be.encaps;
+-- 
+1.7.7.6
+