Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> Applied, with the nits fixed as discussed.

Awesome, thanks!

> Come to think of it, if somebody is bored ;-) perhaps a hwsim option
> to use TXQs (should be optional I guess) would be nice so we can
> exercise this code with the wpa_supplicant hwsim tests. That would
> have caught the TKIP issues etc. pretty early on too, I think.

Noted. I'll look into that the next time I'm bored ;)

-Toke

Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

[Johannes Berg]

Applied, with the nits fixed as discussed.

Come to think of it, if somebody is bored ;-) perhaps a hwsim option to
use TXQs (should be optional I guess) would be nice so we can exercise
this code with the wpa_supplicant hwsim tests. That would have caught
the TKIP issues etc. pretty early on too, I think.

johannes

Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

>> Not sure if you want a v10, or if you're satisfied with the above
>> comments and will just fix up the nits on merging?
>> 
>
> I'll fix it up. Thanks!

Cool, thanks :)

-Toke

Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

[Johannes Berg]

> Because I need to run it anyway for the xmit_fast path on dequeue. I
> thought doing it this way simplifies the code (at the cost of the
> handler getting called twice when xmit_fast is not active).

Ok, that's fair.

> > I *think* it should commute with the rate control handler, but even
> > so, wouldn't it make more sense to have rate control late? Assuming
> > the packets are queued for some amount of time, having rate control
> > information queued with them would get stale.
> 
> Yes, having rate control run at dequeue would be good, and that's
> what I did initially. However, I found that this would lead to a
> deadlock because the rate control handler would send out packets in
> some cases (I forget the details but can go back and check if
> needed). And since the dequeue function is called with the driver TXQ
> lock held, that would lead to a deadlock when those packets made it
> to the driver TX path.

That seems really odd, but I can see how a deadlock happens then.

> So I decided to just keep it this way for now; I plan to go poking
> into the rate controller later anyway, so moving the handler to later
> could be part of that.

Sure, that's fair.

> But that handler only sets a few flags? Is
> tx->sdata->control_port_protocol likely to change while the packet is
> queued?

Oh right, I confused things there. We check the controlled port much
earlier, but anyway that should be OK.

> > It's a bit unfortunate that you lose fast-xmit here completely for
> > the key stuff, but I don't see a good way to avoid that, other than
> > completely rejiggering all the (possibly affected) queues when keys
> > change... might be very complex to do that, certainly a follow-up
> > patch if it's desired.
> 
> Yeah, figured it was better to have something that's correct and then
> go back and change it if the performance hit turns out to be too
> high.

Makes sense.

> > This check seems a bit weird though - how could fast-xmit be set
> > without a TXQ station?
> 
> I think that is probably just left over from before I introduced the
> control flag. Should be fine to remove it.

Ok.

> > 
> > > 
> > > +++ b/net/mac80211/util.c
> > > @@ -3393,11 +3393,18 @@ void ieee80211_txq_get_depth(struct
> > > ieee80211_txq *txq,
> > >    unsigned long *byte_cnt)
> > >  {
> > >   struct txq_info *txqi = to_txq_info(txq);
> > > + u32 frag_cnt = 0, frag_bytes = 0;
> > > + struct sk_buff *skb;
> > > +
> > > + skb_queue_walk(>frags, skb) {
> > > + frag_cnt++;
> > > + frag_bytes += skb->len;
> > > + }
> > 
> > I hope this is called infrequently :)
> 
> Well, ath10k is the only user. It does get called on each
> wake_tx_queue, though, so not that infrequently. My reasoning was
> that since the frags queue is never going to have more than a fairly
> small number of packets in it (those produced from a single split
> packet), counting this way is acceptable instead of keeping a state
> variable up to date. Can change it if you disagree :)

No, I guess you're right, it can't be a long queue.

> Not sure if you want a v10, or if you're satisfied with the above
> comments and will just fix up the nits on merging?
> 

I'll fix it up. Thanks!

johannes

Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> Hi Toke,
>
> Sorry for the delay reviewing this.
>
> I think I still have a few comments/questions.

No worries. And not terribly surprised ;)

>> +static inline bool txq_has_queue(struct ieee80211_txq *txq)
>> +{
>> +struct txq_info *txqi = to_txq_info(txq);
>> +return !(skb_queue_empty(>frags) && !txqi->tin.backlog_packets);
>> +}
>
> Tiny nit - there should probably be a blank line between the two lines
> here, but I could just fix that when I apply if you don't resend anyway
> for some other reason.

