[RFC 1/3] mac80211: Add TXQ scheduling API

[RFC 1/3] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

This adds an API to mac80211 to handle scheduling of TXQs and changes the
interface between driver and mac80211 for TXQ handling as follows:

- The wake_tx_queue callback interface no longer includes the TXQ. Instead, the
  driver is expected to retrieve that from ieee80211_next_txq()

- Two new mac80211 functions are added: ieee80211_next_txq() and
  ieee80211_schedule_txq(). The former returns the next TXQ that should be
  scheduled, and is how the driver gets a queue to pull packets from. The latter
  is called internally by mac80211 to start scheduling a queue, and the driver
  is supposed to call it to re-schedule the TXQ after it is finished pulling
  packets from it (unless the queue emptied).

- A drv_buffered flag is added to struct ieee80211_txq which the driver can set
  to request mac80211 to re-schedule the TXQ at PS wakeup.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
This version of the scheduler is pretty dumb and just does round-robin
scheduling (pretty much identical to what ath10k does today). In the
future, this will be extended to do airtime fairness scheduling for
drivers that provide airtime information.

I have only compile tested this series; will runtime test it later this
week, but for now comments on the API would be appreciated. In
particular, I'm not sure what the right thing to do in regards to PS
wakeup is...

 include/net/mac80211.h     | 34 +++++++++++++++++++++++++++++-----
 net/mac80211/agg-tx.c      |  4 +++-
 net/mac80211/driver-ops.h  | 12 +++---------
 net/mac80211/ieee80211_i.h |  7 ++++++-
 net/mac80211/main.c        |  3 +++
 net/mac80211/sta_info.c    |  3 ++-
 net/mac80211/trace.h       | 21 ++++-----------------
 net/mac80211/tx.c          | 40 +++++++++++++++++++++++++++++++++++++++-
 8 files changed, 89 insertions(+), 35 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index cc9073e45be9..80a80efa06b1 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -105,9 +105,12 @@
  * The driver is expected to initialize its private per-queue data for stations
  * and interfaces in the .add_interface and .sta_add ops.
  *
- * The driver can't access the queue directly. To dequeue a frame, it calls
- * ieee80211_tx_dequeue(). Whenever mac80211 adds a new frame to a queue, it
- * calls the .wake_tx_queue driver op.
+ * The driver can't access the queue directly. To obtain the next queue to pull
+ * frames from, the driver calls ieee80211_next_txq(). To dequeue a frame from a
+ * txq, it calls ieee80211_tx_dequeue(). Whenever mac80211 adds a new frame to a
+ * queue, it calls the .wake_tx_queue driver op. The driver is expected to
+ * re-schedule the txq using ieee80211_schedule_txq() if it is still active
+ * after the driver has finished pulling packets from it.
  *
  * For AP powersave TIM handling, the driver only needs to indicate if it has
  * buffered packets in the driver specific data structures by calling
@@ -1870,6 +1873,7 @@ struct ieee80211_txq {
 	u8 tid;
 	u8 ac;
 
+	bool drv_buffered;
 	/* must be last */
 	u8 drv_priv[0] __aligned(sizeof(void *));
 };
@@ -3723,8 +3727,7 @@ struct ieee80211_ops {
 					 struct ieee80211_vif *vif,
 					 struct ieee80211_tdls_ch_sw_params *params);
 
-	void (*wake_tx_queue)(struct ieee80211_hw *hw,
-			      struct ieee80211_txq *txq);
+	void (*wake_tx_queue)(struct ieee80211_hw *hw);
 	void (*sync_rx_queues)(struct ieee80211_hw *hw);
 
 	int (*start_nan)(struct ieee80211_hw *hw,
@@ -5876,6 +5879,27 @@ void ieee80211_unreserve_tid(struct ieee80211_sta *sta, u8 tid);
 struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 				     struct ieee80211_txq *txq);
 
+/**
+ * ieee80211_schedule_txq - add txq to scheduling loop
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ */
+void ieee80211_schedule_txq(struct ieee80211_hw *hw,
+			    struct ieee80211_txq *txq);
+
+/**
+ * ieee80211_next_txq - get next tx queue to pull packets from
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ *
+ * Returns the next txq if successful, %NULL if no queue is eligible. If a txq
+ * is returned, it will have been removed from the scheduler queue and needs to
+ * be re-scheduled with ieee80211_schedule_txq() to continue to be active.
+ */
+struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw);
+
 /**
  * ieee80211_txq_get_depth - get pending frame/byte count of given txq
  *
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index bef516ec47f9..77bd0ff90677 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -226,9 +226,11 @@ ieee80211_agg_start_txq(struct sta_info *sta, int tid, bool enable)
 		clear_bit(IEEE80211_TXQ_AMPDU, &txqi->flags);
 
 	clear_bit(IEEE80211_TXQ_STOP, &txqi->flags);
+	ieee80211_schedule_txq(&sta->sdata->local->hw, txq);
+
 	local_bh_disable();
 	rcu_read_lock();
-	drv_wake_tx_queue(sta->sdata->local, txqi);
+	drv_wake_tx_queue(sta->sdata->local);
 	rcu_read_unlock();
 	local_bh_enable();
 }
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 09f77e4a8a79..e20a9e2acd53 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1157,16 +1157,10 @@ drv_tdls_recv_channel_switch(struct ieee80211_local *local,
 	trace_drv_return_void(local);
 }
 
-static inline void drv_wake_tx_queue(struct ieee80211_local *local,
-				     struct txq_info *txq)
+static inline void drv_wake_tx_queue(struct ieee80211_local *local)
 {
-	struct ieee80211_sub_if_data *sdata = vif_to_sdata(txq->txq.vif);
-
-	if (!check_sdata_in_driver(sdata))
-		return;
-
-	trace_drv_wake_tx_queue(local, sdata, txq);
-	local->ops->wake_tx_queue(&local->hw, &txq->txq);
+	trace_drv_wake_tx_queue(local);
+	local->ops->wake_tx_queue(&local->hw);
 }
 
 static inline int drv_start_nan(struct ieee80211_local *local,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 9675814f64db..521bedb5dc86 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -832,6 +832,7 @@ struct txq_info {
 	struct codel_vars def_cvars;
 	struct codel_stats cstats;
 	struct sk_buff_head frags;
+	struct list_head schedule_order;
 	unsigned long flags;
 
 	/* keep last! */
