Rajkumar Manoharan <rmanohar@xxxxxxxxxxxxxx> writes:
> Once a txq is selected for transmission by next_txq(), all data from
> txq are dequeued by driver till hardware descriptors are drained.
> i.e the driver is still allowed to dequeue frames from txq even when
> its fairness deficit goes negative during transmission. This breaks
> fairness and also cause inefficient fq-codel in mac80211 when data
> queues are maintained in driver/firmware. To address this problem,
> pause txq transmission immediately upon driver airtime report.
Hmm, interesting observation about the queues moving to mac80211. Not
sure it will actually work that way; depends on how timely the ath10k
airtime report is, I guess. But would be interesting to test! :)
Just one comment on your patch, below.
> +static bool ieee80211_txq_requeued(struct ieee80211_local *local,
> + struct txq_info *txqi)
> +{
> + struct sta_info *sta;
> +
> + lockdep_assert_held(&local->active_txq_lock);
> +
> + if (!txqi->txq.sta)
> + return false;
> +
> + sta = container_of(txqi->txq.sta, struct sta_info, sta);
> +
> + if (sta->airtime.deficit[txqi->txq.ac] > 0) {
> + clear_bit(IEEE80211_TXQ_PAUSE, &txqi->flags);
> + return false;
> + }
> +
> + sta->airtime.deficit[txqi->txq.ac] +=
> + local->airtime_quantum * sta->airtime.weight;
> + list_move_tail(&txqi->schedule_order,
> + &local->active_txqs[txqi->txq.ac]);
> +
> + return true;
> +}
> +
> struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
> bool inc_seqno)
> {
> @@ -3655,18 +3682,8 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
> if (!txqi)
> goto out;
>
> - if (txqi->txq.sta) {
> - struct sta_info *sta = container_of(txqi->txq.sta,
> - struct sta_info, sta);
> -
> - if (sta->airtime.deficit[txqi->txq.ac] < 0) {
> - sta->airtime.deficit[txqi->txq.ac] +=
> - local->airtime_quantum * sta->airtime.weight;
> - list_move_tail(&txqi->schedule_order,
> - &local->active_txqs[txqi->txq.ac]);
> - goto begin;
> - }
> - }
> + if (ieee80211_txq_requeued(local, txqi))
> + goto begin;
>
> /* If seqnos are equal, we've seen this txqi before in this scheduling
> * round, so abort.
> @@ -3687,6 +3704,39 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
> }
> EXPORT_SYMBOL(ieee80211_next_txq);
>
> +bool ieee80211_txq_can_transmit(struct ieee80211_hw *hw,
> + struct ieee80211_txq *txq)
> +{
> + struct ieee80211_local *local = hw_to_local(hw);
> + struct txq_info *txqi, *f_txqi;
> + bool can_tx;
> +
> + txqi = to_txq_info(txq);
> + /* Check whether txq is paused or not */
> + if (test_bit(IEEE80211_TXQ_PAUSE, &txqi->flags))
> + return false;
> +
> + can_tx = false;
> + spin_lock_bh(&local->active_txq_lock);
> + f_txqi = find_txqi(local, txq->ac);
> + if (!f_txqi)
> + goto out;
> +
> + /* Allow only head node to ensure fairness */
> + if (f_txqi != txqi)
> + goto out;
> +
> + /* Check if txq is in negative deficit */
> + if (!ieee80211_txq_requeued(local, txqi))
> + can_tx = true;
> +
> + list_del_init(&txqi->schedule_order);
Why are you removing the txq from the list here, and how do you expect
it to get added back?
-Toke
