From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Yibo Zhao <yiboz@codeaurora.org>, linux-wireless@vger.kernel.org
Cc: ath10k@lists.infradead.org, Yibo Zhao <yiboz@codeaurora.org>
Subject: Re: [PATCH V3 4/4] mac80211: Sync airtime weight sum with per AC synced sta airtime weight together
Date: Mon, 23 Sep 2019 13:00:42 +0200 [thread overview]
Message-ID: <87blvb5lb9.fsf@toke.dk> (raw)
In-Reply-To: <1569223201-1490-5-git-send-email-yiboz@codeaurora.org>
Yibo Zhao <yiboz@codeaurora.org> writes:
> Global airtime weight sum is updated only when txq is added/removed
> from rbtree. If upper layer configures sta weight during high load,
> airtime weight sum will not be updated since txq is most likely on the
> tree. It could a little late for upper layer to reconfigure sta weight
> when txq is already in the rbtree. And thus, incorrect airtime weight sum
> will lead to incorrect global virtual time calculation as well as overflow
> of airtime weight sum during txq removed.
>
> Hence, need to update airtime weight sum upon receiving event for
> configuring sta weight once sta's txq is on the rbtree.
>
> Besides, if airtime weight sum of ACs and sta weight is synced under the
> same per AC lock protection, there can be a very short window causing
> incorrct airtime weight sum calculation as below:
>
> active_txq_lock_VO .
> VO weight sum is syncd .
> sta airtime weight sum is synced .
> active_txq_unlock_VO .
> . .
> active_txq_lock_VI .
> VI weight sum is syncd .
> sta airtime weight sum active_txq_lock_BE
> active_txq_unlock_VI Remove txq and thus sum
> . is calculated with synced
> . sta airtime weight
> . active_txq_unlock_BE
>
> So introduce a per ac synced station airtime weight synced with per
> AC synced weight sum together. And the per-AC station airtime weight
> is used to calculate weight sum.
>
> Signed-off-by: Yibo Zhao <yiboz@codeaurora.org>
> ---
> net/mac80211/cfg.c | 29 ++++++++++++++++++++++++++---
> net/mac80211/debugfs_sta.c | 2 +-
> net/mac80211/sta_info.c | 9 ++++-----
> net/mac80211/sta_info.h | 5 +++--
> net/mac80211/tx.c | 4 ++--
> 5 files changed, 36 insertions(+), 13 deletions(-)
>
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index d65aa01..c8a0683 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -1284,7 +1284,8 @@ static int sta_apply_parameters(struct ieee80211_local *local,
> int ret = 0;
> struct ieee80211_supported_band *sband;
> struct ieee80211_sub_if_data *sdata = sta->sdata;
> - u32 mask, set;
> + u32 mask, set, tid, ac, pre_weight;
> + struct txq_info *txqi;
>
> sband = ieee80211_get_sband(sdata);
> if (!sband)
> @@ -1452,8 +1453,30 @@ static int sta_apply_parameters(struct ieee80211_local *local,
> if (ieee80211_vif_is_mesh(&sdata->vif))
> sta_apply_mesh_params(local, sta, params);
>
> - if (params->airtime_weight)
> - sta->airtime_weight = params->airtime_weight;
> + if (params->airtime_weight &&
> + params->airtime_weight != sta->airtime_weight) {
This check doesn't work I think? You're not using the array-based
sta->airtime_weight[], and there are locking issues by just checking
like this; so maybe just keep the if() on params->airtime_weight, and do
the checking inside the loop while holding the lock?
Or could we just turn the weights into atomics to avoid the locking
entirely?
> + for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
> + spin_lock_bh(&local->active_txq_lock[ac]);
> + for (tid = 0; tid < IEEE80211_NUM_TIDS + 1; tid++) {
> + if (!sta->sta.txq[tid] ||
> + ac != ieee80211_ac_from_tid(tid))
> + continue;
> +
> + pre_weight = sta->airtime_weight[ac];
> + sta->airtime_weight[ac] =
> + params->airtime_weight;
> +
> + txqi = to_txq_info(sta->sta.txq[tid]);
> + if (RB_EMPTY_NODE(&txqi->schedule_order))
> + continue;
> +
> + local->airtime_weight_sum[ac] = local->airtime_weight_sum[ac] +
> + params->airtime_weight -
> + pre_weight;
> + }
> + spin_unlock_bh(&local->active_txq_lock[ac]);
> + }
> + }
>
> /* set the STA state after all sta info from usermode has been set */
> if (test_sta_flag(sta, WLAN_STA_TDLS_PEER) ||
> diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
> index 80028da..43a7e6a 100644
> --- a/net/mac80211/debugfs_sta.c
> +++ b/net/mac80211/debugfs_sta.c
> @@ -223,7 +223,7 @@ static ssize_t sta_airtime_read(struct file *file, char __user *userbuf,
> "Virt-T: VO: %lld us VI: %lld us BE: %lld us BK: %lld us\n",
> rx_airtime,
> tx_airtime,
> - sta->airtime_weight,
> + sta->airtime_weight[0],
> v_t[0],
> v_t[1],
> v_t[2],
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index feac975..e599cf1 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -384,11 +384,10 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
> if (sta_prepare_rate_control(local, sta, gfp))
> goto free_txq;
>
> - sta->airtime_weight = IEEE80211_DEFAULT_AIRTIME_WEIGHT;
> -
> for (i = 0; i < IEEE80211_NUM_ACS; i++) {
> skb_queue_head_init(&sta->ps_tx_buf[i]);
> skb_queue_head_init(&sta->tx_filtered[i]);
> + sta->airtime_weight[i] = IEEE80211_DEFAULT_AIRTIME_WEIGHT;
> }
>
> for (i = 0; i < IEEE80211_NUM_TIDS; i++)
> @@ -1850,11 +1849,11 @@ void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
> sta->airtime[ac].tx_airtime += tx_airtime;
> sta->airtime[ac].rx_airtime += rx_airtime;
>
> - weight_sum = local->airtime_weight_sum[ac] ?: sta->airtime_weight;
> + weight_sum = local->airtime_weight_sum[ac] ?: sta->airtime_weight[ac];
>
> /* Round the calculation of global vt */
> local->airtime_v_t[ac] += (airtime + (weight_sum >> 1)) / weight_sum;
> - sta->airtime[ac].v_t += airtime / sta->airtime_weight;
> + sta->airtime[ac].v_t += airtime / sta->airtime_weight[ac];
> ieee80211_resort_txq(&local->hw, txq);
>
> spin_unlock_bh(&local->active_txq_lock[ac]);
> @@ -2236,7 +2235,7 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
> }
>
> if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_AIRTIME_WEIGHT))) {
> - sinfo->airtime_weight = sta->airtime_weight;
> + sinfo->airtime_weight = sta->airtime_weight[0];
> sinfo->filled |= BIT_ULL(NL80211_STA_INFO_AIRTIME_WEIGHT);
> }
>
> diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
> index 5055f94..2697343 100644
> --- a/net/mac80211/sta_info.h
> +++ b/net/mac80211/sta_info.h
> @@ -476,7 +476,8 @@ struct ieee80211_sta_rx_stats {
> * @tid_seq: per-TID sequence numbers for sending to this STA
> * @airtime: per-AC struct airtime_info describing airtime statistics for this
> * station
> - * @airtime_weight: station weight for airtime fairness calculation purposes
> + * @airtime_weight: station per-AC weight for airtime fairness calculation
> + * purposes
> * @ampdu_mlme: A-MPDU state machine state
> * @mesh: mesh STA information
> * @debugfs_dir: debug filesystem directory dentry
> @@ -602,7 +603,7 @@ struct sta_info {
> u16 tid_seq[IEEE80211_QOS_CTL_TID_MASK + 1];
>
> struct airtime_info airtime[IEEE80211_NUM_ACS];
> - u16 airtime_weight;
> + u16 airtime_weight[IEEE80211_NUM_ACS];
>
> /*
> * Aggregation information, locked with lock.
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 60cf569..286cf14 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -3735,7 +3735,7 @@ void ieee80211_schedule_txq(struct ieee80211_hw *hw,
> struct sta_info *sta = container_of(txq->sta,
> struct sta_info, sta);
>
> - local->airtime_weight_sum[ac] += sta->airtime_weight;
> + local->airtime_weight_sum[ac] += sta->airtime_weight[ac];
> if (local->airtime_v_t[ac] > AIRTIME_GRACE)
> sta->airtime[ac].v_t = max(local->airtime_v_t[ac] - AIRTIME_GRACE,
> sta->airtime[ac].v_t);
> @@ -3779,7 +3779,7 @@ static void __ieee80211_unschedule_txq(struct ieee80211_hw *hw,
> struct sta_info *sta = container_of(txq->sta,
> struct sta_info, sta);
>
> - local->airtime_weight_sum[ac] -= sta->airtime_weight;
> + local->airtime_weight_sum[ac] -= sta->airtime_weight[ac];
> }
>
> rb_erase_cached(&txqi->schedule_order,
> --
> 1.9.1
next prev parent reply other threads:[~2019-09-23 11:00 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-23 7:19 [PATCH V3 0/4] Enable virtual time-based airtime scheduler support on ath10k Yibo Zhao
2019-09-23 7:19 ` [PATCH V3 1/4] mac80211: Switch to a virtual time-based airtime scheduler Yibo Zhao
2019-09-23 7:19 ` [PATCH V3 2/4] mac80211: defer txqs removal from rbtree Yibo Zhao
2019-09-23 10:56 ` Toke Høiland-Jørgensen
2019-09-24 2:55 ` Yibo Zhao
2019-09-23 7:20 ` [PATCH V3 3/4] mac80211: fix low throughput in multi-clients situation Yibo Zhao
2019-09-23 10:55 ` Toke Høiland-Jørgensen
2019-09-24 8:22 ` Yibo Zhao
2019-09-24 8:48 ` Toke Høiland-Jørgensen
2019-09-24 8:58 ` Yibo Zhao
2019-09-23 7:20 ` [PATCH V3 4/4] mac80211: Sync airtime weight sum with per AC synced sta airtime weight together Yibo Zhao
2019-09-23 11:00 ` Toke Høiland-Jørgensen [this message]
2019-09-24 3:19 ` Yibo Zhao
2019-09-24 7:27 ` Toke Høiland-Jørgensen
2019-10-01 10:19 ` [PATCH V3 0/4] Enable virtual time-based airtime scheduler support on ath10k Johannes Berg
2019-10-01 10:59 ` Yibo Zhao
2019-10-01 11:05 ` Toke Høiland-Jørgensen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87blvb5lb9.fsf@toke.dk \
--to=toke@redhat.com \
--cc=ath10k@lists.infradead.org \
--cc=linux-wireless@vger.kernel.org \
--cc=yiboz@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).