@@ -1121,6 +1122,9 @@ struct ieee80211_local {
 	struct codel_vars *cvars;
 	struct codel_params cparams;
 
+	struct list_head active_txqs;
+	spinlock_t active_txq_lock;
+
 	const struct ieee80211_ops *ops;
 
 	/*
@@ -1505,7 +1509,8 @@ static inline bool txq_has_queue(struct ieee80211_txq *txq)
 {
 	struct txq_info *txqi = to_txq_info(txq);
 
-	return !(skb_queue_empty(&txqi->frags) && !txqi->tin.backlog_packets);
+	return !(skb_queue_empty(&txqi->frags) && !txqi->tin.backlog_packets
+		 && !txq->drv_buffered);
 }
 
 static inline int ieee80211_bssid_match(const u8 *raddr, const u8 *addr)
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 8aa1f5b6a051..9ad0556aa24b 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -616,6 +616,9 @@ 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);
+	spin_lock_init(&local->active_txq_lock);
+
 	INIT_LIST_HEAD(&local->chanctx_list);
 	mutex_init(&local->chanctx_mtx);
 
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 9673e157bf8f..bd31e6dc808f 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1250,8 +1250,9 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
 			if (!txq_has_queue(sta->sta.txq[i]))
 				continue;
 
-			drv_wake_tx_queue(local, to_txq_info(sta->sta.txq[i]));
+			ieee80211_schedule_txq(&local->hw, sta->sta.txq[i]);
 		}
+		drv_wake_tx_queue(local);
 	}
 
 	skb_queue_head_init(&pending);
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 3d9ac17af407..531c0e7f2358 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -2550,33 +2550,20 @@ TRACE_EVENT(drv_tdls_recv_channel_switch,
 );
 
 TRACE_EVENT(drv_wake_tx_queue,
-	TP_PROTO(struct ieee80211_local *local,
-		 struct ieee80211_sub_if_data *sdata,
-		 struct txq_info *txq),
+	TP_PROTO(struct ieee80211_local *local),
 
-	TP_ARGS(local, sdata, txq),
+	TP_ARGS(local),
 
 	TP_STRUCT__entry(
 		LOCAL_ENTRY
-		VIF_ENTRY
-		STA_ENTRY
-		__field(u8, ac)
-		__field(u8, tid)
 	),
 
 	TP_fast_assign(
-		struct ieee80211_sta *sta = txq->txq.sta;
-
 		LOCAL_ASSIGN;
-		VIF_ASSIGN;
-		STA_ASSIGN;
-		__entry->ac = txq->txq.ac;
-		__entry->tid = txq->txq.tid;
 	),
-
 	TP_printk(
-		LOCAL_PR_FMT  VIF_PR_FMT  STA_PR_FMT " ac:%d tid:%d",
-		LOCAL_PR_ARG, VIF_PR_ARG, STA_PR_ARG, __entry->ac, __entry->tid
+		LOCAL_PR_FMT,
+		LOCAL_PR_ARG
 	)
 );
 
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 94826680cf2b..9c43c17c5144 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1405,6 +1405,7 @@ void ieee80211_txq_init(struct ieee80211_sub_if_data *sdata,
 	codel_vars_init(&txqi->def_cvars);
 	codel_stats_init(&txqi->cstats);
 	__skb_queue_head_init(&txqi->frags);
+	INIT_LIST_HEAD(&txqi->schedule_order);
 
 	txqi->txq.vif = &sdata->vif;
 
@@ -1428,6 +1429,7 @@ void ieee80211_txq_purge(struct ieee80211_local *local,
 
 	fq_tin_reset(fq, tin, fq_skb_free_func);
 	ieee80211_purge_tx_queue(&local->hw, &txqi->frags);
+	list_del_init(&txqi->schedule_order);
 }
 
 int ieee80211_txq_setup_flows(struct ieee80211_local *local)
@@ -1524,7 +1526,8 @@ static bool ieee80211_queue_skb(struct ieee80211_local *local,
 	ieee80211_txq_enqueue(local, txqi, skb);
 	spin_unlock_bh(&fq->lock);
 
-	drv_wake_tx_queue(local, txqi);
+	ieee80211_schedule_txq(&local->hw, &txqi->txq);
+	drv_wake_tx_queue(local);
 
 	return true;
 }
@@ -3517,6 +3520,41 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 }
 EXPORT_SYMBOL(ieee80211_tx_dequeue);
 
+void ieee80211_schedule_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);
+
+	if (!list_empty(&txqi->schedule_order))
+		list_add_tail(&txqi->schedule_order, &local->active_txqs);
+
+	spin_unlock_bh(&local->active_txq_lock);
+}
+EXPORT_SYMBOL(ieee80211_schedule_txq);
+
+struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+	struct txq_info *txqi = NULL;
+
+	spin_lock_bh(&local->active_txq_lock);
+
+	if (list_empty(&local->active_txqs))
+		goto out;
+
+	txqi = list_first_entry(&local->active_txqs, struct txq_info, schedule_order);
+	list_del_init(&txqi->schedule_order);
+
+out:
+	spin_unlock_bh(&local->active_txq_lock);
+
+	return &txqi->txq;
+}
+EXPORT_SYMBOL(ieee80211_next_txq);
+
 void __ieee80211_subif_start_xmit(struct sk_buff *skb,
 				  struct net_device *dev,
 				  u32 info_flags)
-- 
2.14.2

[RFC 2/3] ath9k: Move to mac80211 TXQ scheduling API

[Toke Høiland-Jørgensen]

This removes TXQ scheduling from ath9k and changes it to use the mac80211 TXQ
scheduling API introduced in the previous patch.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 drivers/net/wireless/ath/ath9k/ath9k.h   |   7 +-
 drivers/net/wireless/ath/ath9k/channel.c |   2 -
 drivers/net/wireless/ath/ath9k/main.c    |   3 +-
 drivers/net/wireless/ath/ath9k/xmit.c    | 181 +++++++------------------------
 4 files changed, 40 insertions(+), 153 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index cf076719c27e..cb8f64864199 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -246,7 +246,6 @@ struct ath_atx_tid {
 	s8 bar_index;
 	bool active;
 	bool clear_ps_filter;
-	bool has_queued;
 };
 
 void __ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid);
@@ -591,8 +590,7 @@ bool ath_drain_all_txq(struct ath_softc *sc);
 void ath_draintxq(struct ath_softc *sc, struct ath_txq *txq);
 void ath_tx_node_init(struct ath_softc *sc, struct ath_node *an);
 void ath_tx_node_cleanup(struct ath_softc *sc, struct ath_node *an);
-void ath_txq_schedule(struct ath_softc *sc, struct ath_txq *txq);
-void ath_txq_schedule_all(struct ath_softc *sc);
+void ath_txq_schedule(struct ath_softc *sc);
 int ath_tx_init(struct ath_softc *sc, int nbufs);
 int ath_txq_update(struct ath_softc *sc, int qnum,
 		   struct ath9k_tx_queue_info *q);
@@ -610,7 +608,6 @@ int ath_tx_aggr_start(struct ath_softc *sc, struct ieee80211_sta *sta,
 		      u16 tid, u16 *ssn);
 void ath_tx_aggr_stop(struct ath_softc *sc, struct ieee80211_sta *sta, u16 tid);
 
-void ath_tx_aggr_wakeup(struct ath_softc *sc, struct ath_node *an);
 void ath_tx_aggr_sleep(struct ieee80211_sta *sta, struct ath_softc *sc,
 		       struct ath_node *an);
 void ath9k_release_buffered_frames(struct ieee80211_hw *hw,
@@ -618,7 +615,7 @@ void ath9k_release_buffered_frames(struct ieee80211_hw *hw,
 				   u16 tids, int nframes,
 				   enum ieee80211_frame_release_type reason,
 				   bool more_data);
-void ath9k_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq *queue);
+void ath9k_wake_tx_queue(struct ieee80211_hw *hw);
 
 /********/
 /* VIFs */
diff --git a/drivers/net/wireless/ath/ath9k/channel.c b/drivers/net/wireless/ath/ath9k/channel.c
index f0439f2d566b..6dc60fd53a8b 100644
--- a/drivers/net/wireless/ath/ath9k/channel.c
+++ b/drivers/net/wireless/ath/ath9k/channel.c
@@ -1595,8 +1595,6 @@ void ath9k_p2p_ps_timer(void *priv)
 	an->sleeping = avp->noa.absent;
 	if (an->sleeping)
 		ath_tx_aggr_sleep(sta, sc, an);
-	else
-		ath_tx_aggr_wakeup(sc, an);
 
 out:
 	rcu_read_unlock();
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 8b4ac7f0a09b..1601dd11e460 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -265,7 +265,7 @@ static bool ath_complete_reset(struct ath_softc *sc, bool start)
 		}
 	work:
 		ath_restart_work(sc);