Noted.

> [snip helper stuff that looks fine]
>
>> -if (!tx->sta->sta.txq[0])
>> -hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
>> +hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
>
> Just to make sure I get this right - this is because the handler is now
> run on dequeue, so the special case is no longer needed?

Yup. The same change is made in xmit_fast (but obscured by the moving of
the surrounding code into _finish()).

>>  #define CALL_TXH(txh) \
>> @@ -1656,6 +1684,31 @@ static int invoke_tx_handlers(struct 
>> ieee80211_tx_data *tx)
>>  if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
>>  CALL_TXH(ieee80211_tx_h_rate_ctrl);
>
> Just for reference - the code block here that's unchanged contains
> this:
>
>         CALL_TXH(ieee80211_tx_h_dynamic_ps);
> CALL_TXH(ieee80211_tx_h_check_assoc);
> CALL_TXH(ieee80211_tx_h_ps_buf);
> CALL_TXH(ieee80211_tx_h_check_control_port_protocol);
> CALL_TXH(ieee80211_tx_h_select_key);
> if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
> CALL_TXH(ieee80211_tx_h_rate_ctrl);
>
>> +static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx)
>> +{
>> +struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
>> +ieee80211_tx_result res = TX_CONTINUE;
>> +
>>  if (unlikely(info->flags &
>> IEEE80211_TX_INTFL_RETRANSMISSION)) {
>>  __skb_queue_tail(>skbs, tx->skb);
>>  tx->skb = NULL;
>
> And this code here is also unchanged from the original TX handler
> invocation, so contains this:
>
>         if (unlikely(info->flags & IEEE80211_TX_INTFL_RETRANSMISSION)) {
> __skb_queue_tail(>skbs, tx->skb);
> tx->skb = NULL;
> goto txh_done;
> }
>
> CALL_TXH(ieee80211_tx_h_michael_mic_add);
> CALL_TXH(ieee80211_tx_h_sequence);
> CALL_TXH(ieee80211_tx_h_fragment);
> /* handlers after fragment must be aware of tx info fragmentation! */
> CALL_TXH(ieee80211_tx_h_stats);
> CALL_TXH(ieee80211_tx_h_encrypt);
> if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
> CALL_TXH(ieee80211_tx_h_calculate_duration);
>
> But now you have a problem (that you solved) that the key pointer can
> be invalidated while you have the packet queued between the two points,
> and then the tx_h_michael_mic_add and/or tx_h_encrypt would crash.
>
> You solve this by re-running tx_h_select_key() on dequeue, but it's not
> clear to me why you didn't move that to the late handlers instead?

Because I need to run it anyway for the xmit_fast path on dequeue. I
thought doing it this way simplifies the code (at the cost of the
handler getting called twice when xmit_fast is not active).

> I *think* it should commute with the rate control handler, but even
> so, wouldn't it make more sense to have rate control late? Assuming
> the packets are queued for some amount of time, having rate control
> information queued with them would get stale.

Yes, having rate control run at dequeue would be good, and that's what I
did initially. However, I found that this would lead to a deadlock
because the rate control handler would send out packets in some cases (I
forget the details but can go back and check if needed). And since the
dequeue function is called with the driver TXQ lock held, that would
lead to a deadlock when those packets made it to the driver TX path.

So I decided to just keep it this way for now; I plan to go poking into
the rate controller later anyway, so moving the handler to later could
be part of that.

> Similarly, it seems to me that checking the control port protocol later
> (or perhaps duplicating that?) would be a good idea?

But that handler only sets a few flags? Is
tx->sdata->control_port_protocol likely to change while the packet is
queued?

>> +/*
>> + * Can be called while the sta lock is held. Anything that can cause
>> packets to
>> + * be generated will cause deadlock!
>> + */
>> +static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data
>> *sdata,
>> +   struct sta_info *sta, u8
>> pn_offs,
>> +   struct ieee80211_key *key,
>> +   struct sk_buff *skb)
>
> That should be a void function now, you never check the return value
> and only return true 

Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

[Johannes Berg]

Hi Toke,

Sorry for the delay reviewing this.

I think I still have a few comments/questions.

> +static inline bool txq_has_queue(struct ieee80211_txq *txq)
> +{
> + struct txq_info *txqi = to_txq_info(txq);
> + return !(skb_queue_empty(>frags) && !txqi->tin.backlog_packets);
> +}

Tiny nit - there should probably be a blank line between the two lines
here, but I could just fix that when I apply if you don't resend anyway
for some other reason.

[snip helper stuff that looks fine]

> - if (!tx->sta->sta.txq[0])
> - hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
> + hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);

