* [PATCH 1/5] mac80211: mesh: drop redundant rcu_read_lock/unlock calls @ 2019-03-16 17:06 Felix Fietkau 2019-03-16 17:06 ` [PATCH 2/5] mac80211: fix memory accounting with A-MSDU aggregation Felix Fietkau ` (3 more replies) 0 siblings, 4 replies; 46+ messages in thread From: Felix Fietkau @ 2019-03-16 17:06 UTC (permalink / raw) To: linux-wireless; +Cc: johannes The callers of these functions are all within RCU locked sections Signed-off-by: Felix Fietkau <nbd@nbd.name> --- net/mac80211/mesh_hwmp.c | 26 +++++++------------------- net/mac80211/mesh_pathtbl.c | 2 +- 2 files changed, 8 insertions(+), 20 deletions(-) diff --git a/net/mac80211/mesh_hwmp.c b/net/mac80211/mesh_hwmp.c index f7517668e77a..dcbca7c8c67d 100644 --- a/net/mac80211/mesh_hwmp.c +++ b/net/mac80211/mesh_hwmp.c @@ -1130,16 +1130,13 @@ int mesh_nexthop_resolve(struct ieee80211_sub_if_data *sdata, struct mesh_path *mpath; struct sk_buff *skb_to_free = NULL; u8 *target_addr = hdr->addr3; - int err = 0; /* Nulls are only sent to peers for PS and should be pre-addressed */ if (ieee80211_is_qos_nullfunc(hdr->frame_control)) return 0; - rcu_read_lock(); - err = mesh_nexthop_lookup(sdata, skb); - if (!err) - goto endlookup; + if (!mesh_nexthop_lookup(sdata, skb)) + return 0; /* no nexthop found, start resolving */ mpath = mesh_path_lookup(sdata, target_addr); @@ -1147,8 +1144,7 @@ int mesh_nexthop_resolve(struct ieee80211_sub_if_data *sdata, mpath = mesh_path_add(sdata, target_addr); if (IS_ERR(mpath)) { mesh_path_discard_frame(sdata, skb); - err = PTR_ERR(mpath); - goto endlookup; + return PTR_ERR(mpath); } } @@ -1161,13 +1157,10 @@ int mesh_nexthop_resolve(struct ieee80211_sub_if_data *sdata, info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING; ieee80211_set_qos_hdr(sdata, skb); skb_queue_tail(&mpath->frame_queue, skb); - err = -ENOENT; if (skb_to_free) mesh_path_discard_frame(sdata, skb_to_free); -endlookup: - rcu_read_unlock(); - return err; + return -ENOENT; } /** @@ -1187,13 +1180,10 @@ int mesh_nexthop_lookup(struct ieee80211_sub_if_data *sdata, struct sta_info *next_hop; struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data; u8 *target_addr = hdr->addr3; - int err = -ENOENT; - rcu_read_lock(); mpath = mesh_path_lookup(sdata, target_addr); - if (!mpath || !(mpath->flags & MESH_PATH_ACTIVE)) - goto endlookup; + return -ENOENT; if (time_after(jiffies, mpath->exp_time - @@ -1208,12 +1198,10 @@ int mesh_nexthop_lookup(struct ieee80211_sub_if_data *sdata, memcpy(hdr->addr1, next_hop->sta.addr, ETH_ALEN); memcpy(hdr->addr2, sdata->vif.addr, ETH_ALEN); ieee80211_mps_set_frame_flags(sdata, next_hop, hdr); - err = 0; + return 0; } -endlookup: - rcu_read_unlock(); - return err; + return -ENOENT; } void mesh_path_timer(struct timer_list *t) diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c index 0a1148328f19..498bf580bff4 100644 --- a/net/mac80211/mesh_pathtbl.c +++ b/net/mac80211/mesh_pathtbl.c @@ -219,7 +219,7 @@ static struct mesh_path *mpath_lookup(struct mesh_table *tbl, const u8 *dst, { struct mesh_path *mpath; - mpath = rhashtable_lookup_fast(&tbl->rhead, dst, mesh_rht_params); + mpath = rhashtable_lookup(&tbl->rhead, dst, mesh_rht_params); if (mpath && mpath_expired(mpath)) { spin_lock_bh(&mpath->state_lock); -- 2.17.0 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 2/5] mac80211: fix memory accounting with A-MSDU aggregation 2019-03-16 17:06 [PATCH 1/5] mac80211: mesh: drop redundant rcu_read_lock/unlock calls Felix Fietkau @ 2019-03-16 17:06 ` Felix Fietkau 2019-03-16 18:12 ` Toke Høiland-Jørgensen 2019-03-16 17:06 ` [PATCH 3/5] mac80211: calculate hash for fq without holding fq->lock in itxq enqueue Felix Fietkau ` (2 subsequent siblings) 3 siblings, 1 reply; 46+ messages in thread From: Felix Fietkau @ 2019-03-16 17:06 UTC (permalink / raw) To: linux-wireless; +Cc: johannes skb->truesize can change due to memory reallocation or when adding extra fragments. Adjust fq->memory_usage accordingly Signed-off-by: Felix Fietkau <nbd@nbd.name> --- 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 2/5] mac80211: fix memory accounting with A-MSDU aggregation 2019-03-16 17:06 ` [PATCH 2/5] mac80211: fix memory accounting with A-MSDU aggregation Felix Fietkau @ 2019-03-16 18:12 ` Toke Høiland-Jørgensen 0 siblings, 0 replies; 46+ messages in thread From: Toke Høiland-Jørgensen @ 2019-03-16 18:12 UTC (permalink / raw) To: Felix Fietkau, linux-wireless; +Cc: johannes Felix Fietkau <nbd@nbd.name> 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 <toke@redhat.com> ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 3/5] mac80211: calculate hash for fq without holding fq->lock in itxq enqueue 2019-03-16 17:06 [PATCH 1/5] mac80211: mesh: drop redundant rcu_read_lock/unlock calls Felix Fietkau 2019-03-16 17:06 ` [PATCH 2/5] mac80211: fix memory accounting with A-MSDU aggregation Felix Fietkau @ 2019-03-16 17:06 ` Felix Fietkau 2019-03-16 18:13 ` Toke Høiland-Jørgensen 2019-03-16 17:06 ` [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock Felix Fietkau 2019-03-16 17:06 ` [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues Felix Fietkau 3 siblings, 1 reply; 46+ messages in thread From: Felix Fietkau @ 2019-03-16 17:06 UTC (permalink / raw) To: linux-wireless; +Cc: johannes Reduces lock contention on enqueue/dequeue of iTXQ packets Signed-off-by: Felix Fietkau <nbd@nbd.name> --- 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 3/5] mac80211: calculate hash for fq without holding fq->lock in itxq enqueue 2019-03-16 17:06 ` [PATCH 3/5] mac80211: calculate hash for fq without holding fq->lock in itxq enqueue Felix Fietkau @ 2019-03-16 18:13 ` Toke Høiland-Jørgensen 0 siblings, 0 replies; 46+ messages in thread From: Toke Høiland-Jørgensen @ 2019-03-16 18:13 UTC (permalink / raw) To: Felix Fietkau, linux-wireless; +Cc: johannes Felix Fietkau <nbd@nbd.name> writes: > Reduces lock contention on enqueue/dequeue of iTXQ packets > > Signed-off-by: Felix Fietkau <nbd@nbd.name> Seems reasonable. Acked-by: Toke Høiland-Jørgensen <toke@redhat.com> ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock 2019-03-16 17:06 [PATCH 1/5] mac80211: mesh: drop redundant rcu_read_lock/unlock calls Felix Fietkau 2019-03-16 17:06 ` [PATCH 2/5] mac80211: fix memory accounting with A-MSDU aggregation Felix Fietkau 2019-03-16 17:06 ` [PATCH 3/5] mac80211: calculate hash for fq without holding fq->lock in itxq enqueue Felix Fietkau @ 2019-03-16 17:06 ` Felix Fietkau 2019-03-16 18:13 ` Toke Høiland-Jørgensen 2022-12-05 9:46 ` Wen Gong 2019-03-16 17:06 ` [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues Felix Fietkau 3 siblings, 2 replies; 46+ messages in thread From: Felix Fietkau @ 2019-03-16 17:06 UTC (permalink / raw) To: linux-wireless; +Cc: johannes Reduces lock contention on enqueue/dequeue of iTXQ packets Signed-off-by: Felix Fietkau <nbd@nbd.name> --- 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock 2019-03-16 17:06 ` [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock Felix Fietkau @ 2019-03-16 18:13 ` Toke Høiland-Jørgensen 2022-12-05 9:46 ` Wen Gong 1 sibling, 0 replies; 46+ messages in thread From: Toke Høiland-Jørgensen @ 2019-03-16 18:13 UTC (permalink / raw) To: Felix Fietkau, linux-wireless; +Cc: johannes Felix Fietkau <nbd@nbd.name> writes: > Reduces lock contention on enqueue/dequeue of iTXQ packets > > Signed-off-by: Felix Fietkau <nbd@nbd.name> Also reasonable. Acked-by: Toke Høiland-Jørgensen <toke@redhat.com> ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock 2019-03-16 17:06 ` [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock Felix Fietkau @ 2022-12-05 9:46 ` Wen Gong 2022-12-05 9:46 ` Wen Gong 1 sibling, 0 replies; 46+ messages in thread From: Wen Gong @ 2022-12-05 9:46 UTC (permalink / raw) To: Felix Fietkau, linux-wireless; +Cc: johannes, ath11k On 3/17/2019 1:06 AM, Felix Fietkau wrote: > Reduces lock contention on enqueue/dequeue of iTXQ packets > > Signed-off-by: Felix Fietkau <nbd@nbd.name> > --- > 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); Maybe use-after-free will happened? You can see ieee80211_tx_dequeue() in tx.c as below, after ieee80211_free_txskb(), it will goto begin, If goto out happened in below check, then the skb which is freed will be returned, and use-after-free will happen. https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/net/mac80211/tx.c?id=ded4698b58cb23c22b0dcbd829ced19ce4e6ce02#n3538 begin: spin_lock_bh(&fq->lock); if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) || test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags)) goto out; if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) { set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags); goto out; } /* Make sure fragments stay together. */ skb = __skb_dequeue(&txqi->frags); if (skb) goto out; skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func); if (!skb) goto out; spin_unlock_bh(&fq->lock); Maybe "skb = NULL;" should be added after "begin:". ... ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock @ 2022-12-05 9:46 ` Wen Gong 0 siblings, 0 replies; 46+ messages in thread From: Wen Gong @ 2022-12-05 9:46 UTC (permalink / raw) To: Felix Fietkau, linux-wireless; +Cc: johannes, ath11k On 3/17/2019 1:06 AM, Felix Fietkau wrote: > Reduces lock contention on enqueue/dequeue of iTXQ packets > > Signed-off-by: Felix Fietkau <nbd@nbd.name> > --- > 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); Maybe use-after-free will happened? You can see ieee80211_tx_dequeue() in tx.c as below, after ieee80211_free_txskb(), it will goto begin, If goto out happened in below check, then the skb which is freed will be returned, and use-after-free will happen. https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/net/mac80211/tx.c?id=ded4698b58cb23c22b0dcbd829ced19ce4e6ce02#n3538 begin: spin_lock_bh(&fq->lock); if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) || test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags)) goto out; if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) { set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags); goto out; } /* Make sure fragments stay together. */ skb = __skb_dequeue(&txqi->frags); if (skb) goto out; skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func); if (!skb) goto out; spin_unlock_bh(&fq->lock); Maybe "skb = NULL;" should be added after "begin:". ... -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock 2022-12-05 9:46 ` Wen Gong @ 2022-12-07 6:30 ` Wen Gong -1 siblings, 0 replies; 46+ messages in thread From: Wen Gong @ 2022-12-07 6:30 UTC (permalink / raw) To: Felix Fietkau, linux-wireless; +Cc: johannes, ath11k, johannes.berg Hi Johannes, do you know it? On 12/5/2022 5:46 PM, Wen Gong wrote: > On 3/17/2019 1:06 AM, Felix Fietkau wrote: >> Reduces lock contention on enqueue/dequeue of iTXQ packets >> >> Signed-off-by: Felix Fietkau <nbd@nbd.name> >> --- >> 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); > Maybe use-after-free will happened? > > You can see ieee80211_tx_dequeue() in tx.c as below, after > ieee80211_free_txskb(), it will goto begin, > If goto out happened in below check, then the skb which is freed will > be returned, and use-after-free will happen. > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/net/mac80211/tx.c?id=ded4698b58cb23c22b0dcbd829ced19ce4e6ce02#n3538 > > begin: > spin_lock_bh(&fq->lock); > > if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) || > test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags)) > goto out; > > if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) { > set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags); > goto out; > } > > /* Make sure fragments stay together. */ > skb = __skb_dequeue(&txqi->frags); > if (skb) > goto out; > > skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func); > if (!skb) > goto out; > > spin_unlock_bh(&fq->lock); > > Maybe "skb = NULL;" should be added after "begin:". > > ... > -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock @ 2022-12-07 6:30 ` Wen Gong 0 siblings, 0 replies; 46+ messages in thread From: Wen Gong @ 2022-12-07 6:30 UTC (permalink / raw) To: Felix Fietkau, linux-wireless; +Cc: johannes, ath11k, johannes.berg Hi Johannes, do you know it? On 12/5/2022 5:46 PM, Wen Gong wrote: > On 3/17/2019 1:06 AM, Felix Fietkau wrote: >> Reduces lock contention on enqueue/dequeue of iTXQ packets >> >> Signed-off-by: Felix Fietkau <nbd@nbd.name> >> --- >> 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); > Maybe use-after-free will happened? > > You can see ieee80211_tx_dequeue() in tx.c as below, after > ieee80211_free_txskb(), it will goto begin, > If goto out happened in below check, then the skb which is freed will > be returned, and use-after-free will happen. > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/net/mac80211/tx.c?id=ded4698b58cb23c22b0dcbd829ced19ce4e6ce02#n3538 > > begin: > spin_lock_bh(&fq->lock); > > if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) || > test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags)) > goto out; > > if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) { > set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags); > goto out; > } > > /* Make sure fragments stay together. */ > skb = __skb_dequeue(&txqi->frags); > if (skb) > goto out; > > skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func); > if (!skb) > goto out; > > spin_unlock_bh(&fq->lock); > > Maybe "skb = NULL;" should be added after "begin:". > > ... > ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock 2022-12-07 6:30 ` Wen Gong @ 2022-12-12 8:31 ` Wen Gong -1 siblings, 0 replies; 46+ messages in thread From: Wen Gong @ 2022-12-12 8:31 UTC (permalink / raw) To: Felix Fietkau, linux-wireless; +Cc: johannes, ath11k, johannes.berg I will send a patch for it to avoid the potential user-after-free risk. On 12/7/2022 2:30 PM, Wen Gong wrote: > Hi Johannes, > > do you know it? > > On 12/5/2022 5:46 PM, Wen Gong wrote: >> On 3/17/2019 1:06 AM, Felix Fietkau wrote: >>> Reduces lock contention on enqueue/dequeue of iTXQ packets >>> >>> Signed-off-by: Felix Fietkau <nbd@nbd.name> >>> --- >>> 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); >> Maybe use-after-free will happened? >> >> You can see ieee80211_tx_dequeue() in tx.c as below, after >> ieee80211_free_txskb(), it will goto begin, >> If goto out happened in below check, then the skb which is freed will >> be returned, and use-after-free will happen. >> >> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/net/mac80211/tx.c?id=ded4698b58cb23c22b0dcbd829ced19ce4e6ce02#n3538 >> >> begin: >> spin_lock_bh(&fq->lock); >> >> if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) || >> test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags)) >> goto out; >> >> if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) { >> set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags); >> goto out; >> } >> >> /* Make sure fragments stay together. */ >> skb = __skb_dequeue(&txqi->frags); >> if (skb) >> goto out; >> >> skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func); >> if (!skb) >> goto out; >> >> spin_unlock_bh(&fq->lock); >> >> Maybe "skb = NULL;" should be added after "begin:". >> >> ... >> ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock @ 2022-12-12 8:31 ` Wen Gong 0 siblings, 0 replies; 46+ messages in thread From: Wen Gong @ 2022-12-12 8:31 UTC (permalink / raw) To: Felix Fietkau, linux-wireless; +Cc: johannes, ath11k, johannes.berg I will send a patch for it to avoid the potential user-after-free risk. On 12/7/2022 2:30 PM, Wen Gong wrote: > Hi Johannes, > > do you know it? > > On 12/5/2022 5:46 PM, Wen Gong wrote: >> On 3/17/2019 1:06 AM, Felix Fietkau wrote: >>> Reduces lock contention on enqueue/dequeue of iTXQ packets >>> >>> Signed-off-by: Felix Fietkau <nbd@nbd.name> >>> --- >>> 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); >> Maybe use-after-free will happened? >> >> You can see ieee80211_tx_dequeue() in tx.c as below, after >> ieee80211_free_txskb(), it will goto begin, >> If goto out happened in below check, then the skb which is freed will >> be returned, and use-after-free will happen. >> >> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/net/mac80211/tx.c?id=ded4698b58cb23c22b0dcbd829ced19ce4e6ce02#n3538 >> >> begin: >> spin_lock_bh(&fq->lock); >> >> if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) || >> test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags)) >> goto out; >> >> if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) { >> set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags); >> goto out; >> } >> >> /* Make sure fragments stay together. */ >> skb = __skb_dequeue(&txqi->frags); >> if (skb) >> goto out; >> >> skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func); >> if (!skb) >> goto out; >> >> spin_unlock_bh(&fq->lock); >> >> Maybe "skb = NULL;" should be added after "begin:". >> >> ... >> -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-03-16 17:06 [PATCH 1/5] mac80211: mesh: drop redundant rcu_read_lock/unlock calls Felix Fietkau ` (2 preceding siblings ...) 2019-03-16 17:06 ` [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock Felix Fietkau @ 2019-03-16 17:06 ` Felix Fietkau 2019-03-16 18:14 ` Toke Høiland-Jørgensen 3 siblings, 1 reply; 46+ messages in thread From: Felix Fietkau @ 2019-03-16 17:06 UTC (permalink / raw) To: linux-wireless; +Cc: johannes 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 <nbd@nbd.name> --- 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 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-03-16 17:06 ` [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues Felix Fietkau @ 2019-03-16 18:14 ` Toke Høiland-Jørgensen 2019-04-14 9:44 ` Arend Van Spriel 0 siblings, 1 reply; 46+ messages in thread From: Toke Høiland-Jørgensen @ 2019-03-16 18:14 UTC (permalink / raw) To: Felix Fietkau, linux-wireless; +Cc: johannes Felix Fietkau <nbd@nbd.name> 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 <toke@redhat.com> ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-03-16 18:14 ` Toke Høiland-Jørgensen @ 2019-04-14 9:44 ` Arend Van Spriel 2019-04-14 11:19 ` Felix Fietkau 2019-04-16 7:44 ` Herbert Xu 0 siblings, 2 replies; 46+ messages in thread From: Arend Van Spriel @ 2019-04-14 9:44 UTC (permalink / raw) To: Toke Høiland-Jørgensen, Felix Fietkau, linux-wireless Cc: johannes, Herbert Xu + Herbert On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote: > Felix Fietkau <nbd@nbd.name> 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 <borntraeger@de.ibm.com> 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-14 9:44 ` Arend Van Spriel @ 2019-04-14 11:19 ` Felix Fietkau 2019-04-14 12:34 ` Arend Van Spriel 2019-04-16 7:44 ` Herbert Xu 1 sibling, 1 reply; 46+ messages in thread From: Felix Fietkau @ 2019-04-14 11:19 UTC (permalink / raw) To: Arend Van Spriel, Toke Høiland-Jørgensen, linux-wireless Cc: johannes, Herbert Xu 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 <nbd@nbd.name> 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 <borntraeger@de.ibm.com> > 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-14 11:19 ` Felix Fietkau @ 2019-04-14 12:34 ` Arend Van Spriel 2019-04-16 7:34 ` Arend Van Spriel 0 siblings, 1 reply; 46+ messages in thread From: Arend Van Spriel @ 2019-04-14 12:34 UTC (permalink / raw) To: Felix Fietkau, Toke Høiland-Jørgensen, linux-wireless Cc: johannes, Herbert Xu On April 14, 2019 1:19:49 PM Felix Fietkau <nbd@nbd.name> 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 <nbd@nbd.name> 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 <borntraeger@de.ibm.com> >> 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-14 12:34 ` Arend Van Spriel @ 2019-04-16 7:34 ` Arend Van Spriel 0 siblings, 0 replies; 46+ messages in thread From: Arend Van Spriel @ 2019-04-16 7:34 UTC (permalink / raw) To: linux-wireless, johannes, Herbert Xu Cc: Felix Fietkau, Toke Høiland-Jørgensen On 4/14/2019 2:34 PM, Arend Van Spriel wrote: > On April 14, 2019 1:19:49 PM Felix Fietkau <nbd@nbd.name> 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 <nbd@nbd.name> 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 <borntraeger@de.ibm.com> >>> 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-14 9:44 ` Arend Van Spriel 2019-04-14 11:19 ` Felix Fietkau @ 2019-04-16 7:44 ` Herbert Xu 2019-04-16 8:04 ` Arend Van Spriel 1 sibling, 1 reply; 46+ messages in thread From: Herbert Xu @ 2019-04-16 7:44 UTC (permalink / raw) To: Arend Van Spriel Cc: Toke Høiland-Jørgensen, Felix Fietkau, linux-wireless, johannes 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 <nbd@nbd.name> 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 <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 7:44 ` Herbert Xu @ 2019-04-16 8:04 ` Arend Van Spriel 2019-04-16 8:36 ` Herbert Xu 0 siblings, 1 reply; 46+ messages in thread From: Arend Van Spriel @ 2019-04-16 8:04 UTC (permalink / raw) To: Herbert Xu Cc: Toke Høiland-Jørgensen, Felix Fietkau, linux-wireless, johannes 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 <nbd@nbd.name> 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 8:04 ` Arend Van Spriel @ 2019-04-16 8:36 ` Herbert Xu 2019-04-16 8:37 ` Johannes Berg 0 siblings, 1 reply; 46+ messages in thread From: Herbert Xu @ 2019-04-16 8:36 UTC (permalink / raw) To: Arend Van Spriel Cc: Toke Høiland-Jørgensen, Felix Fietkau, linux-wireless, johannes 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 <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 8:36 ` Herbert Xu @ 2019-04-16 8:37 ` Johannes Berg 2019-04-16 9:17 ` Arend Van Spriel 2019-04-16 9:33 ` Toke Høiland-Jørgensen 0 siblings, 2 replies; 46+ messages in thread From: Johannes Berg @ 2019-04-16 8:37 UTC (permalink / raw) To: Herbert Xu, Arend Van Spriel Cc: Toke Høiland-Jørgensen, Felix Fietkau, linux-wireless 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 8:37 ` Johannes Berg @ 2019-04-16 9:17 ` Arend Van Spriel 2019-04-16 9:29 ` Herbert Xu 2019-04-16 9:33 ` Toke Høiland-Jørgensen 1 sibling, 1 reply; 46+ messages in thread From: Arend Van Spriel @ 2019-04-16 9:17 UTC (permalink / raw) To: Johannes Berg, Herbert Xu Cc: Toke Høiland-Jørgensen, Felix Fietkau, linux-wireless 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 9:17 ` Arend Van Spriel @ 2019-04-16 9:29 ` Herbert Xu 0 siblings, 0 replies; 46+ messages in thread From: Herbert Xu @ 2019-04-16 9:29 UTC (permalink / raw) To: Arend Van Spriel Cc: Johannes Berg, Toke Høiland-Jørgensen, Felix Fietkau, linux-wireless, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 8:37 ` Johannes Berg 2019-04-16 9:17 ` Arend Van Spriel @ 2019-04-16 9:33 ` Toke Høiland-Jørgensen 2019-04-16 9:33 ` Johannes Berg 1 sibling, 1 reply; 46+ messages in thread From: Toke Høiland-Jørgensen @ 2019-04-16 9:33 UTC (permalink / raw) To: Johannes Berg, Herbert Xu, Arend Van Spriel Cc: Felix Fietkau, linux-wireless, Eric Dumazet Johannes Berg <johannes@sipsolutions.net> 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 9:33 ` Toke Høiland-Jørgensen @ 2019-04-16 9:33 ` Johannes Berg 2019-04-16 9:37 ` Herbert Xu 2019-04-16 9:38 ` Toke Høiland-Jørgensen 0 siblings, 2 replies; 46+ messages in thread From: Johannes Berg @ 2019-04-16 9:33 UTC (permalink / raw) To: Toke Høiland-Jørgensen, Herbert Xu, Arend Van Spriel Cc: Felix Fietkau, linux-wireless, Eric Dumazet 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 9:33 ` Johannes Berg @ 2019-04-16 9:37 ` Herbert Xu 2019-04-16 9:39 ` Johannes Berg 2019-04-16 9:38 ` Toke Høiland-Jørgensen 1 sibling, 1 reply; 46+ messages in thread From: Herbert Xu @ 2019-04-16 9:37 UTC (permalink / raw) To: Johannes Berg Cc: Toke Høiland-Jørgensen, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 9:37 ` Herbert Xu @ 2019-04-16 9:39 ` Johannes Berg 2019-04-16 10:02 ` Toke Høiland-Jørgensen 2019-04-16 13:13 ` Herbert Xu 0 siblings, 2 replies; 46+ messages in thread From: Johannes Berg @ 2019-04-16 9:39 UTC (permalink / raw) To: Herbert Xu Cc: Toke Høiland-Jørgensen, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 9:39 ` Johannes Berg @ 2019-04-16 10:02 ` Toke Høiland-Jørgensen 2019-04-17 2:11 ` Herbert Xu 2019-04-16 13:13 ` Herbert Xu 1 sibling, 1 reply; 46+ messages in thread From: Toke Høiland-Jørgensen @ 2019-04-16 10:02 UTC (permalink / raw) To: Johannes Berg, Herbert Xu Cc: Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 10:02 ` Toke Høiland-Jørgensen @ 2019-04-17 2:11 ` Herbert Xu 2019-04-17 8:28 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 46+ messages in thread From: Herbert Xu @ 2019-04-17 2:11 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Johannes Berg, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-17 2:11 ` Herbert Xu @ 2019-04-17 8:28 ` Toke Høiland-Jørgensen 0 siblings, 0 replies; 46+ messages in thread From: Toke Høiland-Jørgensen @ 2019-04-17 8:28 UTC (permalink / raw) To: Herbert Xu Cc: Johannes Berg, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 9:39 ` Johannes Berg 2019-04-16 10:02 ` Toke Høiland-Jørgensen @ 2019-04-16 13:13 ` Herbert Xu 2019-04-16 13:18 ` Toke Høiland-Jørgensen 2019-04-16 19:13 ` Johannes Berg 1 sibling, 2 replies; 46+ messages in thread From: Herbert Xu @ 2019-04-16 13:13 UTC (permalink / raw) To: Johannes Berg Cc: Toke Høiland-Jørgensen, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 13:13 ` Herbert Xu @ 2019-04-16 13:18 ` Toke Høiland-Jørgensen 2019-04-17 3:38 ` Herbert Xu 2019-04-16 19:13 ` Johannes Berg 1 sibling, 1 reply; 46+ messages in thread From: Toke Høiland-Jørgensen @ 2019-04-16 13:18 UTC (permalink / raw) To: Herbert Xu, Johannes Berg Cc: Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 13:18 ` Toke Høiland-Jørgensen @ 2019-04-17 3:38 ` Herbert Xu 2019-04-17 9:09 ` Toke Høiland-Jørgensen 2019-04-17 9:17 ` Toke Høiland-Jørgensen 0 siblings, 2 replies; 46+ messages in thread From: Herbert Xu @ 2019-04-17 3:38 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Johannes Berg, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-17 3:38 ` Herbert Xu @ 2019-04-17 9:09 ` Toke Høiland-Jørgensen 2019-04-17 9:16 ` Arend Van Spriel 2019-04-17 9:17 ` Toke Høiland-Jørgensen 1 sibling, 1 reply; 46+ messages in thread From: Toke Høiland-Jørgensen @ 2019-04-17 9:09 UTC (permalink / raw) To: Herbert Xu Cc: Johannes Berg, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-17 9:09 ` Toke Høiland-Jørgensen @ 2019-04-17 9:16 ` Arend Van Spriel 0 siblings, 0 replies; 46+ messages in thread From: Arend Van Spriel @ 2019-04-17 9:16 UTC (permalink / raw) To: Toke Høiland-Jørgensen, Herbert Xu Cc: Johannes Berg, Felix Fietkau, linux-wireless, Eric Dumazet, netdev, Bob McMahon + 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-17 3:38 ` Herbert Xu 2019-04-17 9:09 ` Toke Høiland-Jørgensen @ 2019-04-17 9:17 ` Toke Høiland-Jørgensen 2019-04-23 12:41 ` Johannes Berg 1 sibling, 1 reply; 46+ messages in thread From: Toke Høiland-Jørgensen @ 2019-04-17 9:17 UTC (permalink / raw) To: Herbert Xu Cc: Johannes Berg, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-17 9:17 ` Toke Høiland-Jørgensen @ 2019-04-23 12:41 ` Johannes Berg 2019-04-25 8:35 ` Herbert Xu 0 siblings, 1 reply; 46+ messages in thread From: Johannes Berg @ 2019-04-23 12:41 UTC (permalink / raw) To: Toke Høiland-Jørgensen, Herbert Xu Cc: Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-23 12:41 ` Johannes Berg @ 2019-04-25 8:35 ` Herbert Xu 2019-04-25 8:39 ` Johannes Berg 0 siblings, 1 reply; 46+ messages in thread From: Herbert Xu @ 2019-04-25 8:35 UTC (permalink / raw) To: Johannes Berg Cc: Toke Høiland-Jørgensen, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-25 8:35 ` Herbert Xu @ 2019-04-25 8:39 ` Johannes Berg 2019-04-25 8:44 ` Herbert Xu 0 siblings, 1 reply; 46+ messages in thread From: Johannes Berg @ 2019-04-25 8:39 UTC (permalink / raw) To: Herbert Xu Cc: Toke Høiland-Jørgensen, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-25 8:39 ` Johannes Berg @ 2019-04-25 8:44 ` Herbert Xu 2019-04-25 8:49 ` Johannes Berg 0 siblings, 1 reply; 46+ messages in thread From: Herbert Xu @ 2019-04-25 8:44 UTC (permalink / raw) To: Johannes Berg Cc: Toke Høiland-Jørgensen, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-25 8:44 ` Herbert Xu @ 2019-04-25 8:49 ` Johannes Berg 0 siblings, 0 replies; 46+ messages in thread From: Johannes Berg @ 2019-04-25 8:49 UTC (permalink / raw) To: Herbert Xu Cc: Toke Høiland-Jørgensen, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 13:13 ` Herbert Xu 2019-04-16 13:18 ` Toke Høiland-Jørgensen @ 2019-04-16 19:13 ` Johannes Berg 2019-04-17 2:13 ` Herbert Xu 1 sibling, 1 reply; 46+ messages in thread From: Johannes Berg @ 2019-04-16 19:13 UTC (permalink / raw) To: Herbert Xu Cc: Toke Høiland-Jørgensen, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 19:13 ` Johannes Berg @ 2019-04-17 2:13 ` Herbert Xu 0 siblings, 0 replies; 46+ messages in thread From: Herbert Xu @ 2019-04-17 2:13 UTC (permalink / raw) To: Johannes Berg Cc: Toke Høiland-Jørgensen, Arend Van Spriel, Felix Fietkau, linux-wireless, Eric Dumazet, netdev 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues 2019-04-16 9:33 ` Johannes Berg 2019-04-16 9:37 ` Herbert Xu @ 2019-04-16 9:38 ` Toke Høiland-Jørgensen 1 sibling, 0 replies; 46+ messages in thread From: Toke Høiland-Jørgensen @ 2019-04-16 9:38 UTC (permalink / raw) To: Johannes Berg, Herbert Xu, Arend Van Spriel Cc: Felix Fietkau, linux-wireless, Eric Dumazet Johannes Berg <johannes@sipsolutions.net> 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
end of thread, other threads:[~2022-12-12 8:31 UTC | newest] Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-16 17:06 [PATCH 1/5] mac80211: mesh: drop redundant rcu_read_lock/unlock calls Felix Fietkau 2019-03-16 17:06 ` [PATCH 2/5] mac80211: fix memory accounting with A-MSDU aggregation Felix Fietkau 2019-03-16 18:12 ` Toke Høiland-Jørgensen 2019-03-16 17:06 ` [PATCH 3/5] mac80211: calculate hash for fq without holding fq->lock in itxq enqueue Felix Fietkau 2019-03-16 18:13 ` Toke Høiland-Jørgensen 2019-03-16 17:06 ` [PATCH 4/5] mac80211: run late dequeue late tx handlers without holding fq->lock Felix Fietkau 2019-03-16 18:13 ` Toke Høiland-Jørgensen 2022-12-05 9:46 ` Wen Gong 2022-12-05 9:46 ` Wen Gong 2022-12-07 6:30 ` Wen Gong 2022-12-07 6:30 ` Wen Gong 2022-12-12 8:31 ` Wen Gong 2022-12-12 8:31 ` Wen Gong 2019-03-16 17:06 ` [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues Felix Fietkau 2019-03-16 18:14 ` Toke Høiland-Jørgensen 2019-04-14 9:44 ` Arend Van Spriel 2019-04-14 11:19 ` Felix Fietkau 2019-04-14 12:34 ` Arend Van Spriel 2019-04-16 7:34 ` Arend Van Spriel 2019-04-16 7:44 ` Herbert Xu 2019-04-16 8:04 ` Arend Van Spriel 2019-04-16 8:36 ` Herbert Xu 2019-04-16 8:37 ` Johannes Berg 2019-04-16 9:17 ` Arend Van Spriel 2019-04-16 9:29 ` Herbert Xu 2019-04-16 9:33 ` Toke Høiland-Jørgensen 2019-04-16 9:33 ` Johannes Berg 2019-04-16 9:37 ` Herbert Xu 2019-04-16 9:39 ` Johannes Berg 2019-04-16 10:02 ` Toke Høiland-Jørgensen 2019-04-17 2:11 ` Herbert Xu 2019-04-17 8:28 ` Toke Høiland-Jørgensen 2019-04-16 13:13 ` Herbert Xu 2019-04-16 13:18 ` Toke Høiland-Jørgensen 2019-04-17 3:38 ` Herbert Xu 2019-04-17 9:09 ` Toke Høiland-Jørgensen 2019-04-17 9:16 ` Arend Van Spriel 2019-04-17 9:17 ` Toke Høiland-Jørgensen 2019-04-23 12:41 ` Johannes Berg 2019-04-25 8:35 ` Herbert Xu 2019-04-25 8:39 ` Johannes Berg 2019-04-25 8:44 ` Herbert Xu 2019-04-25 8:49 ` Johannes Berg 2019-04-16 19:13 ` Johannes Berg 2019-04-17 2:13 ` Herbert Xu 2019-04-16 9:38 ` Toke Høiland-Jørgensen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.