-		ath_txq_schedule_all(sc);
+		ath_txq_schedule(sc);
 	}
 
 	sc->gtt_cnt = 0;
@@ -1619,7 +1619,6 @@ static void ath9k_sta_notify(struct ieee80211_hw *hw,
 	case STA_NOTIFY_AWAKE:
 		ath9k_sta_set_tx_filter(sc->sc_ah, an, false);
 		an->sleeping = false;
-		ath_tx_aggr_wakeup(sc, an);
 		break;
 	}
 }
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 396bf05c6bf6..609f209a2278 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -150,24 +150,11 @@ void ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid)
 }
 
 
-void ath9k_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq *queue)
+void ath9k_wake_tx_queue(struct ieee80211_hw *hw)
 {
 	struct ath_softc *sc = hw->priv;
-	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
-	struct ath_atx_tid *tid = (struct ath_atx_tid *) queue->drv_priv;
-	struct ath_txq *txq = tid->txq;
-
-	ath_dbg(common, QUEUE, "Waking TX queue: %pM (%d)\n",
-		queue->sta ? queue->sta->addr : queue->vif->addr,
-		tid->tidno);
-
-	ath_txq_lock(sc, txq);
-
-	tid->has_queued = true;
-	ath_tx_queue_tid(sc, tid);
-	ath_txq_schedule(sc, txq);
 
-	ath_txq_unlock(sc, txq);
+	ath_txq_schedule(sc);
 }
 
 static struct ath_frame_info *get_frame_info(struct sk_buff *skb)
@@ -230,14 +217,11 @@ ath_tid_pull(struct ath_atx_tid *tid)
 	struct ath_frame_info *fi;
 	int q;
 
-	if (!tid->has_queued)
-		return NULL;
+	txq->drv_buffered = false;
 
 	skb = ieee80211_tx_dequeue(hw, txq);
-	if (!skb) {
-		tid->has_queued = false;
+	if (!skb)
 		return NULL;
-	}
 
 	if (ath_tx_prepare(hw, skb, &txctl)) {
 		ieee80211_free_txskb(hw, skb);
@@ -254,12 +238,6 @@ ath_tid_pull(struct ath_atx_tid *tid)
 	return skb;
  }
 
-
-static bool ath_tid_has_buffered(struct ath_atx_tid *tid)
-{
-	return !skb_queue_empty(&tid->retry_q) || tid->has_queued;
-}
-
 static struct sk_buff *ath_tid_dequeue(struct ath_atx_tid *tid)
 {
 	struct sk_buff *skb;
@@ -670,12 +648,8 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
 			ieee80211_sta_set_buffered(sta, tid->tidno, true);
 
 		skb_queue_splice_tail(&bf_pending, &tid->retry_q);
-		if (!an->sleeping) {
-			ath_tx_queue_tid(sc, tid);
-
-			if (ts->ts_status & (ATH9K_TXERR_FILT | ATH9K_TXERR_XRETRY))
-				tid->clear_ps_filter = true;
-		}
+		if (!an->sleeping && (ts->ts_status & (ATH9K_TXERR_FILT | ATH9K_TXERR_XRETRY)))
+			tid->clear_ps_filter = true;
 	}
 
 	if (bar_index >= 0) {
@@ -719,8 +693,6 @@ static void ath_tx_count_airtime(struct ath_softc *sc, struct ath_node *an,
 
 		spin_lock_bh(&acq->lock);
 		an->airtime_deficit[q] -= airtime;
-		if (an->airtime_deficit[q] <= 0)
-			__ath_tx_queue_tid(sc, tid);
 		spin_unlock_bh(&acq->lock);
 	}
 	ath_debug_airtime(sc, an, 0, airtime);
@@ -770,8 +742,6 @@ static void ath_tx_process_buffer(struct ath_softc *sc, struct ath_txq *txq,
 	} else
 		ath_tx_complete_aggr(sc, txq, bf, bf_head, sta, tid, ts, txok);
 
-	if (!flush)
-		ath_txq_schedule(sc, txq);
 }
 
 static bool ath_lookup_legacy(struct ath_buf *bf)
@@ -1506,7 +1476,7 @@ ath_tx_form_burst(struct ath_softc *sc, struct ath_txq *txq,
 	} while (1);
 }
 
