[RFC 0/3] mac80211: handle push-pull path in ATF

[RFC 0/3] mac80211: handle push-pull path in ATF

[Rajkumar Manoharan]

Toke,

Here are my pending changes on top of your 1st version of ATF.
I am posting these RFCs for more feedback. I havn't posted ath10k changes
and planned to do it later. Kindly review.

-Rajkumar

Rajkumar Manoharan (3):
  mac80211: make airtime txq list per ac
  mac80211: pause txq transmission on negative airtime deficit
  mac80211: add ieee80211_reorder_txq

 include/net/mac80211.h     | 15 ++++++++
 net/mac80211/debugfs_sta.c |  7 ++--
 net/mac80211/ieee80211_i.h |  3 +-
 net/mac80211/main.c        |  3 +-
 net/mac80211/sta_info.c    |  9 +++++
 net/mac80211/tx.c          | 87 +++++++++++++++++++++++++++++++++++-----------
 6 files changed, 98 insertions(+), 26 deletions(-)

-- 
1.9.1

[RFC 1/3] mac80211: make airtime txq list per ac

[Rajkumar Manoharan]

txqs of all access categories are maintained in single list
and in uneven order. To fetch a specific AC's txq from the list,
lookup might have to traverse the entire list in worst case.
To speedup txq lookup, txq list are maintained per each AC.

Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org>
---
 net/mac80211/ieee80211_i.h |  2 +-
 net/mac80211/main.c        |  3 ++-
 net/mac80211/tx.c          | 33 +++++++++++++++++++++++----------
 3 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index bd7f074ccf16..5825824cfe5b 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1129,7 +1129,7 @@ struct ieee80211_local {
 
 	/* protects active_txqs and txqi->schedule_order */
 	spinlock_t active_txq_lock;
-	struct list_head active_txqs;
+	struct list_head active_txqs[IEEE80211_NUM_ACS];
 
 	u16 airtime_flags;
 
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index fbb0bd6183d2..771366464f18 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -634,7 +634,8 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
 	spin_lock_init(&local->rx_path_lock);
 	spin_lock_init(&local->queue_stop_reason_lock);
 
-	INIT_LIST_HEAD(&local->active_txqs);
+	for (i = 0; i < IEEE80211_NUM_ACS; i++)
+		INIT_LIST_HEAD(&local->active_txqs[i]);
 	spin_lock_init(&local->active_txq_lock);
 	local->airtime_flags = AIRTIME_USE_TX | AIRTIME_USE_RX;
 
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 8acab75a0700..6a76852ba1f3 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3597,9 +3597,11 @@ bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
 		 * call to ieee80211_next_txq().
 		 */
 		if ((local->airtime_flags & AIRTIME_ACTIVE) && txqi->txq.sta)
-			list_add(&txqi->schedule_order, &local->active_txqs);
+			list_add(&txqi->schedule_order,
+				 &local->active_txqs[txq->ac]);
 		else
-			list_add_tail(&txqi->schedule_order, &local->active_txqs);
+			list_add_tail(&txqi->schedule_order,
+				      &local->active_txqs[txq->ac]);
 		ret = true;
 	}
 
@@ -3609,15 +3611,26 @@ bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
 }
 EXPORT_SYMBOL(ieee80211_schedule_txq);
 
-static inline struct txq_info *find_txqi(struct list_head *head, s8 ac)
+static inline struct txq_info *find_txqi(struct ieee80211_local *local, s8 ac)
 {
-	struct txq_info *txqi;
+	struct txq_info *txqi = NULL;
+	int i;
 
-	list_for_each_entry(txqi, head, schedule_order) {
-		if (ac < 0 || txqi->txq.ac == ac)
-			return txqi;
+	if (ac >= 0 && ac < IEEE80211_NUM_ACS) {
+		txqi = list_first_entry_or_null(&local->active_txqs[ac],
+						struct txq_info,
+						schedule_order);
+	} else {
+		for (i = 0; i < IEEE80211_NUM_ACS; i++) {
+			if (list_empty(&local->active_txqs[i]))
+				continue;
+			txqi = list_first_entry(&local->active_txqs[i],
+						struct txq_info,
+						schedule_order);
+		}
 	}
-	return NULL;
+
+	return txqi;
 }
 
 struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac)
