| Message ID | 1416352495-82172-1-git-send-email-nbd@openwrt.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, 2014-11-19 at 00:14 +0100, Felix Fietkau wrote: > + struct txq_info *txq; > + atomic_t txq_len[IEEE80211_NUM_ACS]; I think you should consider renaming the latter to txqs_len or so - it doesn't just cover one txq as is be implied by the name now. Otherwise the skb_queue_head also maintains the length anyway, but I think you need the aggregate for all stations here... Some documentation for this and the vif.txq would be good too :) In fact - it might be worthwhile to take parts of the commit message and elaborate a bit on it and write a whole DOC: section? > --- a/net/mac80211/sta_info.h > +++ b/net/mac80211/sta_info.h > @@ -371,6 +371,7 @@ struct sta_info { > struct sk_buff_head ps_tx_buf[IEEE80211_NUM_ACS]; > struct sk_buff_head tx_filtered[IEEE80211_NUM_ACS]; > unsigned long driver_buffered_tids; > + void *txq; You can still use struct txq_info * here even when it's not declared yet (since it's in the other header file) > +static void ieee80211_drv_tx(struct ieee80211_local *local, > + struct ieee80211_vif *vif, > + struct ieee80211_sta *pubsta, > + struct sk_buff *skb) > +{ > + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data; > + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif); > + struct ieee80211_tx_control control = { > + .sta = pubsta > + }; > + struct ieee80211_txq *pubtxq = NULL; > + struct txq_info *txq; > + u8 ac; > + > + if (ieee80211_is_mgmt(hdr->frame_control) || > + ieee80211_is_ctl(hdr->frame_control)) > + goto tx_normal; > + > + if (pubsta) { > + u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK; > + pubtxq = pubsta->txq[tid]; > + } else { > + pubtxq = vif->txq; > + } This is a bit confusing - isn't this the same as &sdata->txq.txq? Then again what even sets vif->txq? Shouldn't those be per-AC? Do you really want to mix 'normal' and txq-TX? I think you should also use txqi as variables for txq_info - it gets cumbersome to distinguish the two everywhere. Also in many cases where you have txq allocation failures you just continue as is, I'm not sure that's such a great idea. Those driver paths will practically never get tested. > + if (!pubtxq) > + goto tx_normal; > + > + ac = pubtxq->ac; > + txq = container_of(pubtxq, struct txq_info, txq); > + atomic_inc(&sdata->txq_len[ac]); > + if (atomic_read(&sdata->txq_len[ac]) >= local->hw.txq_ac_max_pending) > + netif_stop_subqueue(sdata->dev, ac); > + > + skb_queue_tail(&txq->queue, skb); > + drv_wake_tx_queue(local, txq); You might consider doing locking differently here - I think you probably don't need the txq->queue spinlock at all since you're in per-AC and mappings are static. Not sure how that interacts with other parts of the code though. > +int ieee80211_tx_dequeue(struct ieee80211_hw *hw, struct ieee80211_txq *pubtxq, > + struct sk_buff **dest) I'd prefer you return the skb and use ERR_PTR() for errors. > +void ieee80211_init_tx_queue(struct ieee80211_sub_if_data *sdata, > + struct sta_info *sta, > + struct txq_info *txq, int tid) > +{ > + skb_queue_head_init(&txq->queue); > + txq->txq.vif = &sdata->vif; > + > + if (sta) { > + txq->txq.sta = &sta->sta; > + sta->sta.txq[tid] = &txq->txq; > + txq->txq.ac = ieee802_1d_to_ac[tid & 7]; > + } else { > + sdata->vif.txq = &txq->txq; > + txq->txq.ac = IEEE80211_AC_BE; > + } Again, I don't quite understand the single AC queue here per vif. It seems it should be one for each AC and possibly one for cab? Or none at all - I don't really see what this single one would be used for, in the TX code you seem to use it for mcast data only but then I don't really see the point. It's also not part of the queue length accounting. > +void ieee80211_flush_tx_queue(struct ieee80211_local *local, > + struct ieee80211_txq *pubtxq) > +{ > + struct txq_info *txq = container_of(pubtxq, struct txq_info, txq); > + struct ieee80211_sub_if_data *sdata = vif_to_sdata(pubtxq->vif); > + struct sk_buff *skb; > + > + while ((skb = skb_dequeue(&txq->queue)) != NULL) { > + atomic_dec(&sdata->txq_len[pubtxq->ac]); > + ieee80211_free_txskb(&local->hw, skb); > + } > +} You can rewrite this a bit smarter to just do one atomic op. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014-12-12 14:21, Johannes Berg wrote: > On Wed, 2014-11-19 at 00:14 +0100, Felix Fietkau wrote: > >> + struct txq_info *txq; >> + atomic_t txq_len[IEEE80211_NUM_ACS]; > > I think you should consider renaming the latter to txqs_len or so - it > doesn't just cover one txq as is be implied by the name now. Otherwise > the skb_queue_head also maintains the length anyway, but I think you > need the aggregate for all stations here... > > Some documentation for this and the vif.txq would be good too :) > > In fact - it might be worthwhile to take parts of the commit message and > elaborate a bit on it and write a whole DOC: section? Yeah, makes sense. >> --- a/net/mac80211/sta_info.h >> +++ b/net/mac80211/sta_info.h >> @@ -371,6 +371,7 @@ struct sta_info { >> struct sk_buff_head ps_tx_buf[IEEE80211_NUM_ACS]; >> struct sk_buff_head tx_filtered[IEEE80211_NUM_ACS]; >> unsigned long driver_buffered_tids; >> + void *txq; > > You can still use struct txq_info * here even when it's not declared yet > (since it's in the other header file) OK. >> +static void ieee80211_drv_tx(struct ieee80211_local *local, >> + struct ieee80211_vif *vif, >> + struct ieee80211_sta *pubsta, >> + struct sk_buff *skb) >> +{ >> + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data; >> + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif); >> + struct ieee80211_tx_control control = { >> + .sta = pubsta >> + }; >> + struct ieee80211_txq *pubtxq = NULL; >> + struct txq_info *txq; >> + u8 ac; >> + >> + if (ieee80211_is_mgmt(hdr->frame_control) || >> + ieee80211_is_ctl(hdr->frame_control)) >> + goto tx_normal; >> + >> + if (pubsta) { >> + u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK; >> + pubtxq = pubsta->txq[tid]; >> + } else { >> + pubtxq = vif->txq; >> + } > > This is a bit confusing - isn't this the same as &sdata->txq.txq? Right, I should probably use that. > Then > again what even sets vif->txq? Shouldn't those be per-AC? Do you really > want to mix 'normal' and txq-TX? Are we even using multiple ACs for packets that don't belong to a particular sta? I thought normal mcast data frames only use non-QoS frames. And yes, I'm currently mixing normal and txq-TX to prioritize ctl/mgmt frames over other less important traffic. > I think you should also use txqi as variables for txq_info - it gets > cumbersome to distinguish the two everywhere. Will do. > Also in many cases where you have txq allocation failures you just > continue as is, I'm not sure that's such a great idea. Those driver > paths will practically never get tested. It will just do normal tx in that case, which should work. >> + if (!pubtxq) >> + goto tx_normal; >> + >> + ac = pubtxq->ac; >> + txq = container_of(pubtxq, struct txq_info, txq); >> + atomic_inc(&sdata->txq_len[ac]); >> + if (atomic_read(&sdata->txq_len[ac]) >= local->hw.txq_ac_max_pending) >> + netif_stop_subqueue(sdata->dev, ac); >> + >> + skb_queue_tail(&txq->queue, skb); >> + drv_wake_tx_queue(local, txq); > > You might consider doing locking differently here - I think you probably > don't need the txq->queue spinlock at all since you're in per-AC and > mappings are static. Not sure how that interacts with other parts of the > code though. I wanted to use the lock to give the driver the freedom to call ieee80211_tx_dequeue from outside of normal per-AC tx context. >> +int ieee80211_tx_dequeue(struct ieee80211_hw *hw, struct ieee80211_txq *pubtxq, >> + struct sk_buff **dest) > > I'd prefer you return the skb and use ERR_PTR() for errors. Will do >> +void ieee80211_init_tx_queue(struct ieee80211_sub_if_data *sdata, >> + struct sta_info *sta, >> + struct txq_info *txq, int tid) >> +{ >> + skb_queue_head_init(&txq->queue); >> + txq->txq.vif = &sdata->vif; >> + >> + if (sta) { >> + txq->txq.sta = &sta->sta; >> + sta->sta.txq[tid] = &txq->txq; >> + txq->txq.ac = ieee802_1d_to_ac[tid & 7]; >> + } else { >> + sdata->vif.txq = &txq->txq; >> + txq->txq.ac = IEEE80211_AC_BE; >> + } > > Again, I don't quite understand the single AC queue here per vif. It > seems it should be one for each AC and possibly one for cab? Or none at > all - I don't really see what this single one would be used for, in the > TX code you seem to use it for mcast data only but then I don't really > see the point. It's also not part of the queue length accounting. I handle CAB through normal mac80211 mcast buffering. >> +void ieee80211_flush_tx_queue(struct ieee80211_local *local, >> + struct ieee80211_txq *pubtxq) >> +{ >> + struct txq_info *txq = container_of(pubtxq, struct txq_info, txq); >> + struct ieee80211_sub_if_data *sdata = vif_to_sdata(pubtxq->vif); >> + struct sk_buff *skb; >> + >> + while ((skb = skb_dequeue(&txq->queue)) != NULL) { >> + atomic_dec(&sdata->txq_len[pubtxq->ac]); >> + ieee80211_free_txskb(&local->hw, skb); >> + } >> +} > > You can rewrite this a bit smarter to just do one atomic op. Will do. - Felix -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-12-12 at 14:40 +0100, Felix Fietkau wrote: > > Then > > again what even sets vif->txq? Shouldn't those be per-AC? Do you really > > want to mix 'normal' and txq-TX? > Are we even using multiple ACs for packets that don't belong to a > particular sta? I thought normal mcast data frames only use non-QoS > frames. And yes, I'm currently mixing normal and txq-TX to prioritize > ctl/mgmt frames over other less important traffic. Management (and maybe control) frames can have different priorities as well, this is only used for something with TDLS now I think though. > > You might consider doing locking differently here - I think you probably > > don't need the txq->queue spinlock at all since you're in per-AC and > > mappings are static. Not sure how that interacts with other parts of the > > code though. > I wanted to use the lock to give the driver the freedom to call > ieee80211_tx_dequeue from outside of normal per-AC tx context. Ok. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014-12-12 15:01, Johannes Berg wrote: > On Fri, 2014-12-12 at 14:40 +0100, Felix Fietkau wrote: > >> > Then >> > again what even sets vif->txq? Shouldn't those be per-AC? Do you really >> > want to mix 'normal' and txq-TX? >> Are we even using multiple ACs for packets that don't belong to a >> particular sta? I thought normal mcast data frames only use non-QoS >> frames. And yes, I'm currently mixing normal and txq-TX to prioritize >> ctl/mgmt frames over other less important traffic. > > Management (and maybe control) frames can have different priorities as > well, this is only used for something with TDLS now I think though. With my implementation, those would go through the normal tx codepath, bypassing the software tx queues. Can you think of anything else that would need per-AC vif queues? - Felix -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-12-12 at 15:28 +0100, Felix Fietkau wrote: > > Management (and maybe control) frames can have different priorities as > > well, this is only used for something with TDLS now I think though. > With my implementation, those would go through the normal tx codepath, > bypassing the software tx queues. Can you think of anything else that > would need per-AC vif queues? Not off the top of my head. I just didn't even quite understand that you were still using the normal tx path johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
As for drv_wake_tx_queue and ieee80211_tx_dequeue - is it really necessary? There are ieee80211_tx_status and ieee80211_free_txskb already, which can be used to decide from mac80211 level when to dequeue packet. It could be used even in case of drivers that are not aware of new mechanism at all. We could compute difference between drv_tx and tx_status/free_txskb calls, therefore getting number of frames in HW. What could help us to keep queues short. I've already written some code. This http://pastebin.com/dSd1zWt7 is patch that implements counter of frames in hardware in the way described above. It was necessary to differentiate between free_txskb and free_txskb. Information about frames in HW is exported to debugfs. I thought I could submit it, but just now did I found this thread, so I hope that it's adequate place to propose that. I tested it on ath5k and brcmsmac. One more thing - why not to use local->pending for holding packets? There is tx_pending tasklet already. I'm not sure if I understand the idea of local->pending queues correctly, but it seems to be a bit incoherent to have both pending and proposed ieee80211_txq. (It's my first post on linux kernel mailing list. Please, let me know if I did something wrong.) Best regards. 2014-12-15 13:00 GMT+01:00 Johannes Berg <johannes@sipsolutions.net>: > On Fri, 2014-12-12 at 15:28 +0100, Felix Fietkau wrote: > >> > Management (and maybe control) frames can have different priorities as >> > well, this is only used for something with TDLS now I think though. >> With my implementation, those would go through the normal tx codepath, >> bypassing the software tx queues. Can you think of anything else that >> would need per-AC vif queues? > > Not off the top of my head. I just didn't even quite understand that you > were still using the normal tx path > > johannes > > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014-12-16 00:25, Bartosz Szczepanek wrote: > As for drv_wake_tx_queue and ieee80211_tx_dequeue - is it really > necessary? There are ieee80211_tx_status and ieee80211_free_txskb > already, which can be used to decide from mac80211 level when to > dequeue packet. It could be used even in case of drivers that are not > aware of new mechanism at all. We could compute difference between > drv_tx and tx_status/free_txskb calls, therefore getting number of > frames in HW. What could help us to keep queues short. Keeping the driver queue short is a bad idea if the driver needs deeper queues to do aggregation. > I've already written some code. This http://pastebin.com/dSd1zWt7 is > patch that implements counter of frames in hardware in the way > described above. It was necessary to differentiate between free_txskb > and free_txskb. Information about frames in HW is exported to debugfs. > I thought I could submit it, but just now did I found this thread, so > I hope that it's adequate place to propose that. I tested it on ath5k > and brcmsmac. For aggregation, different drivers need different kinds of scheduling. Only packets belonging to the same sta/tid can be aggregated, and in AP mode you can have concurrent traffic of multiple different sta/tid. The only way to keep queues really short in that case without sacrificing throughput is to let the aggregation code fetch packets directly from sta/tid queues. With ath5k there is no aggregation yet (we could add A-MSDU at some point), and with brcmsmac, the driver has an internal layer of queueing to create per-sta/tid queues. What I'm proposing is having per-sta/tid queues that are shared between mac80211 and the driver, which will significantly improve queue management compared to having multiple competing layers of queueing. > One more thing - why not to use local->pending for holding packets? > There is tx_pending tasklet already. I'm not sure if I understand the > idea of local->pending queues correctly, but it seems to be a bit > incoherent to have both pending and proposed ieee80211_txq. local->pending is useless for normal tx queueing purposes, because it's not per-sta/tid. - Felix -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 19, 2014 at 12:14 AM, Felix Fietkau <nbd@openwrt.org> wrote: > > +static void ieee80211_drv_tx(struct ieee80211_local *local, > + struct ieee80211_vif *vif, > + struct ieee80211_sta *pubsta, > + struct sk_buff *skb) > +{ > + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data; > + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif); > + struct ieee80211_tx_control control = { > + .sta = pubsta > + }; > + struct ieee80211_txq *pubtxq = NULL; > + struct txq_info *txq; > + u8 ac; > + > + if (ieee80211_is_mgmt(hdr->frame_control) || > + ieee80211_is_ctl(hdr->frame_control)) > + goto tx_normal; > + > + if (pubsta) { > + u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK; > + pubtxq = pubsta->txq[tid]; > + } else { > + pubtxq = vif->txq; > + } In the monitor frame injection tx path the vif pointer may actually be NULL when we get here, see the function __ieee80211_tx(). This causes a NULL pointer dereference crash. The wperf tool (https://github.com/anyfi/wperf) can be used to reproduce the crash by specifying a BSSID that does not belong to a vif. The following one-line change in the patch fixes the crash for me: if (pubsta) { u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK; pubtxq = pubsta->txq[tid]; - } else { + } else if (vif) { pubtxq = vif->txq; } Johan -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 5f203a6..f1ea966 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1192,6 +1192,8 @@ struct ieee80211_vif { u8 cab_queue; u8 hw_queue[IEEE80211_NUM_ACS]; + struct ieee80211_txq *txq; + struct ieee80211_chanctx_conf __rcu *chanctx_conf; u32 driver_flags; @@ -1448,6 +1450,8 @@ struct ieee80211_sta { bool tdls; bool tdls_initiator; + struct ieee80211_txq *txq[IEEE80211_NUM_TIDS]; + /* must be last */ u8 drv_priv[0] __aligned(sizeof(void *)); }; @@ -1476,6 +1480,27 @@ struct ieee80211_tx_control { }; /** + * struct ieee80211_txq - Software intermediate tx queue + * + * @vif: &struct ieee80211_vif pointer from the add_interface callback. + * @sta: station table entry, may be NULL for per-vif queue + * @tid: the TID for this queue (unset for per-vif queue) + * @ac: the AC for this queue + * + * The driver can obtain packets from this queue by calling + * ieee80211_tx_dequeue(). + */ +struct ieee80211_txq { + struct ieee80211_vif *vif; + struct ieee80211_sta *sta; + u8 tid; + u8 ac; + + /* must be last */ + u8 drv_priv[0] __aligned(sizeof(void *)); +}; + +/** * enum ieee80211_hw_flags - hardware flags * * These flags are used to indicate hardware capabilities to @@ -1698,6 +1723,8 @@ enum ieee80211_hw_flags { * within &struct ieee80211_sta. * @chanctx_data_size: size (in bytes) of the drv_priv data area * within &struct ieee80211_chanctx_conf. + * @txq_data_size: size (in bytes) of the drv_priv data area + * within @struct ieee80211_txq. * * @max_rates: maximum number of alternate rate retry stages the hw * can handle. @@ -1746,6 +1773,9 @@ enum ieee80211_hw_flags { * @n_cipher_schemes: a size of an array of cipher schemes definitions. * @cipher_schemes: a pointer to an array of cipher scheme definitions * supported by HW. + * + * @txq_ac_max_pending: maximum number of frames per AC pending in all txq + * entries for a vif. */ struct ieee80211_hw { struct ieee80211_conf conf; @@ -1758,6 +1788,7 @@ struct ieee80211_hw { int vif_data_size; int sta_data_size; int chanctx_data_size; + int txq_data_size; u16 queues; u16 max_listen_interval; s8 max_signal; @@ -1774,6 +1805,7 @@ struct ieee80211_hw { u8 uapsd_max_sp_len; u8 n_cipher_schemes; const struct ieee80211_cipher_scheme *cipher_schemes; + int txq_ac_max_pending; }; /** @@ -2878,6 +2910,8 @@ enum ieee80211_reconfig_type { * * @get_txpower: get current maximum tx power (in dBm) based on configuration * and hardware limits. + * + * @wake_tx_queue: Called when new packets have been added to the queue. */ struct ieee80211_ops { void (*tx)(struct ieee80211_hw *hw, @@ -3089,6 +3123,9 @@ struct ieee80211_ops { u32 (*get_expected_throughput)(struct ieee80211_sta *sta); int (*get_txpower)(struct ieee80211_hw *hw, struct ieee80211_vif *vif, int *dbm); + + void (*wake_tx_queue)(struct ieee80211_hw *hw, + struct ieee80211_txq *txq); }; /** @@ -5006,4 +5043,17 @@ void ieee80211_tdls_oper_request(struct ieee80211_vif *vif, const u8 *peer, */ size_t ieee80211_ie_split(const u8 *ies, size_t ielen, const u8 *ids, int n_ids, size_t offset); + +/** + * ieee80211_tx_dequeue - dequeue a packet from a software tx queue + * + * @hw: pointer as obtained from ieee80211_alloc_hw() + * @txq: pointer obtained from .add_tx_queue() call + * + * Returns 0 if successful, -EAGAIN if no frame was available. + */ +int ieee80211_tx_dequeue(struct ieee80211_hw *hw, struct ieee80211_txq *txq, + struct sk_buff **skb); + + #endif /* MAC80211_H */ diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h index 9759dd1..1dbc646 100644 --- a/net/mac80211/driver-ops.h +++ b/net/mac80211/driver-ops.h @@ -1296,4 +1296,21 @@ static inline int drv_get_txpower(struct ieee80211_local *local, return ret; } +static inline void drv_wake_tx_queue(struct ieee80211_local *local, + struct txq_info *txq) +{ + struct ieee80211_sub_if_data *sdata = vif_to_sdata(txq->txq.vif); + + if (!check_sdata_in_driver(sdata)) + return; + + if (txq->txq.sta) + trace_drv_wake_sta_tx_queue(local, sdata, txq->txq.sta, + txq->txq.tid); + else + trace_drv_wake_vif_tx_queue(local, sdata); + + local->ops->wake_tx_queue(&local->hw, &txq->txq); +} + #endif /* __MAC80211_DRIVER_OPS */ diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 842e066..8dee646 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -793,6 +793,13 @@ struct mac80211_qos_map { struct rcu_head rcu_head; }; +struct txq_info { + struct sk_buff_head queue; + + /* keep last! */ + struct ieee80211_txq txq; +}; + struct ieee80211_sub_if_data { struct list_head list; @@ -837,6 +844,8 @@ struct ieee80211_sub_if_data { bool control_port_no_encrypt; int encrypt_headroom; + struct txq_info *txq; + atomic_t txq_len[IEEE80211_NUM_ACS]; struct ieee80211_tx_queue_params tx_conf[IEEE80211_NUM_ACS]; struct mac80211_qos_map __rcu *qos_map; @@ -1865,6 +1874,11 @@ void ieee80211_add_pending_skbs(struct ieee80211_local *local, struct sk_buff_head *skbs); void ieee80211_flush_queues(struct ieee80211_local *local, struct ieee80211_sub_if_data *sdata); +void ieee80211_init_tx_queue(struct ieee80211_sub_if_data *sdata, + struct sta_info *sta, + struct txq_info *txq, int tid); +void ieee80211_flush_tx_queue(struct ieee80211_local *local, + struct ieee80211_txq *txq); void ieee80211_send_auth(struct ieee80211_sub_if_data *sdata, u16 transaction, u16 auth_alg, u16 status, diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 9df26ad..86dd429 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -967,6 +967,9 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, } spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags); + if (sdata->vif.txq) + ieee80211_flush_tx_queue(local, sdata->vif.txq); + if (local->open_count == 0) ieee80211_clear_tx_pending(local); @@ -1753,6 +1756,13 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name, /* setup type-dependent data */ ieee80211_setup_sdata(sdata, type); + if (local->ops->wake_tx_queue) { + sdata->txq = kzalloc(sizeof(*sdata->txq) + + local->hw.txq_data_size, GFP_KERNEL); + if (sdata->txq) + ieee80211_init_tx_queue(sdata, NULL, sdata->txq, 0); + } + if (ndev) { if (params) { ndev->ieee80211_ptr->use_4addr = params->use_4addr; diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 282a4f3..c742f7f 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -1004,6 +1004,9 @@ int ieee80211_register_hw(struct ieee80211_hw *hw) local->dynamic_ps_forced_timeout = -1; + if (!local->hw.txq_ac_max_pending) + local->hw.txq_ac_max_pending = 64; + result = ieee80211_wep_init(local); if (result < 0) wiphy_debug(local->hw.wiphy, "Failed to initialize wep: %d\n", diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index adc2537..dd948c3 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -119,6 +119,11 @@ static void __cleanup_single_sta(struct sta_info *sta) sta_info_recalc_tim(sta); } + if (sta->txq) { + for (i = 0; i < IEEE80211_NUM_TIDS; i++) + ieee80211_flush_tx_queue(local, sta->sta.txq[i]); + } + for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) { local->total_ps_buffered -= skb_queue_len(&sta->ps_tx_buf[ac]); ieee80211_purge_tx_queue(&local->hw, &sta->ps_tx_buf[ac]); @@ -241,6 +246,8 @@ void sta_info_free(struct ieee80211_local *local, struct sta_info *sta) kfree(sta->tx_lat); } + kfree(sta->txq); + sta_dbg(sta->sdata, "Destroyed STA %pM\n", sta->sta.addr); kfree(rcu_dereference_raw(sta->sta.rates)); @@ -294,12 +301,13 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata, const u8 *addr, gfp_t gfp) { struct ieee80211_local *local = sdata->local; + struct ieee80211_hw *hw = &local->hw; struct sta_info *sta; struct timespec uptime; struct ieee80211_tx_latency_bin_ranges *tx_latency; int i; - sta = kzalloc(sizeof(*sta) + local->hw.sta_data_size, gfp); + sta = kzalloc(sizeof(*sta) + hw->sta_data_size, gfp); if (!sta) return NULL; @@ -357,6 +365,20 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata, for (i = 0; i < ARRAY_SIZE(sta->chain_signal_avg); i++) ewma_init(&sta->chain_signal_avg[i], 1024, 8); + if (local->ops->wake_tx_queue) { + int size = sizeof(struct txq_info) + + ALIGN(hw->txq_data_size, sizeof(void *)); + + sta->txq = kcalloc(IEEE80211_NUM_TIDS, size, gfp); + if (!sta->txq) + goto free; + + for (i = 0; i < IEEE80211_NUM_TIDS; i++) { + struct txq_info *txq = sta->txq + i * size; + ieee80211_init_tx_queue(sdata, sta, txq, i); + } + } + if (sta_prepare_rate_control(local, sta, gfp)) goto free; @@ -380,7 +402,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata, if (sdata->vif.type == NL80211_IFTYPE_AP || sdata->vif.type == NL80211_IFTYPE_AP_VLAN) { struct ieee80211_supported_band *sband = - local->hw.wiphy->bands[ieee80211_get_sdata_band(sdata)]; + hw->wiphy->bands[ieee80211_get_sdata_band(sdata)]; u8 smps = (sband->ht_cap.cap & IEEE80211_HT_CAP_SM_PS) >> IEEE80211_HT_CAP_SM_PS_SHIFT; /* diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h index bcda2ac..3f73284 100644 --- a/net/mac80211/sta_info.h +++ b/net/mac80211/sta_info.h @@ -371,6 +371,7 @@ struct sta_info { struct sk_buff_head ps_tx_buf[IEEE80211_NUM_ACS]; struct sk_buff_head tx_filtered[IEEE80211_NUM_ACS]; unsigned long driver_buffered_tids; + void *txq; /* Updated from RX path only, no locking requirements */ unsigned long rx_packets; diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h index 809a498..84780ad 100644 --- a/net/mac80211/trace.h +++ b/net/mac80211/trace.h @@ -2194,6 +2194,40 @@ TRACE_EVENT(drv_get_txpower, ) ); +DEFINE_EVENT(local_sdata_evt, drv_wake_vif_tx_queue, + TP_PROTO(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata), + TP_ARGS(local, sdata) +); + +TRACE_EVENT(drv_wake_sta_tx_queue, + TP_PROTO(struct ieee80211_local *local, + struct ieee80211_sub_if_data *sdata, + struct ieee80211_sta *sta, + u8 tid), + + TP_ARGS(local, sdata, sta, tid), + + TP_STRUCT__entry( + LOCAL_ENTRY + VIF_ENTRY + STA_ENTRY + __field(u8, tid) + ), + + TP_fast_assign( + LOCAL_ASSIGN; + VIF_ASSIGN; + STA_ASSIGN; + __entry->tid = tid; + ), + + TP_printk( + LOCAL_PR_FMT VIF_PR_FMT STA_PR_FMT " tid: 0x%x", + LOCAL_PR_ARG, VIF_PR_ARG, STA_PR_ARG, __entry->tid + ) +); + #ifdef CONFIG_MAC80211_MESSAGE_TRACING #undef TRACE_SYSTEM diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 3ffd91f..1d98e62 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -1198,13 +1198,75 @@ ieee80211_tx_prepare(struct ieee80211_sub_if_data *sdata, return TX_CONTINUE; } +static void ieee80211_drv_tx(struct ieee80211_local *local, + struct ieee80211_vif *vif, + struct ieee80211_sta *pubsta, + struct sk_buff *skb) +{ + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data; + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif); + struct ieee80211_tx_control control = { + .sta = pubsta + }; + struct ieee80211_txq *pubtxq = NULL; + struct txq_info *txq; + u8 ac; + + if (ieee80211_is_mgmt(hdr->frame_control) || + ieee80211_is_ctl(hdr->frame_control)) + goto tx_normal; + + if (pubsta) { + u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK; + pubtxq = pubsta->txq[tid]; + } else { + pubtxq = vif->txq; + } + + if (!pubtxq) + goto tx_normal; + + ac = pubtxq->ac; + txq = container_of(pubtxq, struct txq_info, txq); + atomic_inc(&sdata->txq_len[ac]); + if (atomic_read(&sdata->txq_len[ac]) >= local->hw.txq_ac_max_pending) + netif_stop_subqueue(sdata->dev, ac); + + skb_queue_tail(&txq->queue, skb); + drv_wake_tx_queue(local, txq); + + return; + +tx_normal: + drv_tx(local, &control, skb); +} + +int ieee80211_tx_dequeue(struct ieee80211_hw *hw, struct ieee80211_txq *pubtxq, + struct sk_buff **dest) +{ + struct ieee80211_local *local = hw_to_local(hw); + struct ieee80211_sub_if_data *sdata = vif_to_sdata(pubtxq->vif); + struct txq_info *txq = container_of(pubtxq, struct txq_info, txq); + u8 ac = pubtxq->ac; + + *dest = skb_dequeue(&txq->queue); + if (!*dest) + return -EAGAIN; + + atomic_dec(&sdata->txq_len[ac]); + if (__netif_subqueue_stopped(sdata->dev, ac)) + ieee80211_propagate_queue_wake(local, sdata->vif.hw_queue[ac]); + + return 0; +} +EXPORT_SYMBOL(ieee80211_tx_dequeue); + static bool ieee80211_tx_frags(struct ieee80211_local *local, struct ieee80211_vif *vif, struct ieee80211_sta *sta, struct sk_buff_head *skbs, bool txpending) { - struct ieee80211_tx_control control; struct sk_buff *skb, *tmp; unsigned long flags; @@ -1262,10 +1324,9 @@ static bool ieee80211_tx_frags(struct ieee80211_local *local, spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags); info->control.vif = vif; - control.sta = sta; __skb_unlink(skb, skbs); - drv_tx(local, &control, skb); + ieee80211_drv_tx(local, vif, sta, skb); } return true; diff --git a/net/mac80211/util.c b/net/mac80211/util.c index f9319a5..90e9763 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -308,6 +308,11 @@ void ieee80211_propagate_queue_wake(struct ieee80211_local *local, int queue) for (ac = 0; ac < n_acs; ac++) { int ac_queue = sdata->vif.hw_queue[ac]; + if (local->ops->wake_tx_queue && + (atomic_read(&sdata->txq_len[ac]) > + local->hw.txq_ac_max_pending)) + continue; + if (ac_queue == queue || (sdata->vif.cab_queue == queue && local->queue_stop_reasons[ac_queue] == 0 && @@ -3182,3 +3187,33 @@ u8 *ieee80211_add_wmm_info_ie(u8 *buf, u8 qosinfo) return buf; } + +void ieee80211_init_tx_queue(struct ieee80211_sub_if_data *sdata, + struct sta_info *sta, + struct txq_info *txq, int tid) +{ + skb_queue_head_init(&txq->queue); + txq->txq.vif = &sdata->vif; + + if (sta) { + txq->txq.sta = &sta->sta; + sta->sta.txq[tid] = &txq->txq; + txq->txq.ac = ieee802_1d_to_ac[tid & 7]; + } else { + sdata->vif.txq = &txq->txq; + txq->txq.ac = IEEE80211_AC_BE; + } +} + +void ieee80211_flush_tx_queue(struct ieee80211_local *local, + struct ieee80211_txq *pubtxq) +{ + struct txq_info *txq = container_of(pubtxq, struct txq_info, txq); + struct ieee80211_sub_if_data *sdata = vif_to_sdata(pubtxq->vif); + struct sk_buff *skb; + + while ((skb = skb_dequeue(&txq->queue)) != NULL) { + atomic_dec(&sdata->txq_len[pubtxq->ac]); + ieee80211_free_txskb(&local->hw, skb); + } +}
This allows drivers to request per-vif and per-sta-tid queues from which they can pull frames. This makes it easier to keep the hardware queues short, and to improve fairness between clients and vifs. The task of scheduling packet transmission is left up to the driver - queueing is controlled by mac80211. Drivers can only dequeue packets by calling ieee80211_tx_dequeue. This makes it possible to add active queue management later without changing drivers using this code. This can also be used as a starting point to implement A-MSDU aggregation in a way that does not add artificially induced latency. Signed-off-by: Felix Fietkau <nbd@openwrt.org> --- include/net/mac80211.h | 50 ++++++++++++++++++++++++++++++++++ net/mac80211/driver-ops.h | 17 ++++++++++++ net/mac80211/ieee80211_i.h | 14 ++++++++++ net/mac80211/iface.c | 10 +++++++ net/mac80211/main.c | 3 +++ net/mac80211/sta_info.c | 26 ++++++++++++++++-- net/mac80211/sta_info.h | 1 + net/mac80211/trace.h | 34 +++++++++++++++++++++++ net/mac80211/tx.c | 67 +++++++++++++++++++++++++++++++++++++++++++--- net/mac80211/util.c | 35 ++++++++++++++++++++++++ 10 files changed, 252 insertions(+), 5 deletions(-)