-static bool ath_tx_sched_aggr(struct ath_softc *sc, struct ath_txq *txq,
+static int ath_tx_sched_aggr(struct ath_softc *sc, struct ath_txq *txq,
 			      struct ath_atx_tid *tid)
 {
 	struct ath_buf *bf;
@@ -1515,21 +1485,18 @@ static bool ath_tx_sched_aggr(struct ath_softc *sc, struct ath_txq *txq,
 	int aggr_len = 0;
 	bool aggr;
 
-	if (!ath_tid_has_buffered(tid))
-		return false;
-
 	INIT_LIST_HEAD(&bf_q);
 
 	bf = ath_tx_get_tid_subframe(sc, txq, tid);
 	if (!bf)
-		return false;
+		return -ENOENT;
 
 	tx_info = IEEE80211_SKB_CB(bf->bf_mpdu);
 	aggr = !!(tx_info->flags & IEEE80211_TX_CTL_AMPDU);
 	if ((aggr && txq->axq_ampdu_depth >= ATH_AGGR_MIN_QDEPTH) ||
 	    (!aggr && txq->axq_depth >= ATH_NON_AGGR_MIN_QDEPTH)) {
 		__skb_queue_tail(&tid->retry_q, bf->bf_mpdu);
-		return false;
+		return -ENOBUFS;
 	}
 
 	ath_set_rates(tid->an->vif, tid->an->sta, bf);
@@ -1539,7 +1506,7 @@ static bool ath_tx_sched_aggr(struct ath_softc *sc, struct ath_txq *txq,
 		ath_tx_form_burst(sc, txq, tid, &bf_q, bf);
 
 	if (list_empty(&bf_q))
-		return false;
+		return -ENOENT;
 
 	if (tid->clear_ps_filter || tid->an->no_ps_filter) {
 		tid->clear_ps_filter = false;
@@ -1548,7 +1515,7 @@ static bool ath_tx_sched_aggr(struct ath_softc *sc, struct ath_txq *txq,
 
 	ath_tx_fill_desc(sc, bf, txq, aggr_len);
 	ath_tx_txqaddbuf(sc, txq, &bf_q, false);
-	return true;
+	return 0;
 }
 
 int ath_tx_aggr_start(struct ath_softc *sc, struct ieee80211_sta *sta,
@@ -1611,51 +1578,18 @@ void ath_tx_aggr_sleep(struct ieee80211_sta *sta, struct ath_softc *sc,
 {
 	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
 	struct ath_atx_tid *tid;
-	struct ath_txq *txq;
+	struct ieee80211_txq *queue;
 	int tidno;
 
 	ath_dbg(common, XMIT, "%s called\n", __func__);
 
 	for (tidno = 0; tidno < IEEE80211_NUM_TIDS; tidno++) {
 		tid = ath_node_to_tid(an, tidno);
-		txq = tid->txq;
-
-		ath_txq_lock(sc, txq);
-
-		if (list_empty(&tid->list)) {
-			ath_txq_unlock(sc, txq);
-			continue;
-		}
+		queue = container_of((void*)tid, struct ieee80211_txq, drv_priv);
 
 		if (!skb_queue_empty(&tid->retry_q))
-			ieee80211_sta_set_buffered(sta, tid->tidno, true);
+			queue->drv_buffered = true;
 
-		list_del_init(&tid->list);
-
-		ath_txq_unlock(sc, txq);
-	}
-}
-
-void ath_tx_aggr_wakeup(struct ath_softc *sc, struct ath_node *an)
-{
-	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
-	struct ath_atx_tid *tid;
-	struct ath_txq *txq;
-	int tidno;
-
-	ath_dbg(common, XMIT, "%s called\n", __func__);
-
-	for (tidno = 0; tidno < IEEE80211_NUM_TIDS; tidno++) {
-		tid = ath_node_to_tid(an, tidno);
-		txq = tid->txq;
-
-		ath_txq_lock(sc, txq);
-		tid->clear_ps_filter = true;
-		if (ath_tid_has_buffered(tid)) {
-			ath_tx_queue_tid(sc, tid);
-			ath_txq_schedule(sc, txq);
-		}
-		ath_txq_unlock_complete(sc, txq);
 	}
 }
 
@@ -1948,86 +1882,44 @@ void ath_tx_cleanupq(struct ath_softc *sc, struct ath_txq *txq)
 /* For each acq entry, for each tid, try to schedule packets
  * for transmit until ampdu_depth has reached min Q depth.
  */
-void ath_txq_schedule(struct ath_softc *sc, struct ath_txq *txq)
+void ath_txq_schedule(struct ath_softc *sc)
 {
+	struct ieee80211_hw *hw = sc->hw;
 	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	struct ieee80211_txq *queue;
 	struct ath_atx_tid *tid;
-	struct list_head *tid_list;
-	struct ath_acq *acq;
-	bool active = AIRTIME_ACTIVE(sc->airtime_flags);
+	struct ath_txq *txq;
+	int ret = 0;
 
-	if (txq->mac80211_qnum < 0)
+	if (test_bit(ATH_OP_HW_RESET, &common->op_flags))
 		return;
 
-	if (test_bit(ATH_OP_HW_RESET, &common->op_flags))
+	queue = ieee80211_next_txq(hw);
+	if (!queue)
 		return;
 
-	spin_lock_bh(&sc->chan_lock);
-	rcu_read_lock();
-	acq = &sc->cur_chan->acq[txq->mac80211_qnum];
+	tid = (struct ath_atx_tid *)queue->drv_priv;
+	txq = tid->txq;
 
-	if (sc->cur_chan->stopped)
+	ath_txq_lock(sc, txq);
+	if (txq->mac80211_qnum < 0)
 		goto out;
 
-begin:
-	tid_list = &acq->acq_new;
-	if (list_empty(tid_list)) {
-		tid_list = &acq->acq_old;
-		if (list_empty(tid_list))
-			goto out;
-	}
-	tid = list_first_entry(tid_list, struct ath_atx_tid, list);
-
-	if (active && tid->an->airtime_deficit[txq->mac80211_qnum] <= 0) {
-		spin_lock_bh(&acq->lock);
-		tid->an->airtime_deficit[txq->mac80211_qnum] += ATH_AIRTIME_QUANTUM;
-		list_move_tail(&tid->list, &acq->acq_old);
-		spin_unlock_bh(&acq->lock);
-		goto begin;
-	}
-
-	if (!ath_tid_has_buffered(tid)) {
-		spin_lock_bh(&acq->lock);
-		if ((tid_list == &acq->acq_new) && !list_empty(&acq->acq_old))
-			list_move_tail(&tid->list, &acq->acq_old);
-		else {
-			list_del_init(&tid->list);
-		}
-		spin_unlock_bh(&acq->lock);
-		goto begin;
-	}
+	spin_lock_bh(&sc->chan_lock);
+	rcu_read_lock();
 
+	if (!sc->cur_chan->stopped)
+		ret = ath_tx_sched_aggr(sc, txq, tid);
 
-	/*
-	 * If we succeed in scheduling something, immediately restart to make
-	 * sure we keep the HW busy.
-	 */
-	if(ath_tx_sched_aggr(sc, txq, tid)) {
-		if (!active) {
-			spin_lock_bh(&acq->lock);
-			list_move_tail(&tid->list, &acq->acq_old);
-			spin_unlock_bh(&acq->lock);
-		}
-		goto begin;
-	}
-
-out:
 	rcu_read_unlock();
 	spin_unlock_bh(&sc->chan_lock);
-}
 
-void ath_txq_schedule_all(struct ath_softc *sc)
-{
-	struct ath_txq *txq;
-	int i;
+out:
 
-	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
-		txq = sc->tx.txq_map[i];
+	if (ret != -ENOENT)
+		ieee80211_schedule_txq(hw, queue);
 
-		spin_lock_bh(&txq->axq_lock);
-		ath_txq_schedule(sc, txq);
-		spin_unlock_bh(&txq->axq_lock);
-	}
+	ath_txq_unlock(sc, txq);
 }
 
 /***********/
@@ -2645,7 +2537,6 @@ static void ath_tx_processq(struct ath_softc *sc, struct ath_txq *txq)
 
 		if (list_empty(&txq->axq_q)) {
 			txq->axq_link = NULL;
-			ath_txq_schedule(sc, txq);
 			break;
 		}
 		bf = list_first_entry(&txq->axq_q, struct ath_buf, list);
@@ -2697,6 +2588,7 @@ static void ath_tx_processq(struct ath_softc *sc, struct ath_txq *txq)
 		ath_tx_process_buffer(sc, txq, &ts, bf, &bf_head);
 	}
 	ath_txq_unlock_complete(sc, txq);
+	ath_txq_schedule(sc);
 }
 
 void ath_tx_tasklet(struct ath_softc *sc)
@@ -2711,6 +2603,7 @@ void ath_tx_tasklet(struct ath_softc *sc)
 			ath_tx_processq(sc, &sc->tx.txq[i]);
 	}
 	rcu_read_unlock();
+	ath_txq_schedule(sc);
 }
 
 void ath_tx_edma_tasklet(struct ath_softc *sc)
@@ -2796,6 +2689,7 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
 		ath_txq_unlock_complete(sc, txq);
 	}
 	rcu_read_unlock();
+	ath_txq_schedule(sc);
 }
 
 /*****************/
@@ -2875,7 +2769,6 @@ void ath_tx_node_init(struct ath_softc *sc, struct ath_node *an)
 		tid->baw_head  = tid->baw_tail = 0;
 		tid->active	   = false;
 		tid->clear_ps_filter = true;
-		tid->has_queued  = false;
 		__skb_queue_head_init(&tid->retry_q);
 		INIT_LIST_HEAD(&tid->list);
 		acno = TID_TO_WME_AC(tidno);
-- 
2.14.2

[RFC 3/3] ath10k: Move to mac80211 TXQ scheduling API

[Toke Høiland-Jørgensen]

This removes TXQ scheduling from ath10k and changes it to use the mac80211 TXQ
scheduling API introduced in the previous patch.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 drivers/net/wireless/ath/ath10k/core.c |  2 --
 drivers/net/wireless/ath/ath10k/core.h |  3 ---
 drivers/net/wireless/ath/ath10k/mac.c  | 46 +++++++---------------------------
 3 files changed, 9 insertions(+), 42 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index a4f635820f35..759df3297d48 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -2561,9 +2561,7 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev,
 
 	mutex_init(&ar->conf_mutex);
 	spin_lock_init(&ar->data_lock);
-	spin_lock_init(&ar->txqs_lock);
 
