Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 16, 2019 at 11:17:53AM +0200, Arend Van Spriel wrote:
> On 4/16/2019 10:37 AM, Johannes Berg wrote:
> > It is true because we have an entire buffering layer in mac80211 (in
> > this case at least) and never push back to the stack.
> 
> Ok, so the crux is the "never push back to the stack" part? Well, the
> internal TXQ and how that is used is obviously enabling that ;-)

So assuming that these drivers all have a TX queue length of zero
and therefore do not make use of Linux queueing disciplines then
yes techincally LLTX is fine.

However, I must say that it is much better to provide real
congestion feedback to the stack when you can because otherwise
things like UDP may fall apart.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 16, 2019 at 11:33:50AM +0200, Johannes Berg wrote:
> On Tue, 2019-04-16 at 10:33 +0100, Toke Høiland-Jørgensen wrote:
> > 
> > > It is true because we have an entire buffering layer in mac80211 (in
> > > this case at least) and never push back to the stack.
> > 
> > I'm wondering if we should be?
> 
> I don't think so? We'd just buffer packets in yet another place.

But you do realise that you're giving up on the rich queueing
functionality that Linux provides (net/sched), not to mention
breaking certain applications that rely on congestion feedback?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Johannes Berg]

On Tue, 2019-04-16 at 17:37 +0800, Herbert Xu wrote:
> On Tue, Apr 16, 2019 at 11:33:50AM +0200, Johannes Berg wrote:
> > On Tue, 2019-04-16 at 10:33 +0100, Toke Høiland-Jørgensen wrote:
> > > 
> > > > It is true because we have an entire buffering layer in mac80211 (in
> > > > this case at least) and never push back to the stack.
> > > 
> > > I'm wondering if we should be?
> > 
> > I don't think so? We'd just buffer packets in yet another place.
> 
> But you do realise that you're giving up on the rich queueing
> functionality that Linux provides (net/sched),

Yes, that was a trade-off we always knew about. The model that Linux
provides is just not suited for wifi.

> not to mention
> breaking certain applications that rely on congestion feedback?

This I don't understand. The congestion feedback happens through socket
buffer space etc. which is still there (as long as nobody sneaks in an
skb_orphan() call)

johannes

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Tue, 2019-04-16 at 17:37 +0800, Herbert Xu wrote:
>> On Tue, Apr 16, 2019 at 11:33:50AM +0200, Johannes Berg wrote:
>> > On Tue, 2019-04-16 at 10:33 +0100, Toke Høiland-Jørgensen wrote:
>> > > 
>> > > > It is true because we have an entire buffering layer in mac80211 (in
>> > > > this case at least) and never push back to the stack.
>> > > 
>> > > I'm wondering if we should be?
>> > 
>> > I don't think so? We'd just buffer packets in yet another place.
>> 
>> But you do realise that you're giving up on the rich queueing
>> functionality that Linux provides (net/sched),
>
> Yes, that was a trade-off we always knew about. The model that Linux
> provides is just not suited for wifi.

As explained at great length here:
https://www.usenix.org/conference/atc17/technical-sessions/presentation/hoilan-jorgesen
(you already know that of course, Johannes)

>> not to mention
>> breaking certain applications that rely on congestion feedback?
>
> This I don't understand. The congestion feedback happens through socket
> buffer space etc. which is still there (as long as nobody sneaks in an
> skb_orphan() call)

Sure, for TCP, the TSQ mechanism should keep the upper-level queue low
as long as the SKBs are alive. But is this also the case for UDP?

-Toke

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 16, 2019 at 11:39:56AM +0200, Johannes Berg wrote:
> 
> > not to mention
> > breaking certain applications that rely on congestion feedback?
> 
> This I don't understand. The congestion feedback happens through socket
> buffer space etc. which is still there (as long as nobody sneaks in an
> skb_orphan() call)

