From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail2.tohojo.dk (mail2.tohojo.dk [77.235.48.147]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id 2F9073B25E for ; Mon, 5 Sep 2016 12:06:25 -0400 (EDT) X-Virus-Scanned: amavisd-new at mail2.tohojo.dk DKIM-Filter: OpenDKIM Filter v2.10.3 mail2.tohojo.dk 8DF1C4161D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=toke.dk; s=201310; t=1473091581; bh=nGFav2OVs8vtN0dV2FZkal2eMHge3/eTMivduUsITbA=; h=From:To:Subject:References:Date:In-Reply-To:From; b=d70ik/2uVs501U1C3z7MxRd+yS+fwQGlVYo1i/IEH3HI0jylu1cKjowzN/1Q0k3ma D2QAYZinfpKtiCr5xJTxg5JxWzY6LDO0MqY7cwsrUSs1CqFN/cw+VlANqXiWjKYDi/ NsgkRUeS6A1uR1jN2w/OqP8ezbf11e7s3NiD8scM= Received: by alrua-kau.kau.toke.dk (Postfix, from userid 1000) id ABF9CC40236; Mon, 5 Sep 2016 18:06:20 +0200 (CEST) From: =?utf-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= To: make-wifi-fast@lists.bufferbloat.net References: <20160902134104.29309-1-toke@toke.dk> <20160905113042.22271-1-toke@toke.dk> Date: Mon, 05 Sep 2016 18:06:20 +0200 In-Reply-To: <20160905113042.22271-1-toke@toke.dk> ("Toke =?utf-8?Q?H?= =?utf-8?Q?=C3=B8iland-J=C3=B8rgensen=22's?= message of "Mon, 5 Sep 2016 13:30:42 +0200") X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <87oa4249hf.fsf@toke.dk> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Subject: Re: [Make-wifi-fast] [PATCH v7] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue. X-BeenThere: make-wifi-fast@lists.bufferbloat.net X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 05 Sep 2016 16:06:25 -0000 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Toke H=C3=B8iland-J=C3=B8rgensen writes: > The TXQ intermediate queues can cause packet reordering when more than > one flow is active to a single station. Since some of the wifi-specific > packet handling (notably sequence number and encryption handling) is > sensitive to re-ordering, things break if they are applied before the > TXQ. > > This splits up the TX handlers and fast_xmit logic into two parts: An > early part and a late part. The former is applied before TXQ enqueue, > and the latter after dequeue. The non-TXQ path just applies both parts > at once. > > Because fragments shouldn't be split up or reordered, the fragmentation > handler is run after dequeue. Any fragments are then kept in the TXQ and > on subsequent dequeues they take precedence over dequeueing from the FQ > structure. > > This approach avoids having to scatter special cases for when TXQ is > enabled, at the cost of making the fast_xmit and TX handler code > slightly more complex. > > Signed-off-by: Toke H=C3=B8iland-J=C3=B8rgensen > --- > Changes since v6: > - Invoking the rate control handler can cause packets to be generated > (for establishing a BA session). This can cause a deadlock because > dequeue can happen while sta->lock is held. So this version moves > the rate control handler back before the intermediate queue step. > - Fix sequence number allocation on the slow path. Attaching a version suitable for dropping into a LEDE build (where it replaces 220-fq_disable_hack.patch and 346-mac80211-fix-sequence-number-assignment-for-PS-respo.patch). -Toke --=-=-= Content-Type: text/x-diff; charset=utf-8 Content-Disposition: attachment; filename=346-mac80211-move-reorder-sensitive-tx-handlers-to-after-TXQ-dequeue.patch Content-Transfer-Encoding: quoted-printable commit 9659fea5e5561dd851dcc9273e45a0dc8b0c9f69 Author: Toke H=C3=B8iland-J=C3=B8rgensen Date: Tue Aug 23 20:14:07 2016 +0200 mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue. =20=20=20=20 The TXQ intermediate queues can cause packet reordering when more than one flow is active to a single station. Since some of the wifi-specific packet handling (notably sequence number and encryption handling) is sensitive to re-ordering, things break if they are applied before the TXQ. =20=20=20=20 This splits up the TX handlers and fast_xmit logic into two parts: An early part and a late part. The former is applied before TXQ enqueue, and the latter after dequeue. The non-TXQ path just applies both parts at once. =20=20=20=20 Because fragments shouldn't be split up or reordered, the fragmentation handler is run after dequeue. Any fragments are then kept in the TXQ and on subsequent dequeues they take precedence over dequeueing from the FQ structure. =20=20=20=20 This approach avoids having to scatter special cases for when TXQ is enabled, at the cost of making the fast_xmit and TX handler code slightly more complex. =20=20=20=20 Signed-off-by: Toke H=C3=B8iland-J=C3=B8rgensen diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 26b0ea8..3d6bf45 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -711,6 +711,7 @@ enum mac80211_tx_info_flags { * frame (PS-Poll or uAPSD). * @IEEE80211_TX_CTRL_RATE_INJECT: This frame is injected with rate inform= ation * @IEEE80211_TX_CTRL_AMSDU: This frame is an A-MSDU frame + * @IEEE80211_TX_CTRL_FAST_XMIT: This frame is going through the fast_xmit= path * * These flags are used in tx_info->control.flags. */ @@ -719,6 +720,7 @@ enum mac80211_tx_control_flags { IEEE80211_TX_CTRL_PS_RESPONSE =3D BIT(1), IEEE80211_TX_CTRL_RATE_INJECT =3D BIT(2), IEEE80211_TX_CTRL_AMSDU =3D BIT(3), + IEEE80211_TX_CTRL_FAST_XMIT =3D BIT(4), }; =20 /* diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 81548a8..cf99bc1 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -808,11 +808,13 @@ enum txq_info_flags { * @def_flow: used as a fallback flow when a packet destined to @tin hashe= s to * a fq_flow which is already owned by a different tin * @def_cvars: codel vars for @def_flow + * @frags: used to keep fragments created after dequeue */ struct txq_info { struct fq_tin tin; struct fq_flow def_flow; struct codel_vars def_cvars; + struct sk_buff_head frags; unsigned long flags; =20 /* keep last! */ diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 86e806d..85ec649 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -38,6 +38,12 @@ #include "wme.h" #include "rate.h" =20 +static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx); +static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data *sdata, + struct sta_info *sta, + struct ieee80211_fast_tx *fast_tx, + struct sk_buff *skb); + /* misc utils */ =20 static inline void ieee80211_tx_stats(struct net_device *dev, u32 len) @@ -849,8 +855,7 @@ ieee80211_tx_h_sequence(struct ieee80211_tx_data *tx) tid =3D *qc & IEEE80211_QOS_CTL_TID_MASK; tx->sta->tx_stats.msdu[tid]++; =20 - if (!tx->sta->sta.txq[0]) - hdr->seq_ctrl =3D ieee80211_tx_next_seq(tx->sta, tid); + hdr->seq_ctrl =3D ieee80211_tx_next_seq(tx->sta, tid); =20 return TX_CONTINUE; } @@ -1399,6 +1404,7 @@ void ieee80211_txq_init(struct ieee80211_sub_if_data = *sdata, fq_tin_init(&txqi->tin); fq_flow_init(&txqi->def_flow); codel_vars_init(&txqi->def_cvars); + __skb_queue_head_init(&txqi->frags); =20 txqi->txq.vif =3D &sdata->vif; =20 @@ -1421,6 +1427,7 @@ void ieee80211_txq_purge(struct ieee80211_local *loca= l, struct fq_tin *tin =3D &txqi->tin; =20 fq_tin_reset(fq, tin, fq_skb_free_func); + ieee80211_purge_tx_queue(&local->hw, &txqi->frags); } =20 int ieee80211_txq_setup_flows(struct ieee80211_local *local) @@ -1477,12 +1484,19 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee802= 11_hw *hw, struct sk_buff *skb =3D NULL; struct fq *fq =3D &local->fq; struct fq_tin *tin =3D &txqi->tin; + struct ieee80211_tx_info *info; =20 spin_lock_bh(&fq->lock); =20 if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags)) goto out; =20 + /* Make sure fragments stay together. */ + skb =3D __skb_dequeue(&txqi->frags); + if (skb) + goto out; + +begin: skb =3D fq_tin_dequeue(fq, tin, fq_tin_dequeue_func); if (!skb) goto out; @@ -1490,16 +1504,40 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee802= 11_hw *hw, ieee80211_set_skb_vif(skb, txqi); =20 hdr =3D (struct ieee80211_hdr *)skb->data; - if (txq->sta && ieee80211_is_data_qos(hdr->frame_control)) { + info =3D IEEE80211_SKB_CB(skb); + if (txq->sta && info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) { struct sta_info *sta =3D container_of(txq->sta, struct sta_info, sta); - struct ieee80211_tx_info *info =3D IEEE80211_SKB_CB(skb); + struct ieee80211_fast_tx *fast_tx; =20 - hdr->seq_ctrl =3D ieee80211_tx_next_seq(sta, txq->tid); - if (test_bit(IEEE80211_TXQ_AMPDU, &txqi->flags)) - info->flags |=3D IEEE80211_TX_CTL_AMPDU; - else - info->flags &=3D ~IEEE80211_TX_CTL_AMPDU; + fast_tx =3D rcu_dereference(sta->fast_tx); + if (WARN_ON(!fast_tx)) { + /* lost fast_tx pointer while the packet was queued */ + ieee80211_free_txskb(hw, skb); + goto begin; + } + ieee80211_xmit_fast_finish(sta->sdata, sta, fast_tx, skb); + } else { + struct ieee80211_tx_data tx =3D { }; + + __skb_queue_head_init(&tx.skbs); + tx.local =3D local; + tx.skb =3D skb; + tx.hdrlen =3D ieee80211_padded_hdrlen(hw, hdr->frame_control); + if (txq->sta) { + tx.sta =3D container_of(txq->sta, struct sta_info, sta); + tx.sdata =3D tx.sta->sdata; + } else { + tx.sdata =3D vif_to_sdata(info->control.vif); + } + + if (invoke_tx_handlers_late(&tx)) + goto begin; + + skb =3D __skb_dequeue(&tx.skbs); + + if (!skb_queue_empty(&tx.skbs)) + skb_queue_splice_tail(&tx.skbs, &txqi->frags); } =20 out: @@ -1513,6 +1551,47 @@ out: } EXPORT_SYMBOL(ieee80211_tx_dequeue); =20 +static bool ieee80211_queue_skb(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct sta_info *sta, + struct sk_buff *skb) +{ + struct ieee80211_tx_info *info =3D IEEE80211_SKB_CB(skb); + struct fq *fq =3D &local->fq; + struct ieee80211_vif *vif; + struct txq_info *txqi; + struct ieee80211_sta *pubsta; + + if (!local->ops->wake_tx_queue || + sdata->vif.type =3D=3D NL80211_IFTYPE_MONITOR) + return false; + + if (sta && sta->uploaded) + pubsta =3D &sta->sta; + else + pubsta =3D NULL; + + if (sdata->vif.type =3D=3D NL80211_IFTYPE_AP_VLAN) + sdata =3D container_of(sdata->bss, + struct ieee80211_sub_if_data, u.ap); + + vif =3D &sdata->vif; + txqi =3D ieee80211_get_txq(local, vif, pubsta, skb); + + if (!txqi) + return false; + + info->control.vif =3D vif; + + spin_lock_bh(&fq->lock); + ieee80211_txq_enqueue(local, txqi, skb); + spin_unlock_bh(&fq->lock); + + drv_wake_tx_queue(local, txqi); + + return true; +} + static bool ieee80211_tx_frags(struct ieee80211_local *local, struct ieee80211_vif *vif, struct ieee80211_sta *sta, @@ -1520,9 +1599,7 @@ static bool ieee80211_tx_frags(struct ieee80211_local= *local, bool txpending) { struct ieee80211_tx_control control =3D {}; - struct fq *fq =3D &local->fq; struct sk_buff *skb, *tmp; - struct txq_info *txqi; unsigned long flags; =20 skb_queue_walk_safe(skbs, skb, tmp) { @@ -1537,21 +1614,6 @@ static bool ieee80211_tx_frags(struct ieee80211_loca= l *local, } #endif =20 - txqi =3D ieee80211_get_txq(local, vif, sta, skb); - if (txqi) { - info->control.vif =3D vif; - - __skb_unlink(skb, skbs); - - spin_lock_bh(&fq->lock); - ieee80211_txq_enqueue(local, txqi, skb); - spin_unlock_bh(&fq->lock); - - drv_wake_tx_queue(local, txqi); - - continue; - } - spin_lock_irqsave(&local->queue_stop_reason_lock, flags); if (local->queue_stop_reasons[q] || (!txpending && !skb_queue_empty(&local->pending[q]))) { @@ -1672,10 +1734,13 @@ static bool __ieee80211_tx(struct ieee80211_local *= local, /* * Invoke TX handlers, return 0 on success and non-zero if the * frame was dropped or queued. + * + * The handlers are split into an early and late part. The latter is every= thing + * that can be sensitive to reordering, and will be deferred to after pack= ets + * are dequeued from the intermediate queues (when they are enabled). */ -static int invoke_tx_handlers(struct ieee80211_tx_data *tx) +static int invoke_tx_handlers_early(struct ieee80211_tx_data *tx) { - struct ieee80211_tx_info *info =3D IEEE80211_SKB_CB(tx->skb); ieee80211_tx_result res =3D TX_DROP; =20 #define CALL_TXH(txh) \ @@ -1689,16 +1754,42 @@ static int invoke_tx_handlers(struct ieee80211_tx_d= ata *tx) CALL_TXH(ieee80211_tx_h_check_assoc); CALL_TXH(ieee80211_tx_h_ps_buf); CALL_TXH(ieee80211_tx_h_check_control_port_protocol); - CALL_TXH(ieee80211_tx_h_select_key); + if (!ieee80211_hw_check(&tx->local->hw, HAS_RATE_CONTROL)) CALL_TXH(ieee80211_tx_h_rate_ctrl); =20 + txh_done: + if (unlikely(res =3D=3D TX_DROP)) { + I802_DEBUG_INC(tx->local->tx_handlers_drop); + if (tx->skb) + ieee80211_free_txskb(&tx->local->hw, tx->skb); + else + ieee80211_purge_tx_queue(&tx->local->hw, &tx->skbs); + return -1; + } else if (unlikely(res =3D=3D TX_QUEUED)) { + I802_DEBUG_INC(tx->local->tx_handlers_queued); + return -1; + } + + return 0; +} + +/* + * Late handlers can be called while the sta lock is held. Handlers that c= an + * cause packets to be generated will cause deadlock! + */ +static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx) +{ + struct ieee80211_tx_info *info =3D IEEE80211_SKB_CB(tx->skb); + ieee80211_tx_result res =3D TX_CONTINUE; + if (unlikely(info->flags & IEEE80211_TX_INTFL_RETRANSMISSION)) { __skb_queue_tail(&tx->skbs, tx->skb); tx->skb =3D NULL; goto txh_done; } =20 + CALL_TXH(ieee80211_tx_h_select_key); CALL_TXH(ieee80211_tx_h_michael_mic_add); CALL_TXH(ieee80211_tx_h_sequence); CALL_TXH(ieee80211_tx_h_fragment); @@ -1725,6 +1816,15 @@ static int invoke_tx_handlers(struct ieee80211_tx_da= ta *tx) return 0; } =20 +static int invoke_tx_handlers(struct ieee80211_tx_data *tx) +{ + int r =3D invoke_tx_handlers_early(tx); + if (r) + return r; + + return invoke_tx_handlers_late(tx); +} + bool ieee80211_tx_prepare_skb(struct ieee80211_hw *hw, struct ieee80211_vif *vif, struct sk_buff *skb, int band, struct ieee80211_sta **sta) @@ -1799,7 +1899,13 @@ static bool ieee80211_tx(struct ieee80211_sub_if_dat= a *sdata, info->hw_queue =3D sdata->vif.hw_queue[skb_get_queue_mapping(skb)]; =20 - if (!invoke_tx_handlers(&tx)) + if (invoke_tx_handlers_early(&tx)) + return false; + + if (ieee80211_queue_skb(local, sdata, tx.sta, tx.skb)) + return true; + + if (!invoke_tx_handlers_late(&tx)) result =3D __ieee80211_tx(local, &tx.skbs, led_len, tx.sta, txpending); =20 @@ -3120,7 +3226,7 @@ out: } =20 static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata, - struct net_device *dev, struct sta_info *sta, + struct sta_info *sta, struct ieee80211_fast_tx *fast_tx, struct sk_buff *skb) { @@ -3131,9 +3237,9 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_= if_data *sdata, struct ethhdr eth; struct ieee80211_tx_info *info =3D IEEE80211_SKB_CB(skb); struct ieee80211_hdr *hdr =3D (void *)fast_tx->hdr; - struct ieee80211_tx_data tx; - ieee80211_tx_result r; struct tid_ampdu_tx *tid_tx =3D NULL; + ieee80211_tx_result r; + struct ieee80211_tx_data tx; u8 tid =3D IEEE80211_NUM_TIDS; =20 /* control port protocol needs a lot of special handling */ @@ -3171,8 +3277,6 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_= if_data *sdata, return true; } =20 - ieee80211_tx_stats(dev, skb->len + extra_head); - if ((hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) && ieee80211_amsdu_aggregate(sdata, sta, fast_tx, skb)) return true; @@ -3201,24 +3305,7 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub= _if_data *sdata, info->flags =3D IEEE80211_TX_CTL_FIRST_FRAGMENT | IEEE80211_TX_CTL_DONTFRAG | (tid_tx ? IEEE80211_TX_CTL_AMPDU : 0); - - if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) { - *ieee80211_get_qos_ctl(hdr) =3D tid; - if (!sta->sta.txq[0]) - hdr->seq_ctrl =3D ieee80211_tx_next_seq(sta, tid); - } else { - info->flags |=3D IEEE80211_TX_CTL_ASSIGN_SEQ; - hdr->seq_ctrl =3D cpu_to_le16(sdata->sequence_number); - sdata->sequence_number +=3D 0x10; - } - - if (skb_shinfo(skb)->gso_size) - sta->tx_stats.msdu[tid] +=3D - DIV_ROUND_UP(skb->len, skb_shinfo(skb)->gso_size); - else - sta->tx_stats.msdu[tid]++; - - info->hw_queue =3D sdata->vif.hw_queue[skb_get_queue_mapping(skb)]; + info->control.flags =3D IEEE80211_TX_CTRL_FAST_XMIT; =20 __skb_queue_head_init(&tx.skbs); =20 @@ -3244,6 +3331,54 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub= _if_data *sdata, } } =20 + if (ieee80211_queue_skb(local, sdata, sta, skb)) + return true; + + ieee80211_xmit_fast_finish(sdata, sta, fast_tx, skb); + + if (sdata->vif.type =3D=3D NL80211_IFTYPE_AP_VLAN) + sdata =3D container_of(sdata->bss, + struct ieee80211_sub_if_data, u.ap); + + __skb_queue_tail(&tx.skbs, skb); + ieee80211_tx_frags(local, &sdata->vif, &sta->sta, &tx.skbs, false); + + return true; +} + +/* + * Can be called while the sta lock is held. Anything that can cause packe= ts to + * be generated will cause deadlock! + */ +static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data *sdata, + struct sta_info *sta, + struct ieee80211_fast_tx *fast_tx, + struct sk_buff *skb) +{ + struct ieee80211_tx_info *info =3D IEEE80211_SKB_CB(skb); + struct ieee80211_hdr *hdr =3D (void *)skb->data; + u8 tid =3D IEEE80211_NUM_TIDS; + + ieee80211_tx_stats(skb->dev, skb->len); + + if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) { + tid =3D skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK; + *ieee80211_get_qos_ctl(hdr) =3D tid; + hdr->seq_ctrl =3D ieee80211_tx_next_seq(sta, tid); + } else { + info->flags |=3D IEEE80211_TX_CTL_ASSIGN_SEQ; + hdr->seq_ctrl =3D cpu_to_le16(sdata->sequence_number); + sdata->sequence_number +=3D 0x10; + } + + if (skb_shinfo(skb)->gso_size) + sta->tx_stats.msdu[tid] +=3D + DIV_ROUND_UP(skb->len, skb_shinfo(skb)->gso_size); + else + sta->tx_stats.msdu[tid]++; + + info->hw_queue =3D sdata->vif.hw_queue[skb_get_queue_mapping(skb)]; + /* statistics normally done by ieee80211_tx_h_stats (but that * has to consider fragmentation, so is more complex) */ @@ -3270,12 +3405,6 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub= _if_data *sdata, } } =20 - if (sdata->vif.type =3D=3D NL80211_IFTYPE_AP_VLAN) - sdata =3D container_of(sdata->bss, - struct ieee80211_sub_if_data, u.ap); - - __skb_queue_tail(&tx.skbs, skb); - ieee80211_tx_frags(local, &sdata->vif, &sta->sta, &tx.skbs, false); return true; } =20 @@ -3303,7 +3432,7 @@ void __ieee80211_subif_start_xmit(struct sk_buff *skb, fast_tx =3D rcu_dereference(sta->fast_tx); =20 if (fast_tx && - ieee80211_xmit_fast(sdata, dev, sta, fast_tx, skb)) + ieee80211_xmit_fast(sdata, sta, fast_tx, skb)) goto out; } =20 --=-=-=--