Re: PPPoE performance regression

Re: PPPoE performance regression

[David Woodhouse]

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

 On Sun, 2012-06-10 at 10:50 +1000, Nathan Williams wrote:
> > When using iperf with UDP, we can get 20Mbps downstream, but only about
> > 15Mbps throughput when using TCP on a short ADSL line (line sync at
> > 25Mbps).  Using iperf to send UDP traffic upstream at the same time
> > doesn't affect the downstream rate.
>
> ...
>
> I found the change responsible for the performance problem and rebuilt
> OpenWrt with the patch reversed on kernel 3.3.8 to confirm everything
> still works.  So the TX buffer is getting full, which causes the netif
> queue to be stopped and restarted after some skbs have been freed?

The *Ethernet* netif queue, yes. But not the PPP netif queue, I believe.
I think the PPP code keeps just blindly calling dev_queue_xmit() and
throwing away packets when they're not accepted.

> commit 137742cf9738f1b4784058ff79aec7ca85e769d4
> Author: Karl Hiramoto <karl@hiramoto.org>
> Date:   Wed Sep 2 23:26:39 2009 -0700
> 
>     atm/br2684: netif_stop_queue() when atm device busy and
> netif_wake_queue() when we can send packets again.

Nice work; well done finding that. I've added Karl and DaveM, and the
netdev@ list to Cc.

(Btw, I assume the performance problem also goes away if you use PPPoA?
I've made changes in the PPPoA code recently to *eliminate* excessive
calls to netif_wake_queue(), and also to stop it from filling the ATM
device queue. That was commit 9d02daf7 in 3.5-rc1, which is already in
OpenWRT.)

I was already looking vaguely at how we could limit the PPP queue depth
for PPPoE and implement byte queue limits. Currently the PPP code just
throws the packets at the Ethernet device and considers them 'gone',
which is why it's hitting the ATM limits all the time. The patch you
highlight is changing the behaviour in a case that should never *happen*
with PPP. It's suffering massive queue bloat if it's filling the ATM
queue, and we should fix *that*.

I was looking to see if we could (ab)use the skb->destructor somehow so
that we get *notified* when the packet is actually sent (or dropped),
and then that would allow us to manage the queue 'downstream' of PPP
more sanely. But I haven't really got very far with that yet.

I was planning to find some time to look into it a bit better, and then
send mail to netdev@ asking for more clue. But since you're now falling
over it and it isn't just a theoretical problem, this mail will have to
suffice for now...

-- 
dwmw2

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

Re: PPPoE performance regression

[David Woodhouse]

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

On Sun, 2012-06-10 at 09:32 +0100, David Woodhouse wrote:
> I was looking to see if we could (ab)use the skb->destructor somehow so
> that we get *notified* when the packet is actually sent (or dropped),
> and then that would allow us to manage the queue 'downstream' of PPP
> more sanely. But I haven't really got very far with that yet.

This doesn't look *so* evil... if the basic concept of using
skb_orphan() and then setting our own destructor is OK, then I'll work
out the rest of the details and do it for l2tp too.

If the end-goal is to do BQL for PPP and honour the netdev queue limits,
perhaps the best option is to add a 'done sending skb' callback to the
generic ppp code and let it manage the queues (and the BQL callbacks)
*there*.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation



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

Re: PPPoE performance regression

[David Woodhouse]

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

On Wed, 2012-06-13 at 10:57 +0100, David Woodhouse wrote:
> This doesn't look *so* evil... if the basic concept of using
> skb_orphan() and then setting our own destructor is OK, then I'll work
> out the rest of the details and do it for l2tp too.

Stupid dwmw2. With patch this time...

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index cbf7047..ddaf156 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -689,6 +689,8 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
 		sk->sk_state = PPPOX_CONNECTED;
 	}
 
+	atomic_set(&po->inflight, -2);
+
 	po->num = sp->sa_addr.pppoe.sid;
 
 end:
@@ -952,9 +954,34 @@ abort:
  * sends PPP frame over PPPoE socket
  *
  ***********************************************************************/