-	INIT_LIST_HEAD(&ar->txqs);
 	INIT_LIST_HEAD(&ar->peers);
 	init_waitqueue_head(&ar->peer_mapping_wq);
 	init_waitqueue_head(&ar->htt.empty_tx_wq);
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 949ebb3e967b..e7fc241addf6 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -892,10 +892,7 @@ struct ath10k {
 
 	/* protects shared structure data */
 	spinlock_t data_lock;
-	/* protects: ar->txqs, artxq->list */
-	spinlock_t txqs_lock;
 
-	struct list_head txqs;
 	struct list_head arvifs;
 	struct list_head peers;
 	struct ath10k_peer *peer_map[ATH10K_MAX_NUM_PEER_IDS];
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 5683f1a5330e..086d7602fc4a 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3825,7 +3825,6 @@ static void ath10k_mac_txq_init(struct ieee80211_txq *txq)
 
 static void ath10k_mac_txq_unref(struct ath10k *ar, struct ieee80211_txq *txq)
 {
-	struct ath10k_txq *artxq;
 	struct ath10k_skb_cb *cb;
 	struct sk_buff *msdu;
 	int msdu_id;
@@ -3833,12 +3832,6 @@ static void ath10k_mac_txq_unref(struct ath10k *ar, struct ieee80211_txq *txq)
 	if (!txq)
 		return;
 
-	artxq = (void *)txq->drv_priv;
-	spin_lock_bh(&ar->txqs_lock);
-	if (!list_empty(&artxq->list))
-		list_del_init(&artxq->list);
-	spin_unlock_bh(&ar->txqs_lock);
-
 	spin_lock_bh(&ar->htt.tx_lock);
 	idr_for_each_entry(&ar->htt.pending_tx, msdu, msdu_id) {
 		cb = ATH10K_SKB_CB(msdu);
@@ -3969,22 +3962,15 @@ void ath10k_mac_tx_push_pending(struct ath10k *ar)
 {
 	struct ieee80211_hw *hw = ar->hw;
 	struct ieee80211_txq *txq;
-	struct ath10k_txq *artxq;
-	struct ath10k_txq *last;
 	int ret;
 	int max;
 
 	if (ar->htt.num_pending_tx >= (ar->htt.max_num_pending_tx / 2))
 		return;
 
-	spin_lock_bh(&ar->txqs_lock);
 	rcu_read_lock();
 
-	last = list_last_entry(&ar->txqs, struct ath10k_txq, list);
-	while (!list_empty(&ar->txqs)) {
-		artxq = list_first_entry(&ar->txqs, struct ath10k_txq, list);
-		txq = container_of((void *)artxq, struct ieee80211_txq,
-				   drv_priv);
+	while ((txq = ieee80211_next_txq(hw))) {
 
 		/* Prevent aggressive sta/tid taking over tx queue */
 		max = 16;
@@ -3995,18 +3981,16 @@ void ath10k_mac_tx_push_pending(struct ath10k *ar)
 				break;
 		}
 
-		list_del_init(&artxq->list);
 		if (ret != -ENOENT)
-			list_add_tail(&artxq->list, &ar->txqs);
+			ieee80211_schedule_txq(hw, txq);
 
 		ath10k_htt_tx_txq_update(hw, txq);
 
-		if (artxq == last || (ret < 0 && ret != -ENOENT))
+		if (ret < 0 && ret != -ENOENT)
 			break;
 	}
 
 	rcu_read_unlock();
-	spin_unlock_bh(&ar->txqs_lock);
 }
 
 /************/
@@ -4240,34 +4224,22 @@ static void ath10k_mac_op_tx(struct ieee80211_hw *hw,
 	}
 }
 
-static void ath10k_mac_op_wake_tx_queue(struct ieee80211_hw *hw,
-					struct ieee80211_txq *txq)
+static void ath10k_mac_op_wake_tx_queue(struct ieee80211_hw *hw)
 {
-	struct ath10k *ar = hw->priv;
-	struct ath10k_txq *artxq = (void *)txq->drv_priv;
-	struct ieee80211_txq *f_txq;
-	struct ath10k_txq *f_artxq;
+	struct ieee80211_txq *txq;
 	int ret = 0;
 	int max = 16;
 
-	spin_lock_bh(&ar->txqs_lock);
-	if (list_empty(&artxq->list))
-		list_add_tail(&artxq->list, &ar->txqs);
-
-	f_artxq = list_first_entry(&ar->txqs, struct ath10k_txq, list);
-	f_txq = container_of((void *)f_artxq, struct ieee80211_txq, drv_priv);
-	list_del_init(&f_artxq->list);
+	txq = ieee80211_next_txq(hw);
 
-	while (ath10k_mac_tx_can_push(hw, f_txq) && max--) {
-		ret = ath10k_mac_tx_push_txq(hw, f_txq);
+	while (ath10k_mac_tx_can_push(hw, txq) && max--) {
+		ret = ath10k_mac_tx_push_txq(hw, txq);
 		if (ret)
 			break;
 	}
 	if (ret != -ENOENT)
-		list_add_tail(&f_artxq->list, &ar->txqs);
-	spin_unlock_bh(&ar->txqs_lock);
+		ieee80211_schedule_txq(hw, txq);
 
-	ath10k_htt_tx_txq_update(hw, f_txq);
 	ath10k_htt_tx_txq_update(hw, txq);
 }
 
-- 
2.14.2

Re: [RFC 1/3] mac80211: Add TXQ scheduling API

[Johannes Berg]

On Tue, 2017-10-10 at 16:02 +0200, Toke Høiland-Jørgensen wrote:

> +++ b/net/mac80211/agg-tx.c
> @@ -226,9 +226,11 @@ ieee80211_agg_start_txq(struct sta_info *sta,
> int tid, bool enable)
>  		clear_bit(IEEE80211_TXQ_AMPDU, &txqi->flags);
>  
>  	clear_bit(IEEE80211_TXQ_STOP, &txqi->flags);
> +	ieee80211_schedule_txq(&sta->sdata->local->hw, txq);
> +
>  	local_bh_disable();
>  	rcu_read_lock();
> -	drv_wake_tx_queue(sta->sdata->local, txqi);
> +	drv_wake_tx_queue(sta->sdata->local);
>  	rcu_read_unlock();
>  	local_bh_enable();

It seems like there could be some sort of TX batching here - maybe only
call the driver if the queue was actually scheduled?

Return true/false from ieee80211_schedule_txq() depending on whether it
was added or not, and then call the driver only if it was added just
now? That way, we can save a bunch of driver calls, batching the TX.
 
> @@ -1121,6 +1122,9 @@ struct ieee80211_local {
>  	struct codel_vars *cvars;
>  	struct codel_params cparams;
>  
> +	struct list_head active_txqs;
> +	spinlock_t active_txq_lock;

Is there much point in having a separate lock? We probably need the fq
lock in most places related to this anyway?

> +++ b/net/mac80211/sta_info.c
> @@ -1250,8 +1250,9 @@ void ieee80211_sta_ps_deliver_wakeup(struct
> sta_info *sta)
>  			if (!txq_has_queue(sta->sta.txq[i]))
>  				continue;
>  
> -			drv_wake_tx_queue(local, to_txq_info(sta-
> >sta.txq[i]));
> +			ieee80211_schedule_txq(&local->hw, sta-
> >sta.txq[i]);
>  		}
> +		drv_wake_tx_queue(local);

Again, calling the driver could be conditional on having done any
interesting work.

> @@ -1524,7 +1526,8 @@ static bool ieee80211_queue_skb(struct
> ieee80211_local *local,
>  	ieee80211_txq_enqueue(local, txqi, skb);
>  	spin_unlock_bh(&fq->lock);
>  
> -	drv_wake_tx_queue(local, txqi);
> +	ieee80211_schedule_txq(&local->hw, &txqi->txq);
> +	drv_wake_tx_queue(local);

ditto

> +void ieee80211_schedule_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);
> +
> +	if (!list_empty(&txqi->schedule_order))
> +		list_add_tail(&txqi->schedule_order, &local-
> >active_txqs);

What's with the !list_empty()? Seems inverted to me? You need to add it
if it's empty?

Also maybe you should only do that if the TXQ isn't *empty*, so the
driver could call this unconditionally?

> +struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw)
> +{
> +	struct ieee80211_local *local = hw_to_local(hw);
> +	struct txq_info *txqi = NULL;
> +
> +	spin_lock_bh(&local->active_txq_lock);
> +
> +	if (list_empty(&local->active_txqs))
> +		goto out;
> +
> +	txqi = list_first_entry(&local->active_txqs, struct
> txq_info, schedule_order);
> +	list_del_init(&txqi->schedule_order);
> +
> +out:
> +	spin_unlock_bh(&local->active_txq_lock);
> +
> +	return &txqi->txq;

You forgot
	if (!txqi)
		return NULL;

johannes

Re: [RFC 2/3] ath9k: Move to mac80211 TXQ scheduling API

[Johannes Berg]

On Tue, 2017-10-10 at 16:02 +0200, Toke Høiland-Jørgensen wrote:
> This removes TXQ scheduling from ath9k and changes it to use the
> mac80211 TXQ scheduling API introduced in the previous patch.

I can't really comment on this, but I'll note that the patches need to
be all combined into a single one eventually, so that it compiles.

johannes

Re: [RFC 1/3] mac80211: Add TXQ scheduling API

[Johannes Berg]

> In particular, I'm not sure what the right thing to do in regards to
> PS wakeup is...

Can you explain what you were _trying_ to do?

I don't like calling this "driver_buffered" because that's already a
term for frames that are buffered in the driver ... :-)

PS is complicated, we basically transmit, in the following order:
 * filtered frames (tx_filtered)
 * buffered frames (ps_tx_buf)
 * regular frames

This is when we _leave_ powersave. When we deliver frames while the
station is sleeping (PS-Poll or U-APSD), they don't even go through the
TXQ. They still come from this place, but currently go directly to the
->tx() method, which to me is actually pretty weird but that's what it
is now.

As I think I said in my other thread, we should probably eventually
just get rid of ps_tx_buf entirely, instead just keeping the frames on
the TXQ. Then, filtered can just be pushed onto txqi->frags [*], and we
get rid of having that separately as well.

Then, we've completely solved the wakeup scenario, we just start
scheduling that TXQ normally again.

For the deliver-while-sleeping (PS-Poll/U-APSD) scenario, I think the
driver should still pull frames, after calling something like
drv_release_buffered_frames(). We want this to be scheduled pretty much
immediately, so we shouldn't just put the TXQ into the normal rotation,
but otherwise it should work similarly - except limited to a certain
number of frames [**].
In this case the driver probably needs to pull the frames using a
different function so that we can
 a) tag the packets properly (more-data, EOSP)
 b) generate nulldata as EOSP container where needed