@@ -3628,7 +3641,7 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac)
 	spin_lock_bh(&local->active_txq_lock);
 
 begin:
-	txqi = find_txqi(&local->active_txqs, ac);
+	txqi = find_txqi(local, ac);
 	if (!txqi)
 		goto out;
 
@@ -3639,7 +3652,7 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac)
 		if (sta->airtime.deficit[txqi->txq.ac] < 0) {
 			sta->airtime.deficit[txqi->txq.ac] += IEEE80211_AIRTIME_QUANTUM * sta->airtime.weight;
 			list_move_tail(&txqi->schedule_order,
-				       &local->active_txqs);
+				       &local->active_txqs[txqi->txq.ac]);
 			goto begin;
 		}
 	}
-- 
1.9.1

[RFC 2/3] mac80211: pause txq transmission on negative airtime deficit

[Rajkumar Manoharan]

Airtime fairness prioritizes txqs and picks high priority txq. Once
a txq is selected for transmission by next_txq(), dequeue routine is
not preventing over downloading packets. i.e the driver is still
allowed to dequeue frames from txq even when its fairness deficit
goes negative. This is also causing inefficient fq-codel of mac80211
when the driver/firmware is maintaining another set of data queues.
To address this problem, pause txq transmission when given txq's
was already served for the assigned quantum and resume the transmission
upon prioritization.

Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org>
---
 net/mac80211/debugfs_sta.c | 7 ++++---
 net/mac80211/ieee80211_i.h | 1 +
 net/mac80211/sta_info.c    | 9 +++++++++
 net/mac80211/tx.c          | 9 ++++++++-
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index bae0fa7ff95a..5c38994cd95c 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -178,9 +178,10 @@ static ssize_t sta_aqm_read(struct file *file, char __user *userbuf,
 			       txqi->tin.tx_bytes,
 			       txqi->tin.tx_packets,
 			       txqi->flags,
-			       txqi->flags & (1<<IEEE80211_TXQ_STOP) ? "STOP" : "RUN",
-			       txqi->flags & (1<<IEEE80211_TXQ_AMPDU) ? " AMPDU" : "",
-			       txqi->flags & (1<<IEEE80211_TXQ_NO_AMSDU) ? " NO-AMSDU" : "");
+			       txqi->flags & (1 << IEEE80211_TXQ_STOP) ? "STOP" :
+			       txqi->flags & (1 << IEEE80211_TXQ_PAUSE) ? "PAUSE": "RUN",
+			       txqi->flags & (1 << IEEE80211_TXQ_AMPDU) ? " AMPDU" : "",
+			       txqi->flags & (1 << IEEE80211_TXQ_NO_AMSDU) ? " NO-AMSDU" : "");
 	}
 
 	rcu_read_unlock();
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 5825824cfe5b..3ef287075cb1 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -818,6 +818,7 @@ enum txq_info_flags {
 	IEEE80211_TXQ_STOP,
 	IEEE80211_TXQ_AMPDU,
 	IEEE80211_TXQ_NO_AMSDU,
+	IEEE80211_TXQ_PAUSE,
 };
 
 /**
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 8af9e5c05ce4..215ae3a690aa 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1830,6 +1830,8 @@ void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
 {
 	struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
 	struct ieee80211_local *local = sta->sdata->local;
+	struct fq *fq = &local->fq;
+	struct txq_info *txqi;
 	u8 ac = ieee80211_ac_from_tid(tid);
 	u32 airtime = 0;
 
@@ -1844,6 +1846,13 @@ void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
 	sta->airtime.deficit[ac] -= airtime;
 	if (airtime)
 		sta->local->airtime_flags |= AIRTIME_ACTIVE;
+
+	if (sta->airtime.deficit[ac] < 0) {
+		txqi = to_txq_info(pubsta->txq[tid]);
+		spin_lock_bh(&fq->lock);
+		set_bit(IEEE80211_TXQ_PAUSE, &txqi->flags);
+		spin_unlock_bh(&fq->lock);
+	}
 	spin_unlock_bh(&local->active_txq_lock);
 }
 EXPORT_SYMBOL(ieee80211_sta_register_airtime);
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 6a76852ba1f3..0af35c08e0d9 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3475,7 +3475,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 
 	spin_lock_bh(&fq->lock);
 
-	if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags))
+	if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) ||
+	    test_bit(IEEE80211_TXQ_PAUSE, &txqi->flags))
 		goto out;
 
 	/* Make sure fragments stay together. */