+static void pppoe_skb_destructor(struct sk_buff *skb)
+{
+	struct sock *sk = skb->sk;
+	struct pppox_sock *po = pppox_sk(sk);
+
+	atomic_dec(&po->inflight);
+	/* Schedule a call to ppp_output_wakeup(chan), if it was already blocked.
+	   Mind for race conditions with another CPU which is in pppoe_xmit() 
+	   right now. See commit 9d02daf7 in pppoatm. */
+	sock_put(sk);
+}
+
 static int pppoe_xmit(struct ppp_channel *chan, struct sk_buff *skb)
 {
 	struct sock *sk = (struct sock *)chan->private;
+	struct pppox_sock *po = pppox_sk(sk);
+
+	if (!atomic_inc_not_zero(&po->inflight))
+		return 0;
+
+	/* mine! all mine! */
+	skb_orphan(skb);
+	skb->destructor = pppoe_skb_destructor;
+	/* XXX: Are there other implications of setting this? Should we use ->cb? */
+	skb->sk = sk;
+
+	sock_hold(sk);
+
 	return __pppoe_xmit(sk, skb);
 }
 
diff --git a/include/linux/if_pppox.h b/include/linux/if_pppox.h
index 09c474c..339c75d 100644
--- a/include/linux/if_pppox.h
+++ b/include/linux/if_pppox.h
@@ -186,6 +186,7 @@ struct pppox_sock {
 	/* struct sock must be the first member of pppox_sock */
 	struct sock sk;
 	struct ppp_channel chan;
+	atomic_t inflight;
 	struct pppox_sock	*next;	  /* for hash table */
 	union {
 		struct pppoe_opt pppoe;

-- 
dwmw2

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

Re: PPPoE performance regression

[Benjamin LaHaise]

On Wed, Jun 13, 2012 at 02:50:01PM +0100, David Woodhouse wrote:
> On Wed, 2012-06-13 at 10:57 +0100, David Woodhouse wrote:
> > This doesn't look *so* evil... if the basic concept of using
> > skb_orphan() and then setting our own destructor is OK, then I'll work
> > out the rest of the details and do it for l2tp too.
> 
> Stupid dwmw2. With patch this time...

Does this actually work?  Could the skb not end up sitting on the receive 
queue of a user socket indefinitely, deferring all further transmits?  From 
an ISP point of view, PPPoE and L2TP are most typically used on links where 
the congestion point is not the local interface the packets are being pumped 
into (think of the vast majority of ethernet based DSL modems), and this 
kind of transmit overhead is a pure waste of CPU cycles.  The only solution 
that generically works in most PPPoE/L2TP situations is to shape outgoing 
traffic to the speed of limiting link.

Maybe there's a PPP extension that does flow control...

		-ben

Re: PPPoE performance regression

[David Woodhouse]

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

On Wed, 2012-06-13 at 11:55 -0400, Benjamin LaHaise wrote:
> Does this actually work?  Could the skb not end up sitting on the
> receive  queue of a user socket indefinitely, deferring all further
> transmits?  From an ISP point of view, 

I haven't tried it; only compiled it. Certainly, the similar approach in
PPPoATM in commit 9d02daf7 *does* work for limiting the bufferbloat and
keeping the queues under control. And it'll let me do BQL for PPPoA.

I'm looking at this from the client side, not the ISP side. And in that
case the local interface *is* the bottleneck. When it's a PPPoE over
br2684 interface and it's full, we should stop the PPP netdev from
spewing packets at it, rather than just dropping them.

On the ISP side if the skb ends up sitting on a receive queue of a user
socket, and nothing is servicing that socket, surely the transmits on
this channel weren't happening anyway?

-- 
dwmw2

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

Re: PPPoE performance regression

[Benjamin LaHaise]

On Wed, Jun 13, 2012 at 05:11:34PM +0100, David Woodhouse wrote:
> On Wed, 2012-06-13 at 11:55 -0400, Benjamin LaHaise wrote:
> > Does this actually work?  Could the skb not end up sitting on the
> > receive  queue of a user socket indefinitely, deferring all further
> > transmits?  From an ISP point of view, 
> 
> I haven't tried it; only compiled it. Certainly, the similar approach in
> PPPoATM in commit 9d02daf7 *does* work for limiting the bufferbloat and
> keeping the queues under control. And it'll let me do BQL for PPPoA.
> 
> I'm looking at this from the client side, not the ISP side. And in that
> case the local interface *is* the bottleneck. When it's a PPPoE over
> br2684 interface and it's full, we should stop the PPP netdev from
> spewing packets at it, rather than just dropping them.

I would contend that PPPoE over br2684 is not the common case.  The vast 
majority of users in client mode are going to be using PPPoE over an 
ethernet link to a DSL modem (or cable or wireless radios even).  Just look 
at what DSL modems are available for users in computer stores / what ISPs 
actually ship to their users.  Real ATM exposing devices are rare.

> On the ISP side if the skb ends up sitting on a receive queue of a user
> socket, and nothing is servicing that socket, surely the transmits on
> this channel weren't happening anyway?

True, but it's a design issue we've had to contend with elsewhere in the 
various tunnelling protocols.

Don't get me wrong: I am very much in favour of intelligent queue 
management, but this approach simply does not work for the vast majority 
of PPPoE users, while it adds overhead that will negatively impact access 
concentrators.  If you can somehow restrict the overhead to only impacting 
your use-case, that would be an improvement.

		-ben
-- 
"Thought is the essence of where you are now."

RE: PPPoE performance regression

[David Laight]

 
> I would contend that PPPoE over br2684 is not the common case.  The
vast 
> majority of users in client mode are going to be using PPPoE over an 
> ethernet link to a DSL modem (or cable or wireless radios even).  Just
look 
> at what DSL modems are available for users in computer stores / what
ISPs 
> actually ship to their users.  Real ATM exposing devices are rare.

PPPoA is common in the UK.

	David

Re: PPPoE performance regression

[David Woodhouse]

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

On Wed, 2012-06-13 at 12:31 -0400, Benjamin LaHaise wrote:
> I would contend that PPPoE over br2684 is not the common case.  The vast 
> majority of users in client mode are going to be using PPPoE over an 
> ethernet link to a DSL modem (or cable or wireless radios even).  Just look 
> at what DSL modems are available for users in computer stores / what ISPs 
> actually ship to their users.  Real ATM exposing devices are rare.

I'm looking at the class of device on which OpenWRT runs. Linux is *on*
the router with the ADSL port, not connected to it via Ethernet.

I can buy lots of these in the shop. Anything that's an ADSL *router*
rather than *modem* is likely to be running, or at least capable of
running, Linux.

Admittedly, if you have access to the native ADSL interface then you'd
do a lot better to run PPPoA — but I already fixed this issue for PPPoA.
There are people in some parts of the world who are using PPPoEoA and
putting up with the resulting MTU issues because the *ISP* doesn't
support proper PPPoA.

And even if it *were* rare... this is the case that *should* work best,
where we have complete control of the hardware. There's no excuse for
the behaviour that we currently see with PPPoE on BR2684.

> > On the ISP side if the skb ends up sitting on a receive queue of a user
> > socket, and nothing is servicing that socket, surely the transmits on
> > this channel weren't happening anyway?
> 
> True, but it's a design issue we've had to contend with elsewhere in the 
> various tunnelling protocols.
> 
> Don't get me wrong: I am very much in favour of intelligent queue 
> management, but this approach simply does not work for the vast majority 
> of PPPoE users, while it adds overhead that will negatively impact access 
> concentrators. 

I think that's largely true of BQL in general, isn't it? That's OK; it's
a config option. I suspect if I make this accounting of PPPoE / L2TP
packets conditional on BQL (or perhaps on a separate config option
PPP_BQL) that ought to address your concern about the cases where you
don't need it?

-- 
dwmw2

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

Re: PPPoE performance regression

[David Woodhouse]

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

On Wed, 2012-06-13 at 17:32 +0100, David Laight wrote:
> > I would contend that PPPoE over br2684 is not the common case.  The vast 
> > majority of users in client mode are going to be using PPPoE over an 
> > ethernet link to a DSL modem (or cable or wireless radios even).  Just look 
> > at what DSL modems are available for users in computer stores / what ISPs 
> > actually ship to their users.  Real ATM exposing devices are rare.
> 
> PPPoA is common in the UK.

In the UK you tend to have the option of using PPPoA *or* PPPoE over
BR2684. The ISP's kit will handle both.

Ben's comment was about the *hardware*, though. If your "ADSL modem" is
a separate box which just bridges Ethernet to BR2684 on the ADSL link,
you're limited to using the PPPoE protocol over that.

Obviously, if you have a proper ADSL *router* and it's not just a PPP
bridge, then you can — and should — use PPPoA.

-- 
dwmw2

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

Re: PPPoE performance regression

[Benjamin LaHaise]

On Wed, Jun 13, 2012 at 05:53:03PM +0100, David Woodhouse wrote:
> I'm looking at the class of device on which OpenWRT runs. Linux is *on*
> the router with the ADSL port, not connected to it via Ethernet.

Ah, yes, that's a worthwhile pursuit.

> And even if it *were* rare... this is the case that *should* work best,
> where we have complete control of the hardware. There's no excuse for
> the behaviour that we currently see with PPPoE on BR2684.

*nod*

> I think that's largely true of BQL in general, isn't it? That's OK; it's
> a config option. I suspect if I make this accounting of PPPoE / L2TP
> packets conditional on BQL (or perhaps on a separate config option
> PPP_BQL) that ought to address your concern about the cases where you
> don't need it?

That would help.

On the whole question of PPPoE over intermediate ethernet links to ADSL 
modems, I think it would be possible to limit latency by implementing a 
sliding window clocked using LCP ECHO requests.  Does this sound worthwhile 
implementing?  What sort of queue depths are you looking at for the ATM 
devices you're working on?

		-ben
-- 
"Thought is the essence of where you are now."

Re: PPPoE performance regression

[David Woodhouse]

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

On Wed, 2012-06-13 at 13:21 -0400, Benjamin LaHaise wrote:
> On the whole question of PPPoE over intermediate ethernet links to ADSL 
> modems, I think it would be possible to limit latency by implementing a 
> sliding window clocked using LCP ECHO requests.  Does this sound worthwhile 
> implementing?

Maybe, for someone who actually cares about those who are using separate
ADSL modems, and doesn't think they should just better hardware if they
care about the performance and queue management :)

>   What sort of queue depths are you looking at for the ATM devices
> you're working on? 

We're managing the queue to keep it short. On the PPPoATM side, we (now)
strictly limit the number of packets between the ppp_generic code and
the ATM device. It's basically one in-flight and one waiting to be
handed to the device in the TX done interrupt. PPP is designed to feed
new packets with low latency, after all.

The Solos ADSL device *used* to eat as many packets as it had internal
RAM to store them in, acknowledging the "transmit" as soon as it had
buffered them. I got Nathan to fix that some time ago, and the internal
queue is fairly short now.

-- 
dwmw2

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

Re: PPPoE performance regression

[Karl Hiramoto]

On 06/10/12 10:32, David Woodhouse wrote:
>   On Sun, 2012-06-10 at 10:50 +1000, Nathan Williams wrote:
>>> When using iperf with UDP, we can get 20Mbps downstream, but only about
>>> 15Mbps throughput when using TCP on a short ADSL line (line sync at
>>> 25Mbps).  Using iperf to send UDP traffic upstream at the same time
>>> doesn't affect the downstream rate.
>> ...
>>
>> I found the change responsible for the performance problem and rebuilt
>> OpenWrt with the patch reversed on kernel 3.3.8 to confirm everything
>> still works.  So the TX buffer is getting full, which causes the netif
>> queue to be stopped and restarted after some skbs have been freed?
> The *Ethernet* netif queue, yes. But not the PPP netif queue, I believe.
> I think the PPP code keeps just blindly calling dev_queue_xmit() and
> throwing away packets when they're not accepted.
>
>> commit 137742cf9738f1b4784058ff79aec7ca85e769d4
>> Author: Karl Hiramoto <karl@hiramoto.org>
>> Date:   Wed Sep 2 23:26:39 2009 -0700
>>
>>      atm/br2684: netif_stop_queue() when atm device busy and
>> netif_wake_queue() when we can send packets again.
> Nice work; well done finding that. I've added Karl and DaveM, and the
> netdev@ list to Cc.
>
> (Btw, I assume the performance problem also goes away if you use PPPoA?
> I've made changes in the PPPoA code recently to *eliminate* excessive
> calls to netif_wake_queue(), and also to stop it from filling the ATM
> device queue. That was commit 9d02daf7 in 3.5-rc1, which is already in
> OpenWRT.)
>
> I was already looking vaguely at how we could limit the PPP queue depth
> for PPPoE and implement byte queue limits. Currently the PPP code just
> throws the packets at the Ethernet device and considers them 'gone',
> which is why it's hitting the ATM limits all the time. The patch you
> highlight is changing the behaviour in a case that should never *happen*
> with PPP. It's suffering massive queue bloat if it's filling the ATM
> queue, and we should fix *that*.

Agreed,  the issue is the PPP layer.     I've seen this issue with PPPoE 
before,  haven't had  the itch, time or interest to fix it though.   A 
workaround to help mitigate the issue may be to increase the TX queue 
length of the  br2684  interface, and the atm device if possible.   
You'll pay the price  of buffer bloat and latency.


--
Karl

Re: PPPoE performance regression

[Paul Mackerras]

On Wed, Jun 13, 2012 at 02:50:01PM +0100, David Woodhouse wrote:
> On Wed, 2012-06-13 at 10:57 +0100, David Woodhouse wrote:
> > This doesn't look *so* evil... if the basic concept of using
> > skb_orphan() and then setting our own destructor is OK, then I'll work
> > out the rest of the details and do it for l2tp too.
> 
> Stupid dwmw2. With patch this time...

> +static void pppoe_skb_destructor(struct sk_buff *skb)
> +{
> +	struct sock *sk = skb->sk;
> +	struct pppox_sock *po = pppox_sk(sk);
> +
> +	atomic_dec(&po->inflight);
> +	/* Schedule a call to ppp_output_wakeup(chan), if it was already blocked.
> +	   Mind for race conditions with another CPU which is in pppoe_xmit() 
> +	   right now. See commit 9d02daf7 in pppoatm. */
> +	sock_put(sk);
> +}

Umm, how does ppp_output_wakeup() actually get called?

Paul.

Re: PPPoE performance regression

[David Woodhouse]

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

On Thu, 2012-06-14 at 16:18 +1000, Paul Mackerras wrote:
> Umm, how does ppp_output_wakeup() actually get called? 

Not directly from the destructor; it'll need to be from a tasklet like
the PPPoATM code does it. And the same race conditions, and the same
handling of them which makes me slightly uneasy as it depends quite
intimately on ppp_generic's internal locking scheme, will apply.

That's what I meant when I said I'd work out the rest of the details...

-- 
dwmw2

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

Re: PPPoE performance regression

[David Woodhouse]

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

On Thu, 2012-06-14 at 16:18 +1000, Paul Mackerras wrote:
> Umm, how does ppp_output_wakeup() actually get called?

In fact I'm thinking of eliminating ppp_output_wakeup() in the general
case.

The idea (and it is *just* an idea so far) is to introduce
ppp_sent_queue(), ppp_completed_queue() and ppp_reset_queue()¹ functions
which take a ppp_chan and map onto the corresponding netdev_* functions
for BQL.

Having done that, we should be able to trigger the wakeup automatically
from the ppp_completed_queue() function, and there's no need for channel
drivers to call ppp_output_wakeup() directly. Not only do we get proper
holistic queue length management, we also move the flow control into PPP
and get rid of the horrid dependency on internal PPP locking that's
documented in commit 9d02daf75², and which we'd have to address on the
PPPoX side too.

And the overhead that Ben is concerned about should be fairly minimal.

-- 
dwmw2

¹ For ppp_reset_queue in the mlppp case it gets moderately non-trivial.
² Look for 'downl'. Ick.


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