diff mbox series

[V2,3/4] mac80211: fix low throughput in multi-clients situation

Message ID 1568800033-17297-3-git-send-email-yiboz@codeaurora.org (mailing list archive)
State Superseded
Delegated to: Johannes Berg
Headers show
Series [V2,1/4] mac80211: Switch to a virtual time-based airtime scheduler | expand

Commit Message

Yibo Zhao Sept. 18, 2019, 9:47 a.m. UTC 
Not long after the start of multi-clients test, not a single station is
an eligible candidate for transmission since global virtual time(g_vt) is
smaller than the virtual airtime(s_vt) of all the stations. As a result,
the Tx has been blocked and throughput is quite low.

This may mainly due to sync mechanism and accumulative deviation from the
devision calculation of g_vt.

For example:
Suppose we have 50 clients in first round.
Round 1:
STA	weight	Tx_time_round  wt_sum	s_vt	g_vt  valid_for_next_Tx
.	.	.			.		.
.	.	.			.		.
.	.	.			.		.

After this round, all the stations are not valid for next transmission due
to accumulative deviation.

And if we add a new #51,
Round 2:
STA	weight	Tx_time_round	wt_sum  s_vt	g_vt  valid_for_next_Tx
.	.	.			.		.
.	.	.			.		.
.	.	.			.		.

Sync is done by:
max(g_vt of last round - grace period, s_vt)
and s_vt of #51 = max(2000 - 500, 0) + 1024 = 2524, and it is more than the
final g_vt of this round.

After this round, no more station is valid for transmission.

The real situation can be more complicate, above is one of the extremely
case.

To avoid this situation to occur, the new proposal is:

- Increase the airtime grace period a little more to reduce the
  unexpected sync

- If global virtual time is less than the virtual airtime of any station,
  sync it to the airtime of first station in the red-black tree

- Round the division result

Co-developed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Signed-off-by: Yibo Zhao <yiboz@codeaurora.org>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 net/mac80211/sta_info.c |  3 ++-
 net/mac80211/sta_info.h |  2 +-
 net/mac80211/tx.c       | 16 +++++++++++++++-
 3 files changed, 18 insertions(+), 3 deletions(-)

Comments

Toke Høiland-Jørgensen Sept. 18, 2019, 9:59 a.m. UTC | #1 
Yibo Zhao <yiboz@codeaurora.org> writes:

> Not long after the start of multi-clients test, not a single station is
> an eligible candidate for transmission since global virtual time(g_vt) is
> smaller than the virtual airtime(s_vt) of all the stations. As a result,
> the Tx has been blocked and throughput is quite low.
>
> This may mainly due to sync mechanism and accumulative deviation from the
> devision calculation of g_vt.
>
> For example:
> Suppose we have 50 clients in first round.
> Round 1:
> STA	weight	Tx_time_round  wt_sum	s_vt	g_vt  valid_for_next_Tx
> .	.	.			.		.
> .	.	.			.		.
> .	.	.			.		.
>
> After this round, all the stations are not valid for next transmission due
> to accumulative deviation.
>
> And if we add a new #51,
> Round 2:
> STA	weight	Tx_time_round	wt_sum  s_vt	g_vt  valid_for_next_Tx
> .	.	.			.		.
> .	.	.			.		.
> .	.	.			.		.
>
> Sync is done by:
> max(g_vt of last round - grace period, s_vt)
> and s_vt of #51 = max(2000 - 500, 0) + 1024 = 2524, and it is more than the
> final g_vt of this round.
>
> After this round, no more station is valid for transmission.

I'm not sure I understand this. Was there supposed to be numbers in
those tables above?

-Toke
Yibo Zhao Sept. 18, 2019, 10:29 a.m. UTC | #2 
On 2019-09-18 17:59, Toke Høiland-Jørgensen wrote:
> Yibo Zhao <yiboz@codeaurora.org> writes:
> 
>> Not long after the start of multi-clients test, not a single station 
>> is
>> an eligible candidate for transmission since global virtual time(g_vt) 
>> is
>> smaller than the virtual airtime(s_vt) of all the stations. As a 
>> result,
>> the Tx has been blocked and throughput is quite low.
>> 
>> This may mainly due to sync mechanism and accumulative deviation from 
>> the
>> devision calculation of g_vt.
>> 
>> For example:
>> Suppose we have 50 clients in first round.
>> Round 1:
>> STA	weight	Tx_time_round  wt_sum	s_vt	g_vt  valid_for_next_Tx
>> .	.	.			.		.
>> .	.	.			.		.
>> .	.	.			.		.
>> 
>> After this round, all the stations are not valid for next transmission 
>> due
>> to accumulative deviation.
>> 
>> And if we add a new #51,
>> Round 2:
>> STA	weight	Tx_time_round	wt_sum  s_vt	g_vt  valid_for_next_Tx
>> .	.	.			.		.
>> .	.	.			.		.
>> .	.	.			.		.
>> 
>> Sync is done by:
>> max(g_vt of last round - grace period, s_vt)
>> and s_vt of #51 = max(2000 - 500, 0) + 1024 = 2524, and it is more 
>> than the
>> final g_vt of this round.
>> 
>> After this round, no more station is valid for transmission.
> 
> I'm not sure I understand this. Was there supposed to be numbers in
> those tables above?
Yes, it looks like there are some display issues. Will fix it in next 
version.
> 
> -Toke
diff mbox series

Patch

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 9d01fdd..feac975 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1852,7 +1852,8 @@  void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
 
 	weight_sum = local->airtime_weight_sum[ac] ?: sta->airtime_weight;
 
-	local->airtime_v_t[ac] += airtime / weight_sum;
+	/* 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;
 	ieee80211_resort_txq(&local->hw, txq);
 
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 5c1cac9..5055f94 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -130,7 +130,7 @@  enum ieee80211_agg_stop_reason {
 /* Debugfs flags to enable/disable use of RX/TX airtime in scheduler */
 #define AIRTIME_USE_TX		BIT(0)
 #define AIRTIME_USE_RX		BIT(1)
-#define AIRTIME_GRACE 500 /* usec of grace period before reset */
+#define AIRTIME_GRACE 2000 /* usec of grace period before reset */
 
 struct airtime_info {
 	u64 rx_airtime;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 42ca010..60cf569 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3867,15 +3867,29 @@  bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
 				struct ieee80211_txq *txq)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
-	struct txq_info *txqi = to_txq_info(txq);
+	struct txq_info *first_txqi, *txqi = to_txq_info(txq);
+	struct rb_node *node = NULL;
 	struct sta_info *sta;
 	u8 ac = txq->ac;
+	first_txqi = NULL;
 
 	lockdep_assert_held(&local->active_txq_lock[ac]);
 
 	if (!txqi->txq.sta)
 		return true;
 
+	node = rb_first_cached(&local->active_txqs[ac]);
+	if (node) {
+		first_txqi = container_of(node, struct txq_info,
+					  schedule_order);
+		if (first_txqi->txq.sta) {
+			sta = container_of(first_txqi->txq.sta,
+					   struct sta_info, sta);
+			if (local->airtime_v_t[ac] < sta->airtime[ac].v_t)
+				local->airtime_v_t[ac] = sta->airtime[ac].v_t;
+		}
+	}
+
 	sta = container_of(txqi->txq.sta, struct sta_info, sta);
 	return (sta->airtime[ac].v_t <= local->airtime_v_t[ac]);
 }