LinuxLists.cc - [PATCH 1/5] mac80211: mesh: drop redundant rcu_read

[permalink] [raw]

Subject: [PATCH 3/5] mac80211: calculate hash for fq without holding fq->lock in itxq enqueue

Reduces lock contention on enqueue/dequeue of iTXQ packets

Signed-off-by: Felix Fietkau <[email protected]>
---
include/net/fq_impl.h | 18 ++++++++++--------
net/mac80211/tx.c | 15 ++++++++++-----
2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/include/net/fq_impl.h b/include/net/fq_impl.h
index be7c0fab3478..2caa86660ab0 100644
--- a/include/net/fq_impl.h
+++ b/include/net/fq_impl.h
@@ -107,21 +107,23 @@ static struct sk_buff *fq_tin_dequeue(struct fq *fq,
return skb;
}

+static u32 fq_flow_idx(struct fq *fq, struct sk_buff *skb)
+{
+ u32 hash = skb_get_hash_perturb(skb, fq->perturbation);
+
+ return reciprocal_scale(hash, fq->flows_cnt);
+}
+
static struct fq_flow *fq_flow_classify(struct fq *fq,
- struct fq_tin *tin,
+ struct fq_tin *tin, u32 idx,
struct sk_buff *skb,
fq_flow_get_default_t get_default_func)
{
struct fq_flow *flow;
- u32 hash;
- u32 idx;

lockdep_assert_held(&fq->lock);

- hash = skb_get_hash_perturb(skb, fq->perturbation);
- idx = reciprocal_scale(hash, fq->flows_cnt);
flow = &fq->flows[idx];
-
if (flow->tin && flow->tin != tin) {
flow = get_default_func(fq, tin, idx, skb);
tin->collisions++;
@@ -153,7 +155,7 @@ static void fq_recalc_backlog(struct fq *fq,
}

static void fq_tin_enqueue(struct fq *fq,
- struct fq_tin *tin,
+ struct fq_tin *tin, u32 idx,
struct sk_buff *skb,
fq_skb_free_t free_func,
fq_flow_get_default_t get_default_func)
@@ -163,7 +165,7 @@ static void fq_tin_enqueue(struct fq *fq,

lockdep_assert_held(&fq->lock);

- flow = fq_flow_classify(fq, tin, skb, get_default_func);
+ flow = fq_flow_classify(fq, tin, idx, skb, get_default_func);

flow->tin = tin;
flow->backlog += skb->len;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 0b73a0fe8218..8127e43e12b1 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1398,11 +1398,15 @@ static void ieee80211_txq_enqueue(struct ieee80211_local *local,
{
struct fq *fq = &local->fq;
struct fq_tin *tin = &txqi->tin;
+ u32 flow_idx = fq_flow_idx(fq, skb);

ieee80211_set_skb_enqueue_time(skb);
- fq_tin_enqueue(fq, tin, skb,
+
+ spin_lock_bh(&fq->lock);
+ fq_tin_enqueue(fq, tin, flow_idx, skb,
fq_skb_free_func,
fq_flow_get_default_func);
+ spin_unlock_bh(&fq->lock);
}

static bool fq_vlan_filter_func(struct fq *fq, struct fq_tin *tin,
@@ -1589,7 +1593,6 @@ static bool ieee80211_queue_skb(struct ieee80211_local *local,
struct sta_info *sta,
struct sk_buff *skb)
{
- struct fq *fq = &local->fq;
struct ieee80211_vif *vif;
struct txq_info *txqi;

@@ -1607,9 +1610,7 @@ static bool ieee80211_queue_skb(struct ieee80211_local *local,
if (!txqi)
return false;

- spin_lock_bh(&fq->lock);
ieee80211_txq_enqueue(local, txqi, skb);
- spin_unlock_bh(&fq->lock);

schedule_and_wake_txq(local, txqi);

@@ -3227,6 +3228,7 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
u8 max_subframes = sta->sta.max_amsdu_subframes;
int max_frags = local->hw.max_tx_fragments;
int max_amsdu_len = sta->sta.max_amsdu_len;
+ u32 flow_idx;
int orig_truesize;
__be16 len;
void *data;
@@ -3256,6 +3258,8 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
max_amsdu_len = min_t(int, max_amsdu_len,
sta->sta.max_tid_amsdu_len[tid]);

+ flow_idx = fq_flow_idx(fq, skb);
+
spin_lock_bh(&fq->lock);

/* TODO: Ideally aggregation should be done on dequeue to remain
@@ -3263,7 +3267,8 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
*/

tin = &txqi->tin;
- flow = fq_flow_classify(fq, tin, skb, fq_flow_get_default_func);
+ flow = fq_flow_classify(fq, tin, flow_idx, skb,
+ fq_flow_get_default_func);
head = skb_peek_tail(&flow->queue);
if (!head || skb_is_gso(head))
goto out;
--
2.17.0

2019-03-16 17:06:40

[permalink] [raw]

Subject: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock

Reduces lock contention on enqueue/dequeue of iTXQ packets

Signed-off-by: Felix Fietkau <[email protected]>
---
net/mac80211/tx.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 8127e43e12b1..f85344c9af62 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3544,6 +3544,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
ieee80211_tx_result r;
struct ieee80211_vif *vif = txq->vif;

+begin:
spin_lock_bh(&fq->lock);

if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) ||
@@ -3560,11 +3561,12 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
if (skb)
goto out;

-begin:
skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func);
if (!skb)
goto out;

+ spin_unlock_bh(&fq->lock);
+
hdr = (struct ieee80211_hdr *)skb->data;
info = IEEE80211_SKB_CB(skb);

@@ -3610,8 +3612,11 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,

skb = __skb_dequeue(&tx.skbs);

- if (!skb_queue_empty(&tx.skbs))
+ if (!skb_queue_empty(&tx.skbs)) {
+ spin_lock_bh(&fq->lock);
skb_queue_splice_tail(&tx.skbs, &txqi->frags);
+ spin_unlock_bh(&fq->lock);
+ }
}

if (skb_has_frag_list(skb) &&
@@ -3650,6 +3655,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
}

IEEE80211_SKB_CB(skb)->control.vif = vif;
+ return skb;

out:
spin_unlock_bh(&fq->lock);
--
2.17.0

2019-03-16 17:06:41

[permalink] [raw]

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

When using iTXQ, tx sequence number allocation and statistics are run at
dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
allows tx handlers to run on multiple CPUs in parallel.

Signed-off-by: Felix Fietkau <[email protected]>
---
net/mac80211/iface.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 8ab23bbfba3e..7a4ea97d15af 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1227,6 +1227,7 @@ static void ieee80211_if_setup(struct net_device *dev)
static void ieee80211_if_setup_no_queue(struct net_device *dev)
{
ieee80211_if_setup(dev);
+ dev->features |= NETIF_F_LLTX;
dev->priv_flags |= IFF_NO_QUEUE;
}

--
2.17.0

2019-03-16 17:06:42

[permalink] [raw]

Subject: [PATCH 2/5] mac80211: fix memory accounting with A-MSDU aggregation

skb->truesize can change due to memory reallocation or when adding extra
fragments. Adjust fq->memory_usage accordingly

Signed-off-by: Felix Fietkau <[email protected]>
---
net/mac80211/tx.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 51cc37802439..0b73a0fe8218 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3227,6 +3227,7 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
u8 max_subframes = sta->sta.max_amsdu_subframes;
int max_frags = local->hw.max_tx_fragments;
int max_amsdu_len = sta->sta.max_amsdu_len;
+ int orig_truesize;
__be16 len;
void *data;
bool ret = false;
@@ -3267,6 +3268,7 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
if (!head || skb_is_gso(head))
goto out;

+ orig_truesize = head->truesize;
orig_len = head->len;

if (skb->len + head->len > max_amsdu_len)
@@ -3324,6 +3326,7 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
*frag_tail = skb;

out_recalc:
+ fq->memory_usage += head->truesize - orig_truesize;
if (head->len != orig_len) {
flow->backlog += head->len - orig_len;
tin->backlog_bytes += head->len - orig_len;
--
2.17.0

2019-03-16 18:12:54

[permalink] [raw]

Subject: Re: [PATCH 2/5] mac80211: fix memory accounting with A-MSDU aggregation

Felix Fietkau <[email protected]> writes:

> skb->truesize can change due to memory reallocation or when adding extra
> fragments. Adjust fq->memory_usage accordingly

Nice catch.

Acked-by: Toke Høiland-Jørgensen <[email protected]>

2019-03-16 18:13:23

[permalink] [raw]

Subject: Re: [PATCH 3/5] mac80211: calculate hash for fq without holding fq->lock in itxq enqueue

Felix Fietkau <[email protected]> writes:

> Reduces lock contention on enqueue/dequeue of iTXQ packets
>
> Signed-off-by: Felix Fietkau <[email protected]>

Seems reasonable.

Acked-by: Toke Høiland-Jørgensen <[email protected]>

2019-03-16 18:13:46

[permalink] [raw]

Subject: Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock

Felix Fietkau <[email protected]> writes:

> Reduces lock contention on enqueue/dequeue of iTXQ packets
>
> Signed-off-by: Felix Fietkau <[email protected]>

Also reasonable.

Acked-by: Toke Høiland-Jørgensen <[email protected]>

2019-03-16 18:14:07

[permalink] [raw]

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

Felix Fietkau <[email protected]> writes:

> When using iTXQ, tx sequence number allocation and statistics are run at
> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
> allows tx handlers to run on multiple CPUs in parallel.

Cool, didn't know about that flag.

Acked-by: Toke Høiland-Jørgensen <[email protected]>

2019-04-14 09:44:23

[permalink] [raw]

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

+ Herbert

On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
> Felix Fietkau <[email protected]> writes:
>
>> When using iTXQ, tx sequence number allocation and statistics are run at
>> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
>> allows tx handlers to run on multiple CPUs in parallel.
>
> Cool, didn't know about that flag.

It is water under the bridge as this patch got applied already, but I
stumbled upon it just recently and didn't know about that flag either.
So I looked for more information about it and found the definition [1],
but the comment seemed important enough to send this reply.

NETIF_F_LLTX_BIT, /* LockLess TX - deprecated. Please */
/* do not use LLTX in new drivers */

Here is the commit that marked it deprecated:

commit e24eb521fbf2a350ce879dfc1d8e56d4ffa2aa22
Author: Christian Borntraeger <[email protected]>
Date: Tue Sep 25 19:42:02 2007 -0700

[NET]: note that NETIF_F_LLTX is deprecated

So I am not sure we should really do this in mac80211. Maybe Herbert can
comment although it has been over a decade ago.

Regards,
Arend

[1]
https://elixir.bootlin.com/linux/latest/source/include/linux/netdev_features.h#L32

2019-04-14 11:19:49

[permalink] [raw]

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

On 2019-04-14 11:44, Arend Van Spriel wrote:
> + Herbert
>
> On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
>> Felix Fietkau <[email protected]> writes:
>>
>>> When using iTXQ, tx sequence number allocation and statistics are run at
>>> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
>>> allows tx handlers to run on multiple CPUs in parallel.
>>
>> Cool, didn't know about that flag.
>
> It is water under the bridge as this patch got applied already, but I
> stumbled upon it just recently and didn't know about that flag either.
> So I looked for more information about it and found the definition [1],
> but the comment seemed important enough to send this reply.
>
> NETIF_F_LLTX_BIT, /* LockLess TX - deprecated. Please */
> /* do not use LLTX in new drivers */
>
> Here is the commit that marked it deprecated:
>
> commit e24eb521fbf2a350ce879dfc1d8e56d4ffa2aa22
> Author: Christian Borntraeger <[email protected]>
> Date: Tue Sep 25 19:42:02 2007 -0700
>
> [NET]: note that NETIF_F_LLTX is deprecated
>
> So I am not sure we should really do this in mac80211. Maybe Herbert can
> comment although it has been over a decade ago.
There is a lot of comparable code that also uses this flag, e.g.
batman-adv, bridge, vlan, various tunnel implementations. I think
mac80211 fits well with those kinds of use cases.
If I remember correctly, the deprecation was added to avoid quirky
custom locking schemes in ethernet drivers.

- Felix

2019-04-14 12:34:34

[permalink] [raw]

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

On April 14, 2019 1:19:49 PM Felix Fietkau <[email protected]> wrote:

> On 2019-04-14 11:44, Arend Van Spriel wrote:
>> + Herbert
>>
>> On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
>>> Felix Fietkau <[email protected]> writes:
>>>
>>>> When using iTXQ, tx sequence number allocation and statistics are run at
>>>> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
>>>> allows tx handlers to run on multiple CPUs in parallel.
>>>
>>> Cool, didn't know about that flag.
>>
>> It is water under the bridge as this patch got applied already, but I
>> stumbled upon it just recently and didn't know about that flag either.
>> So I looked for more information about it and found the definition [1],
>> but the comment seemed important enough to send this reply.
>>
>> NETIF_F_LLTX_BIT, /* LockLess TX - deprecated. Please */
>> /* do not use LLTX in new drivers */
>>
>> Here is the commit that marked it deprecated:
>>
>> commit e24eb521fbf2a350ce879dfc1d8e56d4ffa2aa22
>> Author: Christian Borntraeger <[email protected]>
>> Date: Tue Sep 25 19:42:02 2007 -0700
>>
>> [NET]: note that NETIF_F_LLTX is deprecated
>>
>> So I am not sure we should really do this in mac80211. Maybe Herbert can
>> comment although it has been over a decade ago.
> There is a lot of comparable code that also uses this flag, e.g.
> batman-adv, bridge, vlan, various tunnel implementations. I think
> mac80211 fits well with those kinds of use cases.

Ok. As said I was not sure so I can/will not argue.

> If I remember correctly, the deprecation was added to avoid quirky
> custom locking schemes in ethernet drivers.

What do you mean by "quirky custom locking schemes"? You mean that TX path
would use driver data that should actually be accessed under some lock?

When seeing the deprecated comment I wanted to know the why and was hoping
the commit message would divulge. It just mentions it is not needed. So now
I am curious as to why it wouldn't be needed especially as you say there
are (valid) use-cases in the kernel today.

Regards,
Arend

2019-04-16 07:34:06

[permalink] [raw]

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

On 4/14/2019 2:34 PM, Arend Van Spriel wrote:
> On April 14, 2019 1:19:49 PM Felix Fietkau <[email protected]> wrote:
>
>> On 2019-04-14 11:44, Arend Van Spriel wrote:
>>> + Herbert
>>>
>>> On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
>>>> Felix Fietkau <[email protected]> writes:
>>>>
>>>>> When using iTXQ, tx sequence number allocation and statistics are
>>>>> run at
>>>>> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX,
>>>>> which
>>>>> allows tx handlers to run on multiple CPUs in parallel.
>>>>
>>>> Cool, didn't know about that flag.
>>>
>>> It is water under the bridge as this patch got applied already, but I
>>> stumbled upon it just recently and didn't know about that flag either.
>>> So I looked for more information about it and found the definition [1],
>>> but the comment seemed important enough to send this reply.
>>>
>>>     NETIF_F_LLTX_BIT,    /* LockLess TX - deprecated. Please */
>>>                 /* do not use LLTX in new drivers */
>>>
>>> Here is the commit that marked it deprecated:
>>>
>>> commit e24eb521fbf2a350ce879dfc1d8e56d4ffa2aa22
>>> Author: Christian Borntraeger <[email protected]>
>>> Date:   Tue Sep 25 19:42:02 2007 -0700
>>>
>>>      [NET]: note that NETIF_F_LLTX is deprecated
>>>
>>> So I am not sure we should really do this in mac80211. Maybe Herbert can
>>> comment although it has been over a decade ago.
>> There is a lot of comparable code that also uses this flag, e.g.
>> batman-adv, bridge, vlan, various tunnel implementations. I think
>> mac80211 fits well with those kinds of use cases.
>
> Ok. As said I was not sure so I can/will not argue.
>
>> If I remember correctly, the deprecation was added to avoid quirky
>> custom locking schemes in ethernet drivers.
>
> What do you mean by "quirky custom locking schemes"? You mean that TX
> path would use driver data that should actually be accessed under some
> lock?
>
> When seeing the deprecated comment I wanted to know the why and was
> hoping the commit message would divulge. It just mentions it is not
> needed. So now I am curious as to why it wouldn't be needed especially
> as you say there are (valid) use-cases in the kernel today.

Getting back to this in an attempt to clarify my question. So from what
Felix is saying the NETIF_F_LLTX flag is not deprecated, but restricted.
What I would like to know is what exactly is required from a driver to
allow the use of this flag.

Regards,
Arend

2019-04-16 07:44:57

[permalink] [raw]

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

On Sun, Apr 14, 2019 at 11:44:17AM +0200, Arend Van Spriel wrote:
> + Herbert
>
> On 3/16/2019 7:14 PM, Toke H?iland-J?rgensen wrote:
> > Felix Fietkau <[email protected]> writes:
> >
> > > When using iTXQ, tx sequence number allocation and statistics are run at
> > > dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
> > > allows tx handlers to run on multiple CPUs in parallel.
> >
> > Cool, didn't know about that flag.
>
> It is water under the bridge as this patch got applied already, but I
> stumbled upon it just recently and didn't know about that flag either. So I
> looked for more information about it and found the definition [1], but the
> comment seemed important enough to send this reply.
>
> NETIF_F_LLTX_BIT, /* LockLess TX - deprecated. Please */
> /* do not use LLTX in new drivers */

The most obvious problem with LLTX is that it can cause AF_PACKET
to see packets twice. But the more subtle issue is that with a
proper design this lock should have no contention anyway.

So please explain why you want to use this in your driver and where
the contention is coming from?

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

2019-04-16 08:04:30

[permalink] [raw]

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

On 4/16/2019 9:44 AM, Herbert Xu wrote:
> On Sun, Apr 14, 2019 at 11:44:17AM +0200, Arend Van Spriel wrote:
>> + Herbert
>>
>> On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
>>> Felix Fietkau <[email protected]> writes:
>>>
>>>> When using iTXQ, tx sequence number allocation and statistics are run at
>>>> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
>>>> allows tx handlers to run on multiple CPUs in parallel.
>>>
>>> Cool, didn't know about that flag.
>>
>> It is water under the bridge as this patch got applied already, but I
>> stumbled upon it just recently and didn't know about that flag either. So I
>> looked for more information about it and found the definition [1], but the
>> comment seemed important enough to send this reply.
>>
>> NETIF_F_LLTX_BIT, /* LockLess TX - deprecated. Please */
>> /* do not use LLTX in new drivers */
>
> The most obvious problem with LLTX is that it can cause AF_PACKET
> to see packets twice. But the more subtle issue is that with a
> proper design this lock should have no contention anyway.
>
> So please explain why you want to use this in your driver and where
> the contention is coming from?

Hi Herbert,

I was just writing up an email clarifying my question. But let me
summarize this email thread. The patch from Felix adds this flag in
mac80211 for drivers that indicate to support pulling packets from the
internal TXQ in mac80211. I found it is deprecated, but as Felix
mentioned it is used in various parts of the network subsystem, ie.
batman-adv, bridge, vlan, tunnel implementations. So its use seems to be
restricted rather than deprecated. Given your response above I guess my
question would be to get details about what you call "proper design" as
I think you are saying with that it is not needed, right?

Regards,
Arend

2019-04-16 08:36:48

[permalink] [raw]

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

On Tue, Apr 16, 2019 at 10:04:24AM +0200, Arend Van Spriel wrote:
>
> I was just writing up an email clarifying my question. But let me summarize
> this email thread. The patch from Felix adds this flag in mac80211 for
> drivers that indicate to support pulling packets from the internal TXQ in
> mac80211. I found it is deprecated, but as Felix mentioned it is used in
> various parts of the network subsystem, ie. batman-adv, bridge, vlan, tunnel
> implementations. So its use seems to be restricted rather than deprecated.
> Given your response above I guess my question would be to get details about
> what you call "proper design" as I think you are saying with that it is not
> needed, right?

Essentially the only time it would be OK to use LLTX in its current
form is if you have no TX queue/congestion feedback which is clearly
not the case with wireless drivers.

Chers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-04-16 08:37:46

[permalink] [raw]

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

On Tue, 2019-04-16 at 16:36 +0800, Herbert Xu wrote:
> On Tue, Apr 16, 2019 at 10:04:24AM +0200, Arend Van Spriel wrote:
> >
> > I was just writing up an email clarifying my question. But let me summarize
> > this email thread. The patch from Felix adds this flag in mac80211 for
> > drivers that indicate to support pulling packets from the internal TXQ in
> > mac80211. I found it is deprecated, but as Felix mentioned it is used in
> > various parts of the network subsystem, ie. batman-adv, bridge, vlan, tunnel
> > implementations. So its use seems to be restricted rather than deprecated.
> > Given your response above I guess my question would be to get details about
> > what you call "proper design" as I think you are saying with that it is not
> > needed, right?
>
> Essentially the only time it would be OK to use LLTX in its current
> form is if you have no TX queue/congestion feedback which is clearly
> not the case with wireless drivers.

It is true because we have an entire buffering layer in mac80211 (in
this case at least) and never push back to the stack.

johannes

2019-04-16 09:17:58

[permalink] [raw]

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

On 4/16/2019 10:37 AM, Johannes Berg wrote:
> On Tue, 2019-04-16 at 16:36 +0800, Herbert Xu wrote:
>> On Tue, Apr 16, 2019 at 10:04:24AM +0200, Arend Van Spriel wrote:
>>>
>>> I was just writing up an email clarifying my question. But let me summarize
>>> this email thread. The patch from Felix adds this flag in mac80211 for
>>> drivers that indicate to support pulling packets from the internal TXQ in
>>> mac80211. I found it is deprecated, but as Felix mentioned it is used in
>>> various parts of the network subsystem, ie. batman-adv, bridge, vlan, tunnel
>>> implementations. So its use seems to be restricted rather than deprecated.
>>> Given your response above I guess my question would be to get details about
>>> what you call "proper design" as I think you are saying with that it is not
>>> needed, right?
>>
>> Essentially the only time it would be OK to use LLTX in its current
>> form is if you have no TX queue/congestion feedback which is clearly
>> not the case with wireless drivers.
>
> 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 ;-)

Regards,
Arend

2019-04-16 09:29:33

[permalink] [raw]

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

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-04-16 09:33:07

[permalink] [raw]

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

Johannes Berg <[email protected]> writes:

> On Tue, 2019-04-16 at 16:36 +0800, Herbert Xu wrote:
>> On Tue, Apr 16, 2019 at 10:04:24AM +0200, Arend Van Spriel wrote:
>> >
>> > I was just writing up an email clarifying my question. But let me summarize
>> > this email thread. The patch from Felix adds this flag in mac80211 for
>> > drivers that indicate to support pulling packets from the internal TXQ in
>> > mac80211. I found it is deprecated, but as Felix mentioned it is used in
>> > various parts of the network subsystem, ie. batman-adv, bridge, vlan, tunnel
>> > implementations. So its use seems to be restricted rather than deprecated.
>> > Given your response above I guess my question would be to get details about
>> > what you call "proper design" as I think you are saying with that it is not
>> > needed, right?
>>
>> Essentially the only time it would be OK to use LLTX in its current
>> form is if you have no TX queue/congestion feedback which is clearly
>> not the case with wireless drivers.
>
> 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? For instance, fq_codel returns
NET_XMIT_CN if it drops a packet from the same flow that it enqueued to.
We could conceivably do the same in mac80211, although we'd have to
carry the return value out through quite a few layers. Not sure if this
is worth it?

Eric, do you have any insight into what impact the _CN return has from
fq_codel?

-Toke

2019-04-16 09:33:57

[permalink] [raw]

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

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.

johannes

2019-04-16 09:37:20

[permalink] [raw]

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

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-04-16 09:38:27

[permalink] [raw]

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

Johannes Berg <[email protected]> writes:

> 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.

Wouldn't we get pushback all the way to the application socket? I.e.,
an UDP application would get sendto() failures?

-Toke

2019-04-16 09:40:05

[permalink] [raw]

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

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

2019-04-16 10:02:57

[permalink] [raw]

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

Johannes Berg <[email protected]> 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

2019-04-16 13:14:09

[permalink] [raw]

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

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-04-16 13:18:42

[permalink] [raw]

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

Herbert Xu <[email protected]> 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

2019-04-16 19:13:36

[permalink] [raw]

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

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

2019-04-17 02:11:17

[permalink] [raw]

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

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-04-17 02:13:52

[permalink] [raw]

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

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-04-17 03:38:48

[permalink] [raw]

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

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 <[email protected]>
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-04-17 08:36:08

[permalink] [raw]

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

Herbert Xu <[email protected]> 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

2019-04-17 09:09:23

[permalink] [raw]

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

Herbert Xu <[email protected]> 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 <[email protected]>
> 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

2019-04-17 09:16:32

[permalink] [raw]

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

+ Bob

On 4/17/2019 11:09 AM, Toke Høiland-Jørgensen wrote:
> Herbert Xu <[email protected]> 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 <[email protected]>
>> 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

2019-04-17 09:26:08

[permalink] [raw]

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

Herbert Xu <[email protected]> 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 <[email protected]>
> 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

2019-04-23 12:41:44

[permalink] [raw]

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

On Wed, 2019-04-17 at 10:17 +0100, Toke Høiland-Jørgensen wrote:
> Herbert Xu <[email protected]> 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 <[email protected]>
> > 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

2019-04-25 09:00:38

[permalink] [raw]

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

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

2019-04-25 09:04:55

[permalink] [raw]

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

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-04-25 09:05:09

[permalink] [raw]

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

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

2019-04-25 09:10:43