Hi,
So I really don't see a clean solution for fixing aggregation to allow
ath9k to work. Atheros HW don't have separate queues for managing
aggregation traffic, and right now, we set hw->ampdu_queues to 1, to just
allow aggregation to work at all.
IMHO, mac80211 should handle HW with no aggr. queues too. With this patch,
aggregation works in ath9k. iwlwifi is still broken though, I leave the rtnl
issue to Intel.
Please review.
Sujith
Signed-off-by: Sujith <[email protected]>
Signed-off-by: Luis Rodriguez <[email protected]>
---
drivers/net/wireless/ath9k/main.c | 2 +-
drivers/net/wireless/ath9k/xmit.c | 2 +-
drivers/net/wireless/iwlwifi/iwl-4965.c | 1 -
drivers/net/wireless/iwlwifi/iwl-5000.c | 1 -
drivers/net/wireless/iwlwifi/iwl-agn-rs.c | 8 ++--
drivers/net/wireless/iwlwifi/iwl-tx.c | 4 +-
include/linux/skbuff.h | 7 ++++
include/net/mac80211.h | 5 +--
net/mac80211/ht.c | 57 +++++++++++++++++------------
net/mac80211/main.c | 7 +---
net/mac80211/rx.c | 7 +---
net/mac80211/tx.c | 20 +++++++---
net/mac80211/wme.c | 24 +++++-------
13 files changed, 77 insertions(+), 68 deletions(-)
diff --git a/drivers/net/wireless/ath9k/main.c b/drivers/net/wireless/ath9k/main.c
index 56552a9..1f37f80 100644
--- a/drivers/net/wireless/ath9k/main.c
+++ b/drivers/net/wireless/ath9k/main.c
@@ -965,7 +965,7 @@ static int ath_attach(u16 devid,
/* FIXME: Have to figure out proper hw init values later */
hw->queues = 4;
- hw->ampdu_queues = 1;
+ hw->ampdu_queues = 0;
/* Register rate control */
hw->rate_control_algorithm = "ath9k_rate_control";
diff --git a/drivers/net/wireless/ath9k/xmit.c b/drivers/net/wireless/ath9k/xmit.c
index 1afca79..be7533a 100644
--- a/drivers/net/wireless/ath9k/xmit.c
+++ b/drivers/net/wireless/ath9k/xmit.c
@@ -301,7 +301,7 @@ static int ath_tx_prepare(struct ath_softc *sc,
/* Enable HT only for DATA frames and not for EAPOL */
txctl->ht = (hw->conf.ht_cap.ht_supported &&
- (tx_info->flags & IEEE80211_TX_CTL_AMPDU));
+ skb->is_part_ampdu);
if (is_multicast_ether_addr(hdr->addr1)) {
rcs[0].rix = (u8)
diff --git a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
index 9838de5..7059ff4 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
@@ -2071,7 +2071,6 @@ static int iwl4965_tx_status_reply_tx(struct iwl_priv *priv,
info = IEEE80211_SKB_CB(priv->txq[txq_id].txb[idx].skb[0]);
info->status.retry_count = tx_resp->failure_frame;
- info->flags &= ~IEEE80211_TX_CTL_AMPDU;
info->flags |= iwl_is_tx_success(status)?
IEEE80211_TX_STAT_ACK : 0;
iwl_hwrate_to_tx_control(priv, rate_n_flags, info);
diff --git a/drivers/net/wireless/iwlwifi/iwl-5000.c b/drivers/net/wireless/iwlwifi/iwl-5000.c
index 56a3f0c..9eb09dc 100644
--- a/drivers/net/wireless/iwlwifi/iwl-5000.c
+++ b/drivers/net/wireless/iwlwifi/iwl-5000.c
@@ -1155,7 +1155,6 @@ static int iwl5000_tx_status_reply_tx(struct iwl_priv *priv,
info = IEEE80211_SKB_CB(priv->txq[txq_id].txb[idx].skb[0]);
info->status.retry_count = tx_resp->failure_frame;
- info->flags &= ~IEEE80211_TX_CTL_AMPDU;
info->flags |= iwl_is_tx_success(status)?
IEEE80211_TX_STAT_ACK : 0;
iwl_hwrate_to_tx_control(priv, rate_n_flags, info);
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-rs.c b/drivers/net/wireless/iwlwifi/iwl-agn-rs.c
index aa300e1..f29562b 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn-rs.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn-rs.c
@@ -796,7 +796,7 @@ static void rs_tx_status(void *priv_r, struct ieee80211_supported_band *sband,
return;
/* This packet was aggregated but doesn't carry rate scale info */
- if ((info->flags & IEEE80211_TX_CTL_AMPDU) &&
+ if (skb->is_part_ampdu &&
!(info->flags & IEEE80211_TX_STAT_AMPDU))
return;
@@ -910,7 +910,7 @@ static void rs_tx_status(void *priv_r, struct ieee80211_supported_band *sband,
tpt = search_tbl->expected_tpt[rs_index];
else
tpt = 0;
- if (info->flags & IEEE80211_TX_CTL_AMPDU)
+ if (skb->is_part_ampdu)
rs_collect_tx_data(search_win, rs_index, tpt,
info->status.ampdu_ack_len,
info->status.ampdu_ack_map);
@@ -926,7 +926,7 @@ static void rs_tx_status(void *priv_r, struct ieee80211_supported_band *sband,
tpt = curr_tbl->expected_tpt[rs_index];
else
tpt = 0;
- if (info->flags & IEEE80211_TX_CTL_AMPDU)
+ if (skb->is_part_ampdu)
rs_collect_tx_data(window, rs_index, tpt,
info->status.ampdu_ack_len,
info->status.ampdu_ack_map);
@@ -938,7 +938,7 @@ static void rs_tx_status(void *priv_r, struct ieee80211_supported_band *sband,
/* If not searching for new mode, increment success/failed counter
* ... these help determine when to start searching again */
if (lq_sta->stay_in_tbl) {
- if (info->flags & IEEE80211_TX_CTL_AMPDU) {
+ if (skb->is_part_ampdu) {
lq_sta->total_success += info->status.ampdu_ack_map;
lq_sta->total_failed +=
(info->status.ampdu_ack_len - info->status.ampdu_ack_map);
diff --git a/drivers/net/wireless/iwlwifi/iwl-tx.c b/drivers/net/wireless/iwlwifi/iwl-tx.c
index 907a53e..62c6f78 100644
--- a/drivers/net/wireless/iwlwifi/iwl-tx.c
+++ b/drivers/net/wireless/iwlwifi/iwl-tx.c
@@ -721,7 +721,7 @@ static void iwl_tx_cmd_build_hwcrypto(struct iwl_priv *priv,
case ALG_CCMP:
tx_cmd->sec_ctl = TX_CMD_SEC_CCM;
memcpy(tx_cmd->key, keyconf->key, keyconf->keylen);
- if (info->flags & IEEE80211_TX_CTL_AMPDU)
+ if (skb_frag->is_part_ampdu)
tx_cmd->tx_flags |= TX_CMD_FLG_AGG_CCMP_MSK;
IWL_DEBUG_TX("tx_cmd with aes hwcrypto\n");
break;
@@ -851,7 +851,7 @@ int iwl_tx_skb(struct iwl_priv *priv, struct sk_buff *skb)
hdr->seq_ctrl |= cpu_to_le16(seq_number);
seq_number += 0x10;
/* aggregation is on for this <sta,tid> */
- if (info->flags & IEEE80211_TX_CTL_AMPDU)
+ if (skb->is_part_ampdu)
txq_id = priv->stations[sta_id].tid[tid].agg.txq_id;
priv->stations[sta_id].tid[tid].tfds_in_queue++;
}
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 9099237..8c4650e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -244,6 +244,11 @@ typedef unsigned char *sk_buff_data_t;
* @tc_verd: traffic control verdict
* @ndisc_nodetype: router type (from link layer)
* @do_not_encrypt: set to prevent encryption of this frame
+ * @requeue: set to indicate that the wireless core should attempt
+ * a software retry on this frame if we failed to
+ * receive an ACK for it
+ * @is_part_ampdu: set to indicate that the wireless core should should
+ * treat this frame as part of an AMPDU
* @dma_cookie: a cookie to one of several possible DMA operations
* done by skb DMA functions
* @secmark: security marking
@@ -319,6 +324,8 @@ struct sk_buff {
#endif
#if defined(CONFIG_MAC80211) || defined(CONFIG_MAC80211_MODULE)
__u8 do_not_encrypt:1;
+ __u8 is_part_ampdu:1;
+ __u8 requeue:1;
#endif
/* 0/13/14 bit hole */
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index afcdfaa..2cc3b7c 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -229,13 +229,11 @@ struct ieee80211_bss_conf {
* @IEEE80211_TX_CTL_RATE_CTRL_PROBE: TBD
* @IEEE80211_TX_CTL_CLEAR_PS_FILT: clear powersave filter for destination
* station
- * @IEEE80211_TX_CTL_REQUEUE: TBD
* @IEEE80211_TX_CTL_FIRST_FRAGMENT: this is a first fragment of the frame
* @IEEE80211_TX_CTL_SHORT_PREAMBLE: TBD
* @IEEE80211_TX_CTL_LONG_RETRY_LIMIT: this frame should be send using the
* through set_retry_limit configured long retry value
* @IEEE80211_TX_CTL_SEND_AFTER_DTIM: send this frame after DTIM beacon
- * @IEEE80211_TX_CTL_AMPDU: this frame should be sent as part of an A-MPDU
* @IEEE80211_TX_CTL_OFDM_HT: this frame can be sent in HT OFDM rates. number
* of streams when this flag is on can be extracted from antenna_sel_tx,
* so if 1 antenna is marked use SISO, 2 antennas marked use MIMO, n
@@ -250,6 +248,7 @@ struct ieee80211_bss_conf {
* @IEEE80211_TX_STAT_ACK: Frame was acknowledged
* @IEEE80211_TX_STAT_AMPDU: The frame was aggregated, so status
* is for the whole aggregation.
+
* @IEEE80211_TX_STAT_AMPDU_NO_BACK: no block ack was returned,
* so consider using block ack request (BAR).
* @IEEE80211_TX_CTL_ASSIGN_SEQ: The driver has to assign a sequence
@@ -271,12 +270,10 @@ enum mac80211_tx_control_flags {
IEEE80211_TX_CTL_NO_ACK = BIT(4),
IEEE80211_TX_CTL_RATE_CTRL_PROBE = BIT(5),
IEEE80211_TX_CTL_CLEAR_PS_FILT = BIT(6),
- IEEE80211_TX_CTL_REQUEUE = BIT(7),
IEEE80211_TX_CTL_FIRST_FRAGMENT = BIT(8),
IEEE80211_TX_CTL_SHORT_PREAMBLE = BIT(9),
IEEE80211_TX_CTL_LONG_RETRY_LIMIT = BIT(10),
IEEE80211_TX_CTL_SEND_AFTER_DTIM = BIT(12),
- IEEE80211_TX_CTL_AMPDU = BIT(13),
IEEE80211_TX_CTL_OFDM_HT = BIT(14),
IEEE80211_TX_CTL_GREEN_FIELD = BIT(15),
IEEE80211_TX_CTL_40_MHZ_WIDTH = BIT(16),
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index d09e8bf..d85953e 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -549,17 +549,19 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
(unsigned long)&sta->timer_to_tid[tid];
init_timer(&sta->ampdu_mlme.tid_tx[tid]->addba_resp_timer);
- /* create a new queue for this aggregation */
- ret = ieee80211_ht_agg_queue_add(local, sta, tid);
+ if (hw->ampdu_queues) {
+ /* create a new queue for this aggregation */
+ ret = ieee80211_ht_agg_queue_add(local, sta, tid);
- /* case no queue is available to aggregation
- * don't switch to aggregation */
- if (ret) {
+ /* case no queue is available to aggregation
+ * don't switch to aggregation */
+ if (ret) {
#ifdef CONFIG_MAC80211_HT_DEBUG
- printk(KERN_DEBUG "BA request denied - queue unavailable for"
- " tid %d\n", tid);
+ printk(KERN_DEBUG "BA request denied - "
+ "queue unavailable for tid %d\n", tid);
#endif /* CONFIG_MAC80211_HT_DEBUG */
- goto err_unlock_queue;
+ goto err_unlock_queue;
+ }
}
sdata = sta->sdata;
@@ -578,7 +580,8 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
/* No need to requeue the packets in the agg queue, since we
* held the tx lock: no packet could be enqueued to the newly
* allocated queue */
- ieee80211_ht_agg_queue_remove(local, sta, tid, 0);
+ if (hw->ampdu_queues)
+ ieee80211_ht_agg_queue_remove(local, sta, tid, 0);
#ifdef CONFIG_MAC80211_HT_DEBUG
printk(KERN_DEBUG "BA request denied - HW unavailable for"
" tid %d\n", tid);
@@ -588,7 +591,8 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
}
/* Will put all the packets in the new SW queue */
- ieee80211_requeue(local, ieee802_1d_to_ac[tid]);
+ if (hw->ampdu_queues)
+ ieee80211_requeue(local, ieee802_1d_to_ac[tid]);
spin_unlock_bh(&sta->lock);
/* send an addBA request */
@@ -657,7 +661,8 @@ int ieee80211_stop_tx_ba_session(struct ieee80211_hw *hw,
print_mac(mac, ra), tid);
#endif /* CONFIG_MAC80211_HT_DEBUG */
- ieee80211_stop_queue(hw, sta->tid_to_tx_q[tid]);
+ if (hw->ampdu_queues)
+ ieee80211_stop_queue(hw, sta->tid_to_tx_q[tid]);
*state = HT_AGG_STATE_REQ_STOP_BA_MSK |
(initiator << HT_AGG_STATE_INITIATOR_SHIFT);
@@ -670,7 +675,8 @@ int ieee80211_stop_tx_ba_session(struct ieee80211_hw *hw,
if (ret) {
WARN_ON(ret != -EBUSY);
*state = HT_AGG_STATE_OPERATIONAL;
- ieee80211_wake_queue(hw, sta->tid_to_tx_q[tid]);
+ if (hw->ampdu_queues)
+ ieee80211_wake_queue(hw, sta->tid_to_tx_q[tid]);
goto stop_BA_exit;
}
@@ -728,7 +734,8 @@ void ieee80211_start_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u16 tid)
#ifdef CONFIG_MAC80211_HT_DEBUG
printk(KERN_DEBUG "Aggregation is on for tid %d \n", tid);
#endif
- ieee80211_wake_queue(hw, sta->tid_to_tx_q[tid]);
+ if (hw->ampdu_queues)
+ ieee80211_wake_queue(hw, sta->tid_to_tx_q[tid]);
}
spin_unlock_bh(&sta->lock);
rcu_read_unlock();
@@ -784,16 +791,18 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid)
ieee80211_send_delba(sta->sdata, ra, tid,
WLAN_BACK_INITIATOR, WLAN_REASON_QSTA_NOT_USE);
- agg_queue = sta->tid_to_tx_q[tid];
-
- ieee80211_ht_agg_queue_remove(local, sta, tid, 1);
-
- /* We just requeued the all the frames that were in the
- * removed queue, and since we might miss a softirq we do
- * netif_schedule_queue. ieee80211_wake_queue is not used
- * here as this queue is not necessarily stopped
- */
- netif_schedule_queue(netdev_get_tx_queue(local->mdev, agg_queue));
+ if (hw->ampdu_queues) {
+ agg_queue = sta->tid_to_tx_q[tid];
+ ieee80211_ht_agg_queue_remove(local, sta, tid, 1);
+
+ /* We just requeued the all the frames that were in the
+ * removed queue, and since we might miss a softirq we do
+ * netif_schedule_queue. ieee80211_wake_queue is not used
+ * here as this queue is not necessarily stopped
+ */
+ netif_schedule_queue(netdev_get_tx_queue(local->mdev,
+ agg_queue));
+ }
spin_lock_bh(&sta->lock);
*state = HT_AGG_STATE_IDLE;
sta->ampdu_mlme.addba_req_num[tid] = 0;
@@ -1050,7 +1059,7 @@ void ieee80211_process_addba_resp(struct ieee80211_local *local,
*state |= HT_ADDBA_RECEIVED_MSK;
sta->ampdu_mlme.addba_req_num[tid] = 0;
- if (*state == HT_AGG_STATE_OPERATIONAL)
+ if (*state == HT_AGG_STATE_OPERATIONAL && local->hw.ampdu_queues)
ieee80211_wake_queue(hw, sta->tid_to_tx_q[tid]);
spin_unlock_bh(&sta->lock);
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index c936017..4883aa9 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -382,8 +382,6 @@ static void ieee80211_handle_filtered_frame(struct ieee80211_local *local,
struct sta_info *sta,
struct sk_buff *skb)
{
- struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
-
sta->tx_filtered_count++;
/*
@@ -430,10 +428,9 @@ static void ieee80211_handle_filtered_frame(struct ieee80211_local *local,
return;
}
- if (!test_sta_flags(sta, WLAN_STA_PS) &&
- !(info->flags & IEEE80211_TX_CTL_REQUEUE)) {
+ if (!test_sta_flags(sta, WLAN_STA_PS) && !skb->requeue) {
/* Software retry the packet once */
- info->flags |= IEEE80211_TX_CTL_REQUEUE;
+ skb->requeue = 1;
ieee80211_remove_tx_extra(local, sta->key, skb);
dev_queue_xmit(skb);
return;
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index a0db162..7c3702c 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -670,7 +670,6 @@ static int ap_sta_ps_end(struct sta_info *sta)
struct ieee80211_local *local = sdata->local;
struct sk_buff *skb;
int sent = 0;
- struct ieee80211_tx_info *info;
DECLARE_MAC_BUF(mac);
atomic_dec(&sdata->bss->num_sta_ps);
@@ -687,13 +686,11 @@ static int ap_sta_ps_end(struct sta_info *sta)
/* Send all buffered frames to the station */
while ((skb = skb_dequeue(&sta->tx_filtered)) != NULL) {
- info = IEEE80211_SKB_CB(skb);
sent++;
- info->flags |= IEEE80211_TX_CTL_REQUEUE;
+ skb->requeue = 1;
dev_queue_xmit(skb);
}
while ((skb = skb_dequeue(&sta->ps_tx_buf)) != NULL) {
- info = IEEE80211_SKB_CB(skb);
local->total_ps_buffered--;
sent++;
#ifdef CONFIG_MAC80211_VERBOSE_PS_DEBUG
@@ -701,7 +698,7 @@ static int ap_sta_ps_end(struct sta_info *sta)
"since STA not sleeping anymore\n", sdata->dev->name,
print_mac(mac, sta->sta.addr), sta->sta.aid);
#endif /* CONFIG_MAC80211_VERBOSE_PS_DEBUG */
- info->flags |= IEEE80211_TX_CTL_REQUEUE;
+ skb->requeue = 1;
dev_queue_xmit(skb);
}
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index cdedd37..2d7cab9 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -658,9 +658,7 @@ ieee80211_tx_h_fragment(struct ieee80211_tx_data *tx)
* This scenario is handled in __ieee80211_tx_prepare but extra
* caution taken here as fragmented ampdu may cause Tx stop.
*/
- if (WARN_ON(tx->flags & IEEE80211_TX_CTL_AMPDU ||
- skb_get_queue_mapping(tx->skb) >=
- ieee80211_num_regular_queues(&tx->local->hw)))
+ if (WARN_ON(tx->skb->is_part_ampdu))
return TX_DROP;
first = tx->skb;
@@ -943,7 +941,8 @@ __ieee80211_tx_prepare(struct ieee80211_tx_data *tx,
struct ieee80211_sub_if_data *sdata;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
- int hdrlen;
+ int hdrlen, tid;
+ u8 *qc, *state;
memset(tx, 0, sizeof(*tx));
tx->skb = skb;
@@ -976,6 +975,15 @@ __ieee80211_tx_prepare(struct ieee80211_tx_data *tx,
tx->sta = sta_info_get(local, hdr->addr1);
+ if (tx->sta && ieee80211_is_data_qos(hdr->frame_control)) {
+ qc = ieee80211_get_qos_ctl(hdr);
+ tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
+
+ state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
+ if (*state == HT_AGG_STATE_OPERATIONAL)
+ skb->is_part_ampdu = 1;
+ }
+
if (is_multicast_ether_addr(hdr->addr1)) {
tx->flags &= ~IEEE80211_TX_UNICAST;
info->flags |= IEEE80211_TX_CTL_NO_ACK;
@@ -988,7 +996,7 @@ __ieee80211_tx_prepare(struct ieee80211_tx_data *tx,
if ((tx->flags & IEEE80211_TX_UNICAST) &&
skb->len + FCS_LEN > local->fragmentation_threshold &&
!local->ops->set_frag_threshold &&
- !(info->flags & IEEE80211_TX_CTL_AMPDU))
+ !skb->is_part_ampdu)
tx->flags |= IEEE80211_TX_FRAGMENTED;
else
tx->flags &= ~IEEE80211_TX_FRAGMENTED;
@@ -1178,7 +1186,7 @@ retry:
* queues, there's no reason for a driver to reject
* a frame there, warn and drop it.
*/
- if (WARN_ON(queue >= ieee80211_num_regular_queues(&local->hw)))
+ if (WARN_ON(skb->is_part_ampdu))
goto drop;
store = &local->pending_packet[queue];
diff --git a/net/mac80211/wme.c b/net/mac80211/wme.c
index f10f770..3247312 100644
--- a/net/mac80211/wme.c
+++ b/net/mac80211/wme.c
@@ -114,8 +114,8 @@ u16 ieee80211_select_queue(struct net_device *dev, struct sk_buff *skb)
{
struct ieee80211_master_priv *mpriv = netdev_priv(dev);
struct ieee80211_local *local = mpriv->local;
+ struct ieee80211_hw *hw = &local->hw;
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
- struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
struct sta_info *sta;
u16 queue;
u8 tid;
@@ -124,21 +124,19 @@ u16 ieee80211_select_queue(struct net_device *dev, struct sk_buff *skb)
if (unlikely(queue >= local->hw.queues))
queue = local->hw.queues - 1;
- if (info->flags & IEEE80211_TX_CTL_REQUEUE) {
+ if (skb->requeue) {
+ if (!hw->ampdu_queues)
+ return queue;
+
rcu_read_lock();
sta = sta_info_get(local, hdr->addr1);
tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
if (sta) {
- struct ieee80211_hw *hw = &local->hw;
int ampdu_queue = sta->tid_to_tx_q[tid];
if ((ampdu_queue < ieee80211_num_queues(hw)) &&
- test_bit(ampdu_queue, local->queue_pool)) {
+ test_bit(ampdu_queue, local->queue_pool))
queue = ampdu_queue;
- info->flags |= IEEE80211_TX_CTL_AMPDU;
- } else {
- info->flags &= ~IEEE80211_TX_CTL_AMPDU;
- }
}
rcu_read_unlock();
@@ -159,20 +157,18 @@ u16 ieee80211_select_queue(struct net_device *dev, struct sk_buff *skb)
*p++ = ack_policy | tid;
*p = 0;
+ if (!hw->ampdu_queues)
+ return queue;
+
rcu_read_lock();
sta = sta_info_get(local, hdr->addr1);
if (sta) {
int ampdu_queue = sta->tid_to_tx_q[tid];
- struct ieee80211_hw *hw = &local->hw;
if ((ampdu_queue < ieee80211_num_queues(hw)) &&
- test_bit(ampdu_queue, local->queue_pool)) {
+ test_bit(ampdu_queue, local->queue_pool))
queue = ampdu_queue;
- info->flags |= IEEE80211_TX_CTL_AMPDU;
- } else {
- info->flags &= ~IEEE80211_TX_CTL_AMPDU;
- }
}
rcu_read_unlock();
--
1.6.0.2
Johannes Berg wrote:
> On Wed, 2008-10-22 at 13:59 +0200, Tomas Winkler wrote:
>
> > It answers BA i n RX path and interpret BA in TX Scheduling
> > retransmission of not acked frames is done in HW (not firmware) no
> > need to requeue them. In corner case when retries are exhausted BA
> > request is issued from mac80211 to advance RX side window.
>
> Ok, but if we do both of that in mac80211, it wouldn't really matter
> since we never see the relevant frames, right? Or we can add a flag to
> ignore them.
>
> > > Like I said, from what I can tell bcom is really similar, and I'd like
> > > to not have to re-implement all this first. mac80211 used to have a more
> > > push-model, which we've deviated from with the HT stuff you did, but for
> > > most hardware the push-model is still appropriate, so imho we should try
> > > to go to one even with HT and then see.
> >
> > Works for me. What I stumble upon is retransmission from within TX
> > response path
> > how this go together with context of regular TX path. Even in iwlwifi
> > we need to be able to kick/start the internal aggregation queue?
>
> Yeah, this is something we definitely need to work out. We haven't even
> managed to decide yet how aggregated packets are given to the driver :)
> I think for atheros and bcom a model where mac80211 decides pretty much
> everything and hands the driver a list of skbs to aggregate would work
> best, but then I think that wouldn't work with your hw at all.
>
Retransmission of unacked frames is done within the driver, in ath9k.
In case of retry exhaustion (failed transmission), we do what Intel does,
i.e, request for a BAR to be sent out.
Sujith
On Mon, 2008-10-20 at 23:46 +0200, Tomas Winkler wrote:
> [snip]
will look at it later, doing first pass email right now...
> That
> >> might leave Intel out a bit, but I'm starting to think that we want to
> >> special-case them a bit anyway.
>
> Let's see first mvel and bcom implementation :)
bcom is the same as Atheros pretty much.
johannes
On Mon, Oct 20, 2008 at 2:04 AM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2008-10-20 at 02:01 -0700, Luis R. Rodriguez wrote:
>
>> > Well the fact that it's in tx_h_sequence is a bit weird, but we should
>> > be able to get around not using an extra bit in skb I think. Especially
>> > for this since it's only really between the driver and mac80211, and
>> > after a requeue the ampdu status might actually change.
>>
>> Good point. I failed to see that. So in that case it should be as
>> sujith had it before my suggestion and just handle it as he had it in
>> the tx handlers. The problem though is that the RC algorithm should be
>> the one to determine if an skb is part of an ampdu for aggregation or
>> not IMHO. Actually this is the real question and hot topic.
>>
>> When should we aggregate and should mac80211 decide that or be left to the RC.
>
> No, the RC algorithm cannot decided that, the RC algorithm can merely
> decide when to set up/tear down aggregation sessions. When a session is
> up all packet belonging to that TID should go into it, surely?
Good question :)
Luis
On Mon, 2008-10-20 at 23:46 +0200, Tomas Winkler wrote:
> > Immediate Block-Ack is mandatory for 11n, delayed BA is optional.
> > ath9k currently maintains the BA window state internally too.
> > Another candidate that can be moved to mac80211, IMO.
> >
>
> TX or RX side?
Both? What exactly does your firmware do, say of the list that Sujith
posted?
> >> What I could also imagine is that mac80211 simply does pretty much
> >> everything and hands a number of skbs to the driver at once with some
> >> additional information about how to aggregate them, what sort of spacing
> >> to use, etc. It should be able to calculate all the required stuff since
> >> it knows from the rate control algorithm what's happening there.
>
> What you need is to compute how many frames can be transmitted within
> TXOP i think we can provide the calculation
> depending on the current state. Just with the retransmission down
> scaling need to be taken into account.
We really need TXOP handling in mac80211 anyway, no? Right now we won't
handle frames that are too long for the TXOP, for example.
> That
> >> might leave Intel out a bit, but I'm starting to think that we want to
> >> special-case them a bit anyway.
>
> Let's see first mvel and bcom implementation :)
Like I said, from what I can tell bcom is really similar, and I'd like
to not have to re-implement all this first. mac80211 used to have a more
push-model, which we've deviated from with the HT stuff you did, but for
most hardware the push-model is still appropriate, so imho we should try
to go to one even with HT and then see. We've since reworked rate
control to be able to cope with HT too, so mac80211 can have much more
information about what's going on and should be able to make many of the
necessary decisions.
johannes
On Mon, 2008-10-20 at 23:33 +0530, Sujith wrote:
> > Can you explain the way it currently works in ath9k?
> >
>
> Alright, it goes something like this:
Thanks.
> * mac80211 sends down a frame
> * Initiate an aggregation session for the <RA,TID> if one isn't already in progress.
It seems that should be a rate control decision? Possibly taking into
account more than just always doing aggregation sessions. Then again, I
suppose aggregation sessions are cheap. What about latency here?
> * Pause the TID, i.e, add further frames to the TID's queue.
> * If ADDBA exchange is successful, resume TID.
> * Form aggregates from the TID's buffer list, send them out.
> ( Take care to maintain minimum HW queue depth for aggregation )
"maintain minimum HW queue depth"? In what way? You mean put in enough
frames?
> * If ADDBA exchange fails, flush TID.
> * Send TID's pending frames as normal frames (non-ampdu)
Obviously. But why isn't this done in parallel? I mean, why not send out
the frame and do addba and don't aggregate until addba was successful,
that would mean no latency for those frames... addba could even time out
which would add a lot of latency, no?
> Now, assuming we have a successful aggregation session going,
> frame handling looks like this:
>
> * mac80211 sends down a frame
> * Append to TID's buffer list.
>
> On TX Completion,
>
> * Process all TX queues
> * Process all complete descriptors.
> * Complete all sub-frames of an aggregate that were ACKed (send status to mac80211).
> * Re-queue sub-frames that were not ACKed back to the TID's pending queue.
> * Schedule this TID for processing.
Those have to go in front of the queue, right? So they're sent out next?
> * Run through all scheduled TIDs
> * Form aggregates from the pending buffers and send them out.
> ( Again, maintain minimum HW depth )
Which TIDs are "scheduled"?
> So, aggregation is currently done on a need-to basis, and changing this
> to a flow where mac80211 sends down frames with A-MPDU related control information
> would mean a complete rewrite of ath9k's TX path. :-)
So what? :) I'm trying to avoid having to do all this again and again in
b43, rt2x00 etc. The hw really behaves very similarly.
> > Also, I'm trying to understand the relation between a block-ack
> > agreement and A-MDPU, I understand that without a block-ack-agreement
> > aggregation isn't very useful, but could we not, for example, implement
> > (regular) delayed block-ack with much the same infrastructure?
> >
>
> Immediate Block-Ack is mandatory for 11n, delayed BA is optional.
> ath9k currently maintains the BA window state internally too.
> Another candidate that can be moved to mac80211, IMO.
Yeah, I agree. There's information on this in the tx status even I
think.
> > What I could also imagine is that mac80211 simply does pretty much
> > everything and hands a number of skbs to the driver at once with some
> > additional information about how to aggregate them, what sort of spacing
> > to use, etc. It should be able to calculate all the required stuff since
> > it knows from the rate control algorithm what's happening there. That
> > might leave Intel out a bit, but I'm starting to think that we want to
> > special-case them a bit anyway.
>
> Well, I really don't know how this would affect performance, but
> I think this _might_ be a better model.
Where would you see it have a noticeable effect on performance?
johannes
Tomas Winkler wrote:
> We may fit into this schema, just some of the book keeping tasks here
> we have off loaded to the hw.
>
Can you explain a bit more about what is offloaded exactly ?
> > Immediate Block-Ack is mandatory for 11n, delayed BA is optional.
> > ath9k currently maintains the BA window state internally too.
> > Another candidate that can be moved to mac80211, IMO.
> >
>
> TX or RX side?
>
ath9k maintains BA state for both RX and TX currently.
Sujith
Johannes Berg wrote:
> On Mon, 2008-10-20 at 01:35 -0700, Luis R. Rodriguez wrote:
>
> > >> + * @is_part_ampdu: set to indicate that the wireless core should should
> > >> + * treat this frame as part of an AMPDU
> > >
> > > I thought we said we could keep the flag instead of moving to the skb
> > > here?
> >
> > That was just because the changes required to add this was too much
> > for 2.6.27 but since we don't care about this fix in 2.6.27 we can do
> > whatever is needed to fix this properly.
>
> Right, but adding random stuff into skb isn't really "properly" imho.
> And we don't really need this afaict.
>
Sorry, just saw that Tomas had fixed it by setting the flag in ieee80211_tx_h_sequence().
Will fix that.
Sujith
On Mon, 2008-10-20 at 01:35 -0700, Luis R. Rodriguez wrote:
> >> + * @is_part_ampdu: set to indicate that the wireless core should should
> >> + * treat this frame as part of an AMPDU
> >
> > I thought we said we could keep the flag instead of moving to the skb
> > here?
>
> That was just because the changes required to add this was too much
> for 2.6.27 but since we don't care about this fix in 2.6.27 we can do
> whatever is needed to fix this properly.
Right, but adding random stuff into skb isn't really "properly" imho.
And we don't really need this afaict.
johannes
On Mon, 2008-10-20 at 02:14 -0700, Luis R. Rodriguez wrote:
> Indeed I agree with this. I think we can do this in three steps:
>
> 1. resolve aggregation for now for ath9k
That's what Sujith's patch does, basically. It just leaves the Intel
hooks in place.
> 2. resolve aggregation for amdpu_queue case. My suggestion here is to
> use skb_buf_head (after Jouni's suggestion for our driver in fact),
> and do not make use of a qdisc at all for this
I agree on both, but this is more of an implementation detail than an
API description, which we really want to have first.
> 3. consider what we can really share when schedulers on aggr differ,
> in one case where everything is in the driver and the other where its
> not. For example -- when a new driver requiring aggregation is needed
> we can consolidate aggr scheduler mechanisms. If this is not
> acceptable this means we have to do it now but I don't know enough
> about any other hardware to do it accurately to make it work for 2
> hardwares.
I think the scheduler probably doesn't really matter as long as we have
a way to let the frames stop coming in/start again. And I also don't
think there are any other possible much different hardware designs.
johannes
On Mon, Oct 20, 2008 at 8:03 PM, Sujith <[email protected]> wrote:
> Johannes Berg wrote:
>> > Probably because 11n aggregation have more rigorous timing requirements
>> > between frames.
>>
>> What kind? I can only find the ampdu spacing/length exponent stuff.
>>
>
> I am not sure, either.
>
>> > > I guess there's no clear answer here. How about "whichever you want"?
>> > > Though I think I prefer pushing them down as that makes the model easier
>> > > to understand. It probably also makes the Intel case easier to
>> > > implement.
>> >
>> > A way to pull down buffered frames for each TID (like ieee80211_get_buffered_bc() )
>> > would be really useful for ath9k.
>>
>> I'm not sure, that seems like a useful thing initially, but leaves a lot
>> of stuff for the driver. We should probably think about moving more
>> things *up* into mac80211 rather than giving the driver more access to
>> low-level details. This would also allow us possibly even send A-MPDU
>> mcast when acting as an HT AP, something the driver cannot easily do.
>>
>> Can you explain the way it currently works in ath9k?
>>
>
> Alright, it goes something like this:
>
> * mac80211 sends down a frame
> * Initiate an aggregation session for the <RA,TID> if one isn't already in progress.
> * Pause the TID, i.e, add further frames to the TID's queue.
> * If ADDBA exchange is successful, resume TID.
> * Form aggregates from the TID's buffer list, send them out.
> ( Take care to maintain minimum HW queue depth for aggregation )
> * If ADDBA exchange fails, flush TID.
> * Send TID's pending frames as normal frames (non-ampdu)
>
> Now, assuming we have a successful aggregation session going,
> frame handling looks like this:
>
> * mac80211 sends down a frame
> * Append to TID's buffer list.
>
> On TX Completion,
>
> * Process all TX queues
> * Process all complete descriptors.
> * Complete all sub-frames of an aggregate that were ACKed (send status to mac80211).
> * Re-queue sub-frames that were not ACKed back to the TID's pending queue.
> * Schedule this TID for processing.
> * Run through all scheduled TIDs
> * Form aggregates from the pending buffers and send them out.
> ( Again, maintain minimum HW depth )
>
> So, aggregation is currently done on a need-to basis, and changing this
> to a flow where mac80211 sends down frames with A-MPDU related control information
> would mean a complete rewrite of ath9k's TX path. :-)
>
We may fit into this schema, just some of the book keeping tasks here
we have off loaded to the hw.
>> Also, I'm trying to understand the relation between a block-ack
>> agreement and A-MDPU, I understand that without a block-ack-agreement
>> aggregation isn't very useful, but could we not, for example, implement
>> (regular) delayed block-ack with much the same infrastructure?
>>
>
> Immediate Block-Ack is mandatory for 11n, delayed BA is optional.
> ath9k currently maintains the BA window state internally too.
> Another candidate that can be moved to mac80211, IMO.
>
TX or RX side?
>> What I could also imagine is that mac80211 simply does pretty much
>> everything and hands a number of skbs to the driver at once with some
>> additional information about how to aggregate them, what sort of spacing
>> to use, etc. It should be able to calculate all the required stuff since
>> it knows from the rate control algorithm what's happening there.
What you need is to compute how many frames can be transmitted within
TXOP i think we can provide the calculation
depending on the current state. Just with the retransmission down
scaling need to be taken into account.
That
>> might leave Intel out a bit, but I'm starting to think that we want to
>> special-case them a bit anyway.
Let's see first mvel and bcom implementation :)
> Well, I really don't know how this would affect performance, but
> I think this _might_ be a better model.
>
> Sujith
>
Tomas
On Mon, Oct 20, 2008 at 1:58 AM, Sujith <[email protected]> wrote:
> Johannes Berg wrote:
> >
> > > --- a/include/linux/skbuff.h
> > > +++ b/include/linux/skbuff.h
> > > @@ -244,6 +244,11 @@ typedef unsigned char *sk_buff_data_t;
> > > * @tc_verd: traffic control verdict
> > > * @ndisc_nodetype: router type (from link layer)
> > > * @do_not_encrypt: set to prevent encryption of this frame
> > > + * @requeue: set to indicate that the wireless core should attempt
> > > + * a software retry on this frame if we failed to
> > > + * receive an ACK for it
> >
> >
> >
> > > + * @is_part_ampdu: set to indicate that the wireless core should should
> > > + * treat this frame as part of an AMPDU
> >
> > I thought we said we could keep the flag instead of moving to the skb
> > here?
> >
> > The next step, imho, is to remove all the "if (hw->num_ampdu_queues)"
> > stuff and figure out what kind of services mac80211 should provide.
>
> Well, currently ath9k maintains a buffer list for each tid.
> When mac80211 sends down a frame, if the recipient has an aggr. session going,
> it is appended to the tid's buffer list. Non-HT frames are sent out immediately.
> On TX completion, we run through all the ACs, STAs and TIDs and send out pending
> frames as aggregates.
>
> IMO, this is heavy stuff for a driver.
> mac80211 can probably help by maintaining the TX state for each TID, maintain the
> buffer list, etc. and provide appropriate mechanisms for drivers to obtain pending
> frames as and when needed.
Indeed I agree with this. I think we can do this in three steps:
1. resolve aggregation for now for ath9k
2. resolve aggregation for amdpu_queue case. My suggestion here is to
use skb_buf_head (after Jouni's suggestion for our driver in fact),
and do not make use of a qdisc at all for this
3. consider what we can really share when schedulers on aggr differ,
in one case where everything is in the driver and the other where its
not. For example -- when a new driver requiring aggregation is needed
we can consolidate aggr scheduler mechanisms. If this is not
acceptable this means we have to do it now but I don't know enough
about any other hardware to do it accurately to make it work for 2
hardwares.
Thoughts?
Luis
On Mon, 2008-10-20 at 14:28 +0530, Sujith wrote:
> Johannes Berg wrote:
> >
Can you change your quote string from " > " to "> "? Just asking, if you
can't/don't want to that's fine, it just always confuses me for a
second. I really should try to figure out if I can convince my mailer to
highlight " > " indented text as quote too.
> Well, currently ath9k maintains a buffer list for each tid.
> When mac80211 sends down a frame, if the recipient has an aggr. session going,
> it is appended to the tid's buffer list. Non-HT frames are sent out immediately.
Incidentally, why the distinction between HT and non-HT frames?
> On TX completion, we run through all the ACs, STAs and TIDs and send out pending
> frames as aggregates.
So basically you can always have one frame or one aggregation
"pack" (for lack of a better word) queued to the hw?
> IMO, this is heavy stuff for a driver.
Yeah most of it probably is.
> mac80211 can probably help by maintaining the TX state for each TID, maintain the
> buffer list, etc. and provide appropriate mechanisms for drivers to obtain pending
> frames as and when needed.
Sure could. Intel's hw would probably like a way to say "stop this TID"
and "wake this TID" which is what they tried to do using real queues.
mac80211 could queue up the frames in per-station state and provide a
functions like
ieee80211_stop_aggr_stream(struct ieee80211_sta *sta, u8 tid);
ieee80211_wake_aggr_stream(struct ieee80211_sta *sta, u8 tid);
or something like that. Then those frames would flow into the right
queue. We might need a multi-layer start/stop management but we already
do, we've just managed to get away without it without tx stalls. Or they
just haven't been reported yet.
johannes
On Mon, 2008-10-27 at 19:55 +0530, Sujith wrote:
> Johannes Berg wrote:
> > We have this "ampdu_ack_map" parameter in the TX status which could be
> > set by the driver. But that assumes we only want a single TX status for
> > all A-MPDU frames, do we really? Or do we want to split them up and
> > handle it all in mac80211?
>
> I wasn't aware of this parameter at all. Looks useful.
>
> Atheros HW reports the Block Ack bitmap of the entire aggregate.
> So passing that up to mac80211, and letting mac80211 handle retransmissions would work.
> I am not sure about Intel.
Intel does the retransmissions in hw. Not sure how we'd want to handle
that difference.
johannes
On Thu, Oct 23, 2008 at 7:45 AM, Johannes Berg
<[email protected]> wrote:
> On Thu, 2008-10-23 at 15:01 +0530, Sujith wrote:
>> Johannes Berg wrote:
>> > It seems that should be a rate control decision? Possibly taking into
>> > account more than just always doing aggregation sessions. Then again, I
>> > suppose aggregation sessions are cheap. What about latency here?
>> >
>>
>> Well, that is what Luis seems to think too, but our RC
>> doesn't do much now, so we try to setup an aggregation session with any
>> associated STA.
>
> If that was done in the RC at least (could easily be moved I suppose)
> and you cleaned up the RC, then surely nbd would play with porting
> minstrel and making it aware of such things, which probably makes for a
> better RC... And since you have to clean up the RC anyway :)
I checked internally to verify where you would decide when to AMPDU
and to try to get different reviews and opinions, and it seems that
the path we take right now is correct as there is not much overhead so
we always use AMPDU with whoever supports it with data frames. If
you're a STA you do it all the time with the AP for data frames.
I noticed iwlagn had some more logic within the RC but I gave up
trying to follow the logic. I suspect they do the same though, Tomas?
Luis
On Mon, 2008-10-20 at 15:37 +0530, Sujith wrote:
> Johannes Berg wrote:
> > On Mon, 2008-10-20 at 02:14 -0700, Luis R. Rodriguez wrote:
> >
> > > Indeed I agree with this. I think we can do this in three steps:
> > >
> > > 1. resolve aggregation for now for ath9k
> >
> > That's what Sujith's patch does, basically. It just leaves the Intel
> > hooks in place.
> >
> > > 2. resolve aggregation for amdpu_queue case. My suggestion here is to
> > > use skb_buf_head (after Jouni's suggestion for our driver in fact),
> > > and do not make use of a qdisc at all for this
> >
> > I agree on both, but this is more of an implementation detail than an
> > API description, which we really want to have first.
>
>
> Would be nice if an interim fix is accepted temporarily,
> until the architectural details of proper mac80211 aggregation
> support for the 2 classes of HW are defined, and implemented.
Yeah I think that if you clean up this patch a bit wrt. that flag and
double-check it won't cause problems on hardware that cannot do
aggregation at all I'd be ok with it. Like I said, a later patch can
remove all the if (ampdu_queues) stuff
johannes
On Thu, Oct 23, 2008 at 7:23 PM, Luis R. Rodriguez <[email protected]> wrote:
> On Thu, Oct 23, 2008 at 7:45 AM, Johannes Berg
> <[email protected]> wrote:
>> On Thu, 2008-10-23 at 15:01 +0530, Sujith wrote:
>>> Johannes Berg wrote:
>>> > It seems that should be a rate control decision? Possibly taking into
>>> > account more than just always doing aggregation sessions. Then again, I
>>> > suppose aggregation sessions are cheap. What about latency here?
>>> >
>>>
>>> Well, that is what Luis seems to think too, but our RC
>>> doesn't do much now, so we try to setup an aggregation session with any
>>> associated STA.
>>
>> If that was done in the RC at least (could easily be moved I suppose)
>> and you cleaned up the RC, then surely nbd would play with porting
>> minstrel and making it aware of such things, which probably makes for a
>> better RC... And since you have to clean up the RC anyway :)
>
> I checked internally to verify where you would decide when to AMPDU
> and to try to get different reviews and opinions, and it seems that
> the path we take right now is correct as there is not much overhead so
> we always use AMPDU with whoever supports it with data frames. If
> you're a STA you do it all the time with the AP for data frames.
>
> I noticed iwlagn had some more logic within the RC but I gave up
> trying to follow the logic. I suspect they do the same though, Tomas?
I got different output from our system engineering. APMDU has overhead
and if there is not requirement
for high throughput we don't initiate one. What is for sure we shounld
not have aggregation enabled for VO AC.
I've payed attention that there are different APs do that differently
some opens BA session on BE AC immediately upon association some do it
on high throughput I haven't seen in the second case it will be opened
on simple ping.
Anyhow if we would implement the triggering on throughput simply
setting starting and stopping thresholds to small values will have
same effect as your solution and setting it to 0 and maximum will
respectively will have effect on opening BA session upon association.
Tomas
On Mon, Oct 20, 2008 at 1:47 AM, Sujith <[email protected]> wrote:
> Johannes Berg wrote:
> > On Mon, 2008-10-20 at 01:35 -0700, Luis R. Rodriguez wrote:
> >
> > > >> + * @is_part_ampdu: set to indicate that the wireless core should should
> > > >> + * treat this frame as part of an AMPDU
> > > >
> > > > I thought we said we could keep the flag instead of moving to the skb
> > > > here?
> > >
> > > That was just because the changes required to add this was too much
> > > for 2.6.27 but since we don't care about this fix in 2.6.27 we can do
> > > whatever is needed to fix this properly.
> >
> > Right, but adding random stuff into skb isn't really "properly" imho.
> > And we don't really need this afaict.
> >
>
> Sorry, just saw that Tomas had fixed it by setting the flag in ieee80211_tx_h_sequence().
> Will fix that.
No that was a hack too, this was to minimize code changes for 2.6.27 as well.
Luis
Johannes Berg wrote:
> > Probably because 11n aggregation have more rigorous timing requirements
> > between frames.
>
> What kind? I can only find the ampdu spacing/length exponent stuff.
>
I am not sure, either.
> > > I guess there's no clear answer here. How about "whichever you want"?
> > > Though I think I prefer pushing them down as that makes the model easier
> > > to understand. It probably also makes the Intel case easier to
> > > implement.
> >
> > A way to pull down buffered frames for each TID (like ieee80211_get_buffered_bc() )
> > would be really useful for ath9k.
>
> I'm not sure, that seems like a useful thing initially, but leaves a lot
> of stuff for the driver. We should probably think about moving more
> things *up* into mac80211 rather than giving the driver more access to
> low-level details. This would also allow us possibly even send A-MPDU
> mcast when acting as an HT AP, something the driver cannot easily do.
>
> Can you explain the way it currently works in ath9k?
>
Alright, it goes something like this:
* mac80211 sends down a frame
* Initiate an aggregation session for the <RA,TID> if one isn't already in progress.
* Pause the TID, i.e, add further frames to the TID's queue.
* If ADDBA exchange is successful, resume TID.
* Form aggregates from the TID's buffer list, send them out.
( Take care to maintain minimum HW queue depth for aggregation )
* If ADDBA exchange fails, flush TID.
* Send TID's pending frames as normal frames (non-ampdu)
Now, assuming we have a successful aggregation session going,
frame handling looks like this:
* mac80211 sends down a frame
* Append to TID's buffer list.
On TX Completion,
* Process all TX queues
* Process all complete descriptors.
* Complete all sub-frames of an aggregate that were ACKed (send status to mac80211).
* Re-queue sub-frames that were not ACKed back to the TID's pending queue.
* Schedule this TID for processing.
* Run through all scheduled TIDs
* Form aggregates from the pending buffers and send them out.
( Again, maintain minimum HW depth )
So, aggregation is currently done on a need-to basis, and changing this
to a flow where mac80211 sends down frames with A-MPDU related control information
would mean a complete rewrite of ath9k's TX path. :-)
> Also, I'm trying to understand the relation between a block-ack
> agreement and A-MDPU, I understand that without a block-ack-agreement
> aggregation isn't very useful, but could we not, for example, implement
> (regular) delayed block-ack with much the same infrastructure?
>
Immediate Block-Ack is mandatory for 11n, delayed BA is optional.
ath9k currently maintains the BA window state internally too.
Another candidate that can be moved to mac80211, IMO.
> What I could also imagine is that mac80211 simply does pretty much
> everything and hands a number of skbs to the driver at once with some
> additional information about how to aggregate them, what sort of spacing
> to use, etc. It should be able to calculate all the required stuff since
> it knows from the rate control algorithm what's happening there. That
> might leave Intel out a bit, but I'm starting to think that we want to
> special-case them a bit anyway.
Well, I really don't know how this would affect performance, but
I think this _might_ be a better model.
Sujith
Johannes Berg wrote:
> It seems that should be a rate control decision? Possibly taking into
> account more than just always doing aggregation sessions. Then again, I
> suppose aggregation sessions are cheap. What about latency here?
>
Well, that is what Luis seems to think too, but our RC
doesn't do much now, so we try to setup an aggregation session with any
associated STA.
> "maintain minimum HW queue depth"? In what way? You mean put in enough
> frames?
I was talking about this:
(txq->axq_depth < ATH_AGGR_MIN_QDEPTH) in ath_tx_sched_aggr()@xmit.c
> > On TX Completion,
> >
> > * Process all TX queues
> > * Process all complete descriptors.
> > * Complete all sub-frames of an aggregate that were ACKed (send status to mac80211).
> > * Re-queue sub-frames that were not ACKed back to the TID's pending queue.
> > * Schedule this TID for processing.
>
> Those have to go in front of the queue, right? So they're sent out next?
Yep, they are spliced back to the beginning of the queue.
>
> > * Run through all scheduled TIDs
> > * Form aggregates from the pending buffers and send them out.
> > ( Again, maintain minimum HW depth )
>
> Which TIDs are "scheduled"?
All of the TIDs that have pending buffers.
> > So, aggregation is currently done on a need-to basis, and changing this
> > to a flow where mac80211 sends down frames with A-MPDU related control information
> > would mean a complete rewrite of ath9k's TX path. :-)
>
> So what? :) I'm trying to avoid having to do all this again and again in
> b43, rt2x00 etc. The hw really behaves very similarly.
>
Agreed.
> > Well, I really don't know how this would affect performance, but
> > I think this _might_ be a better model.
>
> Where would you see it have a noticeable effect on performance?
How would mac80211 buffer frames ? Would it wait for enough sub-frames
to fill an A-MPDU ? When does it decide that buffered frames have to
be pushed down to the driver ?
Sujith
Johannes Berg wrote:
> If that was done in the RC at least (could easily be moved I suppose)
> and you cleaned up the RC, then surely nbd would play with porting
> minstrel and making it aware of such things, which probably makes for a
> better RC... And since you have to clean up the RC anyway :)
>
Sure, just migrating to minstrel would be the better option. It would
probably need to be spruced up a bit to accommodate aggregation specific
data.
> > I was talking about this:
> > (txq->axq_depth < ATH_AGGR_MIN_QDEPTH) in ath_tx_sched_aggr()@xmit.c
>
> But what does this invariant mean? ATH_AGGR_MIN_QDEPTH is 2.
>
Well, this is not relevant when frames are aggregated and sent down
by mac80211. That check was present because frames were being created and
sent in the TX tasklet.
> > How would mac80211 buffer frames ?
>
> Well I suppose it would just put them on a list in the sta_info struct.
>
A list for each TID, no ?
> > Would it wait for enough sub-frames
> > to fill an A-MPDU ?
>
> It should, no? And if for some reason that doesn't happen fast enough it
> can send them out anyway, it'll just be a shorter A-MPDU, no?
>
> > When does it decide that buffered frames have to
> > be pushed down to the driver ?
>
> When the A-MPDU is full, subject to the TX and RX constraints, or when
> too much time has passed maybe?
>
> I really don't know. But I'd like to get to a model where we can
> implement other drivers without having to keep track of all this like
> ath9k does.
To summarize a bit about the state/control information that mac80211 needs
to maintain for an aggregation session:
* Block Ack state for each TID (sequence numbers, BA window etc.)
* Maintain a list of buffers for each <RA,TID> (setup, teardown etc.)
* Handle retransmission of unacked frames.
Forming an aggregate would involve the following.
( These might depend on HW caps as well as the recipient STA's caps)
* Aggregate length
* Delimiters
* Padding of sub-frames
TX control information will probably need to pass aggr. specific information
(number of delimiters etc.)
The API for aggregation can extend the ampdu_action() callback.
Adding a few more states can probably suffice initially.
We'll work on this and come up with an RFC.
Sujith
On Mon, Oct 27, 2008 at 5:04 PM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2008-10-27 at 19:55 +0530, Sujith wrote:
>> Johannes Berg wrote:
>> > We have this "ampdu_ack_map" parameter in the TX status which could be
>> > set by the driver. But that assumes we only want a single TX status for
>> > all A-MPDU frames, do we really? Or do we want to split them up and
>> > handle it all in mac80211?
>>
>> I wasn't aware of this parameter at all. Looks useful.
>>
>> Atheros HW reports the Block Ack bitmap of the entire aggregate.
>> So passing that up to mac80211, and letting mac80211 handle retransmissions would work.
>> I am not sure about Intel.
>
> Intel does the retransmissions in hw. Not sure how we'd want to handle
> that difference.
In addition iwlwifi HW release TX packets in order to mac80211 i.e. no
need to maintain the transition window
so simple flag maybe shell revert the flow into regular tx response
flow in mac80211.
Only rate scaling is aware that the packets were transmitted
aggregated, we did some hacking there around this it should be cleaned
up.
Tomas
On Mon, Oct 27, 2008 at 6:04 PM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2008-10-27 at 17:56 +0200, Tomas Winkler wrote:
>
>> In addition iwlwifi HW release TX packets in order to mac80211 i.e. no
>> need to maintain the transition window
>> so simple flag maybe shell revert the flow into regular tx response
>> flow in mac80211.
>> Only rate scaling is aware that the packets were transmitted
>> aggregated, we did some hacking there around this it should be cleaned
>> up.
>
> I wonder how we're going to pass them down to the driver anyway. And how
> about rate control, is that really done per sub-frame? Does that make
> any sense?
Sub frame? This is bit misleading in this context .This is not AMSDU.
This is rather a regular frame to be sent in a burst.
>I'm thinking that if we pass a fraglist down for the ampdu,
If you are able to do this you can also build AMSDU frame from the
frames, in AMPDU i prefer the streaming approach
and the decision about how many frames get into current TXOP is pushed
as low as possible, But maybe again I'm seeing this from iwlwifi
perspective.
> then we should have the fraglist in tx status too, in one go... If rate
> control is per subframe maybe it just needs to be made aware to walk the
> fraglist?
The rate scale should be aware of aggregation in order to be more
responsive (consider each aggregation as single transition) while
frame completion should be in order to preserve ordered definition of
802.11. protocol to upper layers (only completed sub window is
released up)
Tomas
On Thu, Oct 23, 2008 at 10:18 PM, Luis R. Rodriguez <[email protected]> wrote:
> On Thu, Oct 23, 2008 at 11:31 AM, Tomas Winkler <[email protected]> wrote:
>> On Thu, Oct 23, 2008 at 7:23 PM, Luis R. Rodriguez <[email protected]> wrote:
>
>>> I checked internally to verify where you would decide when to AMPDU
>>> and to try to get different reviews and opinions, and it seems that
>>> the path we take right now is correct as there is not much overhead so
>>> we always use AMPDU with whoever supports it with data frames. If
>>> you're a STA you do it all the time with the AP for data frames.
>>>
>>> I noticed iwlagn had some more logic within the RC but I gave up
>>> trying to follow the logic. I suspect they do the same though, Tomas?
>
> Thanks for the feedback!
>
>> I got different output from our system engineering. APMDU has overhead
>
> Is that because of the interactions required in firmware for scheduling BTW?
>
No HW support make aggregation faster, the overhead is in the air. BA
is bigger then ACK so if you don't have burst than you need to send BA
for single frame it's slower then sending ACK.
>> and if there is not requirement
>> for high throughput we don't initiate one.
>
> This makes sense to me regardless.
>
>> What is for sure we shounld
>> not have aggregation enabled for VO AC.
>
> Any particular reason? I'll review this too, but my guess would be the
> buffering required before TXing. I'll see how we perform with aggr on
No the problem is on the receiver side you don't release packets to
network stuck out of order so there is inherit delay in aggregation.
Voice is sensitive to delay and jitter.
>> I've payed attention that there are different APs do that differently
>> some opens BA session on BE AC immediately upon association some do it
>> on high throughput I haven't seen in the second case it will be opened
>> on simple ping.
>> Anyhow if we would implement the triggering on throughput simply
>> setting starting and stopping thresholds to small values will have
>> same effect as your solution and setting it to 0 and maximum will
>> respectively will have effect on opening BA session upon association.
>
> Seems reasonable. If this is set then as a hw configurable option in
> mac80211 then we can even allow RC control this, should this ever be
> needed (we don't).
>
> Luis
>
On Thu, Oct 23, 2008 at 11:31 AM, Tomas Winkler <[email protected]> wrote:
> On Thu, Oct 23, 2008 at 7:23 PM, Luis R. Rodriguez <[email protected]> wrote:
>> I checked internally to verify where you would decide when to AMPDU
>> and to try to get different reviews and opinions, and it seems that
>> the path we take right now is correct as there is not much overhead so
>> we always use AMPDU with whoever supports it with data frames. If
>> you're a STA you do it all the time with the AP for data frames.
>>
>> I noticed iwlagn had some more logic within the RC but I gave up
>> trying to follow the logic. I suspect they do the same though, Tomas?
Thanks for the feedback!
> I got different output from our system engineering. APMDU has overhead
Is that because of the interactions required in firmware for scheduling BTW?
> and if there is not requirement
> for high throughput we don't initiate one.
This makes sense to me regardless.
> What is for sure we shounld
> not have aggregation enabled for VO AC.
Any particular reason? I'll review this too, but my guess would be the
buffering required before TXing. I'll see how we perform with aggr on
VO AC.
> I've payed attention that there are different APs do that differently
> some opens BA session on BE AC immediately upon association some do it
> on high throughput I haven't seen in the second case it will be opened
> on simple ping.
> Anyhow if we would implement the triggering on throughput simply
> setting starting and stopping thresholds to small values will have
> same effect as your solution and setting it to 0 and maximum will
> respectively will have effect on opening BA session upon association.
Seems reasonable. If this is set then as a hw configurable option in
mac80211 then we can even allow RC control this, should this ever be
needed (we don't).
Luis
Johannes Berg wrote:
> On Mon, 2008-10-20 at 02:14 -0700, Luis R. Rodriguez wrote:
>
> > Indeed I agree with this. I think we can do this in three steps:
> >
> > 1. resolve aggregation for now for ath9k
>
> That's what Sujith's patch does, basically. It just leaves the Intel
> hooks in place.
>
> > 2. resolve aggregation for amdpu_queue case. My suggestion here is to
> > use skb_buf_head (after Jouni's suggestion for our driver in fact),
> > and do not make use of a qdisc at all for this
>
> I agree on both, but this is more of an implementation detail than an
> API description, which we really want to have first.
Would be nice if an interim fix is accepted temporarily,
until the architectural details of proper mac80211 aggregation
support for the 2 classes of HW are defined, and implemented.
Sujith
Johannes Berg wrote:
> On Mon, 2008-10-20 at 02:01 -0700, Luis R. Rodriguez wrote:
>
> > > Well the fact that it's in tx_h_sequence is a bit weird, but we should
> > > be able to get around not using an extra bit in skb I think. Especially
> > > for this since it's only really between the driver and mac80211, and
> > > after a requeue the ampdu status might actually change.
> >
> > Good point. I failed to see that. So in that case it should be as
> > sujith had it before my suggestion and just handle it as he had it in
> > the tx handlers. The problem though is that the RC algorithm should be
> > the one to determine if an skb is part of an ampdu for aggregation or
> > not IMHO. Actually this is the real question and hot topic.
> >
> > When should we aggregate and should mac80211 decide that or be left to the RC.
>
> No, the RC algorithm cannot decided that, the RC algorithm can merely
> decide when to set up/tear down aggregation sessions. When a session is
> up all packet belonging to that TID should go into it, surely?
Right, ath9k currently tries to open an aggregation session for
each station that it is associated with. RC doesn't determine anything now.
Sujith
On Wed, Oct 22, 2008 at 12:00 PM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2008-10-20 at 23:33 +0530, Sujith wrote:
>
>> > Can you explain the way it currently works in ath9k?
>> >
>>
>> Alright, it goes something like this:
>
> Thanks.
>
>> * mac80211 sends down a frame
>> * Initiate an aggregation session for the <RA,TID> if one isn't already in progress.
>
> It seems that should be a rate control decision? Possibly taking into
> account more than just always doing aggregation sessions. Then again, I
> suppose aggregation sessions are cheap. What about latency here?
Aggregation has it's overhead, both computing resources and on the
air. Obvious that it has negative affect on latency so if the 'minimum
HW queue depth' is not satisfied the packets
are delayed till the queue is full enough.
In iwl-agn-rs scale we keep track of current tpt to take decision
about starting aggregation. The throughput watch can be viewed as
different entity it was just convenient and conceptually fit to
incorporate into rate scale.
>
>> * Pause the TID, i.e, add further frames to the TID's queue.
>> * If ADDBA exchange is successful, resume TID.
>> * Form aggregates from the TID's buffer list, send them out.
>> ( Take care to maintain minimum HW queue depth for aggregation )
>
> "maintain minimum HW queue depth"? In what way? You mean put in enough
> frames?
>
>> * If ADDBA exchange fails, flush TID.
>> * Send TID's pending frames as normal frames (non-ampdu)
>
> Obviously. But why isn't this done in parallel? I mean, why not send out
> the frame and do addba and don't aggregate until addba was successful,
> that would mean no latency for those frames... addba could even time out
> which would add a lot of latency, no?
The negotiation is done on starting sequence number.
Tomas
On Wed, 2008-10-22 at 13:59 +0200, Tomas Winkler wrote:
> It answers BA i n RX path and interpret BA in TX Scheduling
> retransmission of not acked frames is done in HW (not firmware) no
> need to requeue them. In corner case when retries are exhausted BA
> request is issued from mac80211 to advance RX side window.
Ok, but if we do both of that in mac80211, it wouldn't really matter
since we never see the relevant frames, right? Or we can add a flag to
ignore them.
> > Like I said, from what I can tell bcom is really similar, and I'd like
> > to not have to re-implement all this first. mac80211 used to have a more
> > push-model, which we've deviated from with the HT stuff you did, but for
> > most hardware the push-model is still appropriate, so imho we should try
> > to go to one even with HT and then see.
>
> Works for me. What I stumble upon is retransmission from within TX
> response path
> how this go together with context of regular TX path. Even in iwlwifi
> we need to be able to kick/start the internal aggregation queue?
Yeah, this is something we definitely need to work out. We haven't even
managed to decide yet how aggregated packets are given to the driver :)
I think for atheros and bcom a model where mac80211 decides pretty much
everything and hands the driver a list of skbs to aggregate would work
best, but then I think that wouldn't work with your hw at all.
> > We've since reworked rate
> > control to be able to cope with HT too, so mac80211 can have much more
> > information about what's going on and should be able to make many of the
> > necessary decisions.
>
> What to remind that iwl-agn-rs update rates scale out of band meaning
> TX packet doesn't bear the rate scale information. The reasoning
> behind is that appropriate rate is picked up when packet is ready to
> go on the air and not when it's somewhere deep in the queue.
> Otherwise it's regular multiretry algorithm.
Well, yeah, but the best thing we can do if we don't do it in firmware
is actually determining it at the point we stick the packet into the
queue. I wouldn't mind moving all iwl-agn rate scaling into the driver
and telling mac80211 that it doesn't need rate scaling, but I'm not
quite sure how this affects aggregation?
johannes
On Mon, 2008-10-27 at 19:41 +0530, Sujith wrote:
> > > How would mac80211 buffer frames ?
> >
> > Well I suppose it would just put them on a list in the sta_info struct.
> >
>
> A list for each TID, no ?
Right.
> To summarize a bit about the state/control information that mac80211 needs
> to maintain for an aggregation session:
>
> * Block Ack state for each TID (sequence numbers, BA window etc.)
> * Maintain a list of buffers for each <RA,TID> (setup, teardown etc.)
> * Handle retransmission of unacked frames.
>
> Forming an aggregate would involve the following.
> ( These might depend on HW caps as well as the recipient STA's caps)
>
> * Aggregate length
> * Delimiters
> * Padding of sub-frames
>
> TX control information will probably need to pass aggr. specific information
> (number of delimiters etc.)
>
> The API for aggregation can extend the ampdu_action() callback.
> Adding a few more states can probably suffice initially.
Sounds good to me. If you need more space in TX control information...
Not sure what to do then. We could take out 'vif' and put that into the
tx() arguments instead, for example.
> We'll work on this and come up with an RFC.
Great!
johannes
On Mon, 2008-10-27 at 17:56 +0200, Tomas Winkler wrote:
> In addition iwlwifi HW release TX packets in order to mac80211 i.e. no
> need to maintain the transition window
> so simple flag maybe shell revert the flow into regular tx response
> flow in mac80211.
> Only rate scaling is aware that the packets were transmitted
> aggregated, we did some hacking there around this it should be cleaned
> up.
I wonder how we're going to pass them down to the driver anyway. And how
about rate control, is that really done per sub-frame? Does that make
any sense? I'm thinking that if we pass a fraglist down for the ampdu,
then we should have the fraglist in tx status too, in one go... If rate
control is per subframe maybe it just needs to be made aware to walk the
fraglist?
johannes
Johannes Berg wrote:
> We have this "ampdu_ack_map" parameter in the TX status which could be
> set by the driver. But that assumes we only want a single TX status for
> all A-MPDU frames, do we really? Or do we want to split them up and
> handle it all in mac80211?
I wasn't aware of this parameter at all. Looks useful.
Atheros HW reports the Block Ack bitmap of the entire aggregate.
So passing that up to mac80211, and letting mac80211 handle retransmissions would work.
I am not sure about Intel.
Sujith
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -244,6 +244,11 @@ typedef unsigned char *sk_buff_data_t;
> * @tc_verd: traffic control verdict
> * @ndisc_nodetype: router type (from link layer)
> * @do_not_encrypt: set to prevent encryption of this frame
> + * @requeue: set to indicate that the wireless core should attempt
> + * a software retry on this frame if we failed to
> + * receive an ACK for it
> + * @is_part_ampdu: set to indicate that the wireless core should should
> + * treat this frame as part of an AMPDU
I thought we said we could keep the flag instead of moving to the skb
here?
The next step, imho, is to remove all the "if (hw->num_ampdu_queues)"
stuff and figure out what kind of services mac80211 should provide.
johannes
On Thu, 2008-10-23 at 14:27 +0530, Sujith wrote:
> > Yeah, this is something we definitely need to work out. We haven't even
> > managed to decide yet how aggregated packets are given to the driver :)
> > I think for atheros and bcom a model where mac80211 decides pretty much
> > everything and hands the driver a list of skbs to aggregate would work
> > best, but then I think that wouldn't work with your hw at all.
> >
>
> Retransmission of unacked frames is done within the driver, in ath9k.
> In case of retry exhaustion (failed transmission), we do what Intel does,
> i.e, request for a BAR to be sent out.
We have this "ampdu_ack_map" parameter in the TX status which could be
set by the driver. But that assumes we only want a single TX status for
all A-MPDU frames, do we really? Or do we want to split them up and
handle it all in mac80211?
johannes
Johannes Berg wrote:
>
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -244,6 +244,11 @@ typedef unsigned char *sk_buff_data_t;
> > * @tc_verd: traffic control verdict
> > * @ndisc_nodetype: router type (from link layer)
> > * @do_not_encrypt: set to prevent encryption of this frame
> > + * @requeue: set to indicate that the wireless core should attempt
> > + * a software retry on this frame if we failed to
> > + * receive an ACK for it
>
>
>
> > + * @is_part_ampdu: set to indicate that the wireless core should should
> > + * treat this frame as part of an AMPDU
>
> I thought we said we could keep the flag instead of moving to the skb
> here?
>
> The next step, imho, is to remove all the "if (hw->num_ampdu_queues)"
> stuff and figure out what kind of services mac80211 should provide.
Well, currently ath9k maintains a buffer list for each tid.
When mac80211 sends down a frame, if the recipient has an aggr. session going,
it is appended to the tid's buffer list. Non-HT frames are sent out immediately.
On TX completion, we run through all the ACs, STAs and TIDs and send out pending
frames as aggregates.
IMO, this is heavy stuff for a driver.
mac80211 can probably help by maintaining the TX state for each TID, maintain the
buffer list, etc. and provide appropriate mechanisms for drivers to obtain pending
frames as and when needed.
Sujith
On Wed, Oct 22, 2008 at 12:03 PM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2008-10-20 at 23:46 +0200, Tomas Winkler wrote:
>
>> > Immediate Block-Ack is mandatory for 11n, delayed BA is optional.
>> > ath9k currently maintains the BA window state internally too.
>> > Another candidate that can be moved to mac80211, IMO.
>> >
>>
>> TX or RX side?
>
> Both? What exactly does your firmware do, say of the list that Sujith
> posted?
>
It answers BA i n RX path and interpret BA in TX Scheduling
retransmission of not acked frames is done in HW (not firmware) no
need to requeue them. In corner case when retries are exhausted BA
request is issued from mac80211 to advance RX side window.
>> >> What I could also imagine is that mac80211 simply does pretty much
>> >> everything and hands a number of skbs to the driver at once with some
>> >> additional information about how to aggregate them, what sort of spacing
>> >> to use, etc. It should be able to calculate all the required stuff since
>> >> it knows from the rate control algorithm what's happening there.
>>
>> What you need is to compute how many frames can be transmitted within
>> TXOP i think we can provide the calculation
>> depending on the current state. Just with the retransmission down
>> scaling need to be taken into account.
>
> We really need TXOP handling in mac80211 anyway, no? Right now we won't
> handle frames that are too long for the TXOP, for example.
>
>> That
>> >> might leave Intel out a bit, but I'm starting to think that we want to
>> >> special-case them a bit anyway.
>>
>> Let's see first mvel and bcom implementation :)
>
> Like I said, from what I can tell bcom is really similar, and I'd like
> to not have to re-implement all this first. mac80211 used to have a more
> push-model, which we've deviated from with the HT stuff you did, but for
> most hardware the push-model is still appropriate, so imho we should try
> to go to one even with HT and then see.
Works for me. What I stumble upon is retransmission from within TX
response path
how this go together with context of regular TX path. Even in iwlwifi
we need to be able to kick/start the internal aggregation queue?
We've since reworked rate
> control to be able to cope with HT too, so mac80211 can have much more
> information about what's going on and should be able to make many of the
> necessary decisions.
What to remind that iwl-agn-rs update rates scale out of band meaning
TX packet doesn't bear the rate scale information. The reasoning
behind is that appropriate rate is picked up when packet is ready to
go on the air and not when it's somewhere deep in the queue.
Otherwise it's regular multiretry algorithm.
Tomas
On Mon, 2008-10-20 at 02:01 -0700, Luis R. Rodriguez wrote:
> > Well the fact that it's in tx_h_sequence is a bit weird, but we should
> > be able to get around not using an extra bit in skb I think. Especially
> > for this since it's only really between the driver and mac80211, and
> > after a requeue the ampdu status might actually change.
>
> Good point. I failed to see that. So in that case it should be as
> sujith had it before my suggestion and just handle it as he had it in
> the tx handlers. The problem though is that the RC algorithm should be
> the one to determine if an skb is part of an ampdu for aggregation or
> not IMHO. Actually this is the real question and hot topic.
>
> When should we aggregate and should mac80211 decide that or be left to the RC.
No, the RC algorithm cannot decided that, the RC algorithm can merely
decide when to set up/tear down aggregation sessions. When a session is
up all packet belonging to that TID should go into it, surely?
johannes
On Mon, Oct 20, 2008 at 1:31 AM, Johannes Berg
<[email protected]> wrote:
>
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -244,6 +244,11 @@ typedef unsigned char *sk_buff_data_t;
>> * @tc_verd: traffic control verdict
>> * @ndisc_nodetype: router type (from link layer)
>> * @do_not_encrypt: set to prevent encryption of this frame
>> + * @requeue: set to indicate that the wireless core should attempt
>> + * a software retry on this frame if we failed to
>> + * receive an ACK for it
>
>
>
>> + * @is_part_ampdu: set to indicate that the wireless core should should
>> + * treat this frame as part of an AMPDU
>
> I thought we said we could keep the flag instead of moving to the skb
> here?
That was just because the changes required to add this was too much
for 2.6.27 but since we don't care about this fix in 2.6.27 we can do
whatever is needed to fix this properly.
Luis
On Mon, Oct 20, 2008 at 1:45 AM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2008-10-20 at 01:35 -0700, Luis R. Rodriguez wrote:
>
>> >> + * @is_part_ampdu: set to indicate that the wireless core should should
>> >> + * treat this frame as part of an AMPDU
>> >
>> > I thought we said we could keep the flag instead of moving to the skb
>> > here?
>>
>> That was just because the changes required to add this was too much
>> for 2.6.27 but since we don't care about this fix in 2.6.27 we can do
>> whatever is needed to fix this properly.
>
> Right, but adding random stuff into skb isn't really "properly" imho.
> And we don't really need this afaict.
Agreed -- which is why I was suggesting we should consider using
skb->do_not_fragment instead. Although I didn't finish complete review
to ensure that is the only reason we need this. That is a more generic
flag. Thoughts?
Luis
On Thu, Oct 23, 2008 at 2:17 PM, Tomas Winkler <[email protected]> wrote:
> On Thu, Oct 23, 2008 at 10:18 PM, Luis R. Rodriguez <[email protected]> wrote:
>> On Thu, Oct 23, 2008 at 11:31 AM, Tomas Winkler <[email protected]> wrote:
>>> On Thu, Oct 23, 2008 at 7:23 PM, Luis R. Rodriguez <[email protected]> wrote:
>>
>>>> I checked internally to verify where you would decide when to AMPDU
>>>> and to try to get different reviews and opinions, and it seems that
>>>> the path we take right now is correct as there is not much overhead so
>>>> we always use AMPDU with whoever supports it with data frames. If
>>>> you're a STA you do it all the time with the AP for data frames.
>>>>
>>>> I noticed iwlagn had some more logic within the RC but I gave up
>>>> trying to follow the logic. I suspect they do the same though, Tomas?
>>
>> Thanks for the feedback!
>>
>>> I got different output from our system engineering. APMDU has overhead
>>
>> Is that because of the interactions required in firmware for scheduling BTW?
>>
> No HW support make aggregation faster, the overhead is in the air. BA
> is bigger then ACK so if you don't have burst than you need to send BA
> for single frame it's slower then sending ACK.
>
>>> and if there is not requirement
>>> for high throughput we don't initiate one.
>>
>> This makes sense to me regardless.
>>
>>> What is for sure we shounld
>>> not have aggregation enabled for VO AC.
>>
>> Any particular reason? I'll review this too, but my guess would be the
>> buffering required before TXing. I'll see how we perform with aggr on
>
> No the problem is on the receiver side you don't release packets to
> network stuck out of order so there is inherit delay in aggregation.
> Voice is sensitive to delay and jitter.
I got some more review on this and it seems you can potentially have
optimizations not only for VO (besides just disabling it) but also for
each AC on aggregation (well BE and BK can use the same). Question is
how do we want to deal with this for now in mac80211, leave it up to
each driver to handle this or do we move this centrally to mac80211?
I'd rather we try to come to some agreement on some decent decision
for each AC and move this decision to mac80211. Thoughts?
Luis
On Mon, 2008-10-20 at 16:05 +0530, Sujith wrote:
> > But couldn't non-HT frames be buffered similarly? Maybe I'm missing one
> > of the finer points of the 11n draft?
>
> Probably because 11n aggregation have more rigorous timing requirements
> between frames.
What kind? I can only find the ampdu spacing/length exponent stuff.
> > I guess there's no clear answer here. How about "whichever you want"?
> > Though I think I prefer pushing them down as that makes the model easier
> > to understand. It probably also makes the Intel case easier to
> > implement.
>
> A way to pull down buffered frames for each TID (like ieee80211_get_buffered_bc() )
> would be really useful for ath9k.
I'm not sure, that seems like a useful thing initially, but leaves a lot
of stuff for the driver. We should probably think about moving more
things *up* into mac80211 rather than giving the driver more access to
low-level details. This would also allow us possibly even send A-MPDU
mcast when acting as an HT AP, something the driver cannot easily do.
Can you explain the way it currently works in ath9k?
Also, I'm trying to understand the relation between a block-ack
agreement and A-MDPU, I understand that without a block-ack-agreement
aggregation isn't very useful, but could we not, for example, implement
(regular) delayed block-ack with much the same infrastructure?
What I could also imagine is that mac80211 simply does pretty much
everything and hands a number of skbs to the driver at once with some
additional information about how to aggregate them, what sort of spacing
to use, etc. It should be able to calculate all the required stuff since
it knows from the rate control algorithm what's happening there. That
might leave Intel out a bit, but I'm starting to think that we want to
special-case them a bit anyway.
johannes
On Mon, 2008-10-20 at 01:55 -0700, Luis R. Rodriguez wrote:
> On Mon, Oct 20, 2008 at 1:47 AM, Sujith <[email protected]> wrote:
> > Johannes Berg wrote:
> > > On Mon, 2008-10-20 at 01:35 -0700, Luis R. Rodriguez wrote:
> > >
> > > > >> + * @is_part_ampdu: set to indicate that the wireless core should should
> > > > >> + * treat this frame as part of an AMPDU
> > > > >
> > > > > I thought we said we could keep the flag instead of moving to the skb
> > > > > here?
> > > >
> > > > That was just because the changes required to add this was too much
> > > > for 2.6.27 but since we don't care about this fix in 2.6.27 we can do
> > > > whatever is needed to fix this properly.
> > >
> > > Right, but adding random stuff into skb isn't really "properly" imho.
> > > And we don't really need this afaict.
> > >
> >
> > Sorry, just saw that Tomas had fixed it by setting the flag in ieee80211_tx_h_sequence().
> > Will fix that.
>
> No that was a hack too, this was to minimize code changes for 2.6.27 as well.
Well the fact that it's in tx_h_sequence is a bit weird, but we should
be able to get around not using an extra bit in skb I think. Especially
for this since it's only really between the driver and mac80211, and
after a requeue the ampdu status might actually change.
johannes
On Thu, 2008-10-23 at 15:01 +0530, Sujith wrote:
> Johannes Berg wrote:
> > It seems that should be a rate control decision? Possibly taking into
> > account more than just always doing aggregation sessions. Then again, I
> > suppose aggregation sessions are cheap. What about latency here?
> >
>
> Well, that is what Luis seems to think too, but our RC
> doesn't do much now, so we try to setup an aggregation session with any
> associated STA.
If that was done in the RC at least (could easily be moved I suppose)
and you cleaned up the RC, then surely nbd would play with porting
minstrel and making it aware of such things, which probably makes for a
better RC... And since you have to clean up the RC anyway :)
> > "maintain minimum HW queue depth"? In what way? You mean put in enough
> > frames?
>
> I was talking about this:
> (txq->axq_depth < ATH_AGGR_MIN_QDEPTH) in ath_tx_sched_aggr()@xmit.c
But what does this invariant mean? ATH_AGGR_MIN_QDEPTH is 2.
> > > Well, I really don't know how this would affect performance, but
> > > I think this _might_ be a better model.
> >
> > Where would you see it have a noticeable effect on performance?
>
> How would mac80211 buffer frames ?
Well I suppose it would just put them on a list in the sta_info struct.
> Would it wait for enough sub-frames
> to fill an A-MPDU ?
It should, no? And if for some reason that doesn't happen fast enough it
can send them out anyway, it'll just be a shorter A-MPDU, no?
> When does it decide that buffered frames have to
> be pushed down to the driver ?
When the A-MPDU is full, subject to the TX and RX constraints, or when
too much time has passed maybe?
I really don't know. But I'd like to get to a model where we can
implement other drivers without having to keep track of all this like
ath9k does.
johannes
On Mon, Oct 20, 2008 at 1:57 AM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2008-10-20 at 01:55 -0700, Luis R. Rodriguez wrote:
>> On Mon, Oct 20, 2008 at 1:47 AM, Sujith <[email protected]> wrote:
>> > Johannes Berg wrote:
>> > > On Mon, 2008-10-20 at 01:35 -0700, Luis R. Rodriguez wrote:
>> > >
>> > > > >> + * @is_part_ampdu: set to indicate that the wireless core should should
>> > > > >> + * treat this frame as part of an AMPDU
>> > > > >
>> > > > > I thought we said we could keep the flag instead of moving to the skb
>> > > > > here?
>> > > >
>> > > > That was just because the changes required to add this was too much
>> > > > for 2.6.27 but since we don't care about this fix in 2.6.27 we can do
>> > > > whatever is needed to fix this properly.
>> > >
>> > > Right, but adding random stuff into skb isn't really "properly" imho.
>> > > And we don't really need this afaict.
>> > >
>> >
>> > Sorry, just saw that Tomas had fixed it by setting the flag in ieee80211_tx_h_sequence().
>> > Will fix that.
>>
>> No that was a hack too, this was to minimize code changes for 2.6.27 as well.
>
> Well the fact that it's in tx_h_sequence is a bit weird, but we should
> be able to get around not using an extra bit in skb I think. Especially
> for this since it's only really between the driver and mac80211, and
> after a requeue the ampdu status might actually change.
Good point. I failed to see that. So in that case it should be as
sujith had it before my suggestion and just handle it as he had it in
the tx handlers. The problem though is that the RC algorithm should be
the one to determine if an skb is part of an ampdu for aggregation or
not IMHO. Actually this is the real question and hot topic.
When should we aggregate and should mac80211 decide that or be left to the RC.
Luis
On Mon, 2008-10-20 at 15:01 +0530, Sujith wrote:
> Johannes Berg wrote:
> > Can you change your quote string from " > " to "> "? Just asking, if you
> > can't/don't want to that's fine, it just always confuses me for a
> > second. I really should try to figure out if I can convince my mailer to
> > highlight " > " indented text as quote too.
>
> Sure, adding (setq vm-included-text-prefix "> ") in my .emacs did the trick. :)
Thanks! :)
> > > Well, currently ath9k maintains a buffer list for each tid.
> > > When mac80211 sends down a frame, if the recipient has an aggr. session going,
> > > it is appended to the tid's buffer list. Non-HT frames are sent out immediately.
> >
> > Incidentally, why the distinction between HT and non-HT frames?
>
> HT frames for aggregation enabled STAs are processed in the TX completion path, so
> they are buffered.
But couldn't non-HT frames be buffered similarly? Maybe I'm missing one
of the finer points of the 11n draft?
> > > On TX completion, we run through all the ACs, STAs and TIDs and send out pending
> > > frames as aggregates.
> >
> > So basically you can always have one frame or one aggregation
> > "pack" (for lack of a better word) queued to the hw?
> >
>
> I am not sure, I should check.
Doesn't really matter, I was just trying to make sense out of your
statement of processing it at completion time.
> Makes sense, ath9k currently has a similar set of functions ( pausing/resuming TIDs).
> How would the pending frames for each TID be managed ?
> Will the driver be able to pull them as and when needed or would mac80211
> push them down on checking if the TID is awake ?
I guess there's no clear answer here. How about "whichever you want"?
Though I think I prefer pushing them down as that makes the model easier
to understand. It probably also makes the Intel case easier to
implement.
johannes
On Wed, 2008-10-22 at 13:41 +0200, Tomas Winkler wrote:
> >> * mac80211 sends down a frame
> >> * Initiate an aggregation session for the <RA,TID> if one isn't already in progress.
> >
> > It seems that should be a rate control decision? Possibly taking into
> > account more than just always doing aggregation sessions. Then again, I
> > suppose aggregation sessions are cheap. What about latency here?
>
> Aggregation has it's overhead, both computing resources and on the
> air. Obvious that it has negative affect on latency so if the 'minimum
> HW queue depth' is not satisfied the packets
> are delayed till the queue is full enough.
That would be "do I have enough frames to aggregate them"?
> In iwl-agn-rs scale we keep track of current tpt to take decision
> about starting aggregation. The throughput watch can be viewed as
> different entity it was just convenient and conceptually fit to
> incorporate into rate scale.
Sure, that kinda makes sense. The RC algorithm is conceptually
responsible for making sure things go as fast as possible :)
> > Obviously. But why isn't this done in parallel? I mean, why not send out
> > the frame and do addba and don't aggregate until addba was successful,
> > that would mean no latency for those frames... addba could even time out
> > which would add a lot of latency, no?
>
> The negotiation is done on starting sequence number.
Ah, good point.
johannes
Johannes Berg wrote:
> > HT frames for aggregation enabled STAs are processed in the TX completion path, so
> > they are buffered.
>
> But couldn't non-HT frames be buffered similarly? Maybe I'm missing one
> of the finer points of the 11n draft?
Probably because 11n aggregation have more rigorous timing requirements
between frames.
> > Makes sense, ath9k currently has a similar set of functions ( pausing/resuming TIDs).
> > How would the pending frames for each TID be managed ?
> > Will the driver be able to pull them as and when needed or would mac80211
> > push them down on checking if the TID is awake ?
>
> I guess there's no clear answer here. How about "whichever you want"?
> Though I think I prefer pushing them down as that makes the model easier
> to understand. It probably also makes the Intel case easier to
> implement.
A way to pull down buffered frames for each TID (like ieee80211_get_buffered_bc() )
would be really useful for ath9k.
Sujith
Johannes Berg wrote:
> Can you change your quote string from " > " to "> "? Just asking, if you
> can't/don't want to that's fine, it just always confuses me for a
> second. I really should try to figure out if I can convince my mailer to
> highlight " > " indented text as quote too.
Sure, adding (setq vm-included-text-prefix "> ") in my .emacs did the trick. :)
> > Well, currently ath9k maintains a buffer list for each tid.
> > When mac80211 sends down a frame, if the recipient has an aggr. session going,
> > it is appended to the tid's buffer list. Non-HT frames are sent out immediately.
>
> Incidentally, why the distinction between HT and non-HT frames?
HT frames for aggregation enabled STAs are processed in the TX completion path, so
they are buffered.
> > On TX completion, we run through all the ACs, STAs and TIDs and send out pending
> > frames as aggregates.
>
> So basically you can always have one frame or one aggregation
> "pack" (for lack of a better word) queued to the hw?
>
I am not sure, I should check.
> > mac80211 can probably help by maintaining the TX state for each TID, maintain the
> > buffer list, etc. and provide appropriate mechanisms for drivers to obtain pending
> > frames as and when needed.
>
> Sure could. Intel's hw would probably like a way to say "stop this TID"
> and "wake this TID" which is what they tried to do using real queues.
> mac80211 could queue up the frames in per-station state and provide a
> functions like
>
> ieee80211_stop_aggr_stream(struct ieee80211_sta *sta, u8 tid);
> ieee80211_wake_aggr_stream(struct ieee80211_sta *sta, u8 tid);
>
> or something like that. Then those frames would flow into the right
> queue. We might need a multi-layer start/stop management but we already
> do, we've just managed to get away without it without tx stalls. Or they
> just haven't been reported yet.
>
Makes sense, ath9k currently has a similar set of functions ( pausing/resuming TIDs).
How would the pending frames for each TID be managed ?
Will the driver be able to pull them as and when needed or would mac80211
push them down on checking if the TID is awake ?
Sujith