[RFC,v4,0/4] Move TXQ scheduling into mac80211

Message

Toke Høiland-Jørgensen Sept. 16, 2018, 5:42 p.m. UTC

Another update, addressing most of the concerns raised in the last round:

- Added schedule_start()/end() functions that adds locking around the
  whole scheduling operation, which means we can get rid of the 'first'
  parameter to ieee80211_next_txq().

- Adds a callback in the wake_txqs tasklet which will ensure that any
  TXQs throttled by ieee80211_txq_may_transmit() will get woken up
  again. This also makes it possible to ensure all TXQs' deficits are
  increased in the case where the rotation in may_transmit is not
  effective because TXQs are not scheduled in round-robin order by the
  hardware. As part of this, bring back the flag that marks a TXQ as
  throttled.

- Rename ieee80211_schedule_txq() to ieee80211_return_txq() and add a
  check of empty TXQs inside it, so the driver can just call it
  unconditionally.

- Add a call to ieee80211_sta_register_airtime() from the existing
  tx_status path if tx_time is set in the tx_info status field.

- Reorder the patches to the cfg80211 airtime changes come before the
  changes to mac80211.

I didn't port over Kan's "airtime queue limits" stuff yet, partly
because I ran out of time, and partly because I wasn't use if he wanted
to do it himself :)

-Toke

---

Toke Høiland-Jørgensen (4):
      mac80211: Add TXQ scheduling API
      cfg80211: Add airtime statistics and settings
      mac80211: Add airtime accounting and scheduling to TXQs
      ath9k: Switch to mac80211 TXQ scheduling and airtime APIs


 drivers/net/wireless/ath/ath9k/ath9k.h     |   14 --
 drivers/net/wireless/ath/ath9k/debug.c     |    3 
 drivers/net/wireless/ath/ath9k/debug.h     |    8 -
 drivers/net/wireless/ath/ath9k/debug_sta.c |   54 ------
 drivers/net/wireless/ath/ath9k/init.c      |    3 
 drivers/net/wireless/ath/ath9k/recv.c      |    9 -
 drivers/net/wireless/ath/ath9k/xmit.c      |  244 ++++++++--------------------
 include/net/cfg80211.h                     |   10 +
 include/net/mac80211.h                     |  114 +++++++++++++
 include/uapi/linux/nl80211.h               |   15 ++
 net/mac80211/agg-tx.c                      |    2 
 net/mac80211/cfg.c                         |    3 
 net/mac80211/debugfs.c                     |    3 
 net/mac80211/debugfs_sta.c                 |   51 ++++++
 net/mac80211/driver-ops.h                  |    9 +
 net/mac80211/ieee80211_i.h                 |   12 +
 net/mac80211/main.c                        |    6 +
 net/mac80211/sta_info.c                    |   51 ++++++
 net/mac80211/sta_info.h                    |   13 +
 net/mac80211/status.c                      |    6 +
 net/mac80211/tx.c                          |  134 +++++++++++++++
 net/mac80211/util.c                        |   55 ++++++
 net/wireless/nl80211.c                     |   29 +++
 23 files changed, 575 insertions(+), 273 deletions(-)

X-Clacks-Overhead: GNU Terry Pratchett

Comments

Rajkumar Manoharan Sept. 20, 2018, 9:29 p.m. UTC | #1

On 2018-09-16 10:42, Toke Høiland-Jørgensen wrote:
> Another update, addressing most of the concerns raised in the last 
> round:
> 
> - Added schedule_start()/end() functions that adds locking around the
>   whole scheduling operation, which means we can get rid of the 'first'
>   parameter to ieee80211_next_txq().
> 
Toke,

Wouldn't this start()/end() APIs cause deadlock if mac80211 tries to 
acquire
active_txq_lock[ac] again? Or am I missing?

schedule_start()
while (next_txq()) {
   push_txq -> tx_dequeue()
   return_txq()
}
schedule_end()

tx_dequeue()
  ieee80211_free_txskb
   -> ieee80211_report_used_skb
    -> ieee80211_tdls_td_tx_handle
     -> ieee80211_subif_start_xmit
     -> __ieee80211_subif_start_xmit
       -> ieee80211_xmit_fast
         -> ieee80211_queue_skb
          -> schedule_and_wake_txq

-Rajkumar

Toke Høiland-Jørgensen Sept. 21, 2018, 12:41 p.m. UTC | #2

Rajkumar Manoharan <[e-mail]> writes:

> On 2018-09-16 10:42, Toke Høiland-Jørgensen wrote:
>> Another update, addressing most of the concerns raised in the last 
>> round:
>> 
>> - Added schedule_start()/end() functions that adds locking around the
>>   whole scheduling operation, which means we can get rid of the 'first'
>>   parameter to ieee80211_next_txq().
>> 
> Toke,
>
> Wouldn't this start()/end() APIs cause deadlock if mac80211 tries to 
> acquire
> active_txq_lock[ac] again? Or am I missing?
>
> schedule_start()
> while (next_txq()) {
>    push_txq -> tx_dequeue()
>    return_txq()
> }
> schedule_end()
>
> tx_dequeue()
>   ieee80211_free_txskb
>    -> ieee80211_report_used_skb
>     -> ieee80211_tdls_td_tx_handle
>      -> ieee80211_subif_start_xmit
>      -> __ieee80211_subif_start_xmit
>        -> ieee80211_xmit_fast
>          -> ieee80211_queue_skb
>           -> schedule_and_wake_txq

Hmm, yeah, that call sequence would certainly deadlock. But earlier, I
think; ieee80211_queue_skb() already takes fq->lock, which is being held
by tx_dequeue(), so this would deadlock on that?

I guess retransmits of TDLS teardown packets don't happen so often?
Otherwise someone would have run into this by now? Or are those frames
not being transmitted through the TXQs at all?

In any case it does seem a bit dangerous to have a possible path where
ieee80211_free_txskb() will result in a call to
ieee80211_subif_start_xmit(). So we should probably fix that in any
case?

-Toke