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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
