| Message ID | 1453382588-27105-2-git-send-email-michal.kazior@tieto.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
On 2016-01-21 14:23, Michal Kazior wrote: > This will allow drivers to make more educated > decisions whether to defer transmission or not. > > Relying on wake_tx_queue() call count implicitly > was not possible because it could be called > without queued frame count actually changing on > software tx aggregation start/stop code paths. > > It was also not possible to know how long > byte-wise queue was without dequeueing. > > Signed-off-by: Michal Kazior <michal.kazior@tieto.com> Instead of exposing these in the struct to the driver directly, please make a function to get them. Since the number of frames is already tracked in txqi->queue, you can avoid counter duplication that way. Also, that way you can fix a race condition between accessing the number of frames counter and the bytes counter. - 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 26 January 2016 at 11:45, Felix Fietkau <nbd@openwrt.org> wrote: > On 2016-01-21 14:23, Michal Kazior wrote: >> This will allow drivers to make more educated >> decisions whether to defer transmission or not. >> >> Relying on wake_tx_queue() call count implicitly >> was not possible because it could be called >> without queued frame count actually changing on >> software tx aggregation start/stop code paths. >> >> It was also not possible to know how long >> byte-wise queue was without dequeueing. >> >> Signed-off-by: Michal Kazior <michal.kazior@tieto.com> > Instead of exposing these in the struct to the driver directly, please > make a function to get them. Since the number of frames is already > tracked in txqi->queue, you can avoid counter duplication that way. Hmm, so you suggest to have something like: void ieee80211_get_txq_depth(struct ieee80211_txq *txq, unsigned int *frame_cnt, unsigned int *byte_count) { struct txq_info *txqi = txq_to_info(txq); if (frame_cnt) *frame_cnt = txqi->queue.qlen; if (byte_count) *byte_cnt = txqi->byte_cnt; } Correct? Sounds reasonable. > Also, that way you can fix a race condition between accessing the number > of frames counter and the bytes counter. I don't see a point in maintaining coherency between the two counters with regard to each other alone. Do you have a use-case that would actually make use of that property? I'd like to avoid any unnecessary spinlocks. Micha? -- 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 2016-01-26 12:56, Michal Kazior wrote: > On 26 January 2016 at 11:45, Felix Fietkau <nbd@openwrt.org> wrote: >> On 2016-01-21 14:23, Michal Kazior wrote: >>> This will allow drivers to make more educated >>> decisions whether to defer transmission or not. >>> >>> Relying on wake_tx_queue() call count implicitly >>> was not possible because it could be called >>> without queued frame count actually changing on >>> software tx aggregation start/stop code paths. >>> >>> It was also not possible to know how long >>> byte-wise queue was without dequeueing. >>> >>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com> >> Instead of exposing these in the struct to the driver directly, please >> make a function to get them. Since the number of frames is already >> tracked in txqi->queue, you can avoid counter duplication that way. > > Hmm, so you suggest to have something like: > > void > ieee80211_get_txq_depth(struct ieee80211_txq *txq, > unsigned int *frame_cnt, > unsigned int *byte_count) { > struct txq_info *txqi = txq_to_info(txq); > > if (frame_cnt) > *frame_cnt = txqi->queue.qlen; > > if (byte_count) > *byte_cnt = txqi->byte_cnt; > } > > Correct? > > Sounds reasonable. Right. >> Also, that way you can fix a race condition between accessing the number >> of frames counter and the bytes counter. > > I don't see a point in maintaining coherency between the two counters > with regard to each other alone. Do you have a use-case that would > actually make use of that property? > > I'd like to avoid any unnecessary spinlocks. OK. I guess we can leave them out for now. How frequently are you going to call this function? - 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 26 January 2016 at 13:04, Felix Fietkau <nbd@openwrt.org> wrote: > On 2016-01-26 12:56, Michal Kazior wrote: >> On 26 January 2016 at 11:45, Felix Fietkau <nbd@openwrt.org> wrote: >>> On 2016-01-21 14:23, Michal Kazior wrote: >>>> This will allow drivers to make more educated >>>> decisions whether to defer transmission or not. >>>> >>>> Relying on wake_tx_queue() call count implicitly >>>> was not possible because it could be called >>>> without queued frame count actually changing on >>>> software tx aggregation start/stop code paths. >>>> >>>> It was also not possible to know how long >>>> byte-wise queue was without dequeueing. >>>> >>>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com> >>> Instead of exposing these in the struct to the driver directly, please >>> make a function to get them. Since the number of frames is already >>> tracked in txqi->queue, you can avoid counter duplication that way. >> >> Hmm, so you suggest to have something like: [...] >>> Also, that way you can fix a race condition between accessing the number >>> of frames counter and the bytes counter. >> >> I don't see a point in maintaining coherency between the two counters >> with regard to each other alone. Do you have a use-case that would >> actually make use of that property? >> >> I'd like to avoid any unnecessary spinlocks. > OK. I guess we can leave them out for now. How frequently are you going > to call this function? Depends, but with new data path in ath10k[1][2] it'll be at least once for each wake_tx_queue() + once for each txq in each fetch-ind event. Micha? [1]: https://patchwork.kernel.org/patch/8081301/ [2]: https://patchwork.kernel.org/patch/8081331/ -- 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
> I don't see a point in maintaining coherency between the two counters > with regard to each other alone. Do you have a use-case that would > actually make use of that property? > > I'd like to avoid any unnecessary spinlocks. > Make sure you document the lack of consistency between the two values then, please. 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
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 566df20dc957..c29ca8be9ac2 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1781,6 +1781,8 @@ struct ieee80211_tx_control { * * @vif: &struct ieee80211_vif pointer from the add_interface callback. * @sta: station table entry, %NULL for per-vif queue + * @qdepth: number of pending frames + * @qsize: number of pending bytes * @tid: the TID for this queue (unused for per-vif queue) * @ac: the AC for this queue * @drv_priv: driver private area, sized by hw->txq_data_size @@ -1791,6 +1793,8 @@ struct ieee80211_tx_control { struct ieee80211_txq { struct ieee80211_vif *vif; struct ieee80211_sta *sta; + int qdepth; + int qsize; u8 tid; u8 ac; diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 0451f120746e..dfcb19080eb0 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -979,6 +979,8 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, spin_lock_bh(&txqi->queue.lock); ieee80211_purge_tx_queue(&local->hw, &txqi->queue); + txqi->txq.qdepth = 0; + txqi->txq.qsize = 0; spin_unlock_bh(&txqi->queue.lock); atomic_set(&sdata->txqs_len[txqi->txq.ac], 0); diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index 7e007cf12cb2..4b93a11f4a0d 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -116,6 +116,8 @@ static void __cleanup_single_sta(struct sta_info *sta) ieee80211_purge_tx_queue(&local->hw, &txqi->queue); atomic_sub(n, &sdata->txqs_len[txqi->txq.ac]); + txqi->txq.qdepth = 0; + txqi->txq.qsize = 0; } } diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 3311ce0f3d6c..6f9a0db3824e 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -1266,7 +1266,13 @@ static void ieee80211_drv_tx(struct ieee80211_local *local, if (atomic_read(&sdata->txqs_len[ac]) >= local->hw.txq_ac_max_pending) netif_stop_subqueue(sdata->dev, ac); - skb_queue_tail(&txqi->queue, skb); + spin_lock_bh(&txqi->queue.lock); + txq->qdepth++; + txq->qsize += skb->len; + + __skb_queue_tail(&txqi->queue, skb); + spin_unlock_bh(&txqi->queue.lock); + drv_wake_tx_queue(local, txqi); return; @@ -1294,6 +1300,9 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw, if (!skb) goto out; + txq->qdepth--; + txq->qsize -= skb->len; + atomic_dec(&sdata->txqs_len[ac]); if (__netif_subqueue_stopped(sdata->dev, ac)) ieee80211_propagate_queue_wake(local, sdata->vif.hw_queue[ac]);
This will allow drivers to make more educated decisions whether to defer transmission or not. Relying on wake_tx_queue() call count implicitly was not possible because it could be called without queued frame count actually changing on software tx aggregation start/stop code paths. It was also not possible to know how long byte-wise queue was without dequeueing. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- include/net/mac80211.h | 4 ++++ net/mac80211/iface.c | 2 ++ net/mac80211/sta_info.c | 2 ++ net/mac80211/tx.c | 11 ++++++++++- 4 files changed, 18 insertions(+), 1 deletion(-)