This patch series implements Airtime-based Queue Limit (AQL) in the mac80211 and Ath10k driver. It is based on an earlier version from the ChromiumOS tree[0].
This version has been tested with QCA9884 platform with 4.14 kernel. Tests show AQL is able to reduce latency by an order of magnitude in a congested environment without negative impact on the throughput.
[0] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1703105/7
Kan Yan (2):
mac80211: Implement Airtime-based Queue Limit (AQL)
ath10k: Enable Airtime-based Queue Limit (AQL)
drivers/net/wireless/ath/ath10k/mac.c | 7 ++-
drivers/net/wireless/ath/ath10k/txrx.c | 12 +++-
include/net/cfg80211.h | 7 +++
include/net/mac80211.h | 34 +++++++++++
net/mac80211/debugfs.c | 79 ++++++++++++++++++++++++++
net/mac80211/debugfs_sta.c | 43 ++++++++++----
net/mac80211/ieee80211_i.h | 4 ++
net/mac80211/main.c | 7 ++-
net/mac80211/sta_info.c | 23 ++++++++
net/mac80211/sta_info.h | 4 ++
net/mac80211/tx.c | 60 +++++++++++++++----
11 files changed, 253 insertions(+), 27 deletions(-)
--
Calculate the estimated airtime pending in the txqs and apply AQL to
prevent excessive amounts of packets being queued in the firmware queue.
Change-Id: I4047ce0938f7b97440fb040b16b74f2e49aa3433
Signed-off-by: Kan Yan <[email protected]>
---
drivers/net/wireless/ath/ath10k/mac.c | 7 +++++--
drivers/net/wireless/ath/ath10k/txrx.c | 12 +++++++++---
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 0606416dc971..37eb56c19f19 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3983,6 +3983,9 @@ static bool ath10k_mac_tx_can_push(struct ieee80211_hw *hw,
struct ath10k_txq *artxq = (void *)txq->drv_priv;
/* No need to get locks */
+ if (!ieee80211_txq_airtime_check(hw, txq))
+ return false;
+
if (ar->htt.tx_q_state.mode == HTT_TX_MODE_SWITCH_PUSH)
return true;
@@ -4014,8 +4017,6 @@ static u16 ath10k_mac_update_airtime(struct ath10k *ar,
if (!txq || !txq->sta)
return airtime;
- if (test_bit(WMI_SERVICE_REPORT_AIRTIME, ar->wmi.svc_map))
- return airtime;
spin_lock_bh(&ar->data_lock);
arsta = (struct ath10k_sta *)txq->sta->drv_priv;
@@ -4038,6 +4039,8 @@ static u16 ath10k_mac_update_airtime(struct ath10k *ar,
}
spin_unlock_bh(&ar->data_lock);
+ ieee80211_sta_register_pending_airtime(txq->sta, txq->tid, airtime,
+ false);
return airtime;
}
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index 4102df016931..9266d3dd6adb 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -84,9 +84,15 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
wake_up(&htt->empty_tx_wq);
spin_unlock_bh(&htt->tx_lock);
- if (txq && txq->sta && skb_cb->airtime_est)
- ieee80211_sta_register_airtime(txq->sta, txq->tid,
- skb_cb->airtime_est, 0);
+ if (txq && txq->sta && skb_cb->airtime_est) {
+ if (!test_bit(WMI_SERVICE_REPORT_AIRTIME, ar->wmi.svc_map))
+ ieee80211_sta_register_airtime(txq->sta, txq->tid,
+ skb_cb->airtime_est, 0);
+
+ ieee80211_sta_register_pending_airtime(txq->sta, txq->tid,
+ skb_cb->airtime_est,
+ true);
+ }
if (ar->bus_param.dev_type != ATH10K_DEV_TYPE_HL)
dma_unmap_single(dev, skb_cb->paddr, msdu->len, DMA_TO_DEVICE);
--
2.23.0.581.g78d2f28ef7-goog
In order for the Fq_CoDel integrated in mac80211 layer operates
effectively to control excessive queueing latency, the CoDel algorithm
requires an accurate measure of how long the packets stays in the
queue, aka sojourn time. The sojourn time measured at mac80211 layer
doesn't include queueing latency in lower layer (firmware/hardware)
and CoDel expects lower layer to have a short queue. However, most
802.11ac chipsets offload tasks such TX aggregation to firmware or
hardware, thus have a deep lower layer queue. Without a mechanism to
control the lower layer queue size, packets only stays in mac80211
layer transiently before being released to firmware due to the deep
queue in the lower layer. In this case, the sojourn time measured by
CoDel in the mac80211 layer is always lower than the CoDel latency
target hence it does little to control the latency, even when the lower
layer queue causes excessive latency.
Byte Queue limits (BQL) is commonly used to address the similar issue
with wired network interface. However, this method can not be applied
directly to the wireless network interface. Byte is not a suitable
measure of queue depth in the wireless network, as the data rate can
vary dramatically from station to station in the same network, from a
few Mbps to over 1 Gbps.
This patch implemented an Airtime-based Queue Limit (AQL) to make CoDel
works effectively with wireless drivers that utilized firmware/hardware
offloading. It only allows each txq to release just enough packets to
form 1-2 large aggregations to keep hardware fully utilized and keep the
rest of frames in mac80211 layer to be controlled by CoDel.
Change-Id: I1427db2c4c7fcb4162b087514dcb06d5613fa5d2
Signed-off-by: Kan Yan <[email protected]>
---
include/net/cfg80211.h | 7 ++++
include/net/mac80211.h | 34 ++++++++++++++++
net/mac80211/debugfs.c | 79 ++++++++++++++++++++++++++++++++++++++
net/mac80211/debugfs_sta.c | 43 +++++++++++++++------
net/mac80211/ieee80211_i.h | 4 ++
net/mac80211/main.c | 7 +++-
net/mac80211/sta_info.c | 23 +++++++++++
net/mac80211/sta_info.h | 4 ++
net/mac80211/tx.c | 60 ++++++++++++++++++++++++-----
9 files changed, 239 insertions(+), 22 deletions(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 26e2ad2c7027..301c11be366a 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2499,6 +2499,13 @@ enum wiphy_params_flags {
#define IEEE80211_DEFAULT_AIRTIME_WEIGHT 256
+/* The per TXQ firmware queue limit in airtime */
+#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L 4000
+#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H 8000
+
+/* The firmware's transmit queue size limit in airtime */
+#define IEEE80211_DEFAULT_AQL_INTERFACE_LIMIT 24000
+
/**
* struct cfg80211_pmksa - PMK Security Association
*
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d26da013f7c0..4d62aba3e4b2 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5543,6 +5543,40 @@ void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
u32 tx_airtime, u32 rx_airtime);
+/**
+ * ieee80211_sta_register_pending_airtime - register the estimated airtime for
+ * the frames pending in lower layer (firmware/hardware) txq.
+ *
+ * Update the total pending airtime of the frames released to firmware. The
+ * pending airtime is used by AQL to control queue size in the lower layer.
+ *
+ * The airtime is estimated using frame length and the last reported data
+ * rate. The pending airtime for a txq is increased by the estimated
+ * airtime when the frame is relased to the lower layer, and decreased by the
+ * same amount at the tx completion event.
+ *
+ * @pubsta: the station
+ * @tid: the TID to register airtime for
+ * @tx_airtime: the estimated airtime (in usec)
+ * @tx_commpleted: true if called from the tx completion event.
+ */
+void ieee80211_sta_register_pending_airtime(struct ieee80211_sta *pubsta,
+ u8 tid, u32 tx_airtime,
+ bool tx_completed);
+
+/**
+ * ieee80211_txq_aritme_check - check if the airtime limit of AQL (Airtime based
+ * queue limit) has been reached.
+ * @hw: pointer obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ * Retrun true if the queue limit has not been reached and txq can continue to
+ * release packets to the lower layer.
+ * Return false if the queue limit has been reached and the txq should not
+ * release more frames to the lower layer.
+ */
+bool
+ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
+
/**
* ieee80211_iter_keys - iterate keys programmed into the device
* @hw: pointer obtained from ieee80211_alloc_hw()
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 2e7f75938c51..a7da9a93b63e 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -148,6 +148,81 @@ static const struct file_operations aqm_ops = {
.llseek = default_llseek,
};
+static ssize_t aql_txq_limit_read(struct file *file,
+ char __user *user_buf,
+ size_t count,
+ loff_t *ppos)
+{
+ struct ieee80211_local *local = file->private_data;
+ char buf[400];
+ int len = 0;
+
+ rcu_read_lock();
+ len = scnprintf(buf, sizeof(buf),
+ "AC AQL limit low AQL limit high\n"
+ "0 %u %u\n"
+ "1 %u %u\n"
+ "2 %u %u\n"
+ "3 %u %u\n",
+ local->aql_txq_limit_low[0],
+ local->aql_txq_limit_high[0],
+ local->aql_txq_limit_low[1],
+ local->aql_txq_limit_high[1],
+ local->aql_txq_limit_low[2],
+ local->aql_txq_limit_high[2],
+ local->aql_txq_limit_low[3],
+ local->aql_txq_limit_high[3]);
+ rcu_read_unlock();
+ return simple_read_from_buffer(user_buf, count, ppos,
+ buf, len);
+}
+
+static ssize_t aql_txq_limit_write(struct file *file,
+ const char __user *user_buf,
+ size_t count,
+ loff_t *ppos)
+{
+ struct ieee80211_local *local = file->private_data;
+ char buf[100];
+ size_t len;
+ u32 ac, q_limit_low, q_limit_high;
+ struct sta_info *sta;
+
+ if (count > sizeof(buf))
+ return -EINVAL;
+
+ if (copy_from_user(buf, user_buf, count))
+ return -EFAULT;
+
+ buf[sizeof(buf) - 1] = '\0';
+ len = strlen(buf);
+ if (len > 0 && buf[len - 1] == '\n')
+ buf[len - 1] = 0;
+
+ if (sscanf(buf, "%u %u %u", &ac, &q_limit_low, &q_limit_high) == 3) {
+ if (ac < IEEE80211_NUM_ACS) {
+ local->aql_txq_limit_low[ac] = q_limit_low;
+ local->aql_txq_limit_high[ac] = q_limit_high;
+
+ mutex_lock(&local->sta_mtx);
+ list_for_each_entry(sta, &local->sta_list, list) {
+ sta->airtime[ac].aql_limit_low = q_limit_low;
+ sta->airtime[ac].aql_limit_high = q_limit_high;
+ }
+ mutex_unlock(&local->sta_mtx);
+ return count;
+ }
+ }
+ return -EINVAL;
+}
+
+static const struct file_operations aql_txq_limit_ops = {
+ .write = aql_txq_limit_write,
+ .read = aql_txq_limit_read,
+ .open = simple_open,
+ .llseek = default_llseek,
+};
+
static ssize_t force_tx_status_read(struct file *file,
char __user *user_buf,
size_t count,
@@ -442,6 +517,10 @@ void debugfs_hw_add(struct ieee80211_local *local)
debugfs_create_u16("airtime_flags", 0600,
phyd, &local->airtime_flags);
+ DEBUGFS_ADD(aql_txq_limit);
+ debugfs_create_u32("aql_interface_limit", 0600,
+ phyd, &local->aql_interface_limit);
+
statsd = debugfs_create_dir("statistics", phyd);
/* if the dir failed, don't put all the other things into the root! */
diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index c8ad20c28c43..3db2afb9f6c8 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -197,10 +197,12 @@ static ssize_t sta_airtime_read(struct file *file, char __user *userbuf,
{
struct sta_info *sta = file->private_data;
struct ieee80211_local *local = sta->sdata->local;
- size_t bufsz = 200;
+ size_t bufsz = 400;
char *buf = kzalloc(bufsz, GFP_KERNEL), *p = buf;
u64 rx_airtime = 0, tx_airtime = 0;
s64 deficit[IEEE80211_NUM_ACS];
+ u32 q_depth[IEEE80211_NUM_ACS];
+ u32 q_limit_l[IEEE80211_NUM_ACS], q_limit_h[IEEE80211_NUM_ACS];
ssize_t rv;
int ac;
@@ -212,19 +214,22 @@ static ssize_t sta_airtime_read(struct file *file, char __user *userbuf,
rx_airtime += sta->airtime[ac].rx_airtime;
tx_airtime += sta->airtime[ac].tx_airtime;
deficit[ac] = sta->airtime[ac].deficit;
+ q_limit_l[ac] = sta->airtime[ac].aql_limit_low;
+ q_limit_h[ac] = sta->airtime[ac].aql_limit_high;
+ q_depth[ac] = sta->airtime[ac].aql_tx_pending;
spin_unlock_bh(&local->active_txq_lock[ac]);
}
p += scnprintf(p, bufsz + buf - p,
"RX: %llu us\nTX: %llu us\nWeight: %u\n"
- "Deficit: VO: %lld us VI: %lld us BE: %lld us BK: %lld us\n",
- rx_airtime,
- tx_airtime,
- sta->airtime_weight,
- deficit[0],
- deficit[1],
- deficit[2],
- deficit[3]);
+ "Deficit: VO: %lld us VI: %lld us BE: %lld us BK: %lld us\n"
+ "Q depth: VO: %u us VI: %u us BE: %u us BK: %u us\n"
+ "Q limit[low/high]: VO: %u/%u VI: %u/%u BE: %u/%u BK: %u/%u\n",
+ rx_airtime, tx_airtime, sta->airtime_weight,
+ deficit[0], deficit[1], deficit[2], deficit[3],
+ q_depth[0], q_depth[1], q_depth[2], q_depth[3],
+ q_limit_l[0], q_limit_h[0], q_limit_l[1], q_limit_h[1],
+ q_limit_l[2], q_limit_h[2], q_limit_l[3], q_limit_h[3]),
rv = simple_read_from_buffer(userbuf, count, ppos, buf, p - buf);
kfree(buf);
@@ -236,7 +241,23 @@ static ssize_t sta_airtime_write(struct file *file, const char __user *userbuf,
{
struct sta_info *sta = file->private_data;
struct ieee80211_local *local = sta->sdata->local;
- int ac;
+ u32 ac, q_limit_l, q_limit_h;
+ char _buf[100] = {}, *buf = _buf;
+
+ if (count > sizeof(_buf))
+ return -EINVAL;
+
+ if (copy_from_user(buf, userbuf, count))
+ return -EFAULT;
+
+ buf[sizeof(_buf) - 1] = '\0';
+ if (sscanf(buf, "queue limit %u %u %u", &ac, &q_limit_l, &q_limit_h)
+ == 3) {
+ if (ac < IEEE80211_NUM_ACS) {
+ sta->airtime[ac].aql_limit_low = q_limit_l;
+ sta->airtime[ac].aql_limit_high = q_limit_h;
+ }
+ }
for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
spin_lock_bh(&local->active_txq_lock[ac]);
@@ -245,8 +266,8 @@ static ssize_t sta_airtime_write(struct file *file, const char __user *userbuf,
sta->airtime[ac].deficit = sta->airtime_weight;
spin_unlock_bh(&local->active_txq_lock[ac]);
}
-
return count;
+
}
STA_OPS_RW(airtime);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 004e2e3adb88..3b1508ed2570 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1142,6 +1142,10 @@ struct ieee80211_local {
u16 schedule_round[IEEE80211_NUM_ACS];
u16 airtime_flags;
+ u32 aql_txq_limit_low[IEEE80211_NUM_ACS];
+ u32 aql_txq_limit_high[IEEE80211_NUM_ACS];
+ u32 aql_interface_limit;
+ s32 aql_total_pending_airtime;
const struct ieee80211_ops *ops;
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 4c2702f128f3..960299317047 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -666,8 +666,13 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
for (i = 0; i < IEEE80211_NUM_ACS; i++) {
INIT_LIST_HEAD(&local->active_txqs[i]);
spin_lock_init(&local->active_txq_lock[i]);
+ local->aql_txq_limit_low[i] = IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L;
+ local->aql_txq_limit_high[i] = IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H;
}
- local->airtime_flags = AIRTIME_USE_TX | AIRTIME_USE_RX;
+
+ local->airtime_flags =
+ AIRTIME_USE_TX | AIRTIME_USE_RX | AIRTIME_USE_AQL;
+ local->aql_interface_limit = IEEE80211_DEFAULT_AQL_INTERFACE_LIMIT;
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 95eb8220e2e4..0baae63e0222 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -396,6 +396,9 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
skb_queue_head_init(&sta->ps_tx_buf[i]);
skb_queue_head_init(&sta->tx_filtered[i]);
sta->airtime[i].deficit = sta->airtime_weight;
+ sta->airtime[i].aql_tx_pending = 0;
+ sta->airtime[i].aql_limit_low = local->aql_txq_limit_low[i];
+ sta->airtime[i].aql_limit_high = local->aql_txq_limit_high[i];
}
for (i = 0; i < IEEE80211_NUM_TIDS; i++)
@@ -1894,6 +1897,26 @@ void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
}
EXPORT_SYMBOL(ieee80211_sta_register_airtime);
+void ieee80211_sta_register_pending_airtime(struct ieee80211_sta *pubsta,
+ u8 tid, u32 tx_airtime,
+ bool tx_completed)
+{
+ u8 ac = ieee80211_ac_from_tid(tid);
+ struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
+ struct ieee80211_local *local = sta->local;
+
+ spin_lock_bh(&local->active_txq_lock[ac]);
+ if (tx_completed) {
+ sta->airtime[ac].aql_tx_pending -= tx_airtime;
+ local->aql_total_pending_airtime -= tx_airtime;
+ } else {
+ sta->airtime[ac].aql_tx_pending += tx_airtime;
+ local->aql_total_pending_airtime += tx_airtime;
+ }
+ spin_unlock_bh(&local->active_txq_lock[ac]);
+}
+EXPORT_SYMBOL(ieee80211_sta_register_pending_airtime);
+
int sta_info_move_state(struct sta_info *sta,
enum ieee80211_sta_state new_state)
{
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 3260d4234920..d1ba4553c557 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -127,11 +127,15 @@ 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_USE_AQL BIT(2)
struct airtime_info {
u64 rx_airtime;
u64 tx_airtime;
s64 deficit;
+ s32 aql_tx_pending; /* Estimated airtime for frames pending in queue */
+ u32 aql_limit_low;
+ u32 aql_limit_high;
};
struct sta_info;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index f13eb2f61ccf..354ff4e4dce4 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3669,7 +3669,7 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
{
struct ieee80211_local *local = hw_to_local(hw);
struct ieee80211_txq *ret = NULL;
- struct txq_info *txqi = NULL;
+ struct txq_info *txqi = NULL, *head = NULL;
spin_lock_bh(&local->active_txq_lock[ac]);
@@ -3680,13 +3680,20 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
if (!txqi)
goto out;
+ if (txqi == head)
+ goto out;
+
+ if (!head)
+ head = txqi;
+
if (txqi->txq.sta) {
struct sta_info *sta = container_of(txqi->txq.sta,
struct sta_info, sta);
- if (sta->airtime[txqi->txq.ac].deficit < 0) {
+ if (sta->airtime[txqi->txq.ac].deficit < 0)
sta->airtime[txqi->txq.ac].deficit +=
sta->airtime_weight;
+ if (!ieee80211_txq_airtime_check(hw, &txqi->txq)) {
list_move_tail(&txqi->schedule_order,
&local->active_txqs[txqi->txq.ac]);
goto begin;
@@ -3726,9 +3733,7 @@ void __ieee80211_schedule_txq(struct ieee80211_hw *hw,
* get immediately moved to the back of the list on the next
* call to ieee80211_next_txq().
*/
- if (txqi->txq.sta &&
- wiphy_ext_feature_isset(local->hw.wiphy,
- NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
+ if (txqi->txq.sta && local->airtime_flags & AIRTIME_USE_TX)
list_add(&txqi->schedule_order,
&local->active_txqs[txq->ac]);
else
@@ -3740,6 +3745,37 @@ void __ieee80211_schedule_txq(struct ieee80211_hw *hw,
}
EXPORT_SYMBOL(__ieee80211_schedule_txq);
+bool ieee80211_txq_airtime_check(struct ieee80211_hw *hw,
+ struct ieee80211_txq *txq)
+{
+ struct sta_info *sta;
+ struct ieee80211_local *local = hw_to_local(hw);
+
+
+ if (!(local->airtime_flags & AIRTIME_USE_TX))
+ return true;
+
+ if (!txq->sta)
+ return true;
+
+ sta = container_of(txq->sta, struct sta_info, sta);
+ if (sta->airtime[txq->ac].deficit < 0)
+ return false;
+
+ if (!(local->airtime_flags & AIRTIME_USE_AQL))
+ return true;
+
+ if ((sta->airtime[txq->ac].aql_tx_pending <
+ sta->airtime[txq->ac].aql_limit_low) ||
+ (local->aql_total_pending_airtime < local->aql_interface_limit &&
+ sta->airtime[txq->ac].aql_tx_pending <
+ sta->airtime[txq->ac].aql_limit_high))
+ return true;
+ else
+ return false;
+}
+EXPORT_SYMBOL(ieee80211_txq_airtime_check);
+
bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
struct ieee80211_txq *txq)
{
@@ -3748,10 +3784,13 @@ bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
struct sta_info *sta;
u8 ac = txq->ac;
- spin_lock_bh(&local->active_txq_lock[ac]);
-
if (!txqi->txq.sta)
- goto out;
+ return true;
+
+ if (!(local->airtime_flags & AIRTIME_USE_TX))
+ return true;
+
+ spin_lock_bh(&local->active_txq_lock[ac]);
if (list_empty(&txqi->schedule_order))
goto out;
@@ -3773,10 +3812,11 @@ bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
}
sta = container_of(txqi->txq.sta, struct sta_info, sta);
- if (sta->airtime[ac].deficit >= 0)
+ if (ieee80211_txq_airtime_check(hw, &txqi->txq))
goto out;
- sta->airtime[ac].deficit += sta->airtime_weight;
+ if (sta->airtime[ac].deficit < 0)
+ sta->airtime[ac].deficit += sta->airtime_weight;
list_move_tail(&txqi->schedule_order, &local->active_txqs[ac]);
spin_unlock_bh(&local->active_txq_lock[ac]);
--
2.23.0.581.g78d2f28ef7-goog
Just took a very brief look so I can think about it while offline ;-)
But some (editorial) comments:
> +/**
> + * ieee80211_sta_register_pending_airtime - register the estimated airtime for
> + * the frames pending in lower layer (firmware/hardware) txq.
That doesn't work - the short description must be on a single line.
> + * Update the total pending airtime of the frames released to firmware. The
> + * pending airtime is used by AQL to control queue size in the lower layer.
What does "released to firmware" mean? I guess it should either be
something like "transmitted by the device" or "sitting on the hardware
queues" - I'm guessing the latter?
> + * The airtime is estimated using frame length and the last reported data
> + * rate. The pending airtime for a txq is increased by the estimated
> + * airtime when the frame is relased to the lower layer, and decreased by the
> + * same amount at the tx completion event.
> + *
> + * @pubsta: the station
> + * @tid: the TID to register airtime for
> + * @tx_airtime: the estimated airtime (in usec)
> + * @tx_commpleted: true if called from the tx completion event.
> + */
> +void ieee80211_sta_register_pending_airtime(struct ieee80211_sta *pubsta,
> + u8 tid, u32 tx_airtime,
> + bool tx_completed);
The "bool tx_completed" is a bit confusing IMHO.
Probably better to do something like this:
void ieee80211_sta_update_pending_airtime(sta, tid, *s32* airtime);
static inline void ieee80211_sta_register_pending_airtime(...)
{
ieee80211_sta_update_pending_airtime(..., airtime);
}
static inline void ieee80211_sta_release_pending_airtime(...)
{
ieee8021
1_sta_update_pending_airtime(..., -airtime);
}
or something like that?
> +/**
> + * ieee80211_txq_aritme_check - check if the airtime limit of AQL (Airtime based
> + * queue limit) has been reached.
same comment as above - and there's a typo
> + * @hw: pointer obtained from ieee80211_alloc_hw()
> + * @txq: pointer obtained from station or virtual interface
> + * Retrun
typo
> true if the queue limit has not been reached and txq can continue to
> + * release packets to the lower layer.
> + * Return false if the queue limit has been reached and the txq should not
> + * release more frames to the lower layer.
> + */
> +bool
> +ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
Why is it necessary to call this, vs. just returning NULL when an SKB is
requested?
> +static ssize_t aql_txq_limit_read(struct file *file,
> + char __user *user_buf,
> + size_t count,
> + loff_t *ppos)
> +{
> + struct ieee80211_local *local = file->private_data;
> + char buf[400];
> + int len = 0;
> +
> + rcu_read_lock();
> + len = scnprintf(buf, sizeof(buf),
> + "AC AQL limit low AQL limit high\n"
> + "0 %u %u\n"
> + "1 %u %u\n"
> + "2 %u %u\n"
> + "3 %u %u\n",
> + local->aql_txq_limit_low[0],
> + local->aql_txq_limit_high[0],
> + local->aql_txq_limit_low[1],
> + local->aql_txq_limit_high[1],
> + local->aql_txq_limit_low[2],
> + local->aql_txq_limit_high[2],
> + local->aql_txq_limit_low[3],
> + local->aql_txq_limit_high[3]);
> + rcu_read_unlock();
I don't understand the RCI critical section here, you do nothing that
would require it.
> + return simple_read_from_buffer(user_buf, count, ppos,
> + buf, len);
> +}
> +
> +static ssize_t aql_txq_limit_write(struct file *file,
> + const char __user *user_buf,
> + size_t count,
> + loff_t *ppos)
> +{
> + struct ieee80211_local *local = file->private_data;
> + char buf[100];
> + size_t len;
> + u32 ac, q_limit_low, q_limit_high;
> + struct sta_info *sta;
> +
> + if (count > sizeof(buf))
> + return -EINVAL;
> +
> + if (copy_from_user(buf, user_buf, count))
> + return -EFAULT;
> +
> + buf[sizeof(buf) - 1] = '\0';
> + len = strlen(buf);
> + if (len > 0 && buf[len - 1] == '\n')
> + buf[len - 1] = 0;
> +
> + if (sscanf(buf, "%u %u %u", &ac, &q_limit_low, &q_limit_high) == 3) {
> + if (ac < IEEE80211_NUM_ACS) {
The double indentation is a bit odd here - why not return -EINVAL
immediately if any of the checks doesn't work?
if (sscanf(...) != 3)
return -EINVAL;
if (ac >= NUM_ACS)
return -EINVAL;
[...]
> + buf[sizeof(_buf) - 1] = '\0';
> + if (sscanf(buf, "queue limit %u %u %u", &ac, &q_limit_l, &q_limit_h)
> + == 3) {
> + if (ac < IEEE80211_NUM_ACS) {
same here
> @@ -245,8 +266,8 @@ static ssize_t sta_airtime_write(struct file *file, const char __user *userbuf,
> sta->airtime[ac].deficit = sta->airtime_weight;
> spin_unlock_bh(&local->active_txq_lock[ac]);
> }
> -
> return count;
> +
> }
spurious change
> +void ieee80211_sta_register_pending_airtime(struct ieee80211_sta *pubsta,
> + u8 tid, u32 tx_airtime,
> + bool tx_completed)
> +{
> + u8 ac = ieee80211_ac_from_tid(tid);
> + struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
> + struct ieee80211_local *local = sta->local;
> +
> + spin_lock_bh(&local->active_txq_lock[ac]);
> + if (tx_completed) {
> + sta->airtime[ac].aql_tx_pending -= tx_airtime;
> + local->aql_total_pending_airtime -= tx_airtime;
maybe this should check for underflow, just in case the driver has some
symmetry bug?
> +bool ieee80211_txq_airtime_check(struct ieee80211_hw *hw,
> + struct ieee80211_txq *txq)
> +{
> + struct sta_info *sta;
> + struct ieee80211_local *local = hw_to_local(hw);
> +
> +
please remove one blank line
> bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
> struct ieee80211_txq *txq)
> {
> @@ -3748,10 +3784,13 @@ bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
> struct sta_info *sta;
> u8 ac = txq->ac;
>
> - spin_lock_bh(&local->active_txq_lock[ac]);
> -
> if (!txqi->txq.sta)
> - goto out;
> + return true;
> +
> + if (!(local->airtime_flags & AIRTIME_USE_TX))
> + return true;
> +
> + spin_lock_bh(&local->active_txq_lock[ac]);
>
> if (list_empty(&txqi->schedule_order))
> goto out;
> @@ -3773,10 +3812,11 @@ bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
> }
>
> sta = container_of(txqi->txq.sta, struct sta_info, sta);
> - if (sta->airtime[ac].deficit >= 0)
> + if (ieee80211_txq_airtime_check(hw, &txqi->txq))
> goto out;
OK, so you *do* call it here, but then why are you exporting it too?
> - sta->airtime[ac].deficit += sta->airtime_weight;
> + if (sta->airtime[ac].deficit < 0)
> + sta->airtime[ac].deficit += sta->airtime_weight;
something seems wrong with indentation here (spaces instead of tabs?)
Anyway, like I said - very cursory and mostly editorial view.
Thanks,
johannes
On Thu, 2019-10-03 at 23:21 -0700, Kan Yan wrote:
>
> +/* The firmware's transmit queue size limit in airtime */
> +#define IEEE80211_DEFAULT_AQL_INTERFACE_LIMIT 24000
I'm not sure I understand this completely, but IMHO rewording it to be
like a "NIC limit" would be better.
However, I'm not sure it *shouldn't* actually be per interface (i.e.
moving from
local->aql_total_pending_airtime
to
sdata->aql_total_pending_airtime
because you could have multiple channels etc. involved and then using a
single airtime limit across two interfaces that are actually on two
different channels (e.g. 2.4 and 5 GHz) doesn't make that much sense.
Actually, it does make some sense as long as the NIC is actually
channel-hopping ... but that's in the process of changing now, there's
going to be hardware really soon (or perhaps already exists) that has
real dual-band capabilities...
Maybe we can live with this now, but we'd probably expect to change this
really soon.
johannes
Kan Yan <[email protected]> writes:
> In order for the Fq_CoDel integrated in mac80211 layer operates
> effectively to control excessive queueing latency, the CoDel algorithm
> requires an accurate measure of how long the packets stays in the
> queue, aka sojourn time. The sojourn time measured at mac80211 layer
> doesn't include queueing latency in lower layer (firmware/hardware)
> and CoDel expects lower layer to have a short queue. However, most
> 802.11ac chipsets offload tasks such TX aggregation to firmware or
> hardware, thus have a deep lower layer queue. Without a mechanism to
> control the lower layer queue size, packets only stays in mac80211
> layer transiently before being released to firmware due to the deep
> queue in the lower layer. In this case, the sojourn time measured by
> CoDel in the mac80211 layer is always lower than the CoDel latency
> target hence it does little to control the latency, even when the lower
> layer queue causes excessive latency.
>
> Byte Queue limits (BQL) is commonly used to address the similar issue
> with wired network interface. However, this method can not be applied
> directly to the wireless network interface. Byte is not a suitable
> measure of queue depth in the wireless network, as the data rate can
> vary dramatically from station to station in the same network, from a
> few Mbps to over 1 Gbps.
>
> This patch implemented an Airtime-based Queue Limit (AQL) to make CoDel
> works effectively with wireless drivers that utilized firmware/hardware
> offloading. It only allows each txq to release just enough packets to
> form 1-2 large aggregations to keep hardware fully utilized and keep the
> rest of frames in mac80211 layer to be controlled by CoDel.
>
> Change-Id: I1427db2c4c7fcb4162b087514dcb06d5613fa5d2
> Signed-off-by: Kan Yan <[email protected]>
> ---
> include/net/cfg80211.h | 7 ++++
> include/net/mac80211.h | 34 ++++++++++++++++
> net/mac80211/debugfs.c | 79 ++++++++++++++++++++++++++++++++++++++
> net/mac80211/debugfs_sta.c | 43 +++++++++++++++------
> net/mac80211/ieee80211_i.h | 4 ++
> net/mac80211/main.c | 7 +++-
> net/mac80211/sta_info.c | 23 +++++++++++
> net/mac80211/sta_info.h | 4 ++
> net/mac80211/tx.c | 60 ++++++++++++++++++++++++-----
> 9 files changed, 239 insertions(+), 22 deletions(-)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 26e2ad2c7027..301c11be366a 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -2499,6 +2499,13 @@ enum wiphy_params_flags {
>
> #define IEEE80211_DEFAULT_AIRTIME_WEIGHT 256
>
> +/* The per TXQ firmware queue limit in airtime */
> +#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L 4000
> +#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H 8000
> +
> +/* The firmware's transmit queue size limit in airtime */
> +#define IEEE80211_DEFAULT_AQL_INTERFACE_LIMIT 24000
> +
> /**
> * struct cfg80211_pmksa - PMK Security Association
> *
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index d26da013f7c0..4d62aba3e4b2 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -5543,6 +5543,40 @@ void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
> void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
> u32 tx_airtime, u32 rx_airtime);
>
> +/**
> + * ieee80211_sta_register_pending_airtime - register the estimated airtime for
> + * the frames pending in lower layer (firmware/hardware) txq.
> + *
> + * Update the total pending airtime of the frames released to firmware. The
> + * pending airtime is used by AQL to control queue size in the lower layer.
> + *
> + * The airtime is estimated using frame length and the last reported data
> + * rate. The pending airtime for a txq is increased by the estimated
> + * airtime when the frame is relased to the lower layer, and decreased by the
> + * same amount at the tx completion event.
> + *
> + * @pubsta: the station
> + * @tid: the TID to register airtime for
> + * @tx_airtime: the estimated airtime (in usec)
> + * @tx_commpleted: true if called from the tx completion event.
> + */
> +void ieee80211_sta_register_pending_airtime(struct ieee80211_sta *pubsta,
> + u8 tid, u32 tx_airtime,
> + bool tx_completed);
> +
> +/**
> + * ieee80211_txq_aritme_check - check if the airtime limit of AQL (Airtime based
> + * queue limit) has been reached.
> + * @hw: pointer obtained from ieee80211_alloc_hw()
> + * @txq: pointer obtained from station or virtual interface
> + * Retrun true if the queue limit has not been reached and txq can continue to
> + * release packets to the lower layer.
> + * Return false if the queue limit has been reached and the txq should not
> + * release more frames to the lower layer.
> + */
> +bool
> +ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
> +
So I think it'll be better to pass up the last_rate and have mac80211
calculate the airtime (like I did in my patch set). But we can keep the
rest of your logic and just switch the calculation, I guess?
I'd like to use the new rate calculation code that Felix added to mt76.
Is the arsta->txrate info in ath10k suitable to be passed up to mac80211
and used in that, do you think? Because then that would probably be the
easiest way to go about it...
[...]
> @@ -3726,9 +3733,7 @@ void __ieee80211_schedule_txq(struct ieee80211_hw *hw,
> * get immediately moved to the back of the list on the next
> * call to ieee80211_next_txq().
> */
> - if (txqi->txq.sta &&
> - wiphy_ext_feature_isset(local->hw.wiphy,
> - NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
> + if (txqi->txq.sta && local->airtime_flags & AIRTIME_USE_TX)
This is wrong. The USE_TX/USE_RX flags just set which types of airtime
will be subtracted from the deficit. We should still be running the
scheduler if *either* of them is set...
> list_add(&txqi->schedule_order,
> &local->active_txqs[txq->ac]);
> else
> @@ -3740,6 +3745,37 @@ void __ieee80211_schedule_txq(struct ieee80211_hw *hw,
> }
> EXPORT_SYMBOL(__ieee80211_schedule_txq);
>
> +bool ieee80211_txq_airtime_check(struct ieee80211_hw *hw,
> + struct ieee80211_txq *txq)
> +{
> + struct sta_info *sta;
> + struct ieee80211_local *local = hw_to_local(hw);
> +
> +
> + if (!(local->airtime_flags & AIRTIME_USE_TX))
Ditto.
> + return true;
> +
> + if (!txq->sta)
> + return true;
> +
> + sta = container_of(txq->sta, struct sta_info, sta);
> + if (sta->airtime[txq->ac].deficit < 0)
> + return false;
> +
> + if (!(local->airtime_flags & AIRTIME_USE_AQL))
> + return true;
> +
> + if ((sta->airtime[txq->ac].aql_tx_pending <
> + sta->airtime[txq->ac].aql_limit_low) ||
> + (local->aql_total_pending_airtime < local->aql_interface_limit &&
> + sta->airtime[txq->ac].aql_tx_pending <
> + sta->airtime[txq->ac].aql_limit_high))
> + return true;
> + else
> + return false;
> +}
> +EXPORT_SYMBOL(ieee80211_txq_airtime_check);
> +
> bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
> struct ieee80211_txq *txq)
> {
> @@ -3748,10 +3784,13 @@ bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
> struct sta_info *sta;
> u8 ac = txq->ac;
>
> - spin_lock_bh(&local->active_txq_lock[ac]);
> -
> if (!txqi->txq.sta)
> - goto out;
> + return true;
> +
> + if (!(local->airtime_flags & AIRTIME_USE_TX))
Ditto.
-Toke
Hi Johannes,
Thanks for help review this patch. I will fix all style errors.
> > +ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
> Why is it necessary to call this, vs. just returning NULL when an SKB is
> requested?
This function is also called by ath10k, from ath10k_mac_tx_can_push(),
which returns a boolean.
> However, I'm not sure it *shouldn't* actually be per interface (i.e.
> moving from
> local->aql_total_pending_airtime
> to
>
> sdata->aql_total_pending_airtime
>
> because you could have multiple channels etc. involved and then using a
> single airtime limit across two interfaces that are actually on two
> different channels (e.g. 2.4 and 5 GHz) doesn't make that much sense.
>
> Actually, it does make some sense as long as the NIC is actually
> channel-hopping ... but that's in the process of changing now, there's
> going to be hardware really soon (or perhaps already exists) that has
> real dual-band capabilities...
That's a good point. I haven't thought about real simultaneous dual
band chipset and such chipset do exists now. Is RSDB support coming to
mac80211 soon? Just curious if it will be just virtual interfaces or
something else. I chose "local" instead of "sdata" thinking about the
case of several virtual interfaces (AP, STA, MESH) operates in the
same channel, then the interface total could be a better choice.
I am ok with moving the "aql_total_pending_airtime" into sdata, but
afraid that's not the most optimal choice for the case of multiple
virtual interfaces operates in the same channel.
Maybe we could leave it in "local" for now. What do you think?
Kan
On Fri, Oct 4, 2019 at 8:44 AM Toke Høiland-Jørgensen <[email protected]> wrote:
>
> Kan Yan <[email protected]> writes:
>
> > In order for the Fq_CoDel integrated in mac80211 layer operates
> > effectively to control excessive queueing latency, the CoDel algorithm
> > requires an accurate measure of how long the packets stays in the
> > queue, aka sojourn time. The sojourn time measured at mac80211 layer
> > doesn't include queueing latency in lower layer (firmware/hardware)
> > and CoDel expects lower layer to have a short queue. However, most
> > 802.11ac chipsets offload tasks such TX aggregation to firmware or
> > hardware, thus have a deep lower layer queue. Without a mechanism to
> > control the lower layer queue size, packets only stays in mac80211
> > layer transiently before being released to firmware due to the deep
> > queue in the lower layer. In this case, the sojourn time measured by
> > CoDel in the mac80211 layer is always lower than the CoDel latency
> > target hence it does little to control the latency, even when the lower
> > layer queue causes excessive latency.
> >
> > Byte Queue limits (BQL) is commonly used to address the similar issue
> > with wired network interface. However, this method can not be applied
> > directly to the wireless network interface. Byte is not a suitable
> > measure of queue depth in the wireless network, as the data rate can
> > vary dramatically from station to station in the same network, from a
> > few Mbps to over 1 Gbps.
> >
> > This patch implemented an Airtime-based Queue Limit (AQL) to make CoDel
> > works effectively with wireless drivers that utilized firmware/hardware
> > offloading. It only allows each txq to release just enough packets to
> > form 1-2 large aggregations to keep hardware fully utilized and keep the
> > rest of frames in mac80211 layer to be controlled by CoDel.
> >
> > Change-Id: I1427db2c4c7fcb4162b087514dcb06d5613fa5d2
> > Signed-off-by: Kan Yan <[email protected]>
> > ---
> > include/net/cfg80211.h | 7 ++++
> > include/net/mac80211.h | 34 ++++++++++++++++
> > net/mac80211/debugfs.c | 79 ++++++++++++++++++++++++++++++++++++++
> > net/mac80211/debugfs_sta.c | 43 +++++++++++++++------
> > net/mac80211/ieee80211_i.h | 4 ++
> > net/mac80211/main.c | 7 +++-
> > net/mac80211/sta_info.c | 23 +++++++++++
> > net/mac80211/sta_info.h | 4 ++
> > net/mac80211/tx.c | 60 ++++++++++++++++++++++++-----
> > 9 files changed, 239 insertions(+), 22 deletions(-)
> >
> > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> > index 26e2ad2c7027..301c11be366a 100644
> > --- a/include/net/cfg80211.h
> > +++ b/include/net/cfg80211.h
> > @@ -2499,6 +2499,13 @@ enum wiphy_params_flags {
> >
> > #define IEEE80211_DEFAULT_AIRTIME_WEIGHT 256
> >
> > +/* The per TXQ firmware queue limit in airtime */
> > +#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L 4000
> > +#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H 8000
> > +
> > +/* The firmware's transmit queue size limit in airtime */
> > +#define IEEE80211_DEFAULT_AQL_INTERFACE_LIMIT 24000
> > +
> > /**
> > * struct cfg80211_pmksa - PMK Security Association
> > *
> > diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> > index d26da013f7c0..4d62aba3e4b2 100644
> > --- a/include/net/mac80211.h
> > +++ b/include/net/mac80211.h
> > @@ -5543,6 +5543,40 @@ void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
> > void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
> > u32 tx_airtime, u32 rx_airtime);
> >
> > +/**
> > + * ieee80211_sta_register_pending_airtime - register the estimated airtime for
> > + * the frames pending in lower layer (firmware/hardware) txq.
> > + *
> > + * Update the total pending airtime of the frames released to firmware. The
> > + * pending airtime is used by AQL to control queue size in the lower layer.
> > + *
> > + * The airtime is estimated using frame length and the last reported data
> > + * rate. The pending airtime for a txq is increased by the estimated
> > + * airtime when the frame is relased to the lower layer, and decreased by the
> > + * same amount at the tx completion event.
> > + *
> > + * @pubsta: the station
> > + * @tid: the TID to register airtime for
> > + * @tx_airtime: the estimated airtime (in usec)
> > + * @tx_commpleted: true if called from the tx completion event.
> > + */
> > +void ieee80211_sta_register_pending_airtime(struct ieee80211_sta *pubsta,
> > + u8 tid, u32 tx_airtime,
> > + bool tx_completed);
> > +
> > +/**
> > + * ieee80211_txq_aritme_check - check if the airtime limit of AQL (Airtime based
> > + * queue limit) has been reached.
> > + * @hw: pointer obtained from ieee80211_alloc_hw()
> > + * @txq: pointer obtained from station or virtual interface
> > + * Retrun true if the queue limit has not been reached and txq can continue to
> > + * release packets to the lower layer.
> > + * Return false if the queue limit has been reached and the txq should not
> > + * release more frames to the lower layer.
> > + */
> > +bool
> > +ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
> > +
>
> So I think it'll be better to pass up the last_rate and have mac80211
> calculate the airtime (like I did in my patch set). But we can keep the
> rest of your logic and just switch the calculation, I guess?
>
> I'd like to use the new rate calculation code that Felix added to mt76.
> Is the arsta->txrate info in ath10k suitable to be passed up to mac80211
> and used in that, do you think? Because then that would probably be the
> easiest way to go about it...
>
> [...]
>
> > @@ -3726,9 +3733,7 @@ void __ieee80211_schedule_txq(struct ieee80211_hw *hw,
> > * get immediately moved to the back of the list on the next
> > * call to ieee80211_next_txq().
> > */
> > - if (txqi->txq.sta &&
> > - wiphy_ext_feature_isset(local->hw.wiphy,
> > - NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
> > + if (txqi->txq.sta && local->airtime_flags & AIRTIME_USE_TX)
>
> This is wrong. The USE_TX/USE_RX flags just set which types of airtime
> will be subtracted from the deficit. We should still be running the
> scheduler if *either* of them is set...
>
> > list_add(&txqi->schedule_order,
> > &local->active_txqs[txq->ac]);
> > else
> > @@ -3740,6 +3745,37 @@ void __ieee80211_schedule_txq(struct ieee80211_hw *hw,
> > }
> > EXPORT_SYMBOL(__ieee80211_schedule_txq);
> >
> > +bool ieee80211_txq_airtime_check(struct ieee80211_hw *hw,
> > + struct ieee80211_txq *txq)
> > +{
> > + struct sta_info *sta;
> > + struct ieee80211_local *local = hw_to_local(hw);
> > +
> > +
> > + if (!(local->airtime_flags & AIRTIME_USE_TX))
>
> Ditto.
>
> > + return true;
> > +
> > + if (!txq->sta)
> > + return true;
> > +
> > + sta = container_of(txq->sta, struct sta_info, sta);
> > + if (sta->airtime[txq->ac].deficit < 0)
> > + return false;
> > +
> > + if (!(local->airtime_flags & AIRTIME_USE_AQL))
> > + return true;
> > +
> > + if ((sta->airtime[txq->ac].aql_tx_pending <
> > + sta->airtime[txq->ac].aql_limit_low) ||
> > + (local->aql_total_pending_airtime < local->aql_interface_limit &&
> > + sta->airtime[txq->ac].aql_tx_pending <
> > + sta->airtime[txq->ac].aql_limit_high))
> > + return true;
> > + else
> > + return false;
> > +}
> > +EXPORT_SYMBOL(ieee80211_txq_airtime_check);
> > +
> > bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
> > struct ieee80211_txq *txq)
> > {
> > @@ -3748,10 +3784,13 @@ bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
> > struct sta_info *sta;
> > u8 ac = txq->ac;
> >
> > - spin_lock_bh(&local->active_txq_lock[ac]);
> > -
> > if (!txqi->txq.sta)
> > - goto out;
> > + return true;
> > +
> > + if (!(local->airtime_flags & AIRTIME_USE_TX))
>
> Ditto.
>
>
> -Toke
>
Hi Toke,
> So I think it'll be better to pass up the last_rate and have mac80211
> calculate the airtime (like I did in my patch set). But we can keep the
> rest of your logic and just switch the calculation, I guess?
Sounds like a good plan to me. Moving the airtime calculation do makes
it easier to support multiple drivers. The only benefit of keep that
part in driver is we can avoid extending ieee80211_tx_info with a new
field "tx_time_est".
I will fix the "AIRTIME_USE_TX" part and prepare a new patch.
Thanks,
Kan
On Fri, Oct 4, 2019 at 7:08 PM Kan Yan <[email protected]> wrote:
>
> Hi Johannes,
>
> Thanks for help review this patch. I will fix all style errors.
>
> > > +ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
> > Why is it necessary to call this, vs. just returning NULL when an SKB is
> > requested?
> This function is also called by ath10k, from ath10k_mac_tx_can_push(),
> which returns a boolean.
>
> > However, I'm not sure it *shouldn't* actually be per interface (i.e.
> > moving from
> > local->aql_total_pending_airtime
> > to
> >
> > sdata->aql_total_pending_airtime
> >
> > because you could have multiple channels etc. involved and then using a
> > single airtime limit across two interfaces that are actually on two
> > different channels (e.g. 2.4 and 5 GHz) doesn't make that much sense.
> >
> > Actually, it does make some sense as long as the NIC is actually
> > channel-hopping ... but that's in the process of changing now, there's
> > going to be hardware really soon (or perhaps already exists) that has
> > real dual-band capabilities...
>
> That's a good point. I haven't thought about real simultaneous dual
> band chipset and such chipset do exists now. Is RSDB support coming to
> mac80211 soon? Just curious if it will be just virtual interfaces or
> something else. I chose "local" instead of "sdata" thinking about the
> case of several virtual interfaces (AP, STA, MESH) operates in the
> same channel, then the interface total could be a better choice.
>
> I am ok with moving the "aql_total_pending_airtime" into sdata, but
> afraid that's not the most optimal choice for the case of multiple
> virtual interfaces operates in the same channel.
> Maybe we could leave it in "local" for now. What do you think?
>
> Kan
>
> On Fri, Oct 4, 2019 at 8:44 AM Toke Høiland-Jørgensen <[email protected]> wrote:
> >
> > Kan Yan <[email protected]> writes:
> >
> > > In order for the Fq_CoDel integrated in mac80211 layer operates
> > > effectively to control excessive queueing latency, the CoDel algorithm
> > > requires an accurate measure of how long the packets stays in the
> > > queue, aka sojourn time. The sojourn time measured at mac80211 layer
> > > doesn't include queueing latency in lower layer (firmware/hardware)
> > > and CoDel expects lower layer to have a short queue. However, most
> > > 802.11ac chipsets offload tasks such TX aggregation to firmware or
> > > hardware, thus have a deep lower layer queue. Without a mechanism to
> > > control the lower layer queue size, packets only stays in mac80211
> > > layer transiently before being released to firmware due to the deep
> > > queue in the lower layer. In this case, the sojourn time measured by
> > > CoDel in the mac80211 layer is always lower than the CoDel latency
> > > target hence it does little to control the latency, even when the lower
> > > layer queue causes excessive latency.
> > >
> > > Byte Queue limits (BQL) is commonly used to address the similar issue
> > > with wired network interface. However, this method can not be applied
> > > directly to the wireless network interface. Byte is not a suitable
> > > measure of queue depth in the wireless network, as the data rate can
> > > vary dramatically from station to station in the same network, from a
> > > few Mbps to over 1 Gbps.
> > >
> > > This patch implemented an Airtime-based Queue Limit (AQL) to make CoDel
> > > works effectively with wireless drivers that utilized firmware/hardware
> > > offloading. It only allows each txq to release just enough packets to
> > > form 1-2 large aggregations to keep hardware fully utilized and keep the
> > > rest of frames in mac80211 layer to be controlled by CoDel.
> > >
> > > Change-Id: I1427db2c4c7fcb4162b087514dcb06d5613fa5d2
> > > Signed-off-by: Kan Yan <[email protected]>
> > > ---
> > > include/net/cfg80211.h | 7 ++++
> > > include/net/mac80211.h | 34 ++++++++++++++++
> > > net/mac80211/debugfs.c | 79 ++++++++++++++++++++++++++++++++++++++
> > > net/mac80211/debugfs_sta.c | 43 +++++++++++++++------
> > > net/mac80211/ieee80211_i.h | 4 ++
> > > net/mac80211/main.c | 7 +++-
> > > net/mac80211/sta_info.c | 23 +++++++++++
> > > net/mac80211/sta_info.h | 4 ++
> > > net/mac80211/tx.c | 60 ++++++++++++++++++++++++-----
> > > 9 files changed, 239 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> > > index 26e2ad2c7027..301c11be366a 100644
> > > --- a/include/net/cfg80211.h
> > > +++ b/include/net/cfg80211.h
> > > @@ -2499,6 +2499,13 @@ enum wiphy_params_flags {
> > >
> > > #define IEEE80211_DEFAULT_AIRTIME_WEIGHT 256
> > >
> > > +/* The per TXQ firmware queue limit in airtime */
> > > +#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L 4000
> > > +#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H 8000
> > > +
> > > +/* The firmware's transmit queue size limit in airtime */
> > > +#define IEEE80211_DEFAULT_AQL_INTERFACE_LIMIT 24000
> > > +
> > > /**
> > > * struct cfg80211_pmksa - PMK Security Association
> > > *
> > > diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> > > index d26da013f7c0..4d62aba3e4b2 100644
> > > --- a/include/net/mac80211.h
> > > +++ b/include/net/mac80211.h
> > > @@ -5543,6 +5543,40 @@ void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
> > > void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
> > > u32 tx_airtime, u32 rx_airtime);
> > >
> > > +/**
> > > + * ieee80211_sta_register_pending_airtime - register the estimated airtime for
> > > + * the frames pending in lower layer (firmware/hardware) txq.
> > > + *
> > > + * Update the total pending airtime of the frames released to firmware. The
> > > + * pending airtime is used by AQL to control queue size in the lower layer.
> > > + *
> > > + * The airtime is estimated using frame length and the last reported data
> > > + * rate. The pending airtime for a txq is increased by the estimated
> > > + * airtime when the frame is relased to the lower layer, and decreased by the
> > > + * same amount at the tx completion event.
> > > + *
> > > + * @pubsta: the station
> > > + * @tid: the TID to register airtime for
> > > + * @tx_airtime: the estimated airtime (in usec)
> > > + * @tx_commpleted: true if called from the tx completion event.
> > > + */
> > > +void ieee80211_sta_register_pending_airtime(struct ieee80211_sta *pubsta,
> > > + u8 tid, u32 tx_airtime,
> > > + bool tx_completed);
> > > +
> > > +/**
> > > + * ieee80211_txq_aritme_check - check if the airtime limit of AQL (Airtime based
> > > + * queue limit) has been reached.
> > > + * @hw: pointer obtained from ieee80211_alloc_hw()
> > > + * @txq: pointer obtained from station or virtual interface
> > > + * Retrun true if the queue limit has not been reached and txq can continue to
> > > + * release packets to the lower layer.
> > > + * Return false if the queue limit has been reached and the txq should not
> > > + * release more frames to the lower layer.
> > > + */
> > > +bool
> > > +ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
> > > +
> >
> > So I think it'll be better to pass up the last_rate and have mac80211
> > calculate the airtime (like I did in my patch set). But we can keep the
> > rest of your logic and just switch the calculation, I guess?
> >
> > I'd like to use the new rate calculation code that Felix added to mt76.
> > Is the arsta->txrate info in ath10k suitable to be passed up to mac80211
> > and used in that, do you think? Because then that would probably be the
> > easiest way to go about it...
> >
> > [...]
> >
> > > @@ -3726,9 +3733,7 @@ void __ieee80211_schedule_txq(struct ieee80211_hw *hw,
> > > * get immediately moved to the back of the list on the next
> > > * call to ieee80211_next_txq().
> > > */
> > > - if (txqi->txq.sta &&
> > > - wiphy_ext_feature_isset(local->hw.wiphy,
> > > - NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
> > > + if (txqi->txq.sta && local->airtime_flags & AIRTIME_USE_TX)
> >
> > This is wrong. The USE_TX/USE_RX flags just set which types of airtime
> > will be subtracted from the deficit. We should still be running the
> > scheduler if *either* of them is set...
> >
> > > list_add(&txqi->schedule_order,
> > > &local->active_txqs[txq->ac]);
> > > else
> > > @@ -3740,6 +3745,37 @@ void __ieee80211_schedule_txq(struct ieee80211_hw *hw,
> > > }
> > > EXPORT_SYMBOL(__ieee80211_schedule_txq);
> > >
> > > +bool ieee80211_txq_airtime_check(struct ieee80211_hw *hw,
> > > + struct ieee80211_txq *txq)
> > > +{
> > > + struct sta_info *sta;
> > > + struct ieee80211_local *local = hw_to_local(hw);
> > > +
> > > +
> > > + if (!(local->airtime_flags & AIRTIME_USE_TX))
> >
> > Ditto.
> >
> > > + return true;
> > > +
> > > + if (!txq->sta)
> > > + return true;
> > > +
> > > + sta = container_of(txq->sta, struct sta_info, sta);
> > > + if (sta->airtime[txq->ac].deficit < 0)
> > > + return false;
> > > +
> > > + if (!(local->airtime_flags & AIRTIME_USE_AQL))
> > > + return true;
> > > +
> > > + if ((sta->airtime[txq->ac].aql_tx_pending <
> > > + sta->airtime[txq->ac].aql_limit_low) ||
> > > + (local->aql_total_pending_airtime < local->aql_interface_limit &&
> > > + sta->airtime[txq->ac].aql_tx_pending <
> > > + sta->airtime[txq->ac].aql_limit_high))
> > > + return true;
> > > + else
> > > + return false;
> > > +}
> > > +EXPORT_SYMBOL(ieee80211_txq_airtime_check);
> > > +
> > > bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
> > > struct ieee80211_txq *txq)
> > > {
> > > @@ -3748,10 +3784,13 @@ bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
> > > struct sta_info *sta;
> > > u8 ac = txq->ac;
> > >
> > > - spin_lock_bh(&local->active_txq_lock[ac]);
> > > -
> > > if (!txqi->txq.sta)
> > > - goto out;
> > > + return true;
> > > +
> > > + if (!(local->airtime_flags & AIRTIME_USE_TX))
> >
> > Ditto.
> >
> >
> > -Toke
> >
Kan Yan <[email protected]> writes:
> Hi Johannes,
>
> Thanks for help review this patch. I will fix all style errors.
>
>> > +ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
>> Why is it necessary to call this, vs. just returning NULL when an SKB is
>> requested?
> This function is also called by ath10k, from ath10k_mac_tx_can_push(),
> which returns a boolean.
>
>> However, I'm not sure it *shouldn't* actually be per interface (i.e.
>> moving from
>> local->aql_total_pending_airtime
>> to
>>
>> sdata->aql_total_pending_airtime
>>
>> because you could have multiple channels etc. involved and then using a
>> single airtime limit across two interfaces that are actually on two
>> different channels (e.g. 2.4 and 5 GHz) doesn't make that much sense.
>>
>> Actually, it does make some sense as long as the NIC is actually
>> channel-hopping ... but that's in the process of changing now, there's
>> going to be hardware really soon (or perhaps already exists) that has
>> real dual-band capabilities...
>
> That's a good point. I haven't thought about real simultaneous dual
> band chipset and such chipset do exists now. Is RSDB support coming to
> mac80211 soon? Just curious if it will be just virtual interfaces or
> something else. I chose "local" instead of "sdata" thinking about the
> case of several virtual interfaces (AP, STA, MESH) operates in the
> same channel, then the interface total could be a better choice.
>
> I am ok with moving the "aql_total_pending_airtime" into sdata, but
> afraid that's not the most optimal choice for the case of multiple
> virtual interfaces operates in the same channel.
> Maybe we could leave it in "local" for now. What do you think?
I'd lean towards keeping it in 'local' for consistency with all the
other airtime stuff. For now, I think having multiple SSIDs on the same
radio is more common than the reverse (multiple bands on a single
radio).
In particular, the per-group airtime fairness stuff is definitely
designed on the assumption that all BSSes share the same band.
So if and when we start supporting true multi-band devices we'll have to
change these things anyway. So might as well keep everything together so
it all gets fixed :)
-Toke
On Sun, 2019-10-06 at 19:40 +0200, Toke Høiland-Jørgensen wrote:
> > That's a good point. I haven't thought about real simultaneous dual
> > band chipset and such chipset do exists now. Is RSDB support coming to
> > mac80211 soon? Just curious if it will be just virtual interfaces or
> > something else. I chose "local" instead of "sdata" thinking about the
> > case of several virtual interfaces (AP, STA, MESH) operates in the
> > same channel, then the interface total could be a better choice.
> >
> > I am ok with moving the "aql_total_pending_airtime" into sdata, but
> > afraid that's not the most optimal choice for the case of multiple
> > virtual interfaces operates in the same channel.
> > Maybe we could leave it in "local" for now. What do you think?
>
> I'd lean towards keeping it in 'local' for consistency with all the
> other airtime stuff. For now, I think having multiple SSIDs on the same
> radio is more common than the reverse (multiple bands on a single
> radio).
>
> In particular, the per-group airtime fairness stuff is definitely
> designed on the assumption that all BSSes share the same band.
s/band/channel/, presumably.
> So if and when we start supporting true multi-band devices we'll have to
> change these things anyway. So might as well keep everything together so
> it all gets fixed :)
I guess I'm OK with that, but I'm pretty sure this will come up sooner
rather than later ...
What else is there though?
johannes
On Mon, Oct 7, 2019 at 12:23 PM Johannes Berg <[email protected]> wrote:
>
> On Sun, 2019-10-06 at 19:40 +0200, Toke Høiland-Jørgensen wrote:
>
> > > That's a good point. I haven't thought about real simultaneous dual
> > > band chipset and such chipset do exists now. Is RSDB support coming to
> > > mac80211 soon? Just curious if it will be just virtual interfaces or
> > > something else. I chose "local" instead of "sdata" thinking about the
> > > case of several virtual interfaces (AP, STA, MESH) operates in the
> > > same channel, then the interface total could be a better choice.
> > >
> > > I am ok with moving the "aql_total_pending_airtime" into sdata, but
> > > afraid that's not the most optimal choice for the case of multiple
> > > virtual interfaces operates in the same channel.
> > > Maybe we could leave it in "local" for now. What do you think?
> >
> > I'd lean towards keeping it in 'local' for consistency with all the
> > other airtime stuff. For now, I think having multiple SSIDs on the same
> > radio is more common than the reverse (multiple bands on a single
> > radio).
> >
> > In particular, the per-group airtime fairness stuff is definitely
> > designed on the assumption that all BSSes share the same band.
>
> s/band/channel/, presumably.
>
> > So if and when we start supporting true multi-band devices we'll have to
> > change these things anyway. So might as well keep everything together so
> > it all gets fixed :)
>
> I guess I'm OK with that, but I'm pretty sure this will come up sooner
> rather than later ...
>
> What else is there though?
World domination!
>
> johannes
>
> _______________________________________________
> Make-wifi-fast mailing list
> [email protected]
> https://lists.bufferbloat.net/listinfo/make-wifi-fast
--
Dave Täht
CTO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-831-205-9740
Johannes Berg <[email protected]> writes:
> On Sun, 2019-10-06 at 19:40 +0200, Toke Høiland-Jørgensen wrote:
>
>> > That's a good point. I haven't thought about real simultaneous dual
>> > band chipset and such chipset do exists now. Is RSDB support coming to
>> > mac80211 soon? Just curious if it will be just virtual interfaces or
>> > something else. I chose "local" instead of "sdata" thinking about the
>> > case of several virtual interfaces (AP, STA, MESH) operates in the
>> > same channel, then the interface total could be a better choice.
>> >
>> > I am ok with moving the "aql_total_pending_airtime" into sdata, but
>> > afraid that's not the most optimal choice for the case of multiple
>> > virtual interfaces operates in the same channel.
>> > Maybe we could leave it in "local" for now. What do you think?
>>
>> I'd lean towards keeping it in 'local' for consistency with all the
>> other airtime stuff. For now, I think having multiple SSIDs on the same
>> radio is more common than the reverse (multiple bands on a single
>> radio).
>>
>> In particular, the per-group airtime fairness stuff is definitely
>> designed on the assumption that all BSSes share the same band.
>
> s/band/channel/, presumably.
Indeed :)
>> So if and when we start supporting true multi-band devices we'll have to
>> change these things anyway. So might as well keep everything together so
>> it all gets fixed :)
>
> I guess I'm OK with that, but I'm pretty sure this will come up sooner
> rather than later ...
>
> What else is there though?
By "it all" I meant "all the airtime fairness stuff". Other than that, I
didn't have anything in particular in mind. I just kinda assumed there
would be lots of places that had an implicit assumption that all devices
on the same phy shares a channel...
-Toke
On Mon, 2019-10-07 at 21:40 +0200, Toke Høiland-Jørgensen wrote:
> > > So if and when we start supporting true multi-band devices we'll have to
> > > change these things anyway. So might as well keep everything together so
> > > it all gets fixed :)
> >
> > I guess I'm OK with that, but I'm pretty sure this will come up sooner
> > rather than later ...
> >
> > What else is there though?
>
> By "it all" I meant "all the airtime fairness stuff". Other than that, I
> didn't have anything in particular in mind. I just kinda assumed there
> would be lots of places that had an implicit assumption that all devices
> on the same phy shares a channel...
Not _that_ much - we do have the channel contexts after all. But except
for hwsim (*cough cough* I was lazy) nothing actually implements real
concurrent multi-channel yet, obviously, but uses a single radio with
channel hopping...
johannes
> I'd like to use the new rate calculation code that Felix added to mt76.
> Is the arsta->txrate info in ath10k suitable to be passed up to mac80211
> and used in that, do you think? Because then that would probably be the
> easiest way to go about it...
Do you mean the mt76 patch that using the EWMA to smooth estimate data
rate? It would be great if you can move that to mac80211.
Yes, arsta->txrate info in ath10k is a good place to extract latest date rate.
On Mon, Oct 7, 2019 at 12:43 PM Johannes Berg <[email protected]> wrote:
>
> On Mon, 2019-10-07 at 21:40 +0200, Toke Høiland-Jørgensen wrote:
>
> > > > So if and when we start supporting true multi-band devices we'll have to
> > > > change these things anyway. So might as well keep everything together so
> > > > it all gets fixed :)
> > >
> > > I guess I'm OK with that, but I'm pretty sure this will come up sooner
> > > rather than later ...
> > >
> > > What else is there though?
> >
> > By "it all" I meant "all the airtime fairness stuff". Other than that, I
> > didn't have anything in particular in mind. I just kinda assumed there
> > would be lots of places that had an implicit assumption that all devices
> > on the same phy shares a channel...
>
> Not _that_ much - we do have the channel contexts after all. But except
> for hwsim (*cough cough* I was lazy) nothing actually implements real
> concurrent multi-channel yet, obviously, but uses a single radio with
> channel hopping...
>
> johannes
>