Thus, it seems likely that we'll want a separate function, "pull for PS
delivery" rather than the normal ieee80211_tx_dequeue().

johannes

[*] ok not exactly, if there are frags there already things get messy.
    but we can probably solve that somehow without needing more special
    cases

Re: [RFC 1/3] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Tue, 2017-10-10 at 16:02 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>
>> +++ b/net/mac80211/agg-tx.c
>> @@ -226,9 +226,11 @@ ieee80211_agg_start_txq(struct sta_info *sta,
>> int tid, bool enable)
>>  		clear_bit(IEEE80211_TXQ_AMPDU, &txqi->flags);
>>=20=20
>>  	clear_bit(IEEE80211_TXQ_STOP, &txqi->flags);
>> +	ieee80211_schedule_txq(&sta->sdata->local->hw, txq);
>> +
>>  	local_bh_disable();
>>  	rcu_read_lock();
>> -	drv_wake_tx_queue(sta->sdata->local, txqi);
>> +	drv_wake_tx_queue(sta->sdata->local);
>>  	rcu_read_unlock();
>>  	local_bh_enable();
>
> It seems like there could be some sort of TX batching here - maybe
> only call the driver if the queue was actually scheduled?

Yeah, I guess that should be doable.

>> @@ -1121,6 +1122,9 @@ struct ieee80211_local {
>>  	struct codel_vars *cvars;
>>  	struct codel_params cparams;
>>=20=20
>> +	struct list_head active_txqs;
>> +	spinlock_t active_txq_lock;
>
> Is there much point in having a separate lock? We probably need the fq
> lock in most places related to this anyway?

Well, once the scheduler gets a bit smarter it may be necessary to much
with the order of TXQs on there without touching any of the queues
(e.g., when calculating airtime usage on TX and RX completion). Not sure
if that is enough to warrant a separate lock, though; I hadn't thought
about just grabbing fq->lock...

>> +void ieee80211_schedule_txq(struct ieee80211_hw *hw,
>> +			     struct ieee80211_txq *txq)
>> +{
>> +	struct ieee80211_local *local =3D hw_to_local(hw);
>> +	struct txq_info *txqi =3D to_txq_info(txq);
>> +
>> +	spin_lock_bh(&local->active_txq_lock);
>> +
>> +	if (!list_empty(&txqi->schedule_order))
>> +		list_add_tail(&txqi->schedule_order, &local-
>> >active_txqs);
>
> What's with the !list_empty()? Seems inverted to me? You need to add
> it if it's empty?

Yeah, nice catch. This started out as an "unschedule" function before I
decided this way was better. Guess I forgot to switch the logic :)

> Also maybe you should only do that if the TXQ isn't *empty*, so the
> driver could call this unconditionally?

There can be cases where the driver wants the queue to be scheduled even
though it looks empty from mac80211's point of view. For ath9k, the
driver keeps its retry queue in the drv_priv part of the txq structure,
so it will check if that is empty before deciding to call the schedule
function.

This is also related to the PS behaviour, so guess this could be changed
once that is all TXQ-based...

-Toke

Re: [RFC 1/3] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

>> In particular, I'm not sure what the right thing to do in regards to
>> PS wakeup is...
>
> Can you explain what you were _trying_ to do?
>
> I don't like calling this "driver_buffered" because that's already a
> term for frames that are buffered in the driver ... :-)

Well I was trying to do The Right Thing(tm), obviously ;)

The driver_buffered field comes from the previous behaviour of ath9k: It
would basically set the station_buffered flag if there was something in
the retry queue at the time it goes to sleep. And on wakeup, it will
reschedule the txq if it has anything in the retry queue. And, well, it
just seemed odd that there was this duplicated logic in the driver and
mac80211, if the driver could just signal to mac80211 to reschedule for
it...

But I guess I was really just getting ahead of myself there; so the
right thing to do for now is to keep the old behaviour, and then fix it
properly afterwards?