Just to make sure I get this right - this is because the handler is now
run on dequeue, so the special case is no longer needed?

>  #define CALL_TXH(txh) \
> @@ -1656,6 +1684,31 @@ static int invoke_tx_handlers(struct ieee80211_tx_data 
> *tx)
>   if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
>   CALL_TXH(ieee80211_tx_h_rate_ctrl);

Just for reference - the code block here that's unchanged contains
this:

        CALL_TXH(ieee80211_tx_h_dynamic_ps);
CALL_TXH(ieee80211_tx_h_check_assoc);
CALL_TXH(ieee80211_tx_h_ps_buf);
CALL_TXH(ieee80211_tx_h_check_control_port_protocol);
CALL_TXH(ieee80211_tx_h_select_key);
if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
CALL_TXH(ieee80211_tx_h_rate_ctrl);

> +static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx)
> +{
> + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
> + ieee80211_tx_result res = TX_CONTINUE;
> +
>   if (unlikely(info->flags &
> IEEE80211_TX_INTFL_RETRANSMISSION)) {
>   __skb_queue_tail(>skbs, tx->skb);
>   tx->skb = NULL;

And this code here is also unchanged from the original TX handler
invocation, so contains this:

        if (unlikely(info->flags & IEEE80211_TX_INTFL_RETRANSMISSION)) {
__skb_queue_tail(>skbs, tx->skb);
tx->skb = NULL;
goto txh_done;
}

CALL_TXH(ieee80211_tx_h_michael_mic_add);
CALL_TXH(ieee80211_tx_h_sequence);
CALL_TXH(ieee80211_tx_h_fragment);
/* handlers after fragment must be aware of tx info fragmentation! */
CALL_TXH(ieee80211_tx_h_stats);
CALL_TXH(ieee80211_tx_h_encrypt);
if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
CALL_TXH(ieee80211_tx_h_calculate_duration);

But now you have a problem (that you solved) that the key pointer can
be invalidated while you have the packet queued between the two points,
and then the tx_h_michael_mic_add and/or tx_h_encrypt would crash.

You solve this by re-running tx_h_select_key() on dequeue, but it's not
clear to me why you didn't move that to the late handlers instead?

I *think* it should commute with the rate control handler, but even so,
wouldn't it make more sense to have rate control late? Assuming the
packets are queued for some amount of time, having rate control
information queued with them would get stale.

Similarly, it seems to me that checking the control port protocol later
(or perhaps duplicating that?) would be a good idea?


> +/*
> + * Can be called while the sta lock is held. Anything that can cause
> packets to
> + * be generated will cause deadlock!
> + */
> +static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data
> *sdata,
> +    struct sta_info *sta, u8
> pn_offs,
> +    struct ieee80211_key *key,
> +    struct sk_buff *skb)

That should be a void function now, you never check the return value
and only return true anyway.

> + struct ieee80211_tx_info *info;
> + struct ieee80211_tx_data tx;
> + ieee80211_tx_result r;
> +

nit: extra blank line

>   spin_lock_bh(>lock);
>  
>   if (test_bit(IEEE80211_TXQ_STOP, >flags))
>   goto out;
>  
> + /* Make sure fragments stay together. */
> + skb = __skb_dequeue(>frags);
> + if (skb)
> + goto out;
> +
> +begin:

I guess now that you introduced that anyway, we should consider making
the skb_linearize() failure go there. Should be a follow-up patch
though.

> + /*
> +  * The key can be removed while the packet was queued, so
> need to call
> +  * this here to get the current key.
> +  */
> + r = ieee80211_tx_h_select_key();
> + if (r != TX_CONTINUE) {
> + ieee80211_free_txskb(>hw, skb);
> + goto begin;
> + }
> +
> + if (txq->sta && info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) {

It's a bit unfortunate that you lose fast-xmit here completely for the
key stuff, but I don't see a good way to avoid that, other than
completely rejiggering all the (possibly affected) queues when keys
change... might be very complex to do that, certainly a follow-up patch
if it's desired.

This check 

[PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

[Toke Høiland-Jørgensen]

The TXQ intermediate queues can cause packet reordering when more than
one flow is active to a single station. Since some of the wifi-specific
packet handling (notably sequence number and encryption handling) is
sensitive to re-ordering, things break if they are applied before the
TXQ.

This splits up the TX handlers and fast_xmit logic into two parts: An
early part and a late part. The former is applied before TXQ enqueue,
and the latter after dequeue. The non-TXQ path just applies both parts
at once.

Because fragments shouldn't be split up or reordered, the fragmentation
handler is run after dequeue. Any fragments are then kept in the TXQ and
on subsequent dequeues they take precedence over dequeueing from the FQ
structure.

This approach avoids having to scatter special cases all over the place
for when TXQ is enabled, at the cost of making the fast_xmit and TX
handler code slightly more complex.

Signed-off-by: Toke Høiland-Jørgensen 
---
 include/net/mac80211.h |   2 +
 net/mac80211/ieee80211_i.h |   8 ++
 net/mac80211/rx.c  |   4 +-
 net/mac80211/sta_info.c|  10 +-
 net/mac80211/tx.c  | 287 +
 net/mac80211/util.c|  11 +-
 6 files changed, 232 insertions(+), 90 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 5296100..9463039 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -715,6 +715,7 @@ enum mac80211_tx_info_flags {
  * frame (PS-Poll or uAPSD).
  * @IEEE80211_TX_CTRL_RATE_INJECT: This frame is injected with rate information
  * @IEEE80211_TX_CTRL_AMSDU: This frame is an A-MSDU frame
+ * @IEEE80211_TX_CTRL_FAST_XMIT: This frame is going through the fast_xmit path
  *
  * These flags are used in tx_info->control.flags.
  */
@@ -723,6 +724,7 @@ enum mac80211_tx_control_flags {
IEEE80211_TX_CTRL_PS_RESPONSE   = BIT(1),
IEEE80211_TX_CTRL_RATE_INJECT   = BIT(2),
IEEE80211_TX_CTRL_AMSDU = BIT(3),
+   IEEE80211_TX_CTRL_FAST_XMIT = BIT(4),
 };
 
 /*
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index c71c735..caca265 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -813,12 +813,14 @@ enum txq_info_flags {
  * @def_flow: used as a fallback flow when a packet destined to @tin hashes to
  * a fq_flow which is already owned by a different tin
  * @def_cvars: codel vars for @def_flow
+ * @frags: used to keep fragments created after dequeue
  */
 struct txq_info {
struct fq_tin tin;
struct fq_flow def_flow;
struct codel_vars def_cvars;
struct codel_stats cstats;
+   struct sk_buff_head frags;
unsigned long flags;
 
/* keep last! */
@@ -1481,6 +1483,12 @@ static inline struct txq_info *to_txq_info(struct 
ieee80211_txq *txq)
return container_of(txq, struct txq_info, txq);
 }
 
+static inline bool txq_has_queue(struct ieee80211_txq *txq)
+{
+   struct txq_info *txqi = to_txq_info(txq);
+   return !(skb_queue_empty(>frags) && !txqi->tin.backlog_packets);
+}
+
 static inline int ieee80211_bssid_match(const u8 *raddr, const u8 *addr)
 {
return ether_addr_equal(raddr, addr) ||
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index e796060..ae5786b8 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1323,9 +1323,7 @@ static void sta_ps_start(struct sta_info *sta)
return;
 
for (tid = 0; tid < ARRAY_SIZE(sta->sta.txq); tid++) {
-   struct txq_info *txqi = to_txq_info(sta->sta.txq[tid]);
-
-   if (txqi->tin.backlog_packets)
+   if (txq_has_queue(sta->sta.txq[tid]))
set_bit(tid, >txq_buffered_tids);
else
clear_bit(tid, >txq_buffered_tids);
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 1b1b28f..167bff0 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1212,12 +1212,10 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_info 
*sta)
 
if (sta->sta.txq[0]) {
for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
-   struct txq_info *txqi = to_txq_info(sta->sta.txq[i]);
-
-   if (!txqi->tin.backlog_packets)
+   if (!txq_has_queue(sta->sta.txq[i]))
continue;
 
-   drv_wake_tx_queue(local, txqi);
+   drv_wake_tx_queue(local, to_txq_info(sta->sta.txq[i]));
}
}
 
@@ -1649,9 +1647,7 @@ ieee80211_sta_ps_deliver_response(struct sta_info *sta,
return;
 
for (tid = 0; tid < ARRAY_SIZE(sta->sta.txq); tid++) {
-   struct txq_info *txqi = to_txq_info(sta->sta.txq[tid]);
-
-   if (!(tids & BIT(tid)) || txqi->tin.backlog_packets)
+   if (!(tids &