I see no real reason for status processing to be in
tasklet context, and moving it into process context
will make implementing some things that look at TX
status information easier.
There are 5 drivers that use ieee80211_tx_status():
ath5k, ath9k, b43, wl1251 and wl1271. Out of those,
only ath5k and ath9k call it from tasklet context,
so need to be changed, the others already call it
from process context.
Signed-off-by: Johannes Berg <[email protected]>
---
Ok so this might /slightly/ impact ath5k/ath9k performance, but maybe
they shouldn't be using a tasklet anyway but also use work structs...
Any other objections? It'll make things that need to know the frame
status easier, like client-side SM PS or anything else that requires
knowing the frame was ACKed and not lost somewhere on the way.
drivers/net/wireless/ath/ath5k/base.c | 2 -
drivers/net/wireless/ath/ath9k/xmit.c | 2 -
include/net/mac80211.h | 24 +++++++--------
net/mac80211/agg-tx.c | 4 +-
net/mac80211/ieee80211_i.h | 27 ++++++++++-------
net/mac80211/main.c | 26 ++++++++--------
net/mac80211/status.c | 54 ++++++++++++++++++++++------------
7 files changed, 83 insertions(+), 56 deletions(-)
--- wireless-testing.orig/net/mac80211/agg-tx.c 2009-11-18 19:49:06.000000000 +0100
+++ wireless-testing/net/mac80211/agg-tx.c 2009-11-18 19:52:03.000000000 +0100
@@ -495,7 +495,7 @@ void ieee80211_start_tx_ba_cb_irqsafe(st
ra_tid->vif = vif;
skb->pkt_type = IEEE80211_ADDBA_MSG;
- skb_queue_tail(&local->skb_queue, skb);
+ skb_queue_tail(&local->tasklet_queue, skb);
tasklet_schedule(&local->tasklet);
}
EXPORT_SYMBOL(ieee80211_start_tx_ba_cb_irqsafe);
@@ -632,7 +632,7 @@ void ieee80211_stop_tx_ba_cb_irqsafe(str
ra_tid->vif = vif;
skb->pkt_type = IEEE80211_DELBA_MSG;
- skb_queue_tail(&local->skb_queue, skb);
+ skb_queue_tail(&local->tasklet_queue, skb);
tasklet_schedule(&local->tasklet);
}
EXPORT_SYMBOL(ieee80211_stop_tx_ba_cb_irqsafe);
--- wireless-testing.orig/net/mac80211/ieee80211_i.h 2009-11-18 19:40:08.000000000 +0100
+++ wireless-testing/net/mac80211/ieee80211_i.h 2009-11-18 19:51:13.000000000 +0100
@@ -505,9 +505,8 @@ ieee80211_sdata_set_mesh_id(struct ieee8
enum {
IEEE80211_RX_MSG = 1,
- IEEE80211_TX_STATUS_MSG = 2,
- IEEE80211_DELBA_MSG = 3,
- IEEE80211_ADDBA_MSG = 4,
+ IEEE80211_DELBA_MSG,
+ IEEE80211_ADDBA_MSG,
};
enum queue_stop_reason {
@@ -612,14 +611,21 @@ struct ieee80211_local {
int tx_headroom; /* required headroom for hardware/radiotap */
- /* Tasklet and skb queue to process calls from IRQ mode. All frames
- * added to skb_queue will be processed, but frames in
- * skb_queue_unreliable may be dropped if the total length of these
- * queues increases over the limit. */
-#define IEEE80211_IRQSAFE_QUEUE_LIMIT 128
+ /*
+ * Tasklet and skb queue to process calls from IRQ mode.
+ */
struct tasklet_struct tasklet;
- struct sk_buff_head skb_queue;
- struct sk_buff_head skb_queue_unreliable;
+ struct sk_buff_head tasklet_queue;
+
+ /*
+ * TX status queue for calls that don't come from
+ * process context. All items on the tx_status queue
+ * will be processed but items on the unreliable one
+ * may be dropped if there's too much work.
+ */
+ struct work_struct tx_status_work;
+#define IEEE80211_TX_STATUS_QUEUE_LIMIT 128
+ struct sk_buff_head tx_status, tx_status_unreliable;
/* Station data */
/*
@@ -960,6 +966,7 @@ struct ieee80211_tx_status_rtap_hdr {
u8 data_retries;
} __attribute__ ((packed));
+void ieee80211_tx_status_work(struct work_struct *work);
/* HT */
void ieee80211_ht_cap_ie_to_sta_ht_cap(struct ieee80211_supported_band *sband,
--- wireless-testing.orig/net/mac80211/main.c 2009-11-18 19:40:08.000000000 +0100
+++ wireless-testing/net/mac80211/main.c 2009-11-18 19:51:51.000000000 +0100
@@ -243,8 +243,7 @@ static void ieee80211_tasklet_handler(un
struct sk_buff *skb;
struct ieee80211_ra_tid *ra_tid;
- while ((skb = skb_dequeue(&local->skb_queue)) ||
- (skb = skb_dequeue(&local->skb_queue_unreliable))) {
+ while ((skb = skb_dequeue(&local->tasklet_queue))) {
switch (skb->pkt_type) {
case IEEE80211_RX_MSG:
/* Clear skb->pkt_type in order to not confuse kernel
@@ -252,10 +251,6 @@ static void ieee80211_tasklet_handler(un
skb->pkt_type = 0;
ieee80211_rx(local_to_hw(local), skb);
break;
- case IEEE80211_TX_STATUS_MSG:
- skb->pkt_type = 0;
- ieee80211_tx_status(local_to_hw(local), skb);
- break;
case IEEE80211_DELBA_MSG:
ra_tid = (struct ieee80211_ra_tid *) &skb->cb;
ieee80211_stop_tx_ba_cb(ra_tid->vif, ra_tid->ra,
@@ -389,8 +384,11 @@ struct ieee80211_hw *ieee80211_alloc_hw(
ieee80211_tasklet_handler,
(unsigned long) local);
- skb_queue_head_init(&local->skb_queue);
- skb_queue_head_init(&local->skb_queue_unreliable);
+ skb_queue_head_init(&local->tasklet_queue);
+
+ INIT_WORK(&local->tx_status_work, ieee80211_tx_status_work);
+ skb_queue_head_init(&local->tx_status);
+ skb_queue_head_init(&local->tx_status_unreliable);
spin_lock_init(&local->ampdu_lock);
@@ -625,16 +623,20 @@ void ieee80211_unregister_hw(struct ieee
cancel_work_sync(&local->reconfig_filter);
+ cancel_work_sync(&local->tx_status_work);
+
ieee80211_clear_tx_pending(local);
sta_info_stop(local);
rate_control_deinitialize(local);
- if (skb_queue_len(&local->skb_queue)
- || skb_queue_len(&local->skb_queue_unreliable))
+ if (skb_queue_len(&local->tasklet_queue) ||
+ skb_queue_len(&local->tx_status) ||
+ skb_queue_len(&local->tx_status_unreliable))
printk(KERN_WARNING "%s: skb_queue not empty\n",
wiphy_name(local->hw.wiphy));
- skb_queue_purge(&local->skb_queue);
- skb_queue_purge(&local->skb_queue_unreliable);
+ skb_queue_purge(&local->tasklet_queue);
+ skb_queue_purge(&local->tx_status);
+ skb_queue_purge(&local->tx_status_unreliable);
destroy_workqueue(local->workqueue);
wiphy_unregister(local->hw.wiphy);
--- wireless-testing.orig/net/mac80211/status.c 2009-11-18 19:40:08.000000000 +0100
+++ wireless-testing/net/mac80211/status.c 2009-11-18 20:05:55.000000000 +0100
@@ -16,25 +16,31 @@
#include "led.h"
-void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw,
- struct sk_buff *skb)
+void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw, struct sk_buff *skb)
{
struct ieee80211_local *local = hw_to_local(hw);
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
- int tmp;
+ struct sk_buff_head *queue;
+ int num;
- skb->pkt_type = IEEE80211_TX_STATUS_MSG;
- skb_queue_tail(info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS ?
- &local->skb_queue : &local->skb_queue_unreliable, skb);
- tmp = skb_queue_len(&local->skb_queue) +
- skb_queue_len(&local->skb_queue_unreliable);
- while (tmp > IEEE80211_IRQSAFE_QUEUE_LIMIT &&
- (skb = skb_dequeue(&local->skb_queue_unreliable))) {
+ if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
+ queue = &local->tx_status;
+ else
+ queue = &local->tx_status_unreliable;
+
+ skb_queue_tail(queue, skb);
+
+ num = skb_queue_len(&local->tx_status) +
+ skb_queue_len(&local->tx_status_unreliable);
+
+ /* if we got over the limit with this frame, try to make room */
+ if (num > IEEE80211_TX_STATUS_QUEUE_LIMIT &&
+ (skb = skb_dequeue(&local->tx_status_unreliable))) {
dev_kfree_skb_irq(skb);
- tmp--;
I802_DEBUG_INC(local->tx_status_drop);
}
- tasklet_schedule(&local->tasklet);
+
+ ieee80211_queue_work(&local->hw, &local->tx_status_work);
}
EXPORT_SYMBOL(ieee80211_tx_status_irqsafe);
@@ -304,8 +310,8 @@ void ieee80211_tx_status(struct ieee8021
skb->protocol = htons(ETH_P_802_2);
memset(skb->cb, 0, sizeof(skb->cb));
- rcu_read_lock();
- list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+ mutex_lock(&local->iflist_mtx);
+ list_for_each_entry(sdata, &local->interfaces, list) {
if (sdata->vif.type == NL80211_IFTYPE_MONITOR) {
if (!netif_running(sdata->dev))
continue;
@@ -316,22 +322,34 @@ void ieee80211_tx_status(struct ieee8021
continue;
if (prev_dev) {
- skb2 = skb_clone(skb, GFP_ATOMIC);
+ skb2 = skb_clone(skb, GFP_KERNEL);
if (skb2) {
skb2->dev = prev_dev;
- netif_rx(skb2);
+ netif_rx_ni(skb2);
}
}
prev_dev = sdata->dev;
}
}
+ mutex_unlock(&local->iflist_mtx);
+
if (prev_dev) {
skb->dev = prev_dev;
- netif_rx(skb);
+ netif_rx_ni(skb);
skb = NULL;
}
- rcu_read_unlock();
dev_kfree_skb(skb);
}
EXPORT_SYMBOL(ieee80211_tx_status);
+
+void ieee80211_tx_status_work(struct work_struct *work)
+{
+ struct ieee80211_local *local =
+ container_of(work, struct ieee80211_local, tx_status_work);
+ struct sk_buff *skb;
+
+ while ((skb = skb_dequeue(&local->tx_status)) ||
+ (skb = skb_dequeue(&local->tx_status_unreliable)))
+ ieee80211_tx_status(&local->hw, skb);
+}
--- wireless-testing.orig/drivers/net/wireless/ath/ath5k/base.c 2009-11-18 20:02:56.000000000 +0100
+++ wireless-testing/drivers/net/wireless/ath/ath5k/base.c 2009-11-18 20:03:09.000000000 +0100
@@ -2002,7 +2002,7 @@ ath5k_tx_processq(struct ath5k_softc *sc
info->status.ack_signal = ts.ts_rssi;
}
- ieee80211_tx_status(sc->hw, skb);
+ ieee80211_tx_status_irqsafe(sc->hw, skb);
sc->tx_stats[txq->qnum].count++;
spin_lock(&sc->txbuflock);
--- wireless-testing.orig/drivers/net/wireless/ath/ath9k/xmit.c 2009-11-18 20:02:56.000000000 +0100
+++ wireless-testing/drivers/net/wireless/ath/ath9k/xmit.c 2009-11-18 20:03:19.000000000 +0100
@@ -1821,7 +1821,7 @@ static void ath_tx_complete(struct ath_s
if (unlikely(tx_info->pad[0] & ATH_TX_INFO_FRAME_TYPE_INTERNAL))
ath9k_tx_status(hw, skb);
else
- ieee80211_tx_status(hw, skb);
+ ieee80211_tx_status_irqsafe(hw, skb);
}
static void ath_tx_complete_buf(struct ath_softc *sc, struct ath_buf *bf,
--- wireless-testing.orig/include/net/mac80211.h 2009-11-18 20:09:21.000000000 +0100
+++ wireless-testing/include/net/mac80211.h 2009-11-18 20:11:00.000000000 +0100
@@ -31,7 +31,7 @@
*/
/**
- * DOC: Calling mac80211 from interrupts
+ * DOC: Calling mac80211 from (soft) interrupts
*
* Only ieee80211_tx_status_irqsafe() and ieee80211_rx_irqsafe() can be
* called in hardware interrupt context. The low-level driver must not call any
@@ -40,6 +40,9 @@
* IEEE 802.11 code call after this, e.g. from a scheduled workqueue or even
* tasklet function.
*
+ * ieee80211_tx_status() can also not be called from tasklet (softirq) context,
+ * call ieee80211_tx_status_irqsafe() instead.
+ *
* NOTE: If the driver opts to use the _irqsafe() functions, it may not also
* use the non-IRQ-safe functions!
*/
@@ -1734,31 +1737,28 @@ static inline void ieee80211_rx_ni(struc
* transmitted. It is permissible to not call this function for
* multicast frames but this can affect statistics.
*
- * This function may not be called in IRQ context. Calls to this function
- * for a single hardware must be synchronized against each other. Calls
- * to this function and ieee80211_tx_status_irqsafe() may not be mixed
- * for a single hardware.
+ * This function may not be called in hard or soft IRQ context. Calls
+ * to this function for a single hardware must be synchronized against
+ * each other. Calls to this function and ieee80211_tx_status_irqsafe()
+ * may not be mixed for a single device.
*
* @hw: the hardware the frame was transmitted by
* @skb: the frame that was transmitted, owned by mac80211 after this call
*/
-void ieee80211_tx_status(struct ieee80211_hw *hw,
- struct sk_buff *skb);
+void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb);
/**
* ieee80211_tx_status_irqsafe - IRQ-safe transmit status callback
*
- * Like ieee80211_tx_status() but can be called in IRQ context
- * (internally defers to a tasklet.)
+ * Like ieee80211_tx_status() but can be called in (soft) IRQ context.
*
* Calls to this function and ieee80211_tx_status() may not be mixed for a
- * single hardware.
+ * single device.
*
* @hw: the hardware the frame was transmitted by
* @skb: the frame that was transmitted, owned by mac80211 after this call
*/
-void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw,
- struct sk_buff *skb);
+void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw, struct sk_buff *skb);
/**
* ieee80211_beacon_get_tim - beacon generation function
On Wed, 2009-11-18 at 13:06 -0800, Luis R. Rodriguez wrote:
> OK, but can't you still have a driver spam mac80211 with a lot of
> ieee80211_tx_status_irqsafe() calls in soft irq context with the final
> skb requiring the tx complete, in that case the queue *will* get stuffed
> and you could potentially free more if so desired.
But it'll get stuffed one by one, and we free them as we stuff the
queue, so it can't ever loop :)
> Also, if our goal is to just avoid adding the skb if it does not require
> a tx complete and our queue size is too large
>
> if (!(info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) &&
> num + 1 > IEEE80211_TX_STATUS_QUEUE_LIMIT)
> dev_kfree_skb_irq(skb);
> else
> skb_queue_tail(&local->skb_queue)
No, we actually want to drop the older ones in that case.
> > However ... right now we never use _any_ unreliable at all, but I
> > suspect we will want to change that again at some point.
>
> Just curious -- what would be a use case for that?
Any time we don't need TX status for rate control it'd be OK to drop
frames that we don't need the status for. For example the hw-rc case my
patch from yesterday introduced.
johannes
On Wed, 2009-11-18 at 12:23 -0800, Luis R. Rodriguez wrote:
> > > > + /* if we got over the limit with this frame, try to make room */
> > > > + if (num > IEEE80211_TX_STATUS_QUEUE_LIMIT &&
> > > > + (skb = skb_dequeue(&local->tx_status_unreliable))) {
> > > > dev_kfree_skb_irq(skb);
> > >
> > > Note before there was a while loop and now just a if branch. Why can we
> > > get away with freeing now just one buffer? It would be nice to see
> > > this explained in the commit log entry as it is not obvious to me.
> >
> > Ah yes, I meant to explain that. We only use the status queues for TX
> > status now and not for RX too as before, so before it could actually
> > happen that we could free more than one off the unreliable queue, while
> > now it really can only be one.
>
> Ah, thanks, so, that skb_queue can go and we can just have one
> unreliable sk_buff ?
No, you misunderstood. Because I'm separating the queue for the tasklet
(tasklet_queue) and for TX status (tx_status/tx_status_unreliable) we
can only possibly free one since we only added one -- the count cannot
increase over that. The loop would only loop once at most.
However ... right now we never use _any_ unreliable at all, but I
suspect we will want to change that again at some point.
> I mean that ieee80211_tx_status_irqsafe() is basicaly doing this:
>
> ieee80211_tx_status_irqsafe()
> {
> skb_queue_tail(queue, skb);
> clear_unreliable_if_needed();
> ieee80211_queue_work(&local->hw, &local->tx_status_work);
> }
>
> Would be easier to read that way too.
Ah. Hmm well, I dunno, splitting it up that much didn't seem useful to
me.
> > > > - * This function may not be called in IRQ context. Calls to this function
> > > ^^^
> > >
> > > > - * for a single hardware must be synchronized against each other. Calls
> > > > - * to this function and ieee80211_tx_status_irqsafe() may not be mixed
> > > > - * for a single hardware.
> > > > + * This function may not be called in hard or soft IRQ context. Calls
> > > ^^^^ ^^ ^^^^ ^^^
> I was just highlighting the current kdoc for my first point on the e-mail.
Oh. Well it said "IRQ", but that was wrong, it should've said "hard
IRQ". Then again sometimes "IRQ" is used to refer to "hard interrupt"
only...
johannes
On Wed, 2009-11-18 at 14:53 -0500, Luis R. Rodriguez wrote:
> > Ok so this might /slightly/ impact ath5k/ath9k performance, but maybe
> > they shouldn't be using a tasklet anyway but also use work structs...
>
> Shoudln't by current mac80211 API documentation or shouldn't as in
> it'd be good to change it? From the current docs I read hard IRQ
> context but that could be clarified.
The latter :) I really see no real reason for it to be using tasklets.
> I'm not opposed to changing ath[59]k, in fact I want this change to
> happen; in the future more of our drivers will hopefully share more
> code and this then does mean eventually doing more things in process
> context. But if something is affecting performance terribly without proper
> coordination for a specific kernel release it would be terrible for
> us and would prefer to try to avoid it. Can you test to see how much
> performance difference you see with this?
Not really, I think it'd be hard to test, I expect a really small
increase in CPU utilisation. It's only a difference between processing
it, and putting it on a list, pulling it off a list and processing it.
> > --- wireless-testing.orig/net/mac80211/main.c 2009-11-18 19:40:08.000000000 +0100
> > +++ wireless-testing/net/mac80211/main.c 2009-11-18 19:51:51.000000000 +0100
> > @@ -625,16 +623,20 @@ void ieee80211_unregister_hw(struct ieee
> >
> > cancel_work_sync(&local->reconfig_filter);
> >
> > + cancel_work_sync(&local->tx_status_work);
> > +
>
> Are there changes required during suspend as well?
Good question, but I think not since we don't touch the tasklet during
suspend right now. Now, that might be wrong too, but at least it
wouldn't change anything (actually we flush the workqueue for suspend
anyway, don't we?)
> > --- wireless-testing.orig/net/mac80211/status.c 2009-11-18 19:40:08.000000000 +0100
> > +++ wireless-testing/net/mac80211/status.c 2009-11-18 20:05:55.000000000 +0100
> > @@ -16,25 +16,31 @@
> > #include "led.h"
> >
> >
> > -void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw,
> > - struct sk_buff *skb)
> > +void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw, struct sk_buff *skb)
> > {
> > struct ieee80211_local *local = hw_to_local(hw);
> > struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> > - int tmp;
> > + struct sk_buff_head *queue;
> > + int num;
> >
> > - skb->pkt_type = IEEE80211_TX_STATUS_MSG;
> > - skb_queue_tail(info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS ?
> > - &local->skb_queue : &local->skb_queue_unreliable, skb);
> > - tmp = skb_queue_len(&local->skb_queue) +
> > - skb_queue_len(&local->skb_queue_unreliable);
> > - while (tmp > IEEE80211_IRQSAFE_QUEUE_LIMIT &&
> > - (skb = skb_dequeue(&local->skb_queue_unreliable))) {
> > + if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
> > + queue = &local->tx_status;
> > + else
> > + queue = &local->tx_status_unreliable;
> > +
> > + skb_queue_tail(queue, skb);
>
> This sort of change could go into a seprate patch to help review
> easier.
Huh? The patch really only changes the if from being inlined into the
function call to not being there .. while changing the skb queue names
anyway.
> > + /* if we got over the limit with this frame, try to make room */
> > + if (num > IEEE80211_TX_STATUS_QUEUE_LIMIT &&
> > + (skb = skb_dequeue(&local->tx_status_unreliable))) {
> > dev_kfree_skb_irq(skb);
>
> Note before there was a while loop and now just a if branch. Why can we
> get away with freeing now just one buffer? It would be nice to see
> this explained in the commit log entry as it is not obvious to me.
Ah yes, I meant to explain that. We only use the status queues for TX
status now and not for RX too as before, so before it could actually
happen that we could free more than one off the unreliable queue, while
now it really can only be one.
Not that it actually matters, I just realised we never ever even use the
unreliable queue since we _always_ set the
IEEE80211_TX_CTL_REQ_TX_STATUS bit!
> > - tmp--;
> > I802_DEBUG_INC(local->tx_status_drop);
> > }
> > - tasklet_schedule(&local->tasklet);
> > +
> > + ieee80211_queue_work(&local->hw, &local->tx_status_work);
>
> How about just moving this last party to a helper as well while at it?
What?
> > - rcu_read_lock();
> > - list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > + mutex_lock(&local->iflist_mtx);
>
> Hm, what change here allows us to lift the rcu_read_lock, I
> was under the impression it would be the for sdata.
Well yeah, but we have the mutex now instead.
> > --- wireless-testing.orig/include/net/mac80211.h 2009-11-18 20:09:21.000000000 +0100
> > +++ wireless-testing/include/net/mac80211.h 2009-11-18 20:11:00.000000000 +0100
> > @@ -1734,31 +1737,28 @@ static inline void ieee80211_rx_ni(struc
> > * transmitted. It is permissible to not call this function for
> > * multicast frames but this can affect statistics.
> > *
> > - * This function may not be called in IRQ context. Calls to this function
> ^^^
>
> > - * for a single hardware must be synchronized against each other. Calls
> > - * to this function and ieee80211_tx_status_irqsafe() may not be mixed
> > - * for a single hardware.
> > + * This function may not be called in hard or soft IRQ context. Calls
> ^^^^ ^^ ^^^^ ^^^
?
johannes
On Wed, Nov 18, 2009 at 12:34:39PM -0800, Johannes Berg wrote:
> On Wed, 2009-11-18 at 12:23 -0800, Luis R. Rodriguez wrote:
>
> > > > > + /* if we got over the limit with this frame, try to make room */
> > > > > + if (num > IEEE80211_TX_STATUS_QUEUE_LIMIT &&
> > > > > + (skb = skb_dequeue(&local->tx_status_unreliable))) {
> > > > > dev_kfree_skb_irq(skb);
> > > >
> > > > Note before there was a while loop and now just a if branch. Why can we
> > > > get away with freeing now just one buffer? It would be nice to see
> > > > this explained in the commit log entry as it is not obvious to me.
> > >
> > > Ah yes, I meant to explain that. We only use the status queues for TX
> > > status now and not for RX too as before, so before it could actually
> > > happen that we could free more than one off the unreliable queue, while
> > > now it really can only be one.
> >
> > Ah, thanks, so, that skb_queue can go and we can just have one
> > unreliable sk_buff ?
>
> No, you misunderstood. Because I'm separating the queue for the tasklet
> (tasklet_queue) and for TX status (tx_status/tx_status_unreliable) we
> can only possibly free one since we only added one -- the count cannot
> increase over that. The loop would only loop once at most.
OK, but can't you still have a driver spam mac80211 with a lot of
ieee80211_tx_status_irqsafe() calls in soft irq context with the final
skb requiring the tx complete, in that case the queue *will* get stuffed
and you could potentially free more if so desired.
Also, if our goal is to just avoid adding the skb if it does not require
a tx complete and our queue size is too large
if (!(info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) &&
num + 1 > IEEE80211_TX_STATUS_QUEUE_LIMIT)
dev_kfree_skb_irq(skb);
else
skb_queue_tail(&local->skb_queue)
> However ... right now we never use _any_ unreliable at all, but I
> suspect we will want to change that again at some point.
Just curious -- what would be a use case for that?
Luis
On Wed, 2009-11-18 at 13:12 -0800, Luis R. Rodriguez wrote:
> On Wed, Nov 18, 2009 at 01:06:54PM -0800, Luis Rodriguez wrote:
> >
> > OK, but can't you still have a driver spam mac80211 with a lot of
> > ieee80211_tx_status_irqsafe() calls in soft irq context with the final
> > skb requiring the tx complete, in that case the queue *will* get stuffed
> > and you could potentially free more if so desired.
> >
> > Also, if our goal is to just avoid adding the skb if it does not require
> > a tx complete and our queue size is too large
>
> Hit send too early, I meant if our goal is to avoid adding the skb
> if we've reached our limit why not just ree it immediately instead
> of queing it and dequeing it. Wouldn't we even be able to just
> return and not bother mac80211 about it at all?
>
> >
> > if (!(info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) &&
> > num + 1 > IEEE80211_TX_STATUS_QUEUE_LIMIT)
> {
> > dev_kfree_skb_irq(skb);
> return;
> }
> > else
> > skb_queue_tail(&local->skb_queue)
>
Ah. Well, then we still would like them for rate control, just if we
can't keep up we drop older ones.
Not that it makes a whole lot of sense ... if you can't keep up with TX
status how did you manage to TX them to start with?
johannes
On Wed, Nov 18, 2009 at 08:16:11PM +0100, Johannes Berg wrote:
> I see no real reason for status processing to be in
> tasklet context, and moving it into process context
> will make implementing some things that look at TX
> status information easier.
>
> There are 5 drivers that use ieee80211_tx_status():
> ath5k, ath9k, b43, wl1251 and wl1271. Out of those,
> only ath5k and ath9k call it from tasklet context,
> so need to be changed, the others already call it
> from process context.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> Ok so this might /slightly/ impact ath5k/ath9k performance, but maybe
> they shouldn't be using a tasklet anyway but also use work structs...
Shoudln't by current mac80211 API documentation or shouldn't as in
it'd be good to change it? From the current docs I read hard IRQ
context but that could be clarified.
I'm not opposed to changing ath[59]k, in fact I want this change to
happen; in the future more of our drivers will hopefully share more
code and this then does mean eventually doing more things in process
context. But if something is affecting performance terribly without proper
coordination for a specific kernel release it would be terrible for
us and would prefer to try to avoid it. Can you test to see how much
performance difference you see with this? We can do so our our side
as well. If performance is affected this would be a good example of
one of those changes which could potentially be good for development
but might bad for users if not coordinated well.
> Any other objections? It'll make things that need to know the frame
> status easier, like client-side SM PS or anything else that requires
> knowing the frame was ACKed and not lost somewhere on the way.
Just a few comments below.
> --- wireless-testing.orig/net/mac80211/main.c 2009-11-18 19:40:08.000000000 +0100
> +++ wireless-testing/net/mac80211/main.c 2009-11-18 19:51:51.000000000 +0100
> @@ -625,16 +623,20 @@ void ieee80211_unregister_hw(struct ieee
>
> cancel_work_sync(&local->reconfig_filter);
>
> + cancel_work_sync(&local->tx_status_work);
> +
Are there changes required during suspend as well?
> --- wireless-testing.orig/net/mac80211/status.c 2009-11-18 19:40:08.000000000 +0100
> +++ wireless-testing/net/mac80211/status.c 2009-11-18 20:05:55.000000000 +0100
> @@ -16,25 +16,31 @@
> #include "led.h"
>
>
> -void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw,
> - struct sk_buff *skb)
> +void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw, struct sk_buff *skb)
> {
> struct ieee80211_local *local = hw_to_local(hw);
> struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> - int tmp;
> + struct sk_buff_head *queue;
> + int num;
>
> - skb->pkt_type = IEEE80211_TX_STATUS_MSG;
> - skb_queue_tail(info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS ?
> - &local->skb_queue : &local->skb_queue_unreliable, skb);
> - tmp = skb_queue_len(&local->skb_queue) +
> - skb_queue_len(&local->skb_queue_unreliable);
> - while (tmp > IEEE80211_IRQSAFE_QUEUE_LIMIT &&
> - (skb = skb_dequeue(&local->skb_queue_unreliable))) {
> + if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
> + queue = &local->tx_status;
> + else
> + queue = &local->tx_status_unreliable;
> +
> + skb_queue_tail(queue, skb);
This sort of change could go into a seprate patch to help review
easier.
> +
> + num = skb_queue_len(&local->tx_status) +
> + skb_queue_len(&local->tx_status_unreliable);
> +
> + /* if we got over the limit with this frame, try to make room */
> + if (num > IEEE80211_TX_STATUS_QUEUE_LIMIT &&
> + (skb = skb_dequeue(&local->tx_status_unreliable))) {
> dev_kfree_skb_irq(skb);
Note before there was a while loop and now just a if branch. Why can we
get away with freeing now just one buffer? It would be nice to see
this explained in the commit log entry as it is not obvious to me.
> - tmp--;
> I802_DEBUG_INC(local->tx_status_drop);
> }
> - tasklet_schedule(&local->tasklet);
> +
> + ieee80211_queue_work(&local->hw, &local->tx_status_work);
How about just moving this last party to a helper as well while at it?
> }
> EXPORT_SYMBOL(ieee80211_tx_status_irqsafe);
>
> @@ -304,8 +310,8 @@ void ieee80211_tx_status(struct ieee8021
> skb->protocol = htons(ETH_P_802_2);
> memset(skb->cb, 0, sizeof(skb->cb));
>
> - rcu_read_lock();
> - list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> + mutex_lock(&local->iflist_mtx);
Hm, what change here allows us to lift the rcu_read_lock, I
was under the impression it would be the for sdata.
> --- wireless-testing.orig/include/net/mac80211.h 2009-11-18 20:09:21.000000000 +0100
> +++ wireless-testing/include/net/mac80211.h 2009-11-18 20:11:00.000000000 +0100
> @@ -1734,31 +1737,28 @@ static inline void ieee80211_rx_ni(struc
> * transmitted. It is permissible to not call this function for
> * multicast frames but this can affect statistics.
> *
> - * This function may not be called in IRQ context. Calls to this function
^^^
> - * for a single hardware must be synchronized against each other. Calls
> - * to this function and ieee80211_tx_status_irqsafe() may not be mixed
> - * for a single hardware.
> + * This function may not be called in hard or soft IRQ context. Calls
^^^^ ^^ ^^^^ ^^^
> + * to this function for a single hardware must be synchronized against
> + * each other. Calls to this function and ieee80211_tx_status_irqsafe()
> + * may not be mixed for a single device.
Luis
On Wed, Nov 18, 2009 at 01:06:54PM -0800, Luis Rodriguez wrote:
>
> OK, but can't you still have a driver spam mac80211 with a lot of
> ieee80211_tx_status_irqsafe() calls in soft irq context with the final
> skb requiring the tx complete, in that case the queue *will* get stuffed
> and you could potentially free more if so desired.
>
> Also, if our goal is to just avoid adding the skb if it does not require
> a tx complete and our queue size is too large
Hit send too early, I meant if our goal is to avoid adding the skb
if we've reached our limit why not just ree it immediately instead
of queing it and dequeing it. Wouldn't we even be able to just
return and not bother mac80211 about it at all?
>
> if (!(info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) &&
> num + 1 > IEEE80211_TX_STATUS_QUEUE_LIMIT)
{
> dev_kfree_skb_irq(skb);
return;
}
> else
> skb_queue_tail(&local->skb_queue)
On Wed, Nov 18, 2009 at 12:01:57PM -0800, Johannes Berg wrote:
> On Wed, 2009-11-18 at 14:53 -0500, Luis R. Rodriguez wrote:
> > I'm not opposed to changing ath[59]k, in fact I want this change to
> > happen; in the future more of our drivers will hopefully share more
> > code and this then does mean eventually doing more things in process
> > context. But if something is affecting performance terribly without proper
> > coordination for a specific kernel release it would be terrible for
> > us and would prefer to try to avoid it. Can you test to see how much
> > performance difference you see with this?
>
> Not really, I think it'd be hard to test, I expect a really small
> increase in CPU utilisation. It's only a difference between processing
> it, and putting it on a list, pulling it off a list and processing it.
Sure, I understand, I wouldn't expect much difference either.
> > > --- wireless-testing.orig/net/mac80211/main.c 2009-11-18 19:40:08.000000000 +0100
> > > +++ wireless-testing/net/mac80211/main.c 2009-11-18 19:51:51.000000000 +0100
> > > @@ -625,16 +623,20 @@ void ieee80211_unregister_hw(struct ieee
> > >
> > > cancel_work_sync(&local->reconfig_filter);
> > >
> > > + cancel_work_sync(&local->tx_status_work);
> > > +
> >
> > Are there changes required during suspend as well?
>
> Good question, but I think not since we don't touch the tasklet during
> suspend right now. Now, that might be wrong too, but at least it
> wouldn't change anything
OK -- just wanted to make sure.
> (actually we flush the workqueue for suspend anyway, don't we?)
Yes.
> > > --- wireless-testing.orig/net/mac80211/status.c 2009-11-18 19:40:08.000000000 +0100
> > > +++ wireless-testing/net/mac80211/status.c 2009-11-18 20:05:55.000000000 +0100
> > > @@ -16,25 +16,31 @@
> > > #include "led.h"
> > >
> > >
> > > -void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw,
> > > - struct sk_buff *skb)
> > > +void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw, struct sk_buff *skb)
> > > {
> > > struct ieee80211_local *local = hw_to_local(hw);
> > > struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> > > - int tmp;
> > > + struct sk_buff_head *queue;
> > > + int num;
> > >
> > > - skb->pkt_type = IEEE80211_TX_STATUS_MSG;
> > > - skb_queue_tail(info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS ?
> > > - &local->skb_queue : &local->skb_queue_unreliable, skb);
> > > - tmp = skb_queue_len(&local->skb_queue) +
> > > - skb_queue_len(&local->skb_queue_unreliable);
> > > - while (tmp > IEEE80211_IRQSAFE_QUEUE_LIMIT &&
> > > - (skb = skb_dequeue(&local->skb_queue_unreliable))) {
> > > + if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
> > > + queue = &local->tx_status;
> > > + else
> > > + queue = &local->tx_status_unreliable;
> > > +
> > > + skb_queue_tail(queue, skb);
> >
> > This sort of change could go into a seprate patch to help review
> > easier.
>
> Huh? The patch really only changes the if from being inlined into the
> function call to not being there .. while changing the skb queue names
> anyway.
Its a small change hence why I used could not should. If you send
an RFC and tell me that it could potentially have an impact on ath9k
I'm going to read it carefully, the smaller the patch the easier the
review.
> > > + /* if we got over the limit with this frame, try to make room */
> > > + if (num > IEEE80211_TX_STATUS_QUEUE_LIMIT &&
> > > + (skb = skb_dequeue(&local->tx_status_unreliable))) {
> > > dev_kfree_skb_irq(skb);
> >
> > Note before there was a while loop and now just a if branch. Why can we
> > get away with freeing now just one buffer? It would be nice to see
> > this explained in the commit log entry as it is not obvious to me.
>
> Ah yes, I meant to explain that. We only use the status queues for TX
> status now and not for RX too as before, so before it could actually
> happen that we could free more than one off the unreliable queue, while
> now it really can only be one.
Ah, thanks, so, that skb_queue can go and we can just have one
unreliable sk_buff ?
> Not that it actually matters, I just realised we never ever even use the
> unreliable queue since we _always_ set the
> IEEE80211_TX_CTL_REQ_TX_STATUS bit!
>
> > > - tmp--;
> > > I802_DEBUG_INC(local->tx_status_drop);
> > > }
> > > - tasklet_schedule(&local->tasklet);
> > > +
> > > + ieee80211_queue_work(&local->hw, &local->tx_status_work);
> >
> > How about just moving this last party to a helper as well while at it?
>
> What?
I mean that ieee80211_tx_status_irqsafe() is basicaly doing this:
ieee80211_tx_status_irqsafe()
{
skb_queue_tail(queue, skb);
clear_unreliable_if_needed();
ieee80211_queue_work(&local->hw, &local->tx_status_work);
}
Would be easier to read that way too.
> > > - rcu_read_lock();
> > > - list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > > + mutex_lock(&local->iflist_mtx);
> >
> > Hm, what change here allows us to lift the rcu_read_lock, I
> > was under the impression it would be the for sdata.
>
> Well yeah, but we have the mutex now instead.
Ah got it.
> > > --- wireless-testing.orig/include/net/mac80211.h 2009-11-18 20:09:21.000000000 +0100
> > > +++ wireless-testing/include/net/mac80211.h 2009-11-18 20:11:00.000000000 +0100
> > > @@ -1734,31 +1737,28 @@ static inline void ieee80211_rx_ni(struc
> > > * transmitted. It is permissible to not call this function for
> > > * multicast frames but this can affect statistics.
> > > *
> > > - * This function may not be called in IRQ context. Calls to this function
> > ^^^
> >
> > > - * for a single hardware must be synchronized against each other. Calls
> > > - * to this function and ieee80211_tx_status_irqsafe() may not be mixed
> > > - * for a single hardware.
> > > + * This function may not be called in hard or soft IRQ context. Calls
> > ^^^^ ^^ ^^^^ ^^^
>
> ?
I was just highlighting the current kdoc for my first point on the e-mail.
Luis
On Wed, 2009-11-18 at 20:16 +0100, Johannes Berg wrote:
> I see no real reason for status processing to be in
> tasklet context,
Actually, I do.
The current code relies on the fact that TX status and RX is processed
in order (RX first) due to the filtered frame/sleeping station problem.
Moving RX into process context would be nice too, but I'm sure that'll
meet even more resistance.
johannes
On Wed, Nov 18, 2009 at 01:11:22PM -0800, Johannes Berg wrote:
> On Wed, 2009-11-18 at 13:06 -0800, Luis R. Rodriguez wrote:
>
> > OK, but can't you still have a driver spam mac80211 with a lot of
> > ieee80211_tx_status_irqsafe() calls in soft irq context with the final
> > skb requiring the tx complete, in that case the queue *will* get stuffed
> > and you could potentially free more if so desired.
>
> But it'll get stuffed one by one, and we free them as we stuff the
> queue, so it can't ever loop :)
Ah yes, I see... :)
> > Also, if our goal is to just avoid adding the skb if it does not require
> > a tx complete and our queue size is too large
> >
> > if (!(info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) &&
> > num + 1 > IEEE80211_TX_STATUS_QUEUE_LIMIT)
> > dev_kfree_skb_irq(skb);
> > else
> > skb_queue_tail(&local->skb_queue)
>
> No, we actually want to drop the older ones in that case.
Got it.. some info added behind this logic might help.
> > > However ... right now we never use _any_ unreliable at all, but I
> > > suspect we will want to change that again at some point.
> >
> > Just curious -- what would be a use case for that?
>
> Any time we don't need TX status for rate control it'd be OK to drop
> frames that we don't need the status for. For example the hw-rc case my
> patch from yesterday introduced.
Oh, haven't gotten to TX just yet, thanks this will probaby help.
Luis