The congestion control happens at two levels.  You are right that
the socket buffer acts as one limit.  However, other applications
may also rely on the TX queue being full as the throttle (by setting
a sufficiently large socket buffer size).

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Toke Høiland-Jørgensen]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Tue, Apr 16, 2019 at 11:39:56AM +0200, Johannes Berg wrote:
>> 
>> > not to mention
>> > breaking certain applications that rely on congestion feedback?
>> 
>> This I don't understand. The congestion feedback happens through socket
>> buffer space etc. which is still there (as long as nobody sneaks in an
>> skb_orphan() call)
>
> The congestion control happens at two levels. You are right that the
> socket buffer acts as one limit. However, other applications may also
> rely on the TX queue being full as the throttle (by setting a
> sufficiently large socket buffer size).

Do you happen to have an example of an application that does this that
could be used for testing? :)

-Toke

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Johannes Berg]

On Tue, 2019-04-16 at 21:13 +0800, Herbert Xu wrote:
> On Tue, Apr 16, 2019 at 11:39:56AM +0200, Johannes Berg wrote:
> > 
> > > not to mention
> > > breaking certain applications that rely on congestion feedback?
> > 
> > This I don't understand. The congestion feedback happens through socket
> > buffer space etc. which is still there (as long as nobody sneaks in an
> > skb_orphan() call)
> 
> The congestion control happens at two levels.  You are right that
> the socket buffer acts as one limit.  However, other applications
> may also rely on the TX queue being full as the throttle (by setting
> a sufficiently large socket buffer size).

I'm not sure how they'd even realize this, other than packets getting
dropped? Which of course we do here as well, we didn't invent something
that let's us expand memory arbitrarily ;-)

johannes

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 16, 2019 at 11:02:50AM +0100, Toke Høiland-Jørgensen wrote:
> 
> As explained at great length here:
> https://www.usenix.org/conference/atc17/technical-sessions/presentation/hoilan-jorgesen
> (you already know that of course, Johannes)

I can understand that wireless needs its own queueing scheme, but
it still seems wrong to place that under net/mac80211 as opposed
to having it as a first-class citizen under net/sched.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 16, 2019 at 09:13:16PM +0200, Johannes Berg wrote:
>
> I'm not sure how they'd even realize this, other than packets getting
> dropped? Which of course we do here as well, we didn't invent something
> that let's us expand memory arbitrarily ;-)

They rely on the queueing mechanism providing feedback via an
error return value when the queue becomes full.  From my cursory
look at net/mac80211 you seem to be always returning success in
ieee80211_subif_start_xmit.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 16, 2019 at 02:18:36PM +0100, Toke Høiland-Jørgensen wrote:
>
> > The congestion control happens at two levels. You are right that the
> > socket buffer acts as one limit. However, other applications may also
> > rely on the TX queue being full as the throttle (by setting a
> > sufficiently large socket buffer size).
> 
> Do you happen to have an example of an application that does this that
> could be used for testing? :)

Have a look at

commit 6ce9e7b5fe3195d1ae6e3a0753d4ddcac5cd699e
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date:   Wed Sep 2 18:05:33 2009 -0700

    ip: Report qdisc packet drops

You should be able to do a UDP flood while setting IP_RECVERR to
detect the packet drop due to a full queue which AFAICS will never
happen with the current mac80211 setup.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Toke Høiland-Jørgensen]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Tue, Apr 16, 2019 at 11:02:50AM +0100, Toke Høiland-Jørgensen wrote:
>> 
>> As explained at great length here:
>> https://www.usenix.org/conference/atc17/technical-sessions/presentation/hoilan-jorgesen
>> (you already know that of course, Johannes)
>
> I can understand that wireless needs its own queueing scheme, but it
> still seems wrong to place that under net/mac80211 as opposed to
> having it as a first-class citizen under net/sched.

This is because we need to resolve the MAC-layer destination station (or
rather, TID) and tie the queueing to that, because of aggregation. We
also use the queueing structure for scheduling stations to achieve
airtime fairness. Both of these would be decidedly non-trivial to pull
up to the qdisc layer. Rather, having them in mac80211 means drivers
don't need to do their own ad-hoc queueing (which was the case before,
leading extra bufferbloat).

