Re: [PATCH v3 3/6] mac80211: Add airtime accounting and scheduling to TXQs

From: Felix Fietkau <nbd@nbd.name>
To: Rajkumar Manoharan <rmanohar@codeaurora.org>,
	linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
Cc: make-wifi-fast@lists.bufferbloat.net,
	"Toke Høiland-Jørgensen" <toke@toke.dk>
Subject: Re: [PATCH v3 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Date: Wed, 14 Nov 2018 11:57:51 +0100	[thread overview]
Message-ID: <e5344cad-2a18-a938-f2ed-43870027251c@nbd.name> (raw)
In-Reply-To: <1542063113-22438-4-git-send-email-rmanohar@codeaurora.org>

On 2018-11-12 23:51, Rajkumar Manoharan wrote:
> From: Toke Høiland-Jørgensen <toke@toke.dk>
> 
> This adds airtime accounting and scheduling to the mac80211 TXQ
> scheduler. A new callback, ieee80211_sta_register_airtime(), is added
> that drivers can call to report airtime usage for stations.
> 
> When airtime information is present, mac80211 will schedule TXQs
> (through ieee80211_next_txq()) in a way that enforces airtime fairness
> between active stations. This scheduling works the same way as the ath9k
> in-driver airtime fairness scheduling. If no airtime usage is reported
> by the driver, the scheduler will default to round-robin scheduling.
> 
> For drivers that don't control TXQ scheduling in software, a new API
> function, ieee80211_txq_may_transmit(), is added which the driver can use
> to check if the TXQ is eligible for transmission, or should be throttled to
> enforce fairness. Calls to this function must also be enclosed in
> ieee80211_txq_schedule_{start,end}() calls to ensure proper locking.
> 
> The API ieee80211_txq_may_transmit() also ensures that TXQ list will be
> aligned aginst driver's own round-robin scheduler list. i.e it rotates
> the TXQ list till it makes the requested node becomes the first entry
> in TXQ list. Thus both the TXQ list and driver's list are in sync.
> 
> Co-Developed-by: Rajkumar Manoharan <rmanohar@codeaurora.org>
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org>
> ---
>  include/net/mac80211.h     | 59 ++++++++++++++++++++++++++++++
>  net/mac80211/cfg.c         |  3 ++
>  net/mac80211/debugfs.c     |  3 ++
>  net/mac80211/debugfs_sta.c | 50 ++++++++++++++++++++++++--
>  net/mac80211/ieee80211_i.h |  2 ++
>  net/mac80211/main.c        |  4 +++
>  net/mac80211/sta_info.c    | 44 +++++++++++++++++++++--
>  net/mac80211/sta_info.h    | 13 +++++++
>  net/mac80211/status.c      |  6 ++++
>  net/mac80211/tx.c          | 90 +++++++++++++++++++++++++++++++++++++++++++---
>  10 files changed, 264 insertions(+), 10 deletions(-)
> 
> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
> index aa4afbf0abaf..a1f1256448f5 100644
> --- a/net/mac80211/status.c
> +++ b/net/mac80211/status.c
> @@ -818,6 +818,12 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
>  			ieee80211_sta_tx_notify(sta->sdata, (void *) skb->data,
>  						acked, info->status.tx_time);
>  
> +		if (info->status.tx_time &&
> +		    wiphy_ext_feature_isset(local->hw.wiphy,
> +					    NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
> +			ieee80211_sta_register_airtime(&sta->sta, tid,
> +						       info->status.tx_time, 0);
> +
>  		if (ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS)) {
>  			if (info->flags & IEEE80211_TX_STAT_ACK) {
>  				if (sta->status_stats.lost_packets)
I think the same is needed in ieee80211_tx_status_ext.

> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 305965283506..3f417e80e041 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -3660,12 +3680,74 @@ void ieee80211_return_txq(struct ieee80211_hw *hw,
>  	lockdep_assert_held(&local->active_txq_lock[txq->ac]);
>  
>  	if (list_empty(&txqi->schedule_order) &&
> -	    (!skb_queue_empty(&txqi->frags) || txqi->tin.backlog_packets))
> -		list_add_tail(&txqi->schedule_order,
> -			      &local->active_txqs[txq->ac]);
> +	    (!skb_queue_empty(&txqi->frags) || txqi->tin.backlog_packets)) {
> +		/* If airtime accounting is active, always enqueue STAs at the
> +		 * head of the list to ensure that they only get moved to the
> +		 * back by the airtime DRR scheduler once they have a negative
> +		 * deficit. A station that already has a negative deficit will
> +		 * get immediately moved to the back of the list on the next
> +		 * call to ieee80211_next_txq().
> +		 */
> +		if (txqi->txq.sta &&
> +		    wiphy_ext_feature_isset(local->hw.wiphy,
> +					    NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
> +			list_add(&txqi->schedule_order,
> +				 &local->active_txqs[txq->ac]);
> +		else
> +			list_add_tail(&txqi->schedule_order,
> +				      &local->active_txqs[txq->ac]);
> +	}
>  }
This part doesn't really make much sense to me, but maybe I'm
misunderstanding how the code works.
Let's assume we have a driver like ath9k or mt76, which tries to keep a
number of aggregates in the hardware queue, and the hardware queue is
currently empty.
If the current txq entry is kept at the head of the schedule list,
wouldn't the code just pull from that one over and over again, until
enough packets are transmitted by the hardware and their tx status
processed?
It seems to me that while fairness is still preserved in the long run,
this could lead to rather bursty scheduling, which may not be
particularly latency friendly.

- Felix