> PS is complicated, we basically transmit, in the following order:
>  * filtered frames (tx_filtered)
>  * buffered frames (ps_tx_buf)
>  * regular frames
>
> This is when we _leave_ powersave. When we deliver frames while the
> station is sleeping (PS-Poll or U-APSD), they don't even go through the
> TXQ. They still come from this place, but currently go directly to the
> ->tx() method, which to me is actually pretty weird but that's what it
> is now.
>
> As I think I said in my other thread, we should probably eventually
> just get rid of ps_tx_buf entirely, instead just keeping the frames on
> the TXQ. Then, filtered can just be pushed onto txqi->frags [*], and we
> get rid of having that separately as well.
>
> Then, we've completely solved the wakeup scenario, we just start
> scheduling that TXQ normally again.

Yes, this makes sense. Each sleep period is pretty short, right? I.e.,
we don't need to deal with interactions between CoDel and the queue
being stopped for a long period of time?

> For the deliver-while-sleeping (PS-Poll/U-APSD) scenario, I think the
> driver should still pull frames, after calling something like
> drv_release_buffered_frames(). We want this to be scheduled pretty much
> immediately, so we shouldn't just put the TXQ into the normal rotation,
> but otherwise it should work similarly - except limited to a certain
> number of frames [**].
> In this case the driver probably needs to pull the frames using a
> different function so that we can
>  a) tag the packets properly (more-data, EOSP)
>  b) generate nulldata as EOSP container where needed
> Thus, it seems likely that we'll want a separate function, "pull for PS
> delivery" rather than the normal ieee80211_tx_dequeue().

I was envisioning that next_txq() could also make those kinds of
decisions (i.e., preempt the normal scheduling algorithm when a
"special" TXQ needs to be scheduled immediately). But not sure if that
is enough for this case?

-Toke

Re: [RFC 1/3] mac80211: Add TXQ scheduling API

[Johannes Berg]

> Well I was trying to do The Right Thing(tm), obviously ;)

:-)

> The driver_buffered field comes from the previous behaviour of ath9k:
> It would basically set the station_buffered flag if there was
> something in the retry queue at the time it goes to sleep. And on
> wakeup, it will reschedule the txq if it has anything in the retry
> queue. And, well, it just seemed odd that there was this duplicated
> logic in the driver and mac80211, if the driver could just signal to
> mac80211 to reschedule for it...

Ok. I'm not sure what ath9k does, but mac80211 makes a distinction
between "do I have something buffered" and "does the driver have
something buffered".

When the station wakes up it's pretty easy, we tell the driver and it
just has to send its own frames first.

But when we get a PS-Poll/U-APSD it's more complicated, and we have to
tell the driver "please release N frames you have buffered" or we may
have to tell it "please allow me to send N frames that I have buffered"
which is why we need the distinction of whether or not the driver has
something buffered.

Ultimately, with TXQs, I hope this distinction can go away because the
driver wouldn't buffer all by itself.

> But I guess I was really just getting ahead of myself there; so the
> right thing to do for now is to keep the old behaviour, and then fix
> it properly afterwards?

I guess that'd be easier.

> Yes, this makes sense. Each sleep period is pretty short, right? 

Not necessarily.

> I.e., we don't need to deal with interactions between CoDel and the
> queue being stopped for a long period of time?

I don't know enough about the possible interactions to answer that.
Sleep periods may be long, though typically if there's traffic for
stations then they want to retrieve that and will wake up relatively
soon, but "relatively soon" may still be on the order of hundreds of
milliseconds (or even seconds) because sometimes clients will only
listen to DTIM beacons (DTIM period * beacon interval), and e.g. with
WNM sleep mode it becomes even longer.

> > For the deliver-while-sleeping (PS-Poll/U-APSD) scenario, I think
> > the
> > driver should still pull frames, after calling something like
> > drv_release_buffered_frames(). We want this to be scheduled pretty
> > much
> > immediately, so we shouldn't just put the TXQ into the normal
> > rotation,
> > but otherwise it should work similarly - except limited to a
> > certain
> > number of frames [**].
> > In this case the driver probably needs to pull the frames using a
> > different function so that we can
> >  a) tag the packets properly (more-data, EOSP)
> >  b) generate nulldata as EOSP container where needed
> > Thus, it seems likely that we'll want a separate function, "pull
> > for PS
> > delivery" rather than the normal ieee80211_tx_dequeue().
> 
> I was envisioning that next_txq() could also make those kinds of
> decisions (i.e., preempt the normal scheduling algorithm when a
> "special" TXQ needs to be scheduled immediately). But not sure if
> that is enough for this case?

Hmm, not sure. We really want this to be scheduled pretty much
immediately because the other side will be waiting for the frames, and
if we don't get an answer out quickly it'll have to assume we're
broken. I don't know what the limit here is for our firmware, but we
should really get this out as soon as possible in this case.

johannes

Re: [RFC 1/3] mac80211: Add TXQ scheduling API

[Johannes Berg]

On Tue, 2017-10-10 at 19:51 +0200, Toke Høiland-Jørgensen wrote:

> > > +	struct list_head active_txqs;
> > > +	spinlock_t active_txq_lock;
> > 
> > Is there much point in having a separate lock? We probably need the
> > fq lock in most places related to this anyway?
> 
> Well, once the scheduler gets a bit smarter it may be necessary to
> much with the order of TXQs on there without touching any of the
> queues (e.g., when calculating airtime usage on TX and RX
> completion). Not sure if that is enough to warrant a separate lock,
> though; I hadn't thought about just grabbing fq->lock...

Ok, dunno. It seemed sort of related but perhaps it's not really.


> > Also maybe you should only do that if the TXQ isn't *empty*, so the
> > driver could call this unconditionally?
> 
> There can be cases where the driver wants the queue to be scheduled
> even though it looks empty from mac80211's point of view. For ath9k,
> the driver keeps its retry queue in the drv_priv part of the txq
> structure, so it will check if that is empty before deciding to call
> the schedule function.
> 
> This is also related to the PS behaviour, so guess this could be
> changed once that is all TXQ-based...

Interesting. I guess scheduling an empty queue doesn't really matter
for mac80211 anyway though - just some extra work if we try to get
frames from it.

johannes

Re: [RFC 1/3] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Tue, 2017-10-10 at 19:51 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>
>> > > +	struct list_head active_txqs;
>> > > +	spinlock_t active_txq_lock;
>> >=20
>> > Is there much point in having a separate lock? We probably need the
>> > fq lock in most places related to this anyway?
>>=20
>> Well, once the scheduler gets a bit smarter it may be necessary to
>> much with the order of TXQs on there without touching any of the
>> queues (e.g., when calculating airtime usage on TX and RX
>> completion). Not sure if that is enough to warrant a separate lock,
>> though; I hadn't thought about just grabbing fq->lock...
>
> Ok, dunno. It seemed sort of related but perhaps it's not really.

Yeah. I'll keep this in mind when I implement the airtime fairness
accounting.

>> > Also maybe you should only do that if the TXQ isn't *empty*, so the
>> > driver could call this unconditionally?
>>=20
>> There can be cases where the driver wants the queue to be scheduled
>> even though it looks empty from mac80211's point of view. For ath9k,
>> the driver keeps its retry queue in the drv_priv part of the txq
>> structure, so it will check if that is empty before deciding to call
>> the schedule function.
>>=20
>> This is also related to the PS behaviour, so guess this could be
>> changed once that is all TXQ-based...
>
> Interesting. I guess scheduling an empty queue doesn't really matter
> for mac80211 anyway though - just some extra work if we try to get
> frames from it.

Yes, and I figure giving the driver that option might be a good way to
have some flexibility.

-Toke

Re: [RFC 1/3] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