Most of the actual queueing structure code lives in
include/net/fq_impl.h, though, so it's not actually that
mac80211-specific. I've been thinking about porting the relevant qdiscs
to use the same code, but I'm not sure that it's worth the trouble.

-Toke

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Toke Høiland-Jørgensen]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Tue, Apr 16, 2019 at 02:18:36PM +0100, Toke Høiland-Jørgensen wrote:
>>
>> > The congestion control happens at two levels. You are right that the
>> > socket buffer acts as one limit. However, other applications may also
>> > rely on the TX queue being full as the throttle (by setting a
>> > sufficiently large socket buffer size).
>> 
>> Do you happen to have an example of an application that does this that
>> could be used for testing? :)
>
> Have a look at
>
> commit 6ce9e7b5fe3195d1ae6e3a0753d4ddcac5cd699e
> Author: Eric Dumazet <eric.dumazet@gmail.com>
> Date:   Wed Sep 2 18:05:33 2009 -0700
>
>     ip: Report qdisc packet drops
>
> You should be able to do a UDP flood while setting IP_RECVERR to
> detect the packet drop due to a full queue which AFAICS will never
> happen with the current mac80211 setup.

Yup, got that part. Was just wondering if you know of any applications
that already do this, that I could test without having to write my
own... :)

-Toke

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Arend Van Spriel]

+ Bob

On 4/17/2019 11:09 AM, Toke Høiland-Jørgensen wrote:
> Herbert Xu <herbert@gondor.apana.org.au> writes:
> 
>> On Tue, Apr 16, 2019 at 02:18:36PM +0100, Toke Høiland-Jørgensen wrote:
>>>
>>>> The congestion control happens at two levels. You are right that the
>>>> socket buffer acts as one limit. However, other applications may also
>>>> rely on the TX queue being full as the throttle (by setting a
>>>> sufficiently large socket buffer size).
>>>
>>> Do you happen to have an example of an application that does this that
>>> could be used for testing? :)
>>
>> Have a look at
>>
>> commit 6ce9e7b5fe3195d1ae6e3a0753d4ddcac5cd699e
>> Author: Eric Dumazet <eric.dumazet@gmail.com>
>> Date:   Wed Sep 2 18:05:33 2009 -0700
>>
>>      ip: Report qdisc packet drops
>>
>> You should be able to do a UDP flood while setting IP_RECVERR to
>> detect the packet drop due to a full queue which AFAICS will never
>> happen with the current mac80211 setup.
> 
> Yup, got that part. Was just wondering if you know of any applications
> that already do this, that I could test without having to write my
> own... :)

Hi Bob,

Is this something that could be easily implemented in iperf?

Regards,
Arend

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Toke Høiland-Jørgensen]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Tue, Apr 16, 2019 at 02:18:36PM +0100, Toke Høiland-Jørgensen wrote:
>>
>> > The congestion control happens at two levels. You are right that the
>> > socket buffer acts as one limit. However, other applications may also
>> > rely on the TX queue being full as the throttle (by setting a
>> > sufficiently large socket buffer size).
>> 
>> Do you happen to have an example of an application that does this that
>> could be used for testing? :)
>
> Have a look at
>
> commit 6ce9e7b5fe3195d1ae6e3a0753d4ddcac5cd699e
> Author: Eric Dumazet <eric.dumazet@gmail.com>
> Date:   Wed Sep 2 18:05:33 2009 -0700
>
>     ip: Report qdisc packet drops
>
> You should be able to do a UDP flood while setting IP_RECVERR to
> detect the packet drop due to a full queue which AFAICS will never
> happen with the current mac80211 setup.

Also, looking at udp.c, it seems it uses net_xmit_errno() - which means
that returning NET_XMIT_CN has the same effect as NET_XMIT_SUCCESS when
propagated back to userspace? Which would kinda defeat the point of
going to the trouble of propagating up the return code (the mac80211
queue will never drop the most recently enqueued packet)...

-Toke

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Johannes Berg]

