diff mbox series

[2/4] mac80211: fix issue in loop scenario

Message ID 1576221593-1086-3-git-send-email-yiboz@codeaurora.org (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series Enable virtual time-based airtime scheduler support on ath10k | expand

Commit Message

Yibo Zhao Dec. 13, 2019, 7:19 a.m. UTC
In a loop txqs dequeue scenario, if the first txq in the rbtree gets
removed from rbtree immediately in the ieee80211_return_txq(), the
loop will break soon in the ieee80211_next_txq() due to schedule_pos
not leading to the second txq in the rbtree. Thus update schedule_pos
to previous node once the node of schedule_pos is either removed from
rbtree or move to other location in rbtree due to airtime update.

Signed-off-by: Yibo Zhao <yiboz@codeaurora.org>
---
 net/mac80211/ieee80211_i.h |  2 ++
 net/mac80211/tx.c          | 14 +++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

Comments

Johannes Berg Dec. 13, 2019, 9:56 a.m. UTC | #1
On Fri, 2019-12-13 at 15:19 +0800, Yibo Zhao wrote:
> In a loop txqs dequeue scenario, if the first txq in the rbtree gets
> removed from rbtree immediately in the ieee80211_return_txq(), the
> loop will break soon in the ieee80211_next_txq() due to schedule_pos
> not leading to the second txq in the rbtree. Thus update schedule_pos
> to previous node once the node of schedule_pos is either removed from
> rbtree or move to other location in rbtree due to airtime update.

For my understanding - this is a fix to the first patch in the series?

I guess you didn't squash it because that's Toke's patch or something?

I tend to think you still should, and annotate the changes, but I wanted
to understand it first.

johannes
Yibo Zhao Dec. 18, 2019, 10:12 a.m. UTC | #2
在 2019-12-13 17:56,Johannes Berg 写道:
> On Fri, 2019-12-13 at 15:19 +0800, Yibo Zhao wrote:
>> In a loop txqs dequeue scenario, if the first txq in the rbtree gets
>> removed from rbtree immediately in the ieee80211_return_txq(), the
>> loop will break soon in the ieee80211_next_txq() due to schedule_pos
>> not leading to the second txq in the rbtree. Thus update schedule_pos
>> to previous node once the node of schedule_pos is either removed from
>> rbtree or move to other location in rbtree due to airtime update.
> 
> For my understanding - this is a fix to the first patch in the series?
> 
> I guess you didn't squash it because that's Toke's patch or something?
> 
> I tend to think you still should, and annotate the changes, but I 
> wanted
> to understand it first.

Hi Johannes,

Yes, this is a fix to the first patch. Actually, the rest of two patches 
are also serve the same. So, are you suggesting to merge them to the 
first patch?

Previouly, I had added Toke's signature in this patch but Toke advised 
to delete it. I feel a little bit confused about how to handle it. 
Appreciate any detail guide.
> 
> johannes
Johannes Berg Dec. 18, 2019, 10:18 a.m. UTC | #3
On Wed, 2019-12-18 at 18:12 +0800, yiboz@codeaurora.org wrote:
> 
> Yes, this is a fix to the first patch. Actually, the rest of two patches 
> are also serve the same. So, are you suggesting to merge them to the 
> first patch?

Yes.

> Previouly, I had added Toke's signature in this patch but Toke advised 
> to delete it. I feel a little bit confused about how to handle it. 
> Appreciate any detail guide.

I guess that you have to discuss that with Toke, how he wants to handle
it ... If he gave you a patch with his signed-off-by, then you should
probably keep it? There's also "Co-developed-by" tag to give some sort
of non-author developer credits.

johannes
Toke Høiland-Jørgensen Dec. 18, 2019, 10:37 a.m. UTC | #4
Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2019-12-18 at 18:12 +0800, yiboz@codeaurora.org wrote:
>> 
>> Yes, this is a fix to the first patch. Actually, the rest of two patches 
>> are also serve the same. So, are you suggesting to merge them to the 
>> first patch?
>
> Yes.
>
>> Previouly, I had added Toke's signature in this patch but Toke advised 
>> to delete it. I feel a little bit confused about how to handle it. 
>> Appreciate any detail guide.
>
> I guess that you have to discuss that with Toke, how he wants to handle
> it ... If he gave you a patch with his signed-off-by, then you should
> probably keep it? There's also "Co-developed-by" tag to give some sort
> of non-author developer credits.

I'll do some squashing and send a new version; in which case I think it
makes sense to have both our s-o-b tags, and maybe a co-developed-by.

I'm hoping to get this done before the holidays (i.e., this week).
Already got everything rebased to current mac80211-next, just need to do
the squashing and fix up the other outstanding issues from last time.

-Toke
diff mbox series

Patch

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index a4556f9..ed85400 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -847,6 +847,7 @@  struct txq_info {
 	struct codel_stats cstats;
 	struct sk_buff_head frags;
 	struct rb_node schedule_order;
+	u16 schedule_round;
 	unsigned long flags;
 
 	/* keep last! */
@@ -1144,6 +1145,7 @@  struct ieee80211_local {
 	struct rb_node *schedule_pos[IEEE80211_NUM_ACS];
 	u64 airtime_v_t[IEEE80211_NUM_ACS];
 	u64 airtime_weight_sum[IEEE80211_NUM_ACS];
+	u16 schedule_round[IEEE80211_NUM_ACS];
 
 	u16 airtime_flags;
 
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index d00baaa..c1444e7 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3644,6 +3644,7 @@  struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
 
 	lockdep_assert_held(&local->active_txq_lock[ac]);
 
+begin:
 	if (!node) {
 		node = rb_first_cached(&local->active_txqs[ac]);
 		first = true;
@@ -3668,7 +3669,10 @@  struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
 		}
 	}
 
+	if (txqi->schedule_round == local->schedule_round[ac])
+		goto begin;
 
+	txqi->schedule_round = local->schedule_round[ac];
 	local->schedule_pos[ac] = node;
 	return &txqi->txq;
 }
@@ -3752,6 +3756,9 @@  void ieee80211_resort_txq(struct ieee80211_hw *hw,
 	u8 ac = txq->ac;
 
 	if (!RB_EMPTY_NODE(&txqi->schedule_order)) {
+		if (local->schedule_pos[ac] == &txqi->schedule_order)
+			local->schedule_pos[ac] = rb_prev(&txqi->schedule_order);
+
 		rb_erase_cached(&txqi->schedule_order,
 				&local->active_txqs[ac]);
 		RB_CLEAR_NODE(&txqi->schedule_order);
@@ -3771,6 +3778,9 @@  static void __ieee80211_unschedule_txq(struct ieee80211_hw *hw,
 	if (RB_EMPTY_NODE(&txqi->schedule_order))
 		return;
 
+	if (local->schedule_pos[ac] == &txqi->schedule_order)
+		local->schedule_pos[ac] = rb_prev(&txqi->schedule_order);
+
 	if (txq->sta) {
 		struct sta_info *sta = container_of(txq->sta,
 						    struct sta_info, sta);
@@ -3803,7 +3813,7 @@  void ieee80211_return_txq(struct ieee80211_hw *hw,
 	lockdep_assert_held(&local->active_txq_lock[txq->ac]);
 
 	if (!RB_EMPTY_NODE(&txqi->schedule_order) &&
-	    (skb_queue_empty(&txqi->frags) && !txqi->tin.backlog_packets))
+	    !txq_has_queue(&txqi->txq))
 		__ieee80211_unschedule_txq(hw, txq);
 }
 EXPORT_SYMBOL(ieee80211_return_txq);
@@ -3832,6 +3842,8 @@  void ieee80211_txq_schedule_start(struct ieee80211_hw *hw, u8 ac)
 	struct ieee80211_local *local = hw_to_local(hw);
 
 	spin_lock_bh(&local->active_txq_lock[ac]);
+	local->schedule_round[ac]++;
+
 }
 EXPORT_SYMBOL(ieee80211_txq_schedule_start);