diff mbox series

[3/4] mac80211: fix low throughput in push pull mode

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

Commit Message

Yibo Zhao Sept. 16, 2019, 1:09 p.m. UTC
If station is ineligible for transmission in ieee80211_txq_may_transmit(),
no packet will be delivered to FW. During the tests in push-pull mode with
many clients, after several seconds, not a single station is an eligible
candidate for transmission since global time is smaller than all the
station's virtual airtime. As a consequence, the Tx has been blocked and
throughput is quite low.

To avoid this situation to occur in push-pull mode, 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 since the process of global virtual time
  involves the division calculation

Co-developed-by: Yibo Zhao <yiboz@codeaurora.org>
Signed-off-by: Yibo Zhao <yiboz@codeaurora.org>
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 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

Johannes Berg Sept. 16, 2019, 3:27 p.m. UTC | #1
Without really looking at the code - 

> If station is ineligible for transmission in ieee80211_txq_may_transmit(),
> no packet will be delivered to FW. During the tests in push-pull mode with
> many clients, after several seconds, not a single station is an eligible
> candidate for transmission since global time is smaller than all the
> station's virtual airtime. As a consequence, the Tx has been blocked and
> throughput is quite low.

You should rewrite this to be, erm, a bit more understandable in
mac80211 context. I assume you're speaking (mostly?) about ath10k, but I
have very little context there. "push pull mode"? "firmware"? These
things are not something mac80211 knows about.

> Co-developed-by: Yibo Zhao <yiboz@codeaurora.org>

That also seems wrong, should be Toke I guess, unless you intended for a
From: Toke to be present?

johannes
Yibo Zhao Sept. 17, 2019, 6:36 a.m. UTC | #2
On 2019-09-16 23:27, Johannes Berg wrote:
> Without really looking at the code -
> 
>> If station is ineligible for transmission in 
>> ieee80211_txq_may_transmit(),
>> no packet will be delivered to FW. During the tests in push-pull mode 
>> with
>> many clients, after several seconds, not a single station is an 
>> eligible
>> candidate for transmission since global time is smaller than all the
>> station's virtual airtime. As a consequence, the Tx has been blocked 
>> and
>> throughput is quite low.
> 
> You should rewrite this to be, erm, a bit more understandable in
> mac80211 context. I assume you're speaking (mostly?) about ath10k, but 
> I
> have very little context there. "push pull mode"? "firmware"? These
> things are not something mac80211 knows about.
Hi Johannes,

Thanks for your kindly reminder. Will rewrite the commit log.

> 
>> Co-developed-by: Yibo Zhao <yiboz@codeaurora.org>
> 
> That also seems wrong, should be Toke I guess, unless you intended for 
> a
> From: Toke to be present?
Do you mean it should be something like:

Co-developed-by: Toke Høiland-Jørgensen <toke@toke.dk>
Signed-off-by: Yibo Zhao <yiboz@codeaurora.org>
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>

Am I understanding right?
> 
> johannes
Johannes Berg Sept. 17, 2019, 6:55 a.m. UTC | #3
On Tue, 2019-09-17 at 14:36 +0800, Yibo Zhao wrote:
> 
> Do you mean it should be something like:
> 
> Co-developed-by: Toke Høiland-Jørgensen <toke@toke.dk>
> Signed-off-by: Yibo Zhao <yiboz@codeaurora.org>
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>

Yes, I think you mean the right thing. For the record, it seems to me it
should be

From: A <...>

[...]

Co-developed-by: B <...>
Signed-off-by: B <...>
Signed-off-by: A <...>

or so.

IOW, I think having the same "From:" (which gets preserved in git as
"Author") and "Co-developed-by" makes no sense?

Your "From" line was implied, but I suppose you did mean that From would
be yourself (as it was in the patch) and then the above seems right.

Or you can add a "From: Toke ..." to your patch message and leave the
"Co-developed-by: yourself" I suppose, the difference is in how git will
record it.

johannes
Toke Høiland-Jørgensen Sept. 17, 2019, 9:12 p.m. UTC | #4
Yibo Zhao <yiboz@codeaurora.org> writes:

> On 2019-09-16 23:27, Johannes Berg wrote:
>> Without really looking at the code -
>> 
>>> If station is ineligible for transmission in 
>>> ieee80211_txq_may_transmit(),
>>> no packet will be delivered to FW. During the tests in push-pull mode 
>>> with
>>> many clients, after several seconds, not a single station is an 
>>> eligible
>>> candidate for transmission since global time is smaller than all the
>>> station's virtual airtime. As a consequence, the Tx has been blocked 
>>> and
>>> throughput is quite low.
>> 
>> You should rewrite this to be, erm, a bit more understandable in
>> mac80211 context. I assume you're speaking (mostly?) about ath10k, but 
>> I
>> have very little context there. "push pull mode"? "firmware"? These
>> things are not something mac80211 knows about.
> Hi Johannes,
>
> Thanks for your kindly reminder. Will rewrite the commit log.
>
>> 
>>> Co-developed-by: Yibo Zhao <yiboz@codeaurora.org>
>> 
>> That also seems wrong, should be Toke I guess, unless you intended for 
>> a
>> From: Toke to be present?
> Do you mean it should be something like:
>
> Co-developed-by: Toke Høiland-Jørgensen <toke@toke.dk>
> Signed-off-by: Yibo Zhao <yiboz@codeaurora.org>
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>
> Am I understanding right?

I think the right thing here, as with the previous patch, is to just
drop my sign-off; you're writing this patch, and I'll add ack/reviews as
appropriate. And in that case, well, no need to have co-developed-by
yourself when your name is on the patch as author :)

-Toke
Yibo Zhao Sept. 18, 2019, 10:02 a.m. UTC | #5
On 2019-09-18 05:12, Toke Høiland-Jørgensen wrote:
> Yibo Zhao <yiboz@codeaurora.org> writes:
> 
>> On 2019-09-16 23:27, Johannes Berg wrote:
>>> Without really looking at the code -
>>> 
>>>> If station is ineligible for transmission in
>>>> ieee80211_txq_may_transmit(),
>>>> no packet will be delivered to FW. During the tests in push-pull 
>>>> mode
>>>> with
>>>> many clients, after several seconds, not a single station is an
>>>> eligible
>>>> candidate for transmission since global time is smaller than all the
>>>> station's virtual airtime. As a consequence, the Tx has been blocked
>>>> and
>>>> throughput is quite low.
>>> 
>>> You should rewrite this to be, erm, a bit more understandable in
>>> mac80211 context. I assume you're speaking (mostly?) about ath10k, 
>>> but
>>> I
>>> have very little context there. "push pull mode"? "firmware"? These
>>> things are not something mac80211 knows about.
>> Hi Johannes,
>> 
>> Thanks for your kindly reminder. Will rewrite the commit log.
>> 
>>> 
>>>> Co-developed-by: Yibo Zhao <yiboz@codeaurora.org>
>>> 
>>> That also seems wrong, should be Toke I guess, unless you intended 
>>> for
>>> a
>>> From: Toke to be present?
>> Do you mean it should be something like:
>> 
>> Co-developed-by: Toke Høiland-Jørgensen <toke@toke.dk>
>> Signed-off-by: Yibo Zhao <yiboz@codeaurora.org>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>> 
>> Am I understanding right?
> 
> I think the right thing here, as with the previous patch, is to just
> drop my sign-off; you're writing this patch, and I'll add ack/reviews 
> as
> appropriate. And in that case, well, no need to have co-developed-by
> yourself when your name is on the patch as author :)
> 
> -Toke
Sorry, I think I have missed checking your reply, please ignore the 
wrong signed-off in PATCH-V2.
Toke Høiland-Jørgensen Sept. 18, 2019, 10:16 a.m. UTC | #6
Yibo Zhao <yiboz@codeaurora.org> writes:

> On 2019-09-18 05:12, Toke Høiland-Jørgensen wrote:
>> Yibo Zhao <yiboz@codeaurora.org> writes:
>> 
>>> On 2019-09-16 23:27, Johannes Berg wrote:
>>>> Without really looking at the code -
>>>> 
>>>>> If station is ineligible for transmission in
>>>>> ieee80211_txq_may_transmit(),
>>>>> no packet will be delivered to FW. During the tests in push-pull 
>>>>> mode
>>>>> with
>>>>> many clients, after several seconds, not a single station is an
>>>>> eligible
>>>>> candidate for transmission since global time is smaller than all the
>>>>> station's virtual airtime. As a consequence, the Tx has been blocked
>>>>> and
>>>>> throughput is quite low.
>>>> 
>>>> You should rewrite this to be, erm, a bit more understandable in
>>>> mac80211 context. I assume you're speaking (mostly?) about ath10k, 
>>>> but
>>>> I
>>>> have very little context there. "push pull mode"? "firmware"? These
>>>> things are not something mac80211 knows about.
>>> Hi Johannes,
>>> 
>>> Thanks for your kindly reminder. Will rewrite the commit log.
>>> 
>>>> 
>>>>> Co-developed-by: Yibo Zhao <yiboz@codeaurora.org>
>>>> 
>>>> That also seems wrong, should be Toke I guess, unless you intended 
>>>> for
>>>> a
>>>> From: Toke to be present?
>>> Do you mean it should be something like:
>>> 
>>> Co-developed-by: Toke Høiland-Jørgensen <toke@toke.dk>
>>> Signed-off-by: Yibo Zhao <yiboz@codeaurora.org>
>>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>>> 
>>> Am I understanding right?
>> 
>> I think the right thing here, as with the previous patch, is to just
>> drop my sign-off; you're writing this patch, and I'll add ack/reviews 
>> as
>> appropriate. And in that case, well, no need to have co-developed-by
>> yourself when your name is on the patch as author :)
>> 
>> -Toke
> Sorry, I think I have missed checking your reply, please ignore the 
> wrong signed-off in PATCH-V2.

While you're re-spinning, could you please add a changelog for the
changes you make? Makes it easier to keep track :)

You can add a cover-letter with a full changelog instead of having a
separate changelog for each patch; that's what I usually do...

-Toke
Yibo Zhao Sept. 18, 2019, 10:18 a.m. UTC | #7
On 2019-09-18 18:16, Toke Høiland-Jørgensen wrote:
> Yibo Zhao <yiboz@codeaurora.org> writes:
> 
>> On 2019-09-18 05:12, Toke Høiland-Jørgensen wrote:
>>> Yibo Zhao <yiboz@codeaurora.org> writes:
>>> 
>>>> On 2019-09-16 23:27, Johannes Berg wrote:
>>>>> Without really looking at the code -
>>>>> 
>>>>>> If station is ineligible for transmission in
>>>>>> ieee80211_txq_may_transmit(),
>>>>>> no packet will be delivered to FW. During the tests in push-pull
>>>>>> mode
>>>>>> with
>>>>>> many clients, after several seconds, not a single station is an
>>>>>> eligible
>>>>>> candidate for transmission since global time is smaller than all 
>>>>>> the
>>>>>> station's virtual airtime. As a consequence, the Tx has been 
>>>>>> blocked
>>>>>> and
>>>>>> throughput is quite low.
>>>>> 
>>>>> You should rewrite this to be, erm, a bit more understandable in
>>>>> mac80211 context. I assume you're speaking (mostly?) about ath10k,
>>>>> but
>>>>> I
>>>>> have very little context there. "push pull mode"? "firmware"? These
>>>>> things are not something mac80211 knows about.
>>>> Hi Johannes,
>>>> 
>>>> Thanks for your kindly reminder. Will rewrite the commit log.
>>>> 
>>>>> 
>>>>>> Co-developed-by: Yibo Zhao <yiboz@codeaurora.org>
>>>>> 
>>>>> That also seems wrong, should be Toke I guess, unless you intended
>>>>> for
>>>>> a
>>>>> From: Toke to be present?
>>>> Do you mean it should be something like:
>>>> 
>>>> Co-developed-by: Toke Høiland-Jørgensen <toke@toke.dk>
>>>> Signed-off-by: Yibo Zhao <yiboz@codeaurora.org>
>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>>>> 
>>>> Am I understanding right?
>>> 
>>> I think the right thing here, as with the previous patch, is to just
>>> drop my sign-off; you're writing this patch, and I'll add ack/reviews
>>> as
>>> appropriate. And in that case, well, no need to have co-developed-by
>>> yourself when your name is on the patch as author :)
>>> 
>>> -Toke
>> Sorry, I think I have missed checking your reply, please ignore the
>> wrong signed-off in PATCH-V2.
> 
> While you're re-spinning, could you please add a changelog for the
> changes you make? Makes it easier to keep track :)
> 
> You can add a cover-letter with a full changelog instead of having a
> separate changelog for each patch; that's what I usually do...
> 
> -Toke
Sure, thanks.
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]);
 }