On Wed, 2019-04-17 at 10:17 +0100, Toke Høiland-Jørgensen wrote:
> Herbert Xu <herbert@gondor.apana.org.au> writes:
> 
> > On Tue, Apr 16, 2019 at 02:18:36PM +0100, Toke Høiland-Jørgensen wrote:
> > > 
> > > > The congestion control happens at two levels. You are right that the
> > > > socket buffer acts as one limit. However, other applications may also
> > > > rely on the TX queue being full as the throttle (by setting a
> > > > sufficiently large socket buffer size).
> > > 
> > > Do you happen to have an example of an application that does this that
> > > could be used for testing? :)
> > 
> > Have a look at
> > 
> > commit 6ce9e7b5fe3195d1ae6e3a0753d4ddcac5cd699e
> > Author: Eric Dumazet <eric.dumazet@gmail.com>
> > Date:   Wed Sep 2 18:05:33 2009 -0700
> > 
> >     ip: Report qdisc packet drops
> > 
> > You should be able to do a UDP flood while setting IP_RECVERR to
> > detect the packet drop due to a full queue which AFAICS will never
> > happen with the current mac80211 setup.
> 
> Also, looking at udp.c, it seems it uses net_xmit_errno() - which means
> that returning NET_XMIT_CN has the same effect as NET_XMIT_SUCCESS when
> propagated back to userspace? Which would kinda defeat the point of
> going to the trouble of propagating up the return code (the mac80211
> queue will never drop the most recently enqueued packet)...

I guess there might be value in returning NET_XMIT_CN anyway, but I
think you're right in that we can never return anything but
NET_XMIT_SUCCESS or NET_XMIT_CN since we never drop this new packet,
just older ones.

Which, btw, is exactly the same with net/sched/sch_fq_codel.c, AFAICT?

johannes

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Tue, Apr 23, 2019 at 02:41:33PM +0200, Johannes Berg wrote:
>
> I guess there might be value in returning NET_XMIT_CN anyway, but I
> think you're right in that we can never return anything but
> NET_XMIT_SUCCESS or NET_XMIT_CN since we never drop this new packet,
> just older ones.
> 
> Which, btw, is exactly the same with net/sched/sch_fq_codel.c, AFAICT?

Pretty sure codel does return NET_XMIT_CN.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Johannes Berg]

On Thu, 2019-04-25 at 16:35 +0800, Herbert Xu wrote:
> On Tue, Apr 23, 2019 at 02:41:33PM +0200, Johannes Berg wrote:
> > 
> > I guess there might be value in returning NET_XMIT_CN anyway, but I
> > think you're right in that we can never return anything but
> > NET_XMIT_SUCCESS or NET_XMIT_CN since we never drop this new packet,
> > just older ones.
> > 
> > Which, btw, is exactly the same with net/sched/sch_fq_codel.c, AFAICT?
> 
> Pretty sure codel does return NET_XMIT_CN.

Yes, that's what I meant, it can only ever return NET_XMIT_SUCCESS or
NET_XMIT_CN. This will not trigger the code you mentioned before though.

johannes

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Herbert Xu]

On Thu, Apr 25, 2019 at 10:39:52AM +0200, Johannes Berg wrote:
>
> Yes, that's what I meant, it can only ever return NET_XMIT_SUCCESS or
> NET_XMIT_CN. This will not trigger the code you mentioned before though.

You are right that it does not.  However, the fact that this
congestion indication is lost is a bug rather than a feature.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues

[Johannes Berg]

On Thu, 2019-04-25 at 16:44 +0800, Herbert Xu wrote:
> On Thu, Apr 25, 2019 at 10:39:52AM +0200, Johannes Berg wrote:
> > 
> > Yes, that's what I meant, it can only ever return NET_XMIT_SUCCESS or
> > NET_XMIT_CN. This will not trigger the code you mentioned before though.
> 
> You are right that it does not.  However, the fact that this
> congestion indication is lost is a bug rather than a feature.

You can argue that way, I guess.

However, *any* queue management algorithm that doesn't *solely* employ
tail drops will necessarily behave this way.

I would argue that you get better queue management if you don't solely
rely on tail drops, so I'd rather pick better queue management than this
(relatively obscure) congestion notification. The more commonly relevant
socket buffer size will work for both.

johannes