@@ -3636,6 +3637,7 @@ static inline struct txq_info *find_txqi(struct ieee80211_local *local, s8 ac)
 struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
+	struct fq *fq = &local->fq;
 	struct txq_info *txqi = NULL;
 
 	spin_lock_bh(&local->active_txq_lock);
@@ -3653,6 +3655,11 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac)
 			sta->airtime.deficit[txqi->txq.ac] += IEEE80211_AIRTIME_QUANTUM * sta->airtime.weight;
 			list_move_tail(&txqi->schedule_order,
 				       &local->active_txqs[txqi->txq.ac]);
+			if (sta->airtime.deficit[txqi->txq.ac] > 0) {
+				spin_lock_bh(&fq->lock);
+				clear_bit(IEEE80211_TXQ_PAUSE, &txqi->flags);
+				spin_unlock_bh(&fq->lock);
+			}
 			goto begin;
 		}
 	}
-- 
1.9.1

[RFC 3/3] mac80211: add ieee80211_reorder_txq

[Rajkumar Manoharan]

This allows the driver to refill airtime fairness deficit
where the driver will not access txqs by ieee80211_next_txq.
In tx push mode data path, high priority txqs will be scheduled
for data transmission by ieee80211_next_txq and driver will not
prioritize txqs whereas in push-pull mode, the drivers can
prioritize txqs and access them directly. In such mode, airtime
deficit can not filled by ieee80211_next_txq.

Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org>
---
 include/net/mac80211.h | 15 +++++++++++++
 net/mac80211/tx.c      | 59 +++++++++++++++++++++++++++++++++++---------------
 2 files changed, 57 insertions(+), 17 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index cc16847bd52d..a2f0b6800100 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -6033,6 +6033,21 @@ bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
 struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac);
 
 /**
+ * ieee80211_reorder_txq - change txq position in scheduling loop
+ *
+ * This function is used to reorder txq's position in scheduling loop.
+ * The txq fairness deficit will be refilled. The drivers calling this
+ * function should ensure the txq won't be accessed by ieee80211_next_txq
+ * in the same path.
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ */
+void ieee80211_reorder_txq(struct ieee80211_hw *hw,
+			   struct ieee80211_txq *txq);
+
+/**
  * ieee80211_txq_get_depth - get pending frame/byte count of given txq
  *
  * The values are not guaranteed to be coherent with regard to each other, i.e.
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 0af35c08e0d9..b7b2f93152f8 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3634,10 +3634,38 @@ static inline struct txq_info *find_txqi(struct ieee80211_local *local, s8 ac)
 	return txqi;
 }
 
+static bool ieee80211_txq_refill_deficit(struct ieee80211_local *local,
+					 struct txq_info *txqi)
+{
+	struct fq *fq = &local->fq;
+	struct sta_info *sta;
+
+	lockdep_assert_held(&local->active_txq_lock);
+
+	if (!txqi->txq.sta)
+		return false;
+
+	sta = container_of(txqi->txq.sta, struct sta_info, sta);
+
+	if (sta->airtime.deficit[txqi->txq.ac] > 0)
+		return false;
+
+	sta->airtime.deficit[txqi->txq.ac] +=
+		IEEE80211_AIRTIME_QUANTUM * sta->airtime.weight;
+	list_move_tail(&txqi->schedule_order,
+		       &local->active_txqs[txqi->txq.ac]);
+
+	if (sta->airtime.deficit[txqi->txq.ac] > 0) {
+		spin_lock_bh(&fq->lock);
+		clear_bit(IEEE80211_TXQ_PAUSE, &txqi->flags);
+		spin_unlock_bh(&fq->lock);
+	}
+	return true;
+}
+
 struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
-	struct fq *fq = &local->fq;
 	struct txq_info *txqi = NULL;
 
 	spin_lock_bh(&local->active_txq_lock);
@@ -3647,22 +3675,8 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac)
 	if (!txqi)
 		goto out;
 
-	if (txqi->txq.sta) {
-		struct sta_info *sta = container_of(txqi->txq.sta,
-						struct sta_info, sta);
-
-		if (sta->airtime.deficit[txqi->txq.ac] < 0) {
-			sta->airtime.deficit[txqi->txq.ac] += IEEE80211_AIRTIME_QUANTUM * sta->airtime.weight;
-			list_move_tail(&txqi->schedule_order,
-				       &local->active_txqs[txqi->txq.ac]);
-			if (sta->airtime.deficit[txqi->txq.ac] > 0) {
-				spin_lock_bh(&fq->lock);
-				clear_bit(IEEE80211_TXQ_PAUSE, &txqi->flags);
-				spin_unlock_bh(&fq->lock);
-			}
-			goto begin;
-		}
-	}
+	if (ieee80211_txq_refill_deficit(local, txqi))
+		goto begin;
 
 	list_del_init(&txqi->schedule_order);
 
@@ -3676,6 +3690,17 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac)
 }
 EXPORT_SYMBOL(ieee80211_next_txq);
 
+void ieee80211_reorder_txq(struct ieee80211_hw *hw, struct ieee80211_txq *txq)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+	struct txq_info *txqi = to_txq_info(txq);
+
+	spin_lock_bh(&local->active_txq_lock);
+	ieee80211_txq_refill_deficit(local, txqi);
+	spin_unlock_bh(&local->active_txq_lock);
+}
+EXPORT_SYMBOL(ieee80211_reorder_txq);
+
 void __ieee80211_subif_start_xmit(struct sk_buff *skb,
 				  struct net_device *dev,
 				  u32 info_flags)
-- 
1.9.1

Re: [RFC 3/3] mac80211: add ieee80211_reorder_txq

[Toke Høiland-Jørgensen]

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

> This allows the driver to refill airtime fairness deficit
> where the driver will not access txqs by ieee80211_next_txq.
> In tx push mode data path, high priority txqs will be scheduled
> for data transmission by ieee80211_next_txq and driver will not
> prioritize txqs whereas in push-pull mode, the drivers can
> prioritize txqs and access them directly. In such mode, airtime
> deficit can not filled by ieee80211_next_txq.
>
> Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org>
> ---
>  include/net/mac80211.h | 15 +++++++++++++
>  net/mac80211/tx.c      | 59 +++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 57 insertions(+), 17 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index cc16847bd52d..a2f0b6800100 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -6033,6 +6033,21 @@ bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
>  struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac);
>  
>  /**
> + * ieee80211_reorder_txq - change txq position in scheduling loop
> + *
> + * This function is used to reorder txq's position in scheduling loop.
> + * The txq fairness deficit will be refilled. The drivers calling this
> + * function should ensure the txq won't be accessed by ieee80211_next_txq
> + * in the same path.
> + *
> + * @hw: pointer as obtained from ieee80211_alloc_hw()
> + * @txq: pointer obtained from station or virtual interface
> + *
> + */
> +void ieee80211_reorder_txq(struct ieee80211_hw *hw,
> +			   struct ieee80211_txq *txq);
> +
> +/**
>   * ieee80211_txq_get_depth - get pending frame/byte count of given txq
>   *
>   * The values are not guaranteed to be coherent with regard to each other, i.e.
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 0af35c08e0d9..b7b2f93152f8 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -3634,10 +3634,38 @@ static inline struct txq_info *find_txqi(struct ieee80211_local *local, s8 ac)
>  	return txqi;
>  }
>  
> +static bool ieee80211_txq_refill_deficit(struct ieee80211_local *local,
> +					 struct txq_info *txqi)
> +{
> +	struct fq *fq = &local->fq;
> +	struct sta_info *sta;
> +
> +	lockdep_assert_held(&local->active_txq_lock);
> +
> +	if (!txqi->txq.sta)
> +		return false;
> +
> +	sta = container_of(txqi->txq.sta, struct sta_info, sta);
> +
> +	if (sta->airtime.deficit[txqi->txq.ac] > 0)
> +		return false;
> +
> +	sta->airtime.deficit[txqi->txq.ac] +=
> +		IEEE80211_AIRTIME_QUANTUM * sta->airtime.weight;
> +	list_move_tail(&txqi->schedule_order,
> +		       &local->active_txqs[txqi->txq.ac]);

This needs to check that the txq is currently at the head of
local->active_txqs[txqi->txq.ac]; otherwise fairness enforcement doesn't
work. And with this check I'm not sure the reorder function is terribly
useful for what you want to use it for?

-Toke

Re: [RFC 3/3] mac80211: add ieee80211_reorder_txq

[Rajkumar Manoharan]

On 2018-08-21 05:24, Toke Høiland-Jørgensen wrote:
> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
> 
>> This allows the driver to refill airtime fairness deficit
>> where the driver will not access txqs by ieee80211_next_txq.
>> In tx push mode data path, high priority txqs will be scheduled
>> for data transmission by ieee80211_next_txq and driver will not
>> prioritize txqs whereas in push-pull mode, the drivers can
>> prioritize txqs and access them directly. In such mode, airtime
>> deficit can not filled by ieee80211_next_txq.
>> 
[...]
> 
> This needs to check that the txq is currently at the head of
> local->active_txqs[txqi->txq.ac]; otherwise fairness enforcement 
> doesn't
> work. And with this check I'm not sure the reorder function is terribly
> useful for what you want to use it for?
> 
Hmm.. I tried to keep the same policy of next_txq(). i.e do not allow 
serving
txqs when the deficit is negative. reorder_txq() limits over serving the 
same
txq by driver. So that driver is given chance to serve other txqs.

As mentioned earlier, next_txq() can not be used for fetching txq 
directly.
So reorder_txq() needs to take care of refilling txq after serving them.

-Rajkumar

Re: [RFC 3/3] mac80211: add ieee80211_reorder_txq

[Toke Høiland-Jørgensen]

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

> On 2018-08-21 05:24, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>>=20
>>> This allows the driver to refill airtime fairness deficit
>>> where the driver will not access txqs by ieee80211_next_txq.
>>> In tx push mode data path, high priority txqs will be scheduled
>>> for data transmission by ieee80211_next_txq and driver will not
>>> prioritize txqs whereas in push-pull mode, the drivers can
>>> prioritize txqs and access them directly. In such mode, airtime
>>> deficit can not filled by ieee80211_next_txq.
>>>=20
> [...]
>>=20
>> This needs to check that the txq is currently at the head of
>> local->active_txqs[txqi->txq.ac]; otherwise fairness enforcement=20
>> doesn't
>> work. And with this check I'm not sure the reorder function is terribly
>> useful for what you want to use it for?
>>=20
> Hmm.. I tried to keep the same policy of next_txq(). i.e do not allow
> serving txqs when the deficit is negative. reorder_txq() limits over
> serving the same txq by driver. So that driver is given chance to
> serve other txqs.

Yeah, but the fairness comes from all TXQs being given the *same amount*
of deficit increase. I.e., with reorder_txq() there needs to be a
guarantee that it is called the same number of times for all active
TXQs. Which is what the round-robin scheduling ensures in next_txq().

> As mentioned earlier, next_txq() can not be used for fetching txq
> directly. So reorder_txq() needs to take care of refilling txq after
> serving them.

Yeah, I got that; but see above. Unless there's a guarantee that the
push/pull mechanism will be round-robin scheduled (which as I'm reading
the code there isn't), just increasing the deficit on every call to
reorder_txq() is not going to ensure fairness (it'll probably help some,
but it won't be completely fair).

However, if we add the check to reorder_txq() so the deficit increase +
rotation is only done if the TXQ is at the head of the list, we'll get
round-robin-equivalent behaviour since a TXQ will only get to continue
when it happens to have rotated to the head of the queue. As I mentioned
previously it will break MIMO, but I we could fix that later once we've
verified that the basic mechanism works.

Alternatively, if we keep it the way you've written it in this patch, we
can iterate on the driver API and make sure that airtime reporting
actually works for ath10k, and then change the scheduler once everything
else is in place...

-Toke

Re: [RFC 3/3] mac80211: add ieee80211_reorder_txq

[Rajkumar Manoharan]

On 2018-08-22 04:10, Toke Høiland-Jørgensen wrote:
> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
> 
>> On 2018-08-21 05:24, Toke Høiland-Jørgensen wrote:
>>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>>> 
[...]

> Yeah, but the fairness comes from all TXQs being given the *same 
> amount*
> of deficit increase. I.e., with reorder_txq() there needs to be a
> guarantee that it is called the same number of times for all active
> TXQs. Which is what the round-robin scheduling ensures in next_txq().
> 
Understood.

>> As mentioned earlier, next_txq() can not be used for fetching txq
>> directly. So reorder_txq() needs to take care of refilling txq after
>> serving them.
> 
> Yeah, I got that; but see above. Unless there's a guarantee that the
> push/pull mechanism will be round-robin scheduled (which as I'm reading
> the code there isn't), just increasing the deficit on every call to
> reorder_txq() is not going to ensure fairness (it'll probably help 
> some,
> but it won't be completely fair).
> 
> However, if we add the check to reorder_txq() so the deficit increase +
> rotation is only done if the TXQ is at the head of the list, we'll get
> round-robin-equivalent behaviour since a TXQ will only get to continue
> when it happens to have rotated to the head of the queue. As I 
> mentioned
> previously it will break MIMO, but I we could fix that later once we've
> verified that the basic mechanism works.
> 
Hmm... The pull mechanism operates in round-robin fashion. Agree that
accessing only head node is right way of ensuring fairness. Will fix it
and rename as ieee80211_txq_can_transmit().

Thanks Toke for your feedback.

-Rajkumar

Re: [RFC 3/3] mac80211: add ieee80211_reorder_txq

[Toke Høiland-Jørgensen]

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

>>> As mentioned earlier, next_txq() can not be used for fetching txq
>>> directly. So reorder_txq() needs to take care of refilling txq after
>>> serving them.
>> 
>> Yeah, I got that; but see above. Unless there's a guarantee that the
>> push/pull mechanism will be round-robin scheduled (which as I'm reading
>> the code there isn't), just increasing the deficit on every call to
>> reorder_txq() is not going to ensure fairness (it'll probably help 
>> some,
>> but it won't be completely fair).
>> 
>> However, if we add the check to reorder_txq() so the deficit increase +
>> rotation is only done if the TXQ is at the head of the list, we'll get
>> round-robin-equivalent behaviour since a TXQ will only get to continue
>> when it happens to have rotated to the head of the queue. As I 
>> mentioned
>> previously it will break MIMO, but I we could fix that later once we've
>> verified that the basic mechanism works.
>> 
> Hmm... The pull mechanism operates in round-robin fashion. Agree that
> accessing only head node is right way of ensuring fairness.


Ah, excellent; then it should be no problem to enforce the head node
access; worst case the pull mechanism will be at a different point in
the round-robin, which will sync up quickly.

> Will fix it and rename as ieee80211_txq_can_transmit().

Cool. If you send an updated patchset I can fold it into an updated
version of my RFC and send it as a proper patch once I've verified
everything works with ath9k :)

> Thanks Toke for your feedback.

You're welcome! And thanks for working on this :)

-Toke

Re: [RFC 3/3] mac80211: add ieee80211_reorder_txq

[Rajkumar Manoharan]

On 2018-08-28 03:20, Toke Høiland-Jørgensen wrote:
> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
> 
> Ah, excellent; then it should be no problem to enforce the head node
> access; worst case the pull mechanism will be at a different point in
> the round-robin, which will sync up quickly.
> 
>> Will fix it and rename as ieee80211_txq_can_transmit().
> 
> Cool. If you send an updated patchset I can fold it into an updated
> version of my RFC and send it as a proper patch once I've verified
> everything works with ath9k :)
> 
Sure. Found an issue in txq pause change. The drivers assume that
ieee80211_tx_dequeue() returns NULL only when txq is empty. Adding a
IEEE80211_TXQ_PAUSE check in tx_dequeue() breaks this assumption. 
Moreover
once txq is paused, it should be queued to tail not at head.

Will send updated version ASAP.

-Rajkumar