DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3568F2085B
From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@toke.dk>
To:     Rajkumar Manoharan <rmanohar@codeaurora.org>,
        linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
Cc:     Rajkumar Manoharan <rmanohar@codeaurora.org>
Subject: Re: [PATCH 3/6] mac80211: Add airtime accounting and scheduling to TXQs
In-Reply-To: <1540033534-11211-4-git-send-email-rmanohar@codeaurora.org>
References: <1540033534-11211-1-git-send-email-rmanohar@codeaurora.org> <1540033534-11211-4-git-send-email-rmanohar@codeaurora.org>
Date:   Fri, 26 Oct 2018 16:16:02 +0200
Message-ID: <8736ssbxp9.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

> From: Toke H=C3=B8iland-J=C3=B8rgensen <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.
>
> Signed-off-by: Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk>
> Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org>
> ---
>  include/net/mac80211.h     | 58 ++++++++++++++++++++++++++++++
>  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    | 45 +++++++++++++++++++++--
>  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/include/net/mac80211.h b/include/net/mac80211.h
> index 2f5c0fbd453c..0ced3adb09ac 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -2334,6 +2334,8 @@ enum ieee80211_hw_flags {
>   * @tx_sk_pacing_shift: Pacing shift to set on TCP sockets when frames f=
rom
>   *	them are encountered. The default should typically not be changed,
>   *	unless the driver has good reasons for needing more buffers.
> + *
> + * @airtime_weight: Default airtime weight preferred by driver.
>   */
>  struct ieee80211_hw {
>  	struct ieee80211_conf conf;
> @@ -2370,6 +2372,7 @@ struct ieee80211_hw {
>  	const struct ieee80211_cipher_scheme *cipher_schemes;
>  	u8 max_nan_de_entries;
>  	u8 tx_sk_pacing_shift;
> +	u32 airtime_weight;
>  };

This doesn't make sense. Airtime weights can be set by userspace, so
even if a driver sets another default it is not guaranteed to be
honoured. So what's the point?

> +bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
> +				struct ieee80211_txq *txq)
> +{
> +	struct ieee80211_local *local =3D hw_to_local(hw);
> +	struct txq_info *iter, *tmp, *txqi =3D to_txq_info(txq);
> +	struct sta_info *sta;
> +	u8 ac =3D txq->ac;
> +
> +	lockdep_assert_held(&local->active_txq_lock[ac]);
> +
> +	if (!txqi->txq.sta)
> +		goto out;
> +
> +	if (list_empty(&txqi->schedule_order))
> +		goto out;
> +
> +	list_for_each_entry_safe(iter, tmp, &local->active_txqs[ac],
> +				 schedule_order) {
> +		if (iter =3D=3D txqi)
> +			break;
> +
> +		if (!iter->txq.sta) {
> +			list_move_tail(&iter->schedule_order,
> +				       &local->active_txqs[ac]);
> +			continue;
> +		}
> +		sta =3D container_of(iter->txq.sta, struct sta_info, sta);
> +		if (sta->airtime[ac].deficit < 0)
> +			sta->airtime[ac].deficit +=3D sta->airtime_weight;
> +		list_move_tail(&iter->schedule_order, &local->active_txqs[ac]);
> +	}

So since we're rotating the queue on every call to the function, I'm
wondering if this actually succeeds in throttling the slow station's
airtime usage enough to achieve fairness? So I'll ask again: Did you
test the fairness performance, and how?


Also, taking a step back:

With this, we're doing lots of work to make sure that the hardware's
round-robin scheduling queue lines up with mac80211; so if we do this
anyway, why can't we just control the order directly from mac80211
(which is what we do with the other next_txq() API)?

-Toke