Instead of stopping the entire AC queue when enabling aggregation
(which was only done for hardware with aggregation queues) buffer
the packets for each station, and release them to the pending skb
queue once aggregation is turned on successfully.
We get a little more code, but it becomes conceptually simpler and
we can remove the entire virtual queue mechanism from mac80211 in
a follow-up patch.
This changes how mac80211 behaves towards drivers that support
aggregation but have no hardware queues -- those drivers will now
not be handed packets while the aggregation session is being
established, but only after it has been fully established.
Signed-off-by: Johannes Berg <[email protected]>
---
include/net/mac80211.h | 4 +
net/mac80211/agg-tx.c | 136 ++++++++++++++++++++++++++-----------------
net/mac80211/ieee80211_i.h | 8 ++
net/mac80211/main.c | 2
net/mac80211/sta_info.c | 5 +
net/mac80211/sta_info.h | 2
net/mac80211/tx.c | 142 ++++++++++++++++++++++++++++++++++++---------
7 files changed, 221 insertions(+), 78 deletions(-)
--- wireless-testing.orig/net/mac80211/sta_info.h 2009-03-23 13:59:18.000000000 +0100
+++ wireless-testing/net/mac80211/sta_info.h 2009-03-23 14:04:30.000000000 +0100
@@ -73,11 +73,13 @@ enum ieee80211_sta_info_flags {
* struct tid_ampdu_tx - TID aggregation information (Tx).
*
* @addba_resp_timer: timer for peer's response to addba request
+ * @pending: pending frames queue -- use sta's spinlock to protect
* @ssn: Starting Sequence Number expected to be aggregated.
* @dialog_token: dialog token for aggregation session
*/
struct tid_ampdu_tx {
struct timer_list addba_resp_timer;
+ struct sk_buff_head pending;
u16 ssn;
u8 dialog_token;
};
--- wireless-testing.orig/include/net/mac80211.h 2009-03-23 14:03:46.000000000 +0100
+++ wireless-testing/include/net/mac80211.h 2009-03-23 14:04:30.000000000 +0100
@@ -248,6 +248,9 @@ struct ieee80211_bss_conf {
* @IEEE80211_TX_INTFL_RCALGO: mac80211 internal flag, do not test or
* set this flag in the driver; indicates that the rate control
* algorithm was used and should be notified of TX status
+ * @IEEE80211_TX_INTFL_NEED_TXPROCESSING: completely internal to mac80211,
+ * used to indicate that a pending frame requires TX processing before
+ * it can be sent out.
*/
enum mac80211_tx_control_flags {
IEEE80211_TX_CTL_REQ_TX_STATUS = BIT(0),
@@ -264,6 +267,7 @@ enum mac80211_tx_control_flags {
IEEE80211_TX_STAT_AMPDU_NO_BACK = BIT(11),
IEEE80211_TX_CTL_RATE_CTRL_PROBE = BIT(12),
IEEE80211_TX_INTFL_RCALGO = BIT(13),
+ IEEE80211_TX_INTFL_NEED_TXPROCESSING = BIT(14),
};
/**
--- wireless-testing.orig/net/mac80211/agg-tx.c 2009-03-23 14:03:46.000000000 +0100
+++ wireless-testing/net/mac80211/agg-tx.c 2009-03-23 14:04:30.000000000 +0100
@@ -132,16 +132,6 @@ static int ___ieee80211_stop_tx_ba_sessi
state = &sta->ampdu_mlme.tid_state_tx[tid];
if (local->hw.ampdu_queues) {
- if (initiator) {
- /*
- * Stop the AC queue to avoid issues where we send
- * unaggregated frames already before the delba.
- */
- ieee80211_stop_queue_by_reason(&local->hw,
- local->hw.queues + sta->tid_to_tx_q[tid],
- IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
- }
-
/*
* Pretend the driver woke the queue, just in case
* it disabled it before the session was stopped.
@@ -158,6 +148,10 @@ static int ___ieee80211_stop_tx_ba_sessi
/* HW shall not deny going back to legacy */
if (WARN_ON(ret)) {
*state = HT_AGG_STATE_OPERATIONAL;
+ /*
+ * We may have pending packets get stuck in this case...
+ * Not bothering with a workaround for now.
+ */
}
return ret;
@@ -226,13 +220,6 @@ int ieee80211_start_tx_ba_session(struct
ra, tid);
#endif /* CONFIG_MAC80211_HT_DEBUG */
- if (hw->ampdu_queues && ieee80211_ac_from_tid(tid) == 0) {
-#ifdef CONFIG_MAC80211_HT_DEBUG
- printk(KERN_DEBUG "rejecting on voice AC\n");
-#endif
- return -EINVAL;
- }
-
rcu_read_lock();
sta = sta_info_get(local, ra);
@@ -267,6 +254,7 @@ int ieee80211_start_tx_ba_session(struct
}
spin_lock_bh(&sta->lock);
+ spin_lock(&local->ampdu_lock);
sdata = sta->sdata;
@@ -308,21 +296,19 @@ int ieee80211_start_tx_ba_session(struct
ret = -ENOSPC;
goto err_unlock_sta;
}
-
- /*
- * If we successfully allocate the session, we can't have
- * anything going on on the queue this TID maps into, so
- * stop it for now. This is a "virtual" stop using the same
- * mechanism that drivers will use.
- *
- * XXX: queue up frames for this session in the sta_info
- * struct instead to avoid hitting all other STAs.
- */
- ieee80211_stop_queue_by_reason(
- &local->hw, hw->queues + qn,
- IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
}
+ /*
+ * While we're asking the driver about the aggregation,
+ * stop the AC queue so that we don't have to worry
+ * about frames that came in while we were doing that,
+ * which would require us to put them to the AC pending
+ * afterwards which just makes the code more complex.
+ */
+ ieee80211_stop_queue_by_reason(
+ &local->hw, ieee80211_ac_from_tid(tid),
+ IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
+
/* prepare A-MPDU MLME for Tx aggregation */
sta->ampdu_mlme.tid_tx[tid] =
kmalloc(sizeof(struct tid_ampdu_tx), GFP_ATOMIC);
@@ -336,6 +322,8 @@ int ieee80211_start_tx_ba_session(struct
goto err_return_queue;
}
+ skb_queue_head_init(&sta->ampdu_mlme.tid_tx[tid]->pending);
+
/* Tx timer */
sta->ampdu_mlme.tid_tx[tid]->addba_resp_timer.function =
sta_addba_resp_timer_expired;
@@ -362,6 +350,12 @@ int ieee80211_start_tx_ba_session(struct
}
sta->tid_to_tx_q[tid] = qn;
+ /* Driver vetoed or OKed, but we can take packets again now */
+ ieee80211_wake_queue_by_reason(
+ &local->hw, ieee80211_ac_from_tid(tid),
+ IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
+
+ spin_unlock(&local->ampdu_lock);
spin_unlock_bh(&sta->lock);
/* send an addBA request */
@@ -388,15 +382,16 @@ int ieee80211_start_tx_ba_session(struct
sta->ampdu_mlme.tid_tx[tid] = NULL;
err_return_queue:
if (qn >= 0) {
- /* We failed, so start queue again right away. */
- ieee80211_wake_queue_by_reason(hw, hw->queues + qn,
- IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
/* give queue back to pool */
spin_lock(&local->queue_stop_reason_lock);
local->ampdu_ac_queue[qn] = -1;
spin_unlock(&local->queue_stop_reason_lock);
}
+ ieee80211_wake_queue_by_reason(
+ &local->hw, ieee80211_ac_from_tid(tid),
+ IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
err_unlock_sta:
+ spin_unlock(&local->ampdu_lock);
spin_unlock_bh(&sta->lock);
unlock:
rcu_read_unlock();
@@ -404,6 +399,45 @@ int ieee80211_start_tx_ba_session(struct
}
EXPORT_SYMBOL(ieee80211_start_tx_ba_session);
+/*
+ * splice packets from the STA's pending to the local pending,
+ * requires a call to ieee80211_agg_splice_finish and holding
+ * local->ampdu_lock across both calls.
+ */
+static void ieee80211_agg_splice_packets(struct ieee80211_local *local,
+ struct sta_info *sta, u16 tid)
+{
+ unsigned long flags;
+ u16 queue = ieee80211_ac_from_tid(tid);
+
+ ieee80211_stop_queue_by_reason(
+ &local->hw, queue,
+ IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
+
+ if (!skb_queue_empty(&sta->ampdu_mlme.tid_tx[tid]->pending)) {
+ spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
+ /* mark queue as pending, it is stopped already */
+ __set_bit(IEEE80211_QUEUE_STOP_REASON_PENDING,
+ &local->queue_stop_reasons[queue]);
+ /* copy over remaining packets */
+ skb_queue_splice_tail_init(
+ &sta->ampdu_mlme.tid_tx[tid]->pending,
+ &local->pending[queue]);
+ spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
+ }
+}
+
+static void ieee80211_agg_splice_finish(struct ieee80211_local *local,
+ struct sta_info *sta, u16 tid)
+{
+ u16 queue = ieee80211_ac_from_tid(tid);
+
+ ieee80211_wake_queue_by_reason(
+ &local->hw, queue,
+ IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
+}
+
+/* caller must hold sta->lock */
static void ieee80211_agg_tx_operational(struct ieee80211_local *local,
struct sta_info *sta, u16 tid)
{
@@ -411,15 +445,16 @@ static void ieee80211_agg_tx_operational
printk(KERN_DEBUG "Aggregation is on for tid %d \n", tid);
#endif
- if (local->hw.ampdu_queues) {
- /*
- * Wake up the A-MPDU queue, we stopped it earlier,
- * this will in turn wake the entire AC.
- */
- ieee80211_wake_queue_by_reason(&local->hw,
- local->hw.queues + sta->tid_to_tx_q[tid],
- IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
- }
+ spin_lock(&local->ampdu_lock);
+ ieee80211_agg_splice_packets(local, sta, tid);
+ /*
+ * NB: we rely on sta->lock being taken in the TX
+ * processing here when adding to the pending queue,
+ * otherwise we could only change the state of the
+ * session to OPERATIONAL _here_.
+ */
+ ieee80211_agg_splice_finish(local, sta, tid);
+ spin_unlock(&local->ampdu_lock);
local->ops->ampdu_action(&local->hw, IEEE80211_AMPDU_TX_OPERATIONAL,
&sta->sta, tid, NULL);
@@ -602,22 +637,19 @@ void ieee80211_stop_tx_ba_cb(struct ieee
WLAN_BACK_INITIATOR, WLAN_REASON_QSTA_NOT_USE);
spin_lock_bh(&sta->lock);
+ spin_lock(&local->ampdu_lock);
- if (*state & HT_AGG_STATE_INITIATOR_MSK &&
- hw->ampdu_queues) {
- /*
- * Wake up this queue, we stopped it earlier,
- * this will in turn wake the entire AC.
- */
- ieee80211_wake_queue_by_reason(hw,
- hw->queues + sta->tid_to_tx_q[tid],
- IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
- }
+ ieee80211_agg_splice_packets(local, sta, tid);
*state = HT_AGG_STATE_IDLE;
+ /* from now on packets are no longer put onto sta->pending */
sta->ampdu_mlme.addba_req_num[tid] = 0;
kfree(sta->ampdu_mlme.tid_tx[tid]);
sta->ampdu_mlme.tid_tx[tid] = NULL;
+
+ ieee80211_agg_splice_finish(local, sta, tid);
+
+ spin_unlock(&local->ampdu_lock);
spin_unlock_bh(&sta->lock);
rcu_read_unlock();
--- wireless-testing.orig/net/mac80211/tx.c 2009-03-23 14:03:47.000000000 +0100
+++ wireless-testing/net/mac80211/tx.c 2009-03-23 14:04:30.000000000 +0100
@@ -984,9 +984,9 @@ __ieee80211_tx_prepare(struct ieee80211_
struct ieee80211_hdr *hdr;
struct ieee80211_sub_if_data *sdata;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
-
int hdrlen, tid;
u8 *qc, *state;
+ bool queued = false;
memset(tx, 0, sizeof(*tx));
tx->skb = skb;
@@ -1013,20 +1013,53 @@ __ieee80211_tx_prepare(struct ieee80211_
*/
}
+ /*
+ * If this flag is set to true anywhere, and we get here,
+ * we are doing the needed processing, so remove the flag
+ * now.
+ */
+ info->flags &= ~IEEE80211_TX_INTFL_NEED_TXPROCESSING;
+
hdr = (struct ieee80211_hdr *) skb->data;
tx->sta = sta_info_get(local, hdr->addr1);
- if (tx->sta && ieee80211_is_data_qos(hdr->frame_control)) {
+ if (tx->sta && ieee80211_is_data_qos(hdr->frame_control) &&
+ (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION)) {
unsigned long flags;
+ struct tid_ampdu_tx *tid_tx;
+
qc = ieee80211_get_qos_ctl(hdr);
tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
spin_lock_irqsave(&tx->sta->lock, flags);
+ /*
+ * XXX: This spinlock could be fairly expensive, but see the
+ * comment in agg-tx.c:ieee80211_agg_tx_operational().
+ * One way to solve this would be to do something RCU-like
+ * for managing the tid_tx struct and using atomic bitops
+ * for the actual state -- by introducing an actual
+ * 'operational' bit that would be possible. It would
+ * require changing ieee80211_agg_tx_operational() to
+ * set that bit, and changing the way tid_tx is managed
+ * everywhere, including races between that bit and
+ * tid_tx going away (tid_tx being added can be easily
+ * committed to memory before the 'operational' bit).
+ */
+ tid_tx = tx->sta->ampdu_mlme.tid_tx[tid];
state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
- if (*state == HT_AGG_STATE_OPERATIONAL)
+ if (*state == HT_AGG_STATE_OPERATIONAL) {
info->flags |= IEEE80211_TX_CTL_AMPDU;
+ } else if (*state != HT_AGG_STATE_IDLE) {
+ /* in progress */
+ queued = true;
+ info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
+ __skb_queue_tail(&tid_tx->pending, skb);
+ }
spin_unlock_irqrestore(&tx->sta->lock, flags);
+
+ if (unlikely(queued))
+ return TX_QUEUED;
}
if (is_multicast_ether_addr(hdr->addr1)) {
@@ -1077,7 +1110,14 @@ static int ieee80211_tx_prepare(struct i
}
if (unlikely(!dev))
return -ENODEV;
- /* initialises tx with control */
+ /*
+ * initialises tx with control
+ *
+ * return value is safe to ignore here because this function
+ * can only be invoked for multicast frames
+ *
+ * XXX: clean up
+ */
__ieee80211_tx_prepare(tx, skb, dev);
dev_put(dev);
return 0;
@@ -1188,7 +1228,8 @@ static int invoke_tx_handlers(struct iee
return 0;
}
-static int ieee80211_tx(struct net_device *dev, struct sk_buff *skb)
+static void ieee80211_tx(struct net_device *dev, struct sk_buff *skb,
+ bool txpending)
{
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
struct sta_info *sta;
@@ -1202,11 +1243,11 @@ static int ieee80211_tx(struct net_devic
queue = skb_get_queue_mapping(skb);
- WARN_ON(!skb_queue_empty(&local->pending[queue]));
+ WARN_ON(!txpending && !skb_queue_empty(&local->pending[queue]));
if (unlikely(skb->len < 10)) {
dev_kfree_skb(skb);
- return 0;
+ return;
}
rcu_read_lock();
@@ -1214,10 +1255,13 @@ static int ieee80211_tx(struct net_devic
/* initialises tx */
res_prepare = __ieee80211_tx_prepare(&tx, skb, dev);
- if (res_prepare == TX_DROP) {
+ if (unlikely(res_prepare == TX_DROP)) {
dev_kfree_skb(skb);
rcu_read_unlock();
- return 0;
+ return;
+ } else if (unlikely(res_prepare == TX_QUEUED)) {
+ rcu_read_unlock();
+ return;
}
sta = tx.sta;
@@ -1251,7 +1295,12 @@ static int ieee80211_tx(struct net_devic
do {
next = skb->next;
skb->next = NULL;
- skb_queue_tail(&local->pending[queue], skb);
+ if (unlikely(txpending))
+ skb_queue_head(&local->pending[queue],
+ skb);
+ else
+ skb_queue_tail(&local->pending[queue],
+ skb);
} while ((skb = next));
/*
@@ -1276,7 +1325,7 @@ static int ieee80211_tx(struct net_devic
}
out:
rcu_read_unlock();
- return 0;
+ return;
drop:
rcu_read_unlock();
@@ -1287,7 +1336,6 @@ static int ieee80211_tx(struct net_devic
dev_kfree_skb(skb);
skb = next;
}
- return 0;
}
/* device xmit handlers */
@@ -1346,7 +1394,6 @@ int ieee80211_master_start_xmit(struct s
FOUND_SDATA,
UNKNOWN_ADDRESS,
} monitor_iface = NOT_MONITOR;
- int ret;
if (skb->iif)
odev = dev_get_by_index(&init_net, skb->iif);
@@ -1360,7 +1407,7 @@ int ieee80211_master_start_xmit(struct s
"originating device\n", dev->name);
#endif
dev_kfree_skb(skb);
- return 0;
+ return NETDEV_TX_OK;
}
if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
@@ -1389,7 +1436,7 @@ int ieee80211_master_start_xmit(struct s
else
if (mesh_nexthop_lookup(skb, osdata)) {
dev_put(odev);
- return 0;
+ return NETDEV_TX_OK;
}
if (memcmp(odev->dev_addr, hdr->addr4, ETH_ALEN) != 0)
IEEE80211_IFSTA_MESH_CTR_INC(&osdata->u.mesh,
@@ -1451,7 +1498,7 @@ int ieee80211_master_start_xmit(struct s
if (ieee80211_skb_resize(osdata->local, skb, headroom, may_encrypt)) {
dev_kfree_skb(skb);
dev_put(odev);
- return 0;
+ return NETDEV_TX_OK;
}
if (osdata->vif.type == NL80211_IFTYPE_AP_VLAN)
@@ -1460,10 +1507,11 @@ int ieee80211_master_start_xmit(struct s
u.ap);
if (likely(monitor_iface != UNKNOWN_ADDRESS))
info->control.vif = &osdata->vif;
- ret = ieee80211_tx(odev, skb);
+
+ ieee80211_tx(odev, skb, false);
dev_put(odev);
- return ret;
+ return NETDEV_TX_OK;
}
int ieee80211_monitor_start_xmit(struct sk_buff *skb,
@@ -1827,6 +1875,54 @@ void ieee80211_clear_tx_pending(struct i
skb_queue_purge(&local->pending[i]);
}
+static bool ieee80211_tx_pending_skb(struct ieee80211_local *local,
+ struct sk_buff *skb)
+{
+ struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+ struct ieee80211_sub_if_data *sdata;
+ struct sta_info *sta;
+ struct ieee80211_hdr *hdr;
+ struct net_device *dev;
+ int ret;
+ bool result = true;
+
+ /* does interface still exist? */
+ dev = dev_get_by_index(&init_net, skb->iif);
+ if (!dev) {
+ dev_kfree_skb(skb);
+ return true;
+ }
+
+ /* validate info->control.vif against skb->iif */
+ sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+ if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
+ sdata = container_of(sdata->bss,
+ struct ieee80211_sub_if_data,
+ u.ap);
+
+ if (unlikely(info->control.vif && info->control.vif != &sdata->vif)) {
+ dev_kfree_skb(skb);
+ result = true;
+ goto out;
+ }
+
+ if (info->flags & IEEE80211_TX_INTFL_NEED_TXPROCESSING) {
+ ieee80211_tx(dev, skb, true);
+ } else {
+ hdr = (struct ieee80211_hdr *)skb->data;
+ sta = sta_info_get(local, hdr->addr1);
+
+ ret = __ieee80211_tx(local, &skb, sta);
+ if (ret != IEEE80211_TX_OK)
+ result = false;
+ }
+
+ out:
+ dev_put(dev);
+
+ return result;
+}
+
/*
* Transmit all pending packets. Called from tasklet, locks master device
* TX lock so that no new packets can come in.
@@ -1835,9 +1931,8 @@ void ieee80211_tx_pending(unsigned long
{
struct ieee80211_local *local = (struct ieee80211_local *)data;
struct net_device *dev = local->mdev;
- struct ieee80211_hdr *hdr;
unsigned long flags;
- int i, ret;
+ int i;
bool next;
rcu_read_lock();
@@ -1868,13 +1963,8 @@ void ieee80211_tx_pending(unsigned long
while (!skb_queue_empty(&local->pending[i])) {
struct sk_buff *skb = skb_dequeue(&local->pending[i]);
- struct sta_info *sta;
-
- hdr = (struct ieee80211_hdr *)skb->data;
- sta = sta_info_get(local, hdr->addr1);
- ret = __ieee80211_tx(local, &skb, sta);
- if (ret != IEEE80211_TX_OK) {
+ if (!ieee80211_tx_pending_skb(local, skb)) {
skb_queue_head(&local->pending[i], skb);
break;
}
--- wireless-testing.orig/net/mac80211/sta_info.c 2009-03-23 13:59:18.000000000 +0100
+++ wireless-testing/net/mac80211/sta_info.c 2009-03-23 14:04:30.000000000 +0100
@@ -239,6 +239,11 @@ void sta_info_destroy(struct sta_info *s
tid_tx = sta->ampdu_mlme.tid_tx[i];
if (tid_tx) {
del_timer_sync(&tid_tx->addba_resp_timer);
+ /*
+ * STA removed while aggregation session being
+ * started? Bit odd, but purge frames anyway.
+ */
+ skb_queue_purge(&tid_tx->pending);
kfree(tid_tx);
}
}
--- wireless-testing.orig/net/mac80211/ieee80211_i.h 2009-03-23 14:03:45.000000000 +0100
+++ wireless-testing/net/mac80211/ieee80211_i.h 2009-03-23 14:04:30.000000000 +0100
@@ -637,6 +637,14 @@ struct ieee80211_local {
struct sk_buff_head pending[IEEE80211_MAX_QUEUES];
struct tasklet_struct tx_pending_tasklet;
+ /*
+ * This lock is used to prevent concurrent A-MPDU
+ * session start/stop processing, this thus also
+ * synchronises the ->ampdu_action() callback to
+ * drivers and limits it to one at a time.
+ */
+ spinlock_t ampdu_lock;
+
/* number of interfaces with corresponding IFF_ flags */
atomic_t iff_allmultis, iff_promiscs;
--- wireless-testing.orig/net/mac80211/main.c 2009-03-23 14:03:45.000000000 +0100
+++ wireless-testing/net/mac80211/main.c 2009-03-23 14:04:30.000000000 +0100
@@ -795,6 +795,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(
skb_queue_head_init(&local->skb_queue);
skb_queue_head_init(&local->skb_queue_unreliable);
+ spin_lock_init(&local->ampdu_lock);
+
return local_to_hw(local);
}
EXPORT_SYMBOL(ieee80211_alloc_hw);
--
On Sat, 2009-03-28 at 13:26 -0700, Luis R. Rodriguez wrote:
> >> > I think it also makes more _sense_ to not ask the driver to handle these
> >> > frames, because we won't set the AMPDU flag correctly if the session
> >> > start is declined.
> >>
> >> Hm, wouldn't we send them as normal frames if its declined here eventually?
> >
> > Yes, we take them off the sta->pending, put them through TX processing
> > and send them as normal frames.
>
> OK so you're not suggesting we change this more, just stating that
> this makes sense.
Yes; I don't see why we should change it. Otherwise the driver needs to
ignore the TXCTL_AMPDU flag and do its own processing, would would seem
odd. By the way ath9k does it, it probably does it automatically though,
but I don't see why we should require it.
> >> OK I see that but I am not sure of what's done to the skb in the old
> >> case if the session is note complete yet, nor now if the session gets
> >> declined. I can check later, right now I'm feeling lazy.
> >
> > Before my patch the skb is still passed to the driver, which would need
> > to be able to handle it (it == session being stopped). I'm not entirely
> > sure ath9k handles it properly but I suspect it does.
>
> Not sure, this begs the question when you were seeing the received
> addba response come up first. SInce both iwlagn and ath9k use the irq
> safe aggregation cb I was suspecting this would happen in general on
> UP boxen and probably even less likely to happen on ath9k as we don't
> have to talk to the firmware during the ampdu start action.
No, it can only happen when the remote station is faster to reply than
the driver.
> >> > Ok that might make some sense, though as a parameter you can easily see
> >> > where it's coming from, I think :) I agree that the entire code is a
> >> > little brain twister...
> >>
> >> Yeah.. I had to re-read aggregation code again to get the hang of it.
> >> Anything we can do to make
> >> it easier for people to pick would be nice. I think I'll go patch up
> >> the aggr-tx kdoc too now, unless we
> >> plan on nuking some stuff soon again. I suspect the next big change
> >> will be to move software based
> >> aggregation queue handling and mapping to ACs within mac80211 for that
> >> type of hardware which
> >> we'll need for ar9170, ralink stuff, broadcom (if that ever happens)
> >> and our next generation 11n USB
> >> driver.
> >
> > Right -- but I don't expect to be working on it any time soon myself.
>
> I didn't expect you to. We may have the need first unless chr gets to
> ar9170 stuff first.
Ok, makes sense.
johannes
On Sat, Mar 28, 2009 at 12:52 PM, Johannes Berg
<[email protected]> wrote:
> On Sat, 2009-03-28 at 12:18 -0700, Luis R. Rodriguez wrote:
>
>> >> That's fine from a logical point of view as compromise here as ou=
r
>> >> ampdu start should be relatively quick. Now while I can say this
>> >> from a logical point of view this patch obviously needed more
>> >> testing but lets see how it holds up and I'm glad you're the one
>> >> who wrote it. I'm confident there won't be any issues but that
>> >> is something I cannot gaurantee just based on the review. We now
>> >> need to go out and tests this. All other patches previous to this
>> >> make 100% perfect sense.
>> >
>> > :)
>> > I think it also makes more _sense_ to not ask the driver to handle=
these
>> > frames, because we won't set the AMPDU flag correctly if the sessi=
on
>> > start is declined.
>>
>> Hm, wouldn't we send them as normal frames if its declined here even=
tually?
>
> Yes, we take them off the sta->pending, put them through TX processin=
g
> and send them as normal frames.
OK so you're not suggesting we change this more, just stating that
this makes sense.
>> >> This piece:
>> >>
>> >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } else if (*state !=3D HT_=
AGG_STATE_IDLE) {
>> >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
/* in progress */
>> >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
queued =3D true;
>> >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
info->flags |=3D IEEE80211_TX_INTFL_NEED_TXPROCESSING;
>> >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
__skb_queue_tail(&tid_tx->pending, skb);
>> >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> >>
>> >> Can probably be ported to stable too, to just TX_DROP.
>> >
>> > No, why? We're handing the frames to the driver. It wants to handl=
e
>> > those frames before agg session is started correctly. That's what =
I was
>> > referring to before with this making more sense now.
>>
>> OK I see that but I am not sure of what's done to the skb in the old
>> case if the session is note complete yet, nor now if the session get=
s
>> declined. I can check later, right now I'm feeling lazy.
>
> Before my patch the skb is still passed to the driver, which would ne=
ed
> to be able to handle it (it =3D=3D session being stopped). I'm not en=
tirely
> sure ath9k handles it properly but I suspect it does.
Not sure, this begs the question when you were seeing the received
addba response come up first. SInce both iwlagn and ath9k use the irq
safe aggregation cb I was suspecting this would happen in general on
UP boxen and probably even less likely to happen on ath9k as we don't
have to talk to the firmware during the ampdu start action.
>> > Ok that might make some sense, though as a parameter you can easil=
y see
>> > where it's coming from, I think :) I agree that the entire code is=
a
>> > little brain twister...
>>
>> Yeah.. I had to re-read aggregation code again to get the hang of it=
=2E
>> Anything we can do to make
>> it easier for people to pick would be nice. I think I'll go patch up
>> the aggr-tx kdoc too now, unless we
>> plan on nuking some stuff soon again. I suspect the next big change
>> will be to move software based
>> aggregation queue handling and mapping to ACs within mac80211 for th=
at
>> type of hardware which
>> we'll need for ar9170, ralink stuff, broadcom (if that ever happens)
>> and our next generation 11n USB
>> driver.
>
> Right -- but I don't expect to be working on it any time soon myself.
I didn't expect you to. We may have the need first unless chr gets to
ar9170 stuff first.
Luis
On Sat, Mar 28, 2009 at 2:17 PM, Johannes Berg
<[email protected]> wrote:
> On Sat, 2009-03-28 at 14:06 -0700, Luis R. Rodriguez wrote:
>
>> My point was that if you have the tasklet handled by a separate CPU
>> the DRV flag would most likely be set by the time the addba response
>> comes back. On a UP box though you have no option but to wait for wait
>> for it to be done after we send off the addba request so then it is a
>> race between the scheduler to schedule the tasklet and the remote
>> station's speed + the IRQ processing of the received response.
>
> Ah, I see what you mean. But this goes off the same tasklet :) so ath9k
> doesn't have a problem in any case.
:D
>> If we would somehow be able to ensure pending BHs complete before we
>> process the addba response this wouldn't be needed in both places.
>
> No, we can't ensure that -- the driver might very well take longer and
> we don't have a way to only process the received frame later.
Well as you pointed out we already do since its on the same tasklet --
the delay will happen within the ampdu start action in the driver, not
the irqsafe callback itself.
>> Anyway I'm also stating that its likely that the ampdu start action is
>> slower with iwlagn as interaction with the firmware is required so I
>> would suspect one can see this behavior more likely on UP iwlagn
>> boxen.
>>
>> Where did you see it?
>
> I didn't actually see it :) But iwlagn can possibly flush the queue
> before calling back to the cb.
Heh, flush what queue? A virtual ampdu queue thing? Or something else,
I'm trying to understand exactly where this would happen, I don't
think I get the picture yet.
Luis
On Mon, Mar 23, 2009 at 05:28:41PM +0100, Johannes Berg wrote:
> Instead of stopping the entire AC queue when enabling aggregation
> (which was only done for hardware with aggregation queues) buffer
> the packets for each station, and release them to the pending skb
> queue once aggregation is turned on successfully.
Thank you for the nice compromised solution.
> We get a little more code, but it becomes conceptually simpler and
> we can remove the entire virtual queue mechanism from mac80211 in
> a follow-up patch.
>
> This changes how mac80211 behaves towards drivers that support
> aggregation but have no hardware queues -- those drivers will now
> not be handed packets while the aggregation session is being
> established, but only after it has been fully established.
That's fine from a logical point of view as compromise here as our
ampdu start should be relatively quick. Now while I can say this
from a logical point of view this patch obviously needed more
testing but lets see how it holds up and I'm glad you're the one
who wrote it. I'm confident there won't be any issues but that
is something I cannot gaurantee just based on the review. We now
need to go out and tests this. All other patches previous to this
make 100% perfect sense.
I'm particularly interested in seeing results with AP support.
Did you get to test that with ath9k by any chance?
Hauke -- just a heads up since you're quick with this now :) :
skb_queue_splice_tail_init() needs some backporting to 2.6.27. I'd do
it but I'm beat and calling it a day now.
More comments below. I've tried to remove all the hunks I didn't have
comments on.
> @@ -308,21 +296,19 @@ int ieee80211_start_tx_ba_session(struct
> ret = -ENOSPC;
> goto err_unlock_sta;
> }
> -
> - /*
> - * If we successfully allocate the session, we can't have
> - * anything going on on the queue this TID maps into, so
> - * stop it for now. This is a "virtual" stop using the same
> - * mechanism that drivers will use.
> - *
> - * XXX: queue up frames for this session in the sta_info
> - * struct instead to avoid hitting all other STAs.
> - */
> - ieee80211_stop_queue_by_reason(
> - &local->hw, hw->queues + qn,
> - IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
> }
>
> + /*
> + * While we're asking the driver about the aggregation,
> + * stop the AC queue so that we don't have to worry
> + * about frames that came in while we were doing that,
> + * which would require us to put them to the AC pending
> + * afterwards which just makes the code more complex.
> + */
> + ieee80211_stop_queue_by_reason(
> + &local->hw, ieee80211_ac_from_tid(tid),
> + IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
> +
ath9k's ampdu start is pretty quick anyway, so this
won't be for long.
> @@ -404,6 +399,45 @@ int ieee80211_start_tx_ba_session(struct
> }
> EXPORT_SYMBOL(ieee80211_start_tx_ba_session);
>
> +/*
> + * splice packets from the STA's pending to the local pending,
> + * requires a call to ieee80211_agg_splice_finish and holding
> + * local->ampdu_lock across both calls.
> + */
> +static void ieee80211_agg_splice_packets(struct ieee80211_local *local,
> + struct sta_info *sta, u16 tid)
> +{
> + unsigned long flags;
> + u16 queue = ieee80211_ac_from_tid(tid);
> +
> + ieee80211_stop_queue_by_reason(
> + &local->hw, queue,
> + IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
No point in stopping the queue if the STA's tid queue is empty.
> @@ -602,22 +637,19 @@ void ieee80211_stop_tx_ba_cb(struct ieee
> WLAN_BACK_INITIATOR, WLAN_REASON_QSTA_NOT_USE);
>
> spin_lock_bh(&sta->lock);
> + spin_lock(&local->ampdu_lock);
>
> - if (*state & HT_AGG_STATE_INITIATOR_MSK &&
> - hw->ampdu_queues) {
> - /*
> - * Wake up this queue, we stopped it earlier,
> - * this will in turn wake the entire AC.
> - */
> - ieee80211_wake_queue_by_reason(hw,
> - hw->queues + sta->tid_to_tx_q[tid],
> - IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
> - }
> + ieee80211_agg_splice_packets(local, sta, tid);
Is it possible for ieee80211_stop_tx_ba_cb() to be called while
state of the tid is not yet operational? from what I can tell
that's the only case when we would have added skbs to the STA's tid
pending queue.
>
> *state = HT_AGG_STATE_IDLE;
> + /* from now on packets are no longer put onto sta->pending */
I think it would help to refer to the pendign queue consitantly instead
as something like "STA's TID pending queue" as that is what it is. We also
now have the local->pending queue. STA's pending queue seems to imply
its also used for non-aggregation sessions.
See -- if the state is not operation nothing would have gone
been put into the STA's tid pending queue. Might also be worth
mentioning that in the comment.
> @@ -1013,20 +1013,53 @@ __ieee80211_tx_prepare(struct ieee80211_
> */
> }
>
> + /*
> + * If this flag is set to true anywhere, and we get here,
> + * we are doing the needed processing, so remove the flag
> + * now.
> + */
> + info->flags &= ~IEEE80211_TX_INTFL_NEED_TXPROCESSING;
> +
> hdr = (struct ieee80211_hdr *) skb->data;
>
> tx->sta = sta_info_get(local, hdr->addr1);
>
> - if (tx->sta && ieee80211_is_data_qos(hdr->frame_control)) {
> + if (tx->sta && ieee80211_is_data_qos(hdr->frame_control) &&
> + (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION)) {
Hm, why were we not crashing here before? I figure we would have
with something like this:
state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
That it itself should be separate patch, but too late now. Anyway
the new added check for IEEE80211_HW_AMPDU_AGGREGATION should go
to stable.
> unsigned long flags;
> + struct tid_ampdu_tx *tid_tx;
> +
> qc = ieee80211_get_qos_ctl(hdr);
> tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
>
> spin_lock_irqsave(&tx->sta->lock, flags);
> + /*
> + * XXX: This spinlock could be fairly expensive, but see the
> + * comment in agg-tx.c:ieee80211_agg_tx_operational().
> + * One way to solve this would be to do something RCU-like
> + * for managing the tid_tx struct and using atomic bitops
> + * for the actual state -- by introducing an actual
> + * 'operational' bit that would be possible. It would
> + * require changing ieee80211_agg_tx_operational() to
> + * set that bit, and changing the way tid_tx is managed
> + * everywhere, including races between that bit and
> + * tid_tx going away (tid_tx being added can be easily
> + * committed to memory before the 'operational' bit).
> + */
> + tid_tx = tx->sta->ampdu_mlme.tid_tx[tid];
> state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
> - if (*state == HT_AGG_STATE_OPERATIONAL)
> + if (*state == HT_AGG_STATE_OPERATIONAL) {
> info->flags |= IEEE80211_TX_CTL_AMPDU;
See, when its operational we don't add stuff to the STA's TID pending
queue.
This piece:
> + } else if (*state != HT_AGG_STATE_IDLE) {
> + /* in progress */
> + queued = true;
> + info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
> + __skb_queue_tail(&tid_tx->pending, skb);
> + }
Can probably be ported to stable too, to just TX_DROP.
> @@ -1251,7 +1295,12 @@ static int ieee80211_tx(struct net_devic
> do {
> next = skb->next;
> skb->next = NULL;
> - skb_queue_tail(&local->pending[queue], skb);
> + if (unlikely(txpending))
> + skb_queue_head(&local->pending[queue],
> + skb);
> + else
> + skb_queue_tail(&local->pending[queue],
> + skb);
For someone who hasn't read all the pending queue code stuff the above would be
a little brain twister. It might be good to leave a comment explaining why
txpendign would be set and why we add it to the head (i.e. because the "skb" sent
at a previous time was put into a pending queue). Maybe even rename txpending to
something like skb_was_pending.
> @@ -1360,7 +1407,7 @@ int ieee80211_master_start_xmit(struct s
> "originating device\n", dev->name);
> #endif
> dev_kfree_skb(skb);
> - return 0;
> + return NETDEV_TX_OK;
All these NETDEV_TX_OK could've gone into a separate patch.
Luis
On Sat, Mar 28, 2009 at 1:42 PM, Johannes Berg
<[email protected]> wrote:
> On Sat, 2009-03-28 at 13:26 -0700, Luis R. Rodriguez wrote:
>> >> OK I see that but I am not sure of what's done to the skb in the old
>> >> case if the session is note complete yet, nor now if the session gets
>> >> declined. I can check later, right now I'm feeling lazy.
>> >
>> > Before my patch the skb is still passed to the driver, which would need
>> > to be able to handle it (it == session being stopped). I'm not entirely
>> > sure ath9k handles it properly but I suspect it does.
>>
>> Not sure, this begs the question when you were seeing the received
>> addba response come up first. SInce both iwlagn and ath9k use the irq
>> safe aggregation cb I was suspecting this would happen in general on
>> UP boxen and probably even less likely to happen on ath9k as we don't
>> have to talk to the firmware during the ampdu start action.
>
> No, it can only happen when the remote station is faster to reply than
> the driver.
My point was that if you have the tasklet handled by a separate CPU
the DRV flag would most likely be set by the time the addba response
comes back. On a UP box though you have no option but to wait for wait
for it to be done after we send off the addba request so then it is a
race between the scheduler to schedule the tasklet and the remote
station's speed + the IRQ processing of the received response.
If we would somehow be able to ensure pending BHs complete before we
process the addba response this wouldn't be needed in both places.
Anyway I'm also stating that its likely that the ampdu start action is
slower with iwlagn as interaction with the firmware is required so I
would suspect one can see this behavior more likely on UP iwlagn
boxen.
Where did you see it?
Luis
On Sat, 2009-03-28 at 00:55 -0400, Luis R. Rodriguez wrote:
> > This changes how mac80211 behaves towards drivers that support
> > aggregation but have no hardware queues -- those drivers will now
> > not be handed packets while the aggregation session is being
> > established, but only after it has been fully established.
>
> That's fine from a logical point of view as compromise here as our
> ampdu start should be relatively quick. Now while I can say this
> from a logical point of view this patch obviously needed more
> testing but lets see how it holds up and I'm glad you're the one
> who wrote it. I'm confident there won't be any issues but that
> is something I cannot gaurantee just based on the review. We now
> need to go out and tests this. All other patches previous to this
> make 100% perfect sense.
:)
I think it also makes more _sense_ to not ask the driver to handle these
frames, because we won't set the AMPDU flag correctly if the session
start is declined.
> > + /*
> > + * While we're asking the driver about the aggregation,
> > + * stop the AC queue so that we don't have to worry
> > + * about frames that came in while we were doing that,
> > + * which would require us to put them to the AC pending
> > + * afterwards which just makes the code more complex.
> > + */
> > + ieee80211_stop_queue_by_reason(
> > + &local->hw, ieee80211_ac_from_tid(tid),
> > + IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
> > +
>
> ath9k's ampdu start is pretty quick anyway, so this
> won't be for long.
Yup.
> > +/*
> > + * splice packets from the STA's pending to the local pending,
> > + * requires a call to ieee80211_agg_splice_finish and holding
> > + * local->ampdu_lock across both calls.
> > + */
> > +static void ieee80211_agg_splice_packets(struct ieee80211_local *local,
> > + struct sta_info *sta, u16 tid)
> > +{
> > + unsigned long flags;
> > + u16 queue = ieee80211_ac_from_tid(tid);
> > +
> > + ieee80211_stop_queue_by_reason(
> > + &local->hw, queue,
> > + IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
>
> No point in stopping the queue if the STA's tid queue is empty.
Hmm, true I guess.
> > @@ -602,22 +637,19 @@ void ieee80211_stop_tx_ba_cb(struct ieee
> > WLAN_BACK_INITIATOR, WLAN_REASON_QSTA_NOT_USE);
> >
> > spin_lock_bh(&sta->lock);
> > + spin_lock(&local->ampdu_lock);
> >
> > - if (*state & HT_AGG_STATE_INITIATOR_MSK &&
> > - hw->ampdu_queues) {
> > - /*
> > - * Wake up this queue, we stopped it earlier,
> > - * this will in turn wake the entire AC.
> > - */
> > - ieee80211_wake_queue_by_reason(hw,
> > - hw->queues + sta->tid_to_tx_q[tid],
> > - IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
> > - }
> > + ieee80211_agg_splice_packets(local, sta, tid);
>
> Is it possible for ieee80211_stop_tx_ba_cb() to be called while
> state of the tid is not yet operational? from what I can tell
> that's the only case when we would have added skbs to the STA's tid
> pending queue.
This happens when the session is denied by the peer.
> > *state = HT_AGG_STATE_IDLE;
> > + /* from now on packets are no longer put onto sta->pending */
>
> I think it would help to refer to the pendign queue consitantly instead
> as something like "STA's TID pending queue" as that is what it is. We also
> now have the local->pending queue. STA's pending queue seems to imply
> its also used for non-aggregation sessions.
>
> See -- if the state is not operation nothing would have gone
> been put into the STA's tid pending queue. Might also be worth
> mentioning that in the comment.
Hmm, yeah, might rewrite the comments a bit. Mind if I do a separate
patch later?
> > @@ -1013,20 +1013,53 @@ __ieee80211_tx_prepare(struct ieee80211_
> > */
> > }
> >
> > + /*
> > + * If this flag is set to true anywhere, and we get here,
> > + * we are doing the needed processing, so remove the flag
> > + * now.
> > + */
> > + info->flags &= ~IEEE80211_TX_INTFL_NEED_TXPROCESSING;
> > +
> > hdr = (struct ieee80211_hdr *) skb->data;
> >
> > tx->sta = sta_info_get(local, hdr->addr1);
> >
> > - if (tx->sta && ieee80211_is_data_qos(hdr->frame_control)) {
> > + if (tx->sta && ieee80211_is_data_qos(hdr->frame_control) &&
> > + (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION)) {
>
> Hm, why were we not crashing here before? I figure we would have
> with something like this:
>
> state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
>
> That it itself should be separate patch, but too late now. Anyway
> the new added check for IEEE80211_HW_AMPDU_AGGREGATION should go
> to stable.
Nah, state_tx always exists (and will be IDLE) but this was just an
added check to make us not need the spinlock for non-ampdu hw.
> > unsigned long flags;
> > + struct tid_ampdu_tx *tid_tx;
> > +
> > qc = ieee80211_get_qos_ctl(hdr);
> > tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
> >
> > spin_lock_irqsave(&tx->sta->lock, flags);
> > + /*
> > + * XXX: This spinlock could be fairly expensive, but see the
> > + * comment in agg-tx.c:ieee80211_agg_tx_operational().
> > + * One way to solve this would be to do something RCU-like
> > + * for managing the tid_tx struct and using atomic bitops
> > + * for the actual state -- by introducing an actual
> > + * 'operational' bit that would be possible. It would
> > + * require changing ieee80211_agg_tx_operational() to
> > + * set that bit, and changing the way tid_tx is managed
> > + * everywhere, including races between that bit and
> > + * tid_tx going away (tid_tx being added can be easily
> > + * committed to memory before the 'operational' bit).
> > + */
> > + tid_tx = tx->sta->ampdu_mlme.tid_tx[tid];
> > state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
> > - if (*state == HT_AGG_STATE_OPERATIONAL)
> > + if (*state == HT_AGG_STATE_OPERATIONAL) {
> > info->flags |= IEEE80211_TX_CTL_AMPDU;
>
> See, when its operational we don't add stuff to the STA's TID pending
> queue.
>
> This piece:
>
> > + } else if (*state != HT_AGG_STATE_IDLE) {
> > + /* in progress */
> > + queued = true;
> > + info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
> > + __skb_queue_tail(&tid_tx->pending, skb);
> > + }
>
> Can probably be ported to stable too, to just TX_DROP.
No, why? We're handing the frames to the driver. It wants to handle
those frames before agg session is started correctly. That's what I was
referring to before with this making more sense now.
> > @@ -1251,7 +1295,12 @@ static int ieee80211_tx(struct net_devic
> > do {
> > next = skb->next;
> > skb->next = NULL;
> > - skb_queue_tail(&local->pending[queue], skb);
> > + if (unlikely(txpending))
> > + skb_queue_head(&local->pending[queue],
> > + skb);
> > + else
> > + skb_queue_tail(&local->pending[queue],
> > + skb);
>
> For someone who hasn't read all the pending queue code stuff the above would be
> a little brain twister. It might be good to leave a comment explaining why
> txpendign would be set and why we add it to the head (i.e. because the "skb" sent
> at a previous time was put into a pending queue). Maybe even rename txpending to
> something like skb_was_pending.
Ok that might make some sense, though as a parameter you can easily see
where it's coming from, I think :) I agree that the entire code is a
little brain twister...
> > @@ -1360,7 +1407,7 @@ int ieee80211_master_start_xmit(struct s
> > "originating device\n", dev->name);
> > #endif
> > dev_kfree_skb(skb);
> > - return 0;
> > + return NETDEV_TX_OK;
>
> All these NETDEV_TX_OK could've gone into a separate patch.
I guess, they're also not strictly necessary since 0 == TX_OK :)
johannes
On Sat, Mar 28, 2009 at 10:41 AM, Johannes Berg
<[email protected]> wrote:
> On Sat, 2009-03-28 at 00:55 -0400, Luis R. Rodriguez wrote:
>
>
>> > This changes how mac80211 behaves towards drivers that support
>> > aggregation but have no hardware queues -- those drivers will now
>> > not be handed packets while the aggregation session is being
>> > established, but only after it has been fully established.
>>
>> That's fine from a logical point of view as compromise here as our
>> ampdu start should be relatively quick. Now while I can say this
>> from a logical point of view this patch obviously needed more
>> testing but lets see how it holds up and I'm glad you're the one
>> who wrote it. I'm confident there won't be any issues but that
>> is something I cannot gaurantee just based on the review. We now
>> need to go out and tests this. All other patches previous to this
>> make 100% perfect sense.
>
> :)
> I think it also makes more _sense_ to not ask the driver to handle th=
ese
> frames, because we won't set the AMPDU flag correctly if the session
> start is declined.
Hm, wouldn't we send them as normal frames if its declined here eventua=
lly?
>> > @@ -602,22 +637,19 @@ void ieee80211_stop_tx_ba_cb(struct ieee
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 WLAN_BACK_INITIATOR, WLAN_REASON_QSTA_NOT_USE);
>> >
>> > =C2=A0 =C2=A0 spin_lock_bh(&sta->lock);
>> > + =C2=A0 spin_lock(&local->ampdu_lock);
>> >
>> > - =C2=A0 if (*state & HT_AGG_STATE_INITIATOR_MSK &&
>> > - =C2=A0 =C2=A0 =C2=A0 hw->ampdu_queues) {
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Wake up this queue, w=
e stopped it earlier,
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* this will in turn wak=
e the entire AC.
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ieee80211_wake_queue_by_reaso=
n(hw,
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 h=
w->queues + sta->tid_to_tx_q[tid],
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 I=
EEE80211_QUEUE_STOP_REASON_AGGREGATION);
>> > - =C2=A0 }
>> > + =C2=A0 ieee80211_agg_splice_packets(local, sta, tid);
>>
>> Is it possible for ieee80211_stop_tx_ba_cb() to be called while
>> state of the tid is not yet operational? from what I can tell
>> that's the only case when we would have added skbs to the STA's tid
>> pending queue.
>
> This happens when the session is denied by the peer.
Ah, got it, thanks.
>> > =C2=A0 =C2=A0 *state =3D HT_AGG_STATE_IDLE;
>> > + =C2=A0 /* from now on packets are no longer put onto sta->pendin=
g */
>>
>> I think it would help to refer to the pendign queue consitantly inst=
ead
>> as something like "STA's TID pending queue" as that is what it is. W=
e also
>> now have the local->pending queue. STA's pending queue seems to impl=
y
>> its also used for non-aggregation sessions.
>>
>> See -- if the state is not operation nothing would have gone
>> been put into the STA's tid pending queue. Might also be worth
>> mentioning that in the comment.
>
> Hmm, yeah, might rewrite the comments a bit. Mind if I do a separate
> patch later?
Nope.
>> > @@ -1013,20 +1013,53 @@ __ieee80211_tx_prepare(struct ieee80211_
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>> > =C2=A0 =C2=A0 }
>> >
>> > + =C2=A0 /*
>> > + =C2=A0 =C2=A0* If this flag is set to true anywhere, and we get =
here,
>> > + =C2=A0 =C2=A0* we are doing the needed processing, so remove the=
flag
>> > + =C2=A0 =C2=A0* now.
>> > + =C2=A0 =C2=A0*/
>> > + =C2=A0 info->flags &=3D ~IEEE80211_TX_INTFL_NEED_TXPROCESSING;
>> > +
>> > =C2=A0 =C2=A0 hdr =3D (struct ieee80211_hdr *) skb->data;
>> >
>> > =C2=A0 =C2=A0 tx->sta =3D sta_info_get(local, hdr->addr1);
>> >
>> > - =C2=A0 if (tx->sta && ieee80211_is_data_qos(hdr->frame_control))=
{
>> > + =C2=A0 if (tx->sta && ieee80211_is_data_qos(hdr->frame_control) =
&&
>> > + =C2=A0 =C2=A0 =C2=A0 (local->hw.flags & IEEE80211_HW_AMPDU_AGGRE=
GATION)) {
>>
>> Hm, why were we not crashing here before? I figure we would have
>> with something like this:
>>
>> state =3D &tx->sta->ampdu_mlme.tid_state_tx[tid];
>>
>> That it itself should be separate patch, but too late now. Anyway
>> the new added check for IEEE80211_HW_AMPDU_AGGREGATION should go
>> to stable.
>
> Nah, state_tx always exists (and will be IDLE) but this was just an
> added check to make us not need the spinlock for non-ampdu hw.
Ah yes, its the ampdu_mlme.tid_tx that gets kmalloc'd only on aggr
session start.
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 unsigned long flags;
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct tid_ampdu_tx *tid_tx;
>> > +
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 qc =3D ieee80211_get_qos=
_ctl(hdr);
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tid =3D *qc & IEEE80211_=
QOS_CTL_TID_MASK;
>> >
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 spin_lock_irqsave(&tx->s=
ta->lock, flags);
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* XXX: This spinlock co=
uld be fairly expensive, but see the
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0c=
omment in agg-tx.c:ieee80211_agg_tx_operational().
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0O=
ne way to solve this would be to do something RCU-like
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0f=
or managing the tid_tx struct and using atomic bitops
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0f=
or the actual state -- by introducing an actual
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0'=
operational' bit that would be possible. It would
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0r=
equire changing ieee80211_agg_tx_operational() to
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0s=
et that bit, and changing the way tid_tx is managed
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0e=
verywhere, including races between that bit and
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0t=
id_tx going away (tid_tx being added can be easily
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0c=
ommitted to memory before the 'operational' bit).
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tid_tx =3D tx->sta->ampdu_mlm=
e.tid_tx[tid];
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 state =3D &tx->sta->ampd=
u_mlme.tid_state_tx[tid];
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (*state =3D=3D HT_AGG_STAT=
E_OPERATIONAL)
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (*state =3D=3D HT_AGG_STAT=
E_OPERATIONAL) {
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 info->flags |=3D IEEE80211_TX_CTL_AMPDU;
>>
>> See, when its operational we don't add stuff to the STA's TID pendin=
g
>> queue.
>>
>> This piece:
>>
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } else if (*state !=3D HT_AGG=
_STATE_IDLE) {
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /=
* in progress */
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 q=
ueued =3D true;
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 i=
nfo->flags |=3D IEEE80211_TX_INTFL_NEED_TXPROCESSING;
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 _=
_skb_queue_tail(&tid_tx->pending, skb);
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>>
>> Can probably be ported to stable too, to just TX_DROP.
>
> No, why? We're handing the frames to the driver. It wants to handle
> those frames before agg session is started correctly. That's what I w=
as
> referring to before with this making more sense now.
OK I see that but I am not sure of what's done to the skb in the old
case if the session is note complete yet, nor now if the session gets
declined. I can check later, right now I'm feeling lazy.
>> > @@ -1251,7 +1295,12 @@ static int ieee80211_tx(struct net_devic
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 do {
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 next =3D skb->next;
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 skb->next =3D NULL;
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 skb_queue_tail(&local->pending[queue], skb);
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 if (unlikely(txpending))
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 skb_queue_head(&lo=
cal->pending[queue],
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0skb);
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 else
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 skb_queue_tail(&lo=
cal->pending[queue],
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0skb);
>>
>> For someone who hasn't read all the pending queue code stuff the abo=
ve would be
>> a little brain twister. It might be good to leave a comment explaini=
ng why
>> txpendign would be set and why we add it to the head (i.e. because t=
he "skb" sent
>> at a previous time was put into a pending queue). Maybe even rename =
txpending to
>> something like skb_was_pending.
>
> Ok that might make some sense, though as a parameter you can easily s=
ee
> where it's coming from, I think :) I agree that the entire code is a
> little brain twister...
Yeah.. I had to re-read aggregation code again to get the hang of it.
Anything we can do to make
it easier for people to pick would be nice. I think I'll go patch up
the aggr-tx kdoc too now, unless we
plan on nuking some stuff soon again. I suspect the next big change
will be to move software based
aggregation queue handling and mapping to ACs within mac80211 for that
type of hardware which
we'll need for ar9170, ralink stuff, broadcom (if that ever happens)
and our next generation 11n USB
driver.
>> > @@ -1360,7 +1407,7 @@ int ieee80211_master_start_xmit(struct s
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0"originating device\n", dev->name);
>> > =C2=A0#endif
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_kfree_skb(skb);
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return 0;
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return NETDEV_TX_OK;
>>
>> All these NETDEV_TX_OK could've gone into a separate patch.
>
> I guess, they're also not strictly necessary since 0 =3D=3D TX_OK :)
Sure, what I'm trying to say is these patches are pretty hard to
review because of the topic,
it just helps to trim them down as much as possible -- but I realize
even that is painful.
Luis
On Sat, 2009-03-28 at 12:18 -0700, Luis R. Rodriguez wrote:
> >> That's fine from a logical point of view as compromise here as our
> >> ampdu start should be relatively quick. Now while I can say this
> >> from a logical point of view this patch obviously needed more
> >> testing but lets see how it holds up and I'm glad you're the one
> >> who wrote it. I'm confident there won't be any issues but that
> >> is something I cannot gaurantee just based on the review. We now
> >> need to go out and tests this. All other patches previous to this
> >> make 100% perfect sense.
> >
> > :)
> > I think it also makes more _sense_ to not ask the driver to handle these
> > frames, because we won't set the AMPDU flag correctly if the session
> > start is declined.
>
> Hm, wouldn't we send them as normal frames if its declined here eventually?
Yes, we take them off the sta->pending, put them through TX processing
and send them as normal frames.
> >> This piece:
> >>
> >> > + } else if (*state != HT_AGG_STATE_IDLE) {
> >> > + /* in progress */
> >> > + queued = true;
> >> > + info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
> >> > + __skb_queue_tail(&tid_tx->pending, skb);
> >> > + }
> >>
> >> Can probably be ported to stable too, to just TX_DROP.
> >
> > No, why? We're handing the frames to the driver. It wants to handle
> > those frames before agg session is started correctly. That's what I was
> > referring to before with this making more sense now.
>
> OK I see that but I am not sure of what's done to the skb in the old
> case if the session is note complete yet, nor now if the session gets
> declined. I can check later, right now I'm feeling lazy.
Before my patch the skb is still passed to the driver, which would need
to be able to handle it (it == session being stopped). I'm not entirely
sure ath9k handles it properly but I suspect it does.
> > Ok that might make some sense, though as a parameter you can easily see
> > where it's coming from, I think :) I agree that the entire code is a
> > little brain twister...
>
> Yeah.. I had to re-read aggregation code again to get the hang of it.
> Anything we can do to make
> it easier for people to pick would be nice. I think I'll go patch up
> the aggr-tx kdoc too now, unless we
> plan on nuking some stuff soon again. I suspect the next big change
> will be to move software based
> aggregation queue handling and mapping to ACs within mac80211 for that
> type of hardware which
> we'll need for ar9170, ralink stuff, broadcom (if that ever happens)
> and our next generation 11n USB
> driver.
Right -- but I don't expect to be working on it any time soon myself.
> >> > @@ -1360,7 +1407,7 @@ int ieee80211_master_start_xmit(struct s
> >> > "originating device\n", dev->name);
> >> > #endif
> >> > dev_kfree_skb(skb);
> >> > - return 0;
> >> > + return NETDEV_TX_OK;
> >>
> >> All these NETDEV_TX_OK could've gone into a separate patch.
> >
> > I guess, they're also not strictly necessary since 0 == TX_OK :)
>
> Sure, what I'm trying to say is these patches are pretty hard to
> review because of the topic,
> it just helps to trim them down as much as possible -- but I realize
> even that is painful.
:)
Thanks man, I really appreciate you giving them a close look!
johannes
On Sat, 2009-03-28 at 14:06 -0700, Luis R. Rodriguez wrote:
> My point was that if you have the tasklet handled by a separate CPU
> the DRV flag would most likely be set by the time the addba response
> comes back. On a UP box though you have no option but to wait for wait
> for it to be done after we send off the addba request so then it is a
> race between the scheduler to schedule the tasklet and the remote
> station's speed + the IRQ processing of the received response.
Ah, I see what you mean. But this goes off the same tasklet :) so ath9k
doesn't have a problem in any case.
> If we would somehow be able to ensure pending BHs complete before we
> process the addba response this wouldn't be needed in both places.
No, we can't ensure that -- the driver might very well take longer and
we don't have a way to only process the received frame later.
> Anyway I'm also stating that its likely that the ampdu start action is
> slower with iwlagn as interaction with the firmware is required so I
> would suspect one can see this behavior more likely on UP iwlagn
> boxen.
>
> Where did you see it?
I didn't actually see it :) But iwlagn can possibly flush the queue
before calling back to the cb.
johannes