>> Well I was trying to do The Right Thing(tm), obviously ;)
>
> :-)
>
>> The driver_buffered field comes from the previous behaviour of ath9k:
>> It would basically set the station_buffered flag if there was
>> something in the retry queue at the time it goes to sleep. And on
>> wakeup, it will reschedule the txq if it has anything in the retry
>> queue. And, well, it just seemed odd that there was this duplicated
>> logic in the driver and mac80211, if the driver could just signal to
>> mac80211 to reschedule for it...
>
> Ok. I'm not sure what ath9k does, but mac80211 makes a distinction
> between "do I have something buffered" and "does the driver have
> something buffered".
>
> When the station wakes up it's pretty easy, we tell the driver and it
> just has to send its own frames first.
>
> But when we get a PS-Poll/U-APSD it's more complicated, and we have to
> tell the driver "please release N frames you have buffered" or we may
> have to tell it "please allow me to send N frames that I have buffered"
> which is why we need the distinction of whether or not the driver has
> something buffered.
>
> Ultimately, with TXQs, I hope this distinction can go away because the
> driver wouldn't buffer all by itself.

Yes, that would be good. Okay, I'll keep PS buffering the way it works
now.

>> But I guess I was really just getting ahead of myself there; so the
>> right thing to do for now is to keep the old behaviour, and then fix
>> it properly afterwards?
>
> I guess that'd be easier.
>
>> Yes, this makes sense. Each sleep period is pretty short, right? 
>
> Not necessarily.
>
>> I.e., we don't need to deal with interactions between CoDel and the
>> queue being stopped for a long period of time?
>
> I don't know enough about the possible interactions to answer that.
> Sleep periods may be long, though typically if there's traffic for
> stations then they want to retrieve that and will wake up relatively
> soon, but "relatively soon" may still be on the order of hundreds of
> milliseconds (or even seconds) because sometimes clients will only
> listen to DTIM beacons (DTIM period * beacon interval), and e.g. with
> WNM sleep mode it becomes even longer.

Ah. Right. Well, guess we'll have to cross that bridge once we get
there. Not sure what the right thing to do from the point of view of an
AQM in this instance. But at least making sure it doesn't drop the
packet at the head of every queue at wakeup would probably be good.

>> I was envisioning that next_txq() could also make those kinds of
>> decisions (i.e., preempt the normal scheduling algorithm when a
>> "special" TXQ needs to be scheduled immediately). But not sure if
>> that is enough for this case?
>
> Hmm, not sure. We really want this to be scheduled pretty much
> immediately because the other side will be waiting for the frames, and
> if we don't get an answer out quickly it'll have to assume we're
> broken. I don't know what the limit here is for our firmware, but we
> should really get this out as soon as possible in this case.

OK. But presumably it can't preempt packets already pushed to the
hardware, right? So telling the driver to immediately schedule a packet,
and making sure that the txq it gets from next_txq() is the right one
should do the trick? But I guess it's a bit of a roundabout way, which
may not be worth it to avoid an extra callback...

-Toke

Re: [RFC 1/3] mac80211: Add TXQ scheduling API

[Johannes Berg]

On Wed, 2017-10-11 at 16:06 +0200, Toke Høiland-Jørgensen wrote:

> > Hmm, not sure. We really want this to be scheduled pretty much
> > immediately because the other side will be waiting for the frames,
> > and
> > if we don't get an answer out quickly it'll have to assume we're
> > broken. I don't know what the limit here is for our firmware, but
> > we
> > should really get this out as soon as possible in this case.
> 
> OK. But presumably it can't preempt packets already pushed to the
> hardware, right? 

True. If there are still packets scheduled then it needs even more
driver tricks to drop those back to tx_pending first ...

> So telling the driver to immediately schedule a packet,
> and making sure that the txq it gets from next_txq() is the right one
> should do the trick? But I guess it's a bit of a roundabout way,
> which may not be worth it to avoid an extra callback...

Yeah, might work, but remember that we need to mangle the packet, and
for that we need to know how many packets will go out. E.g. with U-
APSD, if we tell the driver 8 packets are OK, and it only wants 6, then
that's acceptable, but we have to tag the last of those with EOSP ...

So one way or another I think we need a separate callback here, and
perhaps just have the driver do the EOSP tagging, or have a separate
function to pull the frames so mac80211 can do the tagging, dunno.

Note that this is only for _some_ drivers, others will implement much
of this in firmware.

johannes

Re: [RFC 1/3] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2017-10-11 at 16:06 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>
>> > Hmm, not sure. We really want this to be scheduled pretty much
>> > immediately because the other side will be waiting for the frames,
>> > and
>> > if we don't get an answer out quickly it'll have to assume we're
>> > broken. I don't know what the limit here is for our firmware, but
>> > we
>> > should really get this out as soon as possible in this case.
>>=20
>> OK. But presumably it can't preempt packets already pushed to the
>> hardware, right?=20
>
> True. If there are still packets scheduled then it needs even more
> driver tricks to drop those back to tx_pending first ...

Only for packets to the same station, right?

>> So telling the driver to immediately schedule a packet,
>> and making sure that the txq it gets from next_txq() is the right one
>> should do the trick? But I guess it's a bit of a roundabout way,
>> which may not be worth it to avoid an extra callback...
>
> Yeah, might work, but remember that we need to mangle the packet, and
> for that we need to know how many packets will go out. E.g. with U-
> APSD, if we tell the driver 8 packets are OK, and it only wants 6, then
> that's acceptable, but we have to tag the last of those with EOSP ...
>
> So one way or another I think we need a separate callback here, and
> perhaps just have the driver do the EOSP tagging, or have a separate
> function to pull the frames so mac80211 can do the tagging, dunno.

Yeah, sounds like it'll need a separate callback, or at least a flag.

What part of the standard do I have to read to learn how this is
supposed to work, BTW? Or even better, is there a resource that
describes how PS works that is more accessible than the standard itself?

> Note that this is only for _some_ drivers, others will implement much
> of this in firmware.

Right, of course. Fun times! :)

-Toke

Re: [RFC 1/3] mac80211: Add TXQ scheduling API

[Johannes Berg]

On Wed, 2017-10-11 at 16:29 +0200, Toke Høiland-Jørgensen wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > On Wed, 2017-10-11 at 16:06 +0200, Toke Høiland-Jørgensen wrote:
> > 
> > > > Hmm, not sure. We really want this to be scheduled pretty much
> > > > immediately because the other side will be waiting for the
> > > > frames,
> > > > and
> > > > if we don't get an answer out quickly it'll have to assume
> > > > we're
> > > > broken. I don't know what the limit here is for our firmware,
> > > > but
> > > > we
> > > > should really get this out as soon as possible in this case.
> > > 
> > > OK. But presumably it can't preempt packets already pushed to the
> > > hardware, right? 
> > 
> > True. If there are still packets scheduled then it needs even more
> > driver tricks to drop those back to tx_pending first ...
> 
> Only for packets to the same station, right?

Yes, but they need to be somehow taken off the queue to avoid
reordering. Usually the firmware will filter them, but synchronisation
is difficult, since what might happen is that some frames are already
filtered, others aren't, and then the later ones actually need to go
out while the earlier ones don't ... if that's not done, you get
reordering and the state machines get out of sync.

> What part of the standard do I have to read to learn how this is
> supposed to work, BTW? Or even better, is there a resource that
> describes how PS works that is more accessible than the standard
> itself?

Hmm. I don't think the standard will actually help you much here, it
just describes the over-the-air behaviour.

Most of the complexity here comes from having to deal with driver
buffering, mac80211 buffering, addressing the reordering problem
(described above) from queuing frames for multiple stations on the same
hw queue.

johannes