Re: [RFC 0/5] add XDP support to mt76x2e/mt76x0e drivers

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Mon, 2018-12-03 at 21:36 +0200, Toke Høiland-Jørgensen wrote:
>> Hi Johannes
>> 
>> I think your email can be basically summed up to:
>> 
>> > [ ... ] but really I think it's a can of worms.
>> 
>> ...right? :)
>
> Heh, yeah :)
>
>> I sort of had a feeling it would be, but thank you for spelling out in
>> excruciating detail why that is so.
>
> :-)
>
>> Given this, I think I agree that it's not worth it for now, and we
>> should hold off on adding XDP support until we have 802.3/.11
>> conversion offload working... Which I think is also where you ended
>> up? :)
>
> That case is at least easy, yeah. And it seems kinda likely that we'll
> end up with that in all well-maintained drivers in the relatively near
> future anyway?

Right; you probably know that better than me :)

> BTW, in a sense I still kind of want to add eBPF to the mac80211 ingress
> path, just not in the XDP sense. For example, I had a proposal a while
> ago to add a filter to the monitor mode RX path(s) in eBPF; I still
> think that's useful.
>
> I also think it may be useful to put eBPF programs into per-netdev
> ingress path, in order to e.g. collect statistics, rather than hard-
> coding all kinds of statistics into mac80211.
>
> All of these things I consider absolutely useful and helpful. I like
> eBPF and the flexibility it affords. I just really don't think we should
> call it XDP or let it do similar things to XDP like dropping or
> redirecting frames.

Absolutely; totally on board with that!

-Toke

Re: [RFC 0/5] add XDP support to mt76x2e/mt76x0e drivers

[Toke Høiland-Jørgensen]

Hi Johannes

I think your email can be basically summed up to:

> [ ... ] but really I think it's a can of worms.

...right? :)

I sort of had a feeling it would be, but thank you for spelling out in
excruciating detail why that is so.

Given this, I think I agree that it's not worth it for now, and we
should hold off on adding XDP support until we have 802.3/.11 conversion
offload working... Which I think is also where you ended up? :)

-Toke

Re: [RFC 0/5] add XDP support to mt76x2e/mt76x0e drivers

[Toke Høiland-Jørgensen]

Michał Kazior  writes:

> On Thu, 29 Nov 2018 at 14:00, Lorenzo Bianconi
>  wrote:
> [...]
>> > The other direction will probably be more difficult, at least if 802.11
>> > frames need to be built in software. It *might* be possible with the XDP
>> > egress hook we are planning (with a suitable set of helpers, the eBPF
>> > program could build the 802.11 frames), but I'm not really sure if that
>> > is worth doing as I'm quite sure there are some hairy edge cases
>> > there...
>>
>> The possible issue with XDP_DROP action you are referring to here is A-MPDU
>> reordering on rx side, right? If so I guess the issue will be fixed by
>> tid_agg_rx->reorder_timer. Are you referring to other possible edge cases?
>
> What I'm thinking is reordering could be one of possible things to
> offload to an XDP program. It would require per-station data structure
> to keep track of the frame sequence numbers (among other things). Same
> could be said for crypto offloads (would require XDP programs to be
> able to use crypto apis I guess?).

In principle, all of this could be done. But we need to think carefully
about what things it really makes sense to offload to XDP. In the
general case, we will end up re-implementing all of mac80211 in eBPF,
which is obviously not ideal. However, fast-path handling in XDP, which
will punt to the full stack on edge cases, is probably doable (and is
the general model we envision for XDP programs).

For crypto in particular, I wonder if there is much of a speedup to be
had if the crypto is in software anyway. Wouldn't that dominate the
processing time? Whereas, if the crypto is offloaded to hardware,
fast-path packet processing in XDP might make sense. This would
translate to the fallback mode I mention above: If hardware crypto is
enabled, handle in XDP fast-path, otherwise punt to mac80211 for full
(crypto) processing.

-Toke

Re: [RFC 0/5] add XDP support to mt76x2e/mt76x0e drivers

[Toke Høiland-Jørgensen]

Lorenzo Bianconi  writes:

>> Lorenzo Bianconi  writes:
>> 
>> > On Nov 28, Toke Høiland-Jørgensen wrote:
>> >> Lorenzo Bianconi  writes:
>> >> 
>> >> >> Lorenzo Bianconi  writes:
>> >> >> 
>> >> >> >> >> > This series is intended as a playground to start 
>> >> >> >> >> > experimenting/developing
>> >> >> >> >> > with XDP/eBPF over WiFi and collect ideas/concerns about it.
>> >> >> >> >> > Introduce XDP support to mt76x2e/mt76x0e drivers. Currently 
>> >> >> >> >> > supported
>> >> >> >> >> > actions are:
>> >> >> >> >> > - XDP_PASS
>> >> >> >> >> > - XDP_ABORTED
>> >> >> >> >> > - XDP_DROP
>> >> >> >> >> > Introduce ndo_bpf mac80211 callback in order to to load a bpf
>> >> >> >> >> > program into low level driver XDP rx hook.
>> >> >> >> >> > This series has been tested through a simple bpf program 
>> >> >> >> >> > (available here:
>> >> >> >> >> > https://github.com/LorenzoBianconi/bpf-workspace/tree/master/mt76_xdp_stats)
>> >> >> >> >> > used to count frame types received by the device.
>> >> >> >> >> > Possible eBPF use cases could be:
>> >> >> >> >> > - implement new statistics through bpf maps
>> >> >> >> >> > - implement fast packet filtering (e.g in monitor mode)
>> >> >> >> >> > - ...
>> >> >> >> >
>> >> >> >> > Hi Kalle,
>> >> >> >> >
>> >> >> >> >> 
>> >> >> >> >> This is most likely a stupid question, but why do this in the 
>> >> >> >> >> driver and
>> >> >> >> >> not in mac80211 so that all drivers could benefit from it? I 
>> >> >> >> >> guess there
>> >> >> >> >> are reasons for that, I just can't figure that out.
>> >> >> >> 
>> >> >> >> XDP achieves its speedup by running the eBPF program inside the 
>> >> >> >> driver
>> >> >> >> NAPI loop, before the kernel even touches the data in any other 
>> >> >> >> capacity
>> >> >> >> (and in particular, before it allocates an SKB). Which kinda means 
>> >> >> >> the
>> >> >> >> hook needs to be in the driver... Could be a fallback in mac80211,
>> >> >> >> though; although we'd have to figure out how that interacts with 
>> >> >> >> Generic
>> >> >> >> XDP.
>> >> >> >> 
>> >> >> >> > This is an early stage implementation, at this point I would 
>> >> >> >> > collect
>> >> >> >> > other people opinions/concerns about using bpf/xdp directly on 
>> >> >> >> > 802.11
>> >> >> >> > frames.
>> >> >> >> 
>> >> >> >> Thanks for looking into this!
>> >> >> >
>> >> >> > Hi Toke,
>> >> >> >
>> >> >> >> 
>> >> >> >> I have two concerns with running XDP on 802.11 frames:
>> >> >> >> 
>> >> >> >> 1. It makes it more difficult to add other XDP actions (such as
>> >> >> >>REDIRECT), as the XDP program would then have to make sure that 
>> >> >> >> the
>> >> >> >>outer packet headers are removed before, say, redirecting the 
>> >> >> >> packet
>> >> >> >>out of an ethernet interface. Also, if we do add redirect, we 
>> >> >> >> would
>> >> >> >>be bypassing mac80211 entirely; to what extent would that mess up
>> >> >> >>internal state?
>> >> >> >> 
>> >> >> >
>> >> >> > You are right, my assumption here is the logic/complexity is moved to
>> >> >> > the bpf program that needs to take care 

Re: [RFC 0/5] add XDP support to mt76x2e/mt76x0e drivers

[Toke Høiland-Jørgensen]

Lorenzo Bianconi  writes:

> On Nov 28, Toke Høiland-Jørgensen wrote:
>> Lorenzo Bianconi  writes:
>> 
>> >> Lorenzo Bianconi  writes:
>> >> 
>> >> >> >> > This series is intended as a playground to start 
>> >> >> >> > experimenting/developing
>> >> >> >> > with XDP/eBPF over WiFi and collect ideas/concerns about it.
>> >> >> >> > Introduce XDP support to mt76x2e/mt76x0e drivers. Currently 
>> >> >> >> > supported
>> >> >> >> > actions are:
>> >> >> >> > - XDP_PASS
>> >> >> >> > - XDP_ABORTED
>> >> >> >> > - XDP_DROP
>> >> >> >> > Introduce ndo_bpf mac80211 callback in order to to load a bpf
>> >> >> >> > program into low level driver XDP rx hook.
>> >> >> >> > This series has been tested through a simple bpf program 
>> >> >> >> > (available here:
>> >> >> >> > https://github.com/LorenzoBianconi/bpf-workspace/tree/master/mt76_xdp_stats)
>> >> >> >> > used to count frame types received by the device.
>> >> >> >> > Possible eBPF use cases could be:
>> >> >> >> > - implement new statistics through bpf maps
>> >> >> >> > - implement fast packet filtering (e.g in monitor mode)
>> >> >> >> > - ...
>> >> >> >
>> >> >> > Hi Kalle,
>> >> >> >
>> >> >> >> 
>> >> >> >> This is most likely a stupid question, but why do this in the 
>> >> >> >> driver and
>> >> >> >> not in mac80211 so that all drivers could benefit from it? I guess 
>> >> >> >> there
>> >> >> >> are reasons for that, I just can't figure that out.
>> >> >> 
>> >> >> XDP achieves its speedup by running the eBPF program inside the driver
>> >> >> NAPI loop, before the kernel even touches the data in any other 
>> >> >> capacity
>> >> >> (and in particular, before it allocates an SKB). Which kinda means the
>> >> >> hook needs to be in the driver... Could be a fallback in mac80211,
>> >> >> though; although we'd have to figure out how that interacts with 
>> >> >> Generic
>> >> >> XDP.
>> >> >> 
>> >> >> > This is an early stage implementation, at this point I would collect
>> >> >> > other people opinions/concerns about using bpf/xdp directly on 802.11
>> >> >> > frames.
>> >> >> 
>> >> >> Thanks for looking into this!
>> >> >
>> >> > Hi Toke,
>> >> >
>> >> >> 
>> >> >> I have two concerns with running XDP on 802.11 frames:
>> >> >> 
>> >> >> 1. It makes it more difficult to add other XDP actions (such as
>> >> >>REDIRECT), as the XDP program would then have to make sure that the
>> >> >>outer packet headers are removed before, say, redirecting the packet
>> >> >>out of an ethernet interface. Also, if we do add redirect, we would
>> >> >>be bypassing mac80211 entirely; to what extent would that mess up
>> >> >>internal state?
>> >> >> 
>> >> >
>> >> > You are right, my assumption here is the logic/complexity is moved to
>> >> > the bpf program that needs to take care of all possible issues that
>> >> > can be introduced. More or less it is the same if a bpf program mess
>> >> > up with TCP segments on a wired connection, isn't it?
>> >> 
>> >> No, I guess not; except here it potentially applies to all packets
>> >> (things like BAW tracking), and it is *in addition* to TCP.
>> >
>> > Yes, here it is a little bit harder, but I was meaning that the bpf program
>> > has to be very careful when dropping a packet :)
>> 
>> Yeah. What kind of filtering were you thinking you would use this for in
>> the short term?
>> 
>
> When I started working on XDP for mt76 I was thinking about BSSID
> filtering but I was looking for a more general solution respect to add
> that feature in the driver. Moreover we could use bpf for fast packet
> filtering when you add an interface in monitor mode.

Yup, both of these make sense.

> Nevertheless I guess there could be other use cases not limited to
> frame filtering. My primary goal with this series is to collect
> ideas/concerns on WiFi XDP/eBPF possible uses cases.

Well, Michał's idea about offloading is cool if it is possible to get
vendors to implement it.

Other than that, if we can solve the issues with differences between
802.11 and plain Ethernet frames, I see no reason why it wouldn't be
possible to implement an XDP fast-path for WiFi-to-Ethernet forwarding,
which might be useful in an access point, especially as WiFi speeds
increase.

The other direction will probably be more difficult, at least if 802.11
frames need to be built in software. It *might* be possible with the XDP
egress hook we are planning (with a suitable set of helpers, the eBPF
program could build the 802.11 frames), but I'm not really sure if that
is worth doing as I'm quite sure there are some hairy edge cases
there...

-Toke

Re: [RFC 0/5] add XDP support to mt76x2e/mt76x0e drivers

[Toke Høiland-Jørgensen]

Lorenzo Bianconi  writes:

>> Lorenzo Bianconi  writes:
>> 
>> >> >> > This series is intended as a playground to start 
>> >> >> > experimenting/developing
>> >> >> > with XDP/eBPF over WiFi and collect ideas/concerns about it.
>> >> >> > Introduce XDP support to mt76x2e/mt76x0e drivers. Currently supported
>> >> >> > actions are:
>> >> >> > - XDP_PASS
>> >> >> > - XDP_ABORTED
>> >> >> > - XDP_DROP
>> >> >> > Introduce ndo_bpf mac80211 callback in order to to load a bpf
>> >> >> > program into low level driver XDP rx hook.
>> >> >> > This series has been tested through a simple bpf program (available 
>> >> >> > here:
>> >> >> > https://github.com/LorenzoBianconi/bpf-workspace/tree/master/mt76_xdp_stats)
>> >> >> > used to count frame types received by the device.
>> >> >> > Possible eBPF use cases could be:
>> >> >> > - implement new statistics through bpf maps
>> >> >> > - implement fast packet filtering (e.g in monitor mode)
>> >> >> > - ...
>> >> >
>> >> > Hi Kalle,
>> >> >
>> >> >> 
>> >> >> This is most likely a stupid question, but why do this in the driver 
>> >> >> and
>> >> >> not in mac80211 so that all drivers could benefit from it? I guess 
>> >> >> there
>> >> >> are reasons for that, I just can't figure that out.
>> >> 
>> >> XDP achieves its speedup by running the eBPF program inside the driver
>> >> NAPI loop, before the kernel even touches the data in any other capacity
>> >> (and in particular, before it allocates an SKB). Which kinda means the
>> >> hook needs to be in the driver... Could be a fallback in mac80211,
>> >> though; although we'd have to figure out how that interacts with Generic
>> >> XDP.
>> >> 
>> >> > This is an early stage implementation, at this point I would collect
>> >> > other people opinions/concerns about using bpf/xdp directly on 802.11
>> >> > frames.
>> >> 
>> >> Thanks for looking into this!
>> >
>> > Hi Toke,
>> >
>> >> 
>> >> I have two concerns with running XDP on 802.11 frames:
>> >> 
>> >> 1. It makes it more difficult to add other XDP actions (such as
>> >>REDIRECT), as the XDP program would then have to make sure that the
>> >>outer packet headers are removed before, say, redirecting the packet
>> >>out of an ethernet interface. Also, if we do add redirect, we would
>> >>be bypassing mac80211 entirely; to what extent would that mess up
>> >>internal state?
>> >> 
>> >
>> > You are right, my assumption here is the logic/complexity is moved to
>> > the bpf program that needs to take care of all possible issues that
>> > can be introduced. More or less it is the same if a bpf program mess
>> > up with TCP segments on a wired connection, isn't it?
>> 
>> No, I guess not; except here it potentially applies to all packets
>> (things like BAW tracking), and it is *in addition* to TCP.
>
> Yes, here it is a little bit harder, but I was meaning that the bpf program
> has to be very careful when dropping a packet :)

Yeah. What kind of filtering were you thinking you would use this for in
the short term?

-Toke

Re: [RFC 0/5] add XDP support to mt76x2e/mt76x0e drivers

[Toke Høiland-Jørgensen]

Lorenzo Bianconi  writes:

>> >> > This series is intended as a playground to start 
>> >> > experimenting/developing
>> >> > with XDP/eBPF over WiFi and collect ideas/concerns about it.
>> >> > Introduce XDP support to mt76x2e/mt76x0e drivers. Currently supported
>> >> > actions are:
>> >> > - XDP_PASS
>> >> > - XDP_ABORTED
>> >> > - XDP_DROP
>> >> > Introduce ndo_bpf mac80211 callback in order to to load a bpf
>> >> > program into low level driver XDP rx hook.
>> >> > This series has been tested through a simple bpf program (available 
>> >> > here:
>> >> > https://github.com/LorenzoBianconi/bpf-workspace/tree/master/mt76_xdp_stats)
>> >> > used to count frame types received by the device.
>> >> > Possible eBPF use cases could be:
>> >> > - implement new statistics through bpf maps
>> >> > - implement fast packet filtering (e.g in monitor mode)
>> >> > - ...
>> >
>> > Hi Kalle,
>> >
>> >> 
>> >> This is most likely a stupid question, but why do this in the driver and
>> >> not in mac80211 so that all drivers could benefit from it? I guess there
>> >> are reasons for that, I just can't figure that out.
>> 
>> XDP achieves its speedup by running the eBPF program inside the driver
>> NAPI loop, before the kernel even touches the data in any other capacity
>> (and in particular, before it allocates an SKB). Which kinda means the
>> hook needs to be in the driver... Could be a fallback in mac80211,
>> though; although we'd have to figure out how that interacts with Generic
>> XDP.
>> 
>> > This is an early stage implementation, at this point I would collect
>> > other people opinions/concerns about using bpf/xdp directly on 802.11
>> > frames.
>> 
>> Thanks for looking into this!
>
> Hi Toke,
>
>> 
>> I have two concerns with running XDP on 802.11 frames:
>> 
>> 1. It makes it more difficult to add other XDP actions (such as
>>REDIRECT), as the XDP program would then have to make sure that the
>>outer packet headers are removed before, say, redirecting the packet
>>out of an ethernet interface. Also, if we do add redirect, we would
>>be bypassing mac80211 entirely; to what extent would that mess up
>>internal state?
>> 
>
> You are right, my assumption here is the logic/complexity is moved to
> the bpf program that needs to take care of all possible issues that
> can be introduced. More or less it is the same if a bpf program mess
> up with TCP segments on a wired connection, isn't it?

No, I guess not; except here it potentially applies to all packets
(things like BAW tracking), and it is *in addition* to TCP.

>> 2. UI consistency; suddenly, the user needs to know which kind of
>>frames to expect, and XDP program reuse becomes more difficult. This
>>may be unavoidable given the nature of XDP, but some thought needs to
>>go into this. Especially since we wouldn't necessarily be consistent
>>between WiFi drivers (there are fullmac devices that remove 802.11
>>headers before sending up the frame, right?).
>> 
>
> Right, maybe can we have some kind of 'wifi' bpf helpers?

Yeah, I guess we would at least need helpers to update any internal
state in mac80211 (such as BAW), or BPF programs wouldn't even be able
to drop packets without messing things up...

-Toke

Re: [RFC 0/5] add XDP support to mt76x2e/mt76x0e drivers

[Toke Høiland-Jørgensen]

Michał Kazior  writes:

> On Wed, 28 Nov 2018 at 13:39, Toke Høiland-Jørgensen  
> wrote:[...]
>> > This is an early stage implementation, at this point I would collect
>> > other people opinions/concerns about using bpf/xdp directly on 802.11
>> > frames.
>>
>> Thanks for looking into this!
>
> I'm really hyped up with this especially when I think this could be
> maybe offloaded to wlan cpu's for e.g. deep powersaving (allow host
> system to suspend) without relying on blackbox logic you get today, or
> 802.11 -> 802.3 conversion (to offload host cpu).

Yeah, good point!

>> I have two concerns with running XDP on 802.11 frames:
>>
>> 1. It makes it more difficult to add other XDP actions (such as
>>REDIRECT), as the XDP program would then have to make sure that the
>>outer packet headers are removed before, say, redirecting the packet
>>out of an ethernet interface. Also, if we do add redirect, we would
>>be bypassing mac80211 entirely; to what extent would that mess up
>>internal state?
>>
>> 2. UI consistency; suddenly, the user needs to know which kind of
>>frames to expect, and XDP program reuse becomes more difficult. This
>>may be unavoidable given the nature of XDP, but some thought needs to
>>go into this. Especially since we wouldn't necessarily be consistent
>>between WiFi drivers (there are fullmac devices that remove 802.11
>>headers before sending up the frame, right?).
>
> Yep - you can expect 802.3 frames or amsdu subframes (DA+SA+length)
> too. In some cases you could also expect internal events (broadcom if
> I understand their fullmac arch).

Oh boy. I figured it was going to be a can of worms; didn't realise it
was quite that big :/

-Toke

Re: [RFC 0/5] add XDP support to mt76x2e/mt76x0e drivers

[Toke Høiland-Jørgensen]

Lorenzo Bianconi  writes:

>> Lorenzo Bianconi  writes:
>> 
>> > This series is intended as a playground to start experimenting/developing
>> > with XDP/eBPF over WiFi and collect ideas/concerns about it.
>> > Introduce XDP support to mt76x2e/mt76x0e drivers. Currently supported
>> > actions are:
>> > - XDP_PASS
>> > - XDP_ABORTED
>> > - XDP_DROP
>> > Introduce ndo_bpf mac80211 callback in order to to load a bpf
>> > program into low level driver XDP rx hook.
>> > This series has been tested through a simple bpf program (available here:
>> > https://github.com/LorenzoBianconi/bpf-workspace/tree/master/mt76_xdp_stats)
>> > used to count frame types received by the device.
>> > Possible eBPF use cases could be:
>> > - implement new statistics through bpf maps
>> > - implement fast packet filtering (e.g in monitor mode)
>> > - ...
>
> Hi Kalle,
>
>> 
>> This is most likely a stupid question, but why do this in the driver and
>> not in mac80211 so that all drivers could benefit from it? I guess there
>> are reasons for that, I just can't figure that out.

XDP achieves its speedup by running the eBPF program inside the driver
NAPI loop, before the kernel even touches the data in any other capacity
(and in particular, before it allocates an SKB). Which kinda means the
hook needs to be in the driver... Could be a fallback in mac80211,
though; although we'd have to figure out how that interacts with Generic
XDP.

> This is an early stage implementation, at this point I would collect
> other people opinions/concerns about using bpf/xdp directly on 802.11
> frames.

Thanks for looking into this!

I have two concerns with running XDP on 802.11 frames:

1. It makes it more difficult to add other XDP actions (such as
   REDIRECT), as the XDP program would then have to make sure that the
   outer packet headers are removed before, say, redirecting the packet
   out of an ethernet interface. Also, if we do add redirect, we would
   be bypassing mac80211 entirely; to what extent would that mess up
   internal state?

2. UI consistency; suddenly, the user needs to know which kind of
   frames to expect, and XDP program reuse becomes more difficult. This
   may be unavoidable given the nature of XDP, but some thought needs to
   go into this. Especially since we wouldn't necessarily be consistent
   between WiFi drivers (there are fullmac devices that remove 802.11
   headers before sending up the frame, right?).


Adding in Jesper; maybe he has some thoughts on this?

-Toke

Re: [PATCH RFC v5 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-10-12 03:16, Toke Høiland-Jørgensen wrote:
>> 
>> - Just loop with the smaller quantum until one of the stations go into
>>   the positive (what we do now).
>> 
>> - Go through all active stations, find the one that is closest being in
>>   the positive, and add that amount to the quantum. I.e., something
>>   like (assuming no station has positive deficit; if one does, you 
>> don't
>>   want to add anything anyway):
>> 
>>   to_add = -(max(stn.deficit) for stn in active stations)
>>   for stn in active stations:
>> stn.deficit += to_add + stn.weight
>> 
> Toke,
>
> Sorry for the delayed response. I did lot of experiments. Below are my 
> observations.
> Sorry for lengthy reply.
>
> In current model, next_txq() is main routine that serves DRR and
> fairness is enforced by serving only only first txq. Here the first
> node could be either newly initiated traffic or returned node by
> return_txq(). This works perfectly as long as the driver is running
> any RR algo.
>
> Whereas in ath10k, firmware runs its own RR in pull mode and builds
> txq list based on driver's hostq table. In this case it can not be
> simply assumed that firmware always gives fetch request for first node
> of mac80211's txq list. i.e both RR algo could be out of sync.

So I'm wondering why they don't sync; if the hardware is just doing RR
scheduling, eventually it should hit the TXQ that's first in the queue
and keep in sync after that?

How are you testing, and what metrics are you using?

> On an idle condition a single fetch indication can dequeue ~190 msdus
> from each tid of give stn list.

Wow, that sounds pretty bad. Guess we need the airtime queue limits! :)

> diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c 
> b/drivers/net/wireless/ath/ath10k/htt_rx.c
> index 625a4ab37ea0..269ae8311056 100644
> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
> @@ -2352,7 +2352,7 @@ static void ath10k_htt_rx_tx_fetch_ind(struct ath10k 
> *ar, struct sk_buff *skb)
>   num_msdus++;
>   num_bytes += ret;
>   }
> - ieee80211_return_txq(hw, txq);
> + ieee80211_return_txq(hw, txq, true);

I don't like the extra parameter; a similar one was in an earlier
version of my patch set, but I'd prefer that mac80211 just does the
right thing...

Do I understand it correctly that push/pull mode is selected solely by
hardware/firmware versions? Because in that case we could split it into
two feature flags instead...

> @@ -3670,13 +3670,8 @@ bool ieee80211_txq_may_transmit(struct ieee80211_hw 
> *hw,
>   if (sta->airtime[ac].deficit >= 0)
>   goto out;
>  
> - list_for_each_entry(txqi, >active_txqs[ac], schedule_order) {
> - if (!txqi->txq.sta)
> - continue;
> - sta = container_of(txqi->txq.sta, struct sta_info, sta);
> - sta->airtime[ac].deficit +=
> - (IEEE80211_TXQ_MAY_TX_QUANTUM * sta->airtime_weight);
> - }
> + sta->airtime[ac].deficit += sta->airtime_weight;
> + list_move_tail(>schedule_order, >active_txqs[ac]);

I'm wondering whether this actually succeeds in achieving fairness? This
basically allows a TXQ to be plucked from any point in the list, get a
quantum increase and be put back on, no matter the state of other TXQs.

Did you test how well the stations divide their airtime? And if so,
under which conditions?

-Toke

Re: [PATCH RFC v5 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-10-11 03:38, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan  writes:
>> 
>>> Hmm... mine is bit different. txqs are refilled only once for all 
>>> txqs.
>>> It will give more opportunity for non-served txqs. drv_wake_tx_queue
>>> won't be
>>> called from may_tx as the driver anyway will not push packets in
>>> pull-mode.
>> 
>> So, as far as I can tell, this requires the hardware to "keep trying"?
>> I.e., if it just stops scheduling a TXQ after may_transmit() returns
>> false, there is no guarantee that that TXQ will ever get re-awoken
>> unless a new packet arrives for it?
>> 
> That is true and even now ath10k operates the same way in pull mode.
> Not just packet arrival, even napi poll routine tries to pushes the
> packets.

I'm not sure I'm following? At every NAPI poll, the driver tries to push
to *all* TXQs?

> One more thing, fetch indication may pull ~4ms/8ms of packets from
> each tid. This makes deficit too low and so refilling txqs by just
> airtime_weight becomes cumbersome.

Yeah, in general we can't assume that each dequeue uses the same amount
of airtime as the quantum. This is why there's a loop; to fill up
quantum until the first stations gets into the positive.

> In may_transmit, the deficit are incremented by 20 * airtime_weight.
> In future this will be also replaced by station specific quantum. we
> can revisit this once BQL in place. Performance issue is resolved by
> this approach. Do you foresee any issues?

Just using a larger quantum will work as long as all stations send
roughly the same amount of data (airtime) at each transmission. Which is
often the case when you're benchmarking, but not in general. Think of
the size of the quantum as the granularity that the scheduler can
provide fairness at.

What I'd suggest is that instead of increasing the quantum, you do one
of the following:

- Just loop with the smaller quantum until one of the stations go into
  the positive (what we do now).

- Go through all active stations, find the one that is closest being in
  the positive, and add that amount to the quantum. I.e., something
  like (assuming no station has positive deficit; if one does, you don't
  want to add anything anyway):
  
  to_add = -(max(stn.deficit) for stn in active stations)
  for stn in active stations:
stn.deficit += to_add + stn.weight


-Toke

Re: [PATCH RFC v5 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-10-10 04:15, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan  writes:
>> 
>>> On 2018-10-09 05:32, Toke Høiland-Jørgensen wrote:
>>>> This adds airtime accounting and scheduling to the mac80211 TXQ
>>>> scheduler. A new callback, ieee80211_sta_register_airtime(), is added
>>>> that drivers can call to report airtime usage for stations.
>>>> 
>>>> When airtime information is present, mac80211 will schedule TXQs
>>>> (through ieee80211_next_txq()) in a way that enforces airtime 
>>>> fairness
>>>> between active stations. This scheduling works the same way as the
>>>> ath9k
>>>> in-driver airtime fairness scheduling. If no airtime usage is 
>>>> reported
>>>> by the driver, the scheduler will default to round-robin scheduling.
>>>> 
>>>> For drivers that don't control TXQ scheduling in software, a new API
>>>> function, ieee80211_txq_may_transmit(), is added which the driver can
>>>> use
>>>> to check if the TXQ is eligible for transmission, or should be
>>>> throttled to
>>>> enforce fairness. Calls to this function must also be enclosed in
>>>> ieee80211_txq_schedule_{start,end}() calls to ensure proper locking.
>>>> TXQs
>>>> that are throttled by ieee802111_txq_may_transmit() will be woken up
>>>> again
>>>> by a check added to the ieee80211_wake_txqs() tasklet.
>>>> 
>>> 
>>> Toke,
>>> 
>>> I am observing soft lockup issues again with this new series while
>>> running traffic with 50 clients. I am continuing testing with earlier
>>> series along with snippet I shared.
>> 
>> Are these new lockups (that was not in your patched previous version),
>> or did I just not get all your lock-related fixes incorporated?
>> 
>>> When driver operates in pull-mode, throttled txqs are marked and
>>> refilled in airtime_tasklet. This is causing major throughput drops
>>> and packet loss and I am suspecting the latency in replenishing
>>> deficit. Whereas in push-mode or in ath9k model, refill happens
>>> quicker at every packet indication as well as tx completion.
>> 
>> Yeah, the tasklet shouldn't be the main source of deficit replenishing.
>> Can see why that would give bad performance :)
>> 
>>> I am planning to get rid of tasklet completely as it is only meant for
>>> pull-mode. It would be better to refill in may_transmit() itself.
>> 
>> Hmm, right. So the way to do this correctly (from a fairness point of
>> view) would be something like this (in max_tx()):
>> 
>> if (this_txq.stn.deficit > 0)
>>   return true;
>> 
>> else if (any queued TXQ currently have positive deficit)
>>   return false; /* other TXQ should try may_tx() later and get 
>> permission */
>> 
>> else /* all deficits < 0 */
>>   return replenish_deficits(this_txq);
>> 
>> And replenish_deficits() would be something like:
>> 
>> replenish_deficits(this_txq) {
>> repeat:
>>   for (txq in queued txqs) {
>> txq.stn.deficit += stn.weight;
>> if (txq.stn.deficit > 0 && !wake_txq)
>>   wake_txq = txq;
>>   }
>>   if not wake_txq:
>> goto repeat;
>> 
>>   if (this_txq.stn.deficit > 0)
>> return true;
>>   else
>> drv_wake_tx_queue(wake_txq);
>> }
>> 
>> The wake_tx_queue call may have to be delegated to a tasklet still, to
>> avoid the infinite recursion problem I mentioned earlier. But the
>> tasklet could be made simpler and wouldn't have to be called so 
>> often...
>> 
>> Does the above make sense?
>> 
> Hmm... mine is bit different. txqs are refilled only once for all txqs.
> It will give more opportunity for non-served txqs. drv_wake_tx_queue 
> won't be
> called from may_tx as the driver anyway will not push packets in 
> pull-mode.

So, as far as I can tell, this requires the hardware to "keep trying"?
I.e., if it just stops scheduling a TXQ after may_transmit() returns
false, there is no guarantee that that TXQ will ever get re-awoken
unless a new packet arrives for it?

-Toke

Re: [PATCH RFC v5 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-10-09 05:32, Toke Høiland-Jørgensen wrote:
>> This adds airtime accounting and scheduling to the mac80211 TXQ
>> scheduler. A new callback, ieee80211_sta_register_airtime(), is added
>> that drivers can call to report airtime usage for stations.
>> 
>> When airtime information is present, mac80211 will schedule TXQs
>> (through ieee80211_next_txq()) in a way that enforces airtime fairness
>> between active stations. This scheduling works the same way as the 
>> ath9k
>> in-driver airtime fairness scheduling. If no airtime usage is reported
>> by the driver, the scheduler will default to round-robin scheduling.
>> 
>> For drivers that don't control TXQ scheduling in software, a new API
>> function, ieee80211_txq_may_transmit(), is added which the driver can 
>> use
>> to check if the TXQ is eligible for transmission, or should be 
>> throttled to
>> enforce fairness. Calls to this function must also be enclosed in
>> ieee80211_txq_schedule_{start,end}() calls to ensure proper locking. 
>> TXQs
>> that are throttled by ieee802111_txq_may_transmit() will be woken up 
>> again
>> by a check added to the ieee80211_wake_txqs() tasklet.
>> 
>
> Toke,
>
> I am observing soft lockup issues again with this new series while
> running traffic with 50 clients. I am continuing testing with earlier
> series along with snippet I shared.

Are these new lockups (that was not in your patched previous version),
or did I just not get all your lock-related fixes incorporated?

> When driver operates in pull-mode, throttled txqs are marked and
> refilled in airtime_tasklet. This is causing major throughput drops
> and packet loss and I am suspecting the latency in replenishing
> deficit. Whereas in push-mode or in ath9k model, refill happens
> quicker at every packet indication as well as tx completion.

Yeah, the tasklet shouldn't be the main source of deficit replenishing.
Can see why that would give bad performance :)

> I am planning to get rid of tasklet completely as it is only meant for
> pull-mode. It would be better to refill in may_transmit() itself.

Hmm, right. So the way to do this correctly (from a fairness point of
view) would be something like this (in max_tx()):

if (this_txq.stn.deficit > 0)
  return true;

else if (any queued TXQ currently have positive deficit)
  return false; /* other TXQ should try may_tx() later and get permission */

else /* all deficits < 0 */
  return replenish_deficits(this_txq);

And replenish_deficits() would be something like:

replenish_deficits(this_txq) {
repeat:
  for (txq in queued txqs) {
txq.stn.deficit += stn.weight;
if (txq.stn.deficit > 0 && !wake_txq)
  wake_txq = txq;
  }
  if not wake_txq:
goto repeat;

  if (this_txq.stn.deficit > 0)
return true;
  else
drv_wake_tx_queue(wake_txq);
}

The wake_tx_queue call may have to be delegated to a tasklet still, to
avoid the infinite recursion problem I mentioned earlier. But the
tasklet could be made simpler and wouldn't have to be called so often...

Does the above make sense?

-Toke

[PATCH RFC v5 0/4] Move TXQ scheduling and airtime fairness into mac80211

[Toke Høiland-Jørgensen]

Another updated version, addressing a few issues with the previous
version.

- Moved the airtime deficit queue wakeup code to its own tasklet to
  lower overhead.

- Change the tasklet to just wake a single queue on each invocation,
  relying to TX completion to continue transmissions.

- Don't try to re-schedule TXQs of stations that are being removed.

- A few cleanups and fixes.

The one thing I didn't change was to add another callback that the
driver can use to trigger the tasklet. Since it's now in its own
tasklet, hopefully the overhead is low enough that we can just call it
on every end_schedule(); and I'd rather not complicate the driver API
further.

Thanks to Rajkumar for testing the previous version. I thought I'd have
time to test this version myself and was planning to send as a non-RFC
PATCH after that, but that time didn't materialise. So I thought it was
better to send another RFC version instead of everyone having to suffer
from my tardiness :)

-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 |  113 +
 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 |   14 ++
 net/mac80211/main.c|   11 +
 net/mac80211/sta_info.c|   54 ++
 net/mac80211/sta_info.h|   13 +
 net/mac80211/status.c  |6 +
 net/mac80211/tx.c  |  137 
 net/mac80211/util.c|   75 +
 net/wireless/nl80211.c |   29 +++
 23 files changed, 603 insertions(+), 277 deletions(-)

[PATCH RFC v5 2/4] cfg80211: Add airtime statistics and settings

[Toke Høiland-Jørgensen]

This adds TX airtime statistics to the cfg80211 station dump (to go along
with the RX info already present), and adds a new parameter to set the
airtime weight of each station. The latter allows userspace to implement
policies for different stations by varying their weights.

Signed-off-by: Toke Høiland-Jørgensen 
---
 include/net/cfg80211.h   |   10 +-
 include/uapi/linux/nl80211.h |   15 +++
 net/wireless/nl80211.c   |   29 +
 3 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 9f3ed79c39d7..91d6fc63 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -990,6 +990,7 @@ enum station_parameters_apply_mask {
  * @support_p2p_ps: information if station supports P2P PS mechanism
  * @he_capa: HE capabilities of station
  * @he_capa_len: the length of the HE capabilities
+ * @airtime_weight: airtime scheduler weight for this station
  */
 struct station_parameters {
const u8 *supported_rates;
@@ -1019,6 +1020,7 @@ struct station_parameters {
int support_p2p_ps;
const struct ieee80211_he_cap_elem *he_capa;
u8 he_capa_len;
+   u16 airtime_weight;
 };
 
 /**
@@ -1286,6 +1288,8 @@ struct cfg80211_tid_stats {
  * @rx_beacon_signal_avg: signal strength average (in dBm) for beacons received
  * from this peer
  * @rx_duration: aggregate PPDU duration(usecs) for all the frames from a peer
+ * @tx_duration: aggregate PPDU duration(usecs) for all the frames to a peer
+ * @airtime_weight: current airtime scheduling weight
  * @pertid: per-TID statistics, see  cfg80211_tid_stats, using the last
  * (IEEE80211_NUM_TIDS) index for MSDUs not encapsulated in QoS-MPDUs.
  * Note that this doesn't use the @filled bit, but is used if non-NULL.
@@ -1332,10 +1336,12 @@ struct station_info {
 
u32 expected_throughput;
 
-   u64 rx_beacon;
+   u64 tx_duration;
u64 rx_duration;
+   u64 rx_beacon;
u8 rx_beacon_signal_avg;
struct cfg80211_tid_stats *pertid;
+   u16 airtime_weight;
s8 ack_signal;
s8 avg_ack_signal;
 };
@@ -2363,6 +2369,8 @@ enum wiphy_params_flags {
WIPHY_PARAM_TXQ_QUANTUM = 1 << 8,
 };
 
+#define IEEE80211_DEFAULT_AIRTIME_WEIGHT   256
+
 /**
  * struct cfg80211_pmksa - PMK Security Association
  *
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index cfc94178d608..3664bdc7c8c1 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2241,6 +2241,9 @@ enum nl80211_commands {
  * association request when used with NL80211_CMD_NEW_STATION). Can be set
  * only if %NL80211_STA_FLAG_WME is set.
  *
+ * @NL80211_ATTR_AIRTIME_WEIGHT: Station's weight when scheduled by the airtime
+ *  scheduler.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2682,6 +2685,8 @@ enum nl80211_attrs {
 
NL80211_ATTR_HE_CAPABILITY,
 
+   NL80211_ATTR_AIRTIME_WEIGHT,
+
/* add attributes here, update the policy in nl80211.c */
 
__NL80211_ATTR_AFTER_LAST,
@@ -3051,6 +3056,9 @@ enum nl80211_sta_bss_param {
  * @NL80211_STA_INFO_PAD: attribute used for padding for 64-bit alignment
  * @NL80211_STA_INFO_ACK_SIGNAL: signal strength of the last ACK frame(u8, dBm)
  * @NL80211_STA_INFO_ACK_SIGNAL_AVG: avg signal strength of ACK frames (s8, 
dBm)
+ * @NL80211_STA_INFO_TX_DURATION: aggregate PPDU duration for all frames
+ * sent to the station (u64, usec)
+ * @NL80211_STA_INFO_AIRTIME_WEIGHT: current airtime weight for station (u16)
  * @__NL80211_STA_INFO_AFTER_LAST: internal
  * @NL80211_STA_INFO_MAX: highest possible station info attribute
  */
@@ -3091,6 +3099,8 @@ enum nl80211_sta_info {
NL80211_STA_INFO_PAD,
NL80211_STA_INFO_ACK_SIGNAL,
NL80211_STA_INFO_ACK_SIGNAL_AVG,
+   NL80211_STA_INFO_TX_DURATION,
+   NL80211_STA_INFO_AIRTIME_WEIGHT,
 
/* keep last */
__NL80211_STA_INFO_AFTER_LAST,
@@ -5231,6 +5241,10 @@ enum nl80211_feature_flags {
  *  if this flag is not set. Ignoring this can leak clear text packets 
and/or
  *  freeze the connection.
  *
+ * @NL80211_EXT_FEATURE_AIRTIME_FAIRNESS: Driver supports getting airtime
+ *  fairness for transmitted packets and has enabled airtime fairness
+ *  scheduling.
+ *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
  */
@@ -5269,6 +5283,7 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_SCAN_RANDOM_SN,
NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT,
NL80211_EXT_FEATURE_CAN_REPLACE_PTK0,
+   NL80211_EXT_FEATURE_AIRTIME_FAIRNESS,
 
/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
diff --git a/net/wi

[PATCH RFC v5 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

This adds an API to mac80211 to handle scheduling of TXQs. The interface
between driver and mac80211 for TXQ handling is changed by adding two new
functions: ieee80211_next_txq(), which will return the next TXQ to schedule
in the current round-robin rotation, and ieee80211_return_txq(), which the
driver uses to indicate that it has finished scheduling a TXQ (which will
then be put back in the scheduling rotation if it isn't empty).

The driver must call ieee80211_txq_schedule_start() at the start of each
scheduling session, and ieee80211_txq_schedule_end() at the end. The API
then guarantees that the same TXQ is not returned twice in the same
session (so a driver can loop on ieee80211_next_txq() without worrying
about breaking the loop.

Usage of the new API is optional, so drivers can be ported one at a time.
In this patch, the actual scheduling performed by mac80211 is simple
round-robin, but a subsequent commit adds airtime fairness awareness to the
scheduler.

Signed-off-by: Toke Høiland-Jørgensen 
---
 include/net/mac80211.h |   61 +---
 net/mac80211/agg-tx.c  |2 +
 net/mac80211/driver-ops.h  |9 ++
 net/mac80211/ieee80211_i.h |9 ++
 net/mac80211/main.c|5 
 net/mac80211/sta_info.c|2 +
 net/mac80211/tx.c  |   59 ++-
 7 files changed, 140 insertions(+), 7 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index c4fadbafbf21..469e88a32f94 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -108,9 +108,15 @@
  * The driver is expected to initialize its private per-queue data for stations
  * and interfaces in the .add_interface and .sta_add ops.
  *
- * The driver can't access the queue directly. To dequeue a frame, it calls
- * ieee80211_tx_dequeue(). Whenever mac80211 adds a new frame to a queue, it
- * calls the .wake_tx_queue driver op.
+ * The driver can't access the queue directly. To dequeue a frame from a
+ * txq, it calls ieee80211_tx_dequeue(). Whenever mac80211 adds a new frame to 
a
+ * queue, it calls the .wake_tx_queue driver op.
+ *
+ * Drivers can optionally delegate responsibility for scheduling queues to
+ * mac80211, to take advantage of airtime fairness accounting. In this case, to
+ * obtain the next queue to pull frames from, the driver calls
+ * ieee80211_next_txq(). The driver is then expected to return the txq using
+ * ieee80211_return_txq().
  *
  * For AP powersave TIM handling, the driver only needs to indicate if it has
  * buffered packets in the driver specific data structures by calling
@@ -6045,13 +6051,60 @@ void ieee80211_unreserve_tid(struct ieee80211_sta *sta, 
u8 tid);
  * ieee80211_tx_dequeue - dequeue a packet from a software tx queue
  *
  * @hw: pointer as obtained from ieee80211_alloc_hw()
- * @txq: pointer obtained from station or virtual interface
+ * @txq: pointer obtained from station or virtual interface, or from
+ *   ieee80211_next_txq()
  *
  * Returns the skb if successful, %NULL if no frame was available.
  */
 struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 struct ieee80211_txq *txq);
 
+/**
+ * ieee80211_next_txq - get next tx queue to pull packets from
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @ac: AC number to return packets from.
+ *
+ * Should only be called between calls to ieee80211_txq_schedule_start()
+ * and ieee80211_txq_schedule_end().
+ * Returns the next txq if successful, %NULL if no queue is eligible. If a txq
+ * is returned, it should be returned with ieee80211_return_txq() after the
+ * driver has finished scheduling it.
+ */
+struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac);
+
+/**
+ * ieee80211_return_txq - return a TXQ previously acquired by 
ieee80211_next_txq()
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ * Should only be called between calls to ieee80211_txq_schedule_start()
+ * and ieee80211_txq_schedule_end().
+ */
+void ieee80211_return_txq(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
+
+/**
+ * ieee80211_txq_schedule_start - acquire locks for safe scheduling of an AC
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @ac: AC number to acquire locks for
+ *
+ * Acquire locks needed to schedule TXQs from the given AC. Should be called
+ * before ieee80211_next_txq() or ieee80211_return_txq().
+ */
+void ieee80211_txq_schedule_start(struct ieee80211_hw *hw, u8 ac);
+
+/**
+ * ieee80211_txq_schedule_end - release locks for safe scheduling of an AC
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @ac: AC number to acquire locks for
+ *
+ * Release locks previously acquired by ieee80211_txq_schedule_end().
+ */
+void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac);
+
 /**
  * ieee80211_txq_get_depth - get pending frame/byte count

[PATCH RFC v5 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

This adds airtime accounting and scheduling to the mac80211 TXQ
scheduler. A new callback, ieee80211_sta_register_airtime(), is added
that drivers can call to report airtime usage for stations.

When airtime information is present, mac80211 will schedule TXQs
(through ieee80211_next_txq()) in a way that enforces airtime fairness
between active stations. This scheduling works the same way as the ath9k
in-driver airtime fairness scheduling. If no airtime usage is reported
by the driver, the scheduler will default to round-robin scheduling.

For drivers that don't control TXQ scheduling in software, a new API
function, ieee80211_txq_may_transmit(), is added which the driver can use
to check if the TXQ is eligible for transmission, or should be throttled to
enforce fairness. Calls to this function must also be enclosed in
ieee80211_txq_schedule_{start,end}() calls to ensure proper locking. TXQs
that are throttled by ieee802111_txq_may_transmit() will be woken up again
by a check added to the ieee80211_wake_txqs() tasklet.

Signed-off-by: Toke Høiland-Jørgensen 
---
 include/net/mac80211.h |   52 +++
 net/mac80211/cfg.c |3 ++
 net/mac80211/debugfs.c |3 ++
 net/mac80211/debugfs_sta.c |   51 +-
 net/mac80211/ieee80211_i.h |5 +++
 net/mac80211/main.c|6 +++
 net/mac80211/sta_info.c|   52 +--
 net/mac80211/sta_info.h|   13 +++
 net/mac80211/status.c  |6 +++
 net/mac80211/tx.c  |   86 ++--
 net/mac80211/util.c|   75 ++
 11 files changed, 341 insertions(+), 11 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 469e88a32f94..fa10420a53ff 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5349,6 +5349,34 @@ void ieee80211_sta_eosp(struct ieee80211_sta *pubsta);
  */
 void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
 
+/**
+ * ieee80211_sta_register_airtime - register airtime usage for a sta/tid
+ *
+ * Register airtime usage for a given sta on a given tid. The driver can call
+ * this function to notify mac80211 that a station used a certain amount of
+ * airtime. This information will be used by the TXQ scheduler to schedule
+ * stations in a way that ensures airtime fairness.
+ *
+ * The reported airtime should as a minimum include all time that is spent
+ * transmitting to the remote station, including overhead and padding, but not
+ * including time spent waiting for a TXOP. If the time is not reported by the
+ * hardware it can in some cases be calculated from the rate and known frame
+ * composition. When possible, the time should include any failed transmission
+ * attempts.
+ *
+ * The driver can either call this function synchronously for every packet or
+ * aggregate, or asynchronously as airtime usage information becomes available.
+ * TX and RX airtime can be reported together, or separately by setting one of
+ * them to 0.
+ *
+ * @pubsta: the station
+ * @tid: the TID to register airtime for
+ * @tx_airtime: airtime used during TX (in usec)
+ * @rx_airtime: airtime used during RX (in usec)
+ */
+void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
+   u32 tx_airtime, u32 rx_airtime);
+
 /**
  * ieee80211_iter_keys - iterate keys programmed into the device
  * @hw: pointer obtained from ieee80211_alloc_hw()
@@ -6105,6 +6133,30 @@ void ieee80211_txq_schedule_start(struct ieee80211_hw 
*hw, u8 ac);
  */
 void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac);
 
+/**
+ * ieee80211_txq_may_transmit - check whether TXQ is allowed to transmit
+ *
+ * This function is used to check whether given txq is allowed to transmit by
+ * the airtime scheduler, and can be used by drivers to access the airtime
+ * fairness accounting without going using the scheduling order enfored by
+ * next_txq().
+ *
+ * Returns %true if the airtime scheduler thinks the TXQ should be allowed to
+ * transmit, and %false if it should be throttled. This function can also have
+ * the side effect of rotating the TXQ in the scheduler rotation, which will
+ * eventually bring the deficit to positive and allow the station to transmit
+ * again. If a TXQ is throttled, it will be marked and eventually woken up 
again
+ * through drv_wake_tx_queue().
+ *
+ * If this function returns %true, the driver is expected to schedule packets
+ * for transmission, and then return the TXQ through ieee80211_return_txq().
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ */
+bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
+   struct ieee80211_txq *txq);
+
 /**
  * ieee80211_txq_get_depth - get pending frame/byte count of given txq
  *
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index

[PATCH RFC v5 4/4] ath9k: Switch to mac80211 TXQ scheduling and airtime APIs

[Toke Høiland-Jørgensen]

This moves the ath9k driver to use the mac80211 TXQ scheduling and
airtime accounting APIs, removing the corresponding state tracking
inside the driver.

Signed-off-by: Toke Høiland-Jørgensen 
---
 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 
 7 files changed, 73 insertions(+), 262 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h 
b/drivers/net/wireless/ath/ath9k/ath9k.h
index 21ba20981a80..90b56766dab1 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -112,8 +112,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct 
ath_descdma *dd,
 #define ATH_TXFIFO_DEPTH   8
 #define ATH_TX_ERROR   0x01
 
-#define ATH_AIRTIME_QUANTUM300 /* usec */
-
 /* Stop tx traffic 1ms before the GO goes away */
 #define ATH_P2P_PS_STOP_TIME   1000
 
@@ -246,10 +244,8 @@ struct ath_atx_tid {
s8 bar_index;
bool active;
bool clear_ps_filter;
-   bool has_queued;
 };
 
-void __ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid);
 void ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid);
 
 struct ath_node {
@@ -263,12 +259,9 @@ struct ath_node {
 
bool sleeping;
bool no_ps_filter;
-   s64 airtime_deficit[IEEE80211_NUM_ACS];
-   u32 airtime_rx_start;
 
 #ifdef CONFIG_ATH9K_STATION_STATISTICS
struct ath_rx_rate_stats rx_rate_stats;
-   struct ath_airtime_stats airtime_stats;
 #endif
u8 key_idx[4];
 
@@ -986,11 +979,6 @@ void ath_ant_comb_scan(struct ath_softc *sc, struct 
ath_rx_status *rs);
 
 #define ATH9K_NUM_CHANCTX  2 /* supports 2 operating channels */
 
-#define AIRTIME_USE_TX BIT(0)
-#define AIRTIME_USE_RX BIT(1)
-#define AIRTIME_USE_NEW_QUEUES BIT(2)
-#define AIRTIME_ACTIVE(flags) (!!(flags & (AIRTIME_USE_TX|AIRTIME_USE_RX)))
-
 struct ath_softc {
struct ieee80211_hw *hw;
struct device *dev;
@@ -1034,8 +1022,6 @@ struct ath_softc {
short nbcnvifs;
unsigned long ps_usecount;
 
-   u16 airtime_flags; /* AIRTIME_* */
-
struct ath_rx rx;
struct ath_tx tx;
struct ath_beacon beacon;
diff --git a/drivers/net/wireless/ath/ath9k/debug.c 
b/drivers/net/wireless/ath/ath9k/debug.c
index 0a6eb8a8c1ed..f928d3a07caa 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -1456,9 +1456,6 @@ int ath9k_init_debug(struct ath_hw *ah)
 #endif
debugfs_create_file("tpc", 0600, sc->debug.debugfs_phy, sc, _tpc);
 
-   debugfs_create_u16("airtime_flags", 0600,
-  sc->debug.debugfs_phy, >airtime_flags);
-
debugfs_create_file("nf_override", 0600,
sc->debug.debugfs_phy, sc, _nf_override);
 
diff --git a/drivers/net/wireless/ath/ath9k/debug.h 
b/drivers/net/wireless/ath/ath9k/debug.h
index 249f8141cd00..559d9628f280 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -319,20 +319,12 @@ ath9k_debug_sync_cause(struct ath_softc *sc, u32 
sync_cause)
 void ath_debug_rate_stats(struct ath_softc *sc,
  struct ath_rx_status *rs,
  struct sk_buff *skb);
-void ath_debug_airtime(struct ath_softc *sc,
-  struct ath_node *an,
-  u32 rx, u32 tx);
 #else
 static inline void ath_debug_rate_stats(struct ath_softc *sc,
struct ath_rx_status *rs,
struct sk_buff *skb)
 {
 }
-static inline void ath_debug_airtime(struct ath_softc *sc,
- struct ath_node *an,
- u32 rx, u32 tx)
-{
-}
 #endif /* CONFIG_ATH9K_STATION_STATISTICS */
 
 #endif /* DEBUG_H */
diff --git a/drivers/net/wireless/ath/ath9k/debug_sta.c 
b/drivers/net/wireless/ath/ath9k/debug_sta.c
index a6f45f1bb5bb..bb6f3250aa30 100644
--- a/drivers/net/wireless/ath/ath9k/debug_sta.c
+++ b/drivers/net/wireless/ath/ath9k/debug_sta.c
@@ -242,59 +242,6 @@ static const struct file_operations fops_node_recv = {
.llseek = default_llseek,
 };
 
-void ath_debug_airtime(struct ath_softc *sc,
-   struct ath_node *an,
-   u32 rx,
-   u32 tx)
-{
-   struct ath_airtime_stats *astats = >airtime_stats;
-
-   astats->rx_airtime += rx;
-   astats->tx_airtime += tx;
-}
-
-static ssize_t read_airtime(struct file *file, char __user *user_buf,
-   size_t count, loff_t *ppos)
-{
-   struct ath_node *an = file-

Re: Tool to debug wifi pkt sniffs?

[Toke Høiland-Jørgensen]

Ben Greear  writes:

> Hello,
>
> I often find myself wanting to figure out what equipment is to blame (and why)
> in a wifi environment.
>
> I am thinking writing a tool that would parse a pcap file and look at frames
> in enough detail to flag block-ack bugs, rate-ctrl bugs, guess at the 
> sniffer's
> capture ability, etc.
>
> Does anyone have anything already written that they would like to share, or 
> know
> of projects that might already do some of this?

Not sure if this fits your criteria, but Sven's tool to create airtime
charts from packet sniffing data immediately came to mind:

https://github.com/cloudtrax/airtime-pie-chart

-Toke

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-10-02 16:07, Rajkumar Manoharan wrote:
>> On 2018-10-02 12:00, Toke Høiland-Jørgensen wrote:
>>> Rajkumar Manoharan  writes:
>>>> I noticed a race condition b/w sta cleanup and kick_airtime tasklet.
>>>>>> How do you plan to exit kick_airtime gracefully during sta_cleanup?
>>>>> 
>>>>> Ah, right, there's a lot of stuff going on before we get to 
>>>>> purge_txq.
>>>>> Hmm, I guess we should either make sure we remove the station from
>>>>> active_txqs earlier in the sta cleanup process, or maybe it'd enough 
>>>>> to
>>>>> just check the removed flag in the tasklet?
>>>>> 
>>>>> Does the below patch fix the issue?
>>>>> 
>>>> 
>>>> No. Attaching backtrace. Any clue?
>>> 
>>> Ah, that's my bad. Just having a 'continue' there can make the 
>>> function
>>> loop forever. Oops. Try something like this instead?
>>> 
>> 
>> But 'continue' also used in other places. Will give a try but I think 
>> that
>> calling drv_wake_tx_queue within iteration is dangerous as it alters
>> the list. no?
>> 
> How about below change? Just schedule first txq and remaining will be
> scheduled later by driver upon tx-compl.

Your mail client seems to be mangling the patch somewhat, but I think I
see what your intention is. And yeah, just waking a single TXQ and
letting TX-completion do the rest is a good idea; will fold that into
the next version :)

-Toke

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-10-02 01:22, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan  writes:
>> 
>>>> Great! I'll fold in the rest, test it with ath9k and submit as a 
>>>> proper
>>>> patch :)
>>>> 
>>> Toke,
>>> 
>>> I noticed a race condition b/w sta cleanup and kick_airtime tasklet.
>>> How do you plan to exit kick_airtime gracefully during sta_cleanup?
>> 
>> Ah, right, there's a lot of stuff going on before we get to purge_txq.
>> Hmm, I guess we should either make sure we remove the station from
>> active_txqs earlier in the sta cleanup process, or maybe it'd enough to
>> just check the removed flag in the tasklet?
>> 
>> Does the below patch fix the issue?
>> 
>
> No. Attaching backtrace. Any clue?

Ah, that's my bad. Just having a 'continue' there can make the function
loop forever. Oops. Try something like this instead?

-Toke


diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index eb77cf588d69..b30a4fac1d60 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -258,6 +258,9 @@ static void __ieee80211_kick_airtime(struct ieee80211_local 
*local, int ac)
 
sta = container_of(txqi->txq.sta, struct sta_info, sta);
 
+   if (sta->removed)
+   goto out_reschedule;
+
if (sta->airtime[ac].deficit >= 0) {
seen_eligible = true;
 
@@ -288,7 +291,13 @@ static void __ieee80211_kick_airtime(struct 
ieee80211_local *local, int ac)
}
  out:
rcu_read_unlock();
spin_unlock_bh(>active_txq_lock[ac]);
+   return;
+
+ out_reschedule:
+   rcu_read_unlock();
+   spin_unlock_bh(>active_txq_lock[ac]);
+   tasklet_schedule(>airtime_tasklet);
 }
 
 void ieee80211_kick_airtime(unsigned long data)

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

>> Great! I'll fold in the rest, test it with ath9k and submit as a proper 
>> patch :)
>> 
> Toke,
>
> I noticed a race condition b/w sta cleanup and kick_airtime tasklet.
> How do you plan to exit kick_airtime gracefully during sta_cleanup?

Ah, right, there's a lot of stuff going on before we get to purge_txq.
Hmm, I guess we should either make sure we remove the station from
active_txqs earlier in the sta cleanup process, or maybe it'd enough to
just check the removed flag in the tasklet?

Does the below patch fix the issue?

-Toke


diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 9c889da48ef0..8fa3c09d041c 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -258,6 +258,9 @@ static void __ieee80211_kick_airtime(struct ieee80211_local 
*local, int ac)
 
sta = container_of(txqi->txq.sta, struct sta_info, sta);
 
+   if (sta->removed)
+   continue;
+
if (sta->airtime[ac].deficit >= 0) {
seen_eligible = true;
 

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

On 28 September 2018 12:47:51 CEST, Rajkumar Manoharan 
 wrote:
>On 2018-09-28 03:35, Jonathan Morton wrote:
>>> On 28 Sep, 2018, at 1:19 pm, Rajkumar Manoharan 
>>>  wrote:
>>> 
 This is going to break fairness; we only want to increase deficits 
 when
 all stations' deficits are negative. Hence the two loops. Did you
>see
 any problems with those specifically?
>>> 
>>> No. I didn't see any issue but the loop won't exit until one txq 
>>> becomes positive.
>>> Till then the lock won't be released. Hence I converged into single 
>>> iteration.
>> 
>> The problem is that the fairness behaviour is changed when there are
>> already positive txqs, but the first one happens to not be positive.
>> That's why there were two loops.
>> 
>Yeah.. Understood. we can ignore the cleanup change.

Great! I'll fold in the rest, test it with ath9k and submit as a proper patch :)

-Toke

Re: [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-09-28 00:51, Toke Høiland-Jørgensen wrote:
>> On 28 September 2018 07:29:03 CEST, Rajkumar Manoharan
>>  wrote:
>>> On 2018-09-26 17:09, Rajkumar Manoharan wrote:
>>>> On 2018-09-26 02:22, Toke Høiland-Jørgensen wrote:
>>>>> Rajkumar Manoharan  writes:
>>> 
>>>> :( Yeah... I got confused with attached soft lockup in ARM platform.
>>>> 
>>> Toke,
>>> 
>>> Cause for the soft lockup exposed in multi client scenario is due to
>>> mixed order of fq_lock and active_txqs_lock. In wake_tx_queue or
>>> push_pending
>>> case, driver acquires active_txq_lock first by schedule_start and
>>> followed by
>>> fq_lock in tx_dequeue. The same order should be maintained in sta
>>> cleanup.
>>> Below change fixed the issue.
>> 
>> Ah, great find! I'll fold this into the next version, thanks!
>> 
>
> One more thing. As I mentioned earlier, scheduling wake_txqs_tasklet
> is heavy load and causing random rcu stall issue. Hence I added
> another API to schedule throttled txqs once for all. Also I did
> a cleanup in kick_airtime by traversing list only once. With these
> changes I don't see rcu stall issue. Please review and fold them as 
> well.
>
> -Rajkumar
>
>
> single_iter - clean up kick_airtime
> sched_throttle - new API and separate tasklet for throttled txqs
> diff --git a/net/mac80211/util.c b/net/mac80211/util.c
> index 404c5e82e4ca..023bc81bd4a0 100644
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -242,13 +242,11 @@ EXPORT_SYMBOL(ieee80211_ctstoself_duration);
>  
>  static void __ieee80211_kick_airtime(struct ieee80211_local *local, int ac)
>  {
> - bool seen_eligible = false;
>   struct txq_info *txqi;
>   struct sta_info *sta;
>  
>   spin_lock_bh(>active_txq_lock[ac]);
>  
> - begin:
>   if (list_empty(>active_txqs[ac]))
>   goto out;
>  
> @@ -258,12 +256,12 @@ static void __ieee80211_kick_airtime(struct 
> ieee80211_local *local, int ac)
>  
>   sta = container_of(txqi->txq.sta, struct sta_info, sta);
>  
> - if (sta->airtime[ac].deficit >= 0) {
> - seen_eligible = true;
> -
> - if (!test_and_clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE,
> - >flags))
> + if (test_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, >flags)) {
> + clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, >flags);
> + if (sta->airtime[ac].deficit < 0) {
> + sta->airtime[ac].deficit += sta->airtime_weight;
>   continue;
> + }

This is going to break fairness; we only want to increase deficits when
all stations' deficits are negative. Hence the two loops. Did you see
any problems with those specifically?

-Toke

Re: [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-09-16 10:42, Toke Høiland-Jørgensen wrote:
>> This adds airtime accounting and scheduling to the mac80211 TXQ
>> scheduler. A new callback, ieee80211_sta_register_airtime(), is added
>> that drivers can call to report airtime usage for stations.
>> 
>> +bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
>> +struct ieee80211_txq *txq)
>> +{
>
> [...]
>
>> +if (ret) {
>> +clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, >flags);
>> +list_del_init(>schedule_order);
>> +} else
>> +set_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, >flags);
>> +
>> 
> This looks wrong to me. txqi->flags are protected by fq->lock but here
> it is by active_txq_lock. no?

Both set_bit() and clear_bit() are atomic operations, so they don't need
separate locking. See Documentation/atomic_bitops.txt

>> @@ -3677,6 +3751,7 @@ void ieee80211_txq_schedule_end(struct
>> ieee80211_hw *hw, u8 ac)
>>  struct ieee80211_local *local = hw_to_local(hw);
>> 
>>  spin_unlock_bh(>active_txq_lock[ac]);
>> +tasklet_schedule(>wake_txqs_tasklet);
>>  }
>> 
> It is an overload to schedule wake_txqs_tasklet for each txq unlock.
> Instead I would prefer to call __ieee80211_kick_airtime from 
> schedule_end.
> Thoughts?

Yeah, I realise scheduling the whole wake_txqs_tasklet is maybe a bit
heavy-handed here. However just calling into __ieee80211_kick_airtime()
means we'll end up recursing back to the same place, which is not good
either (we could in theory flip-flop between two queues until we run out
of stack space).

My "backup plan" if the wake_txqs_tasklet turns out to be too heavy was
to define a new tasklet just for this use; but wanted to see if this
actually turned out to be a problem first...

-Toke

Re: [RFC v2] mac80211: budget outstanding airtime for transmission

[Toke Høiland-Jørgensen]

Felix Fietkau  writes:

> On 2018-09-20 21:05, Rajkumar Manoharan wrote:
>> Per frame airtime estimation could be used to track outstanding airtime
>> of each txq and can be used to throttle ieee80211_tx_dequeue(). This
>> mechanism on its own will get us the queue limiting and latency
>> reduction goodness for firmwares with deep queues. And for that it can
>> be completely independent of the airtime fairness scheduler, which can
>> use the after-tx-compl airtime information to presumably get more
>> accurate fairness which includes retransmissions etc.
> What about packets that get dequeued from the txq, but get freed by
> ieee80211_free_txskb?
> I think this may be a bit fragile, since if for any reason accounting
> isn't perfect here, tx queues might stall.

Yeah, I worry about that as well. I guess we basically have three
avenues for fixing this?

1. Make sure packets are not freed after dequeue (probably not
feasible).

2. Add a mechanism to get queues unstuck if accounting does deviate (a
watchdog-type thing, or tie it to packet enqueue or something?).

3. Tie this back into the airtime fairness scheduler deficit, the same
way Kan's original patch did.


The problem with (3) is that we lose the ability to use a different
source of airtime usage information for the fairness scheduler (without
doing weird subtractions in the accounting). In particular, this would
mean we can't account for retransmissions. So I'd prefer (2), I think.

Anyone else with bright ideas? :)

-Toke

Re: [PATCH RFC v4 0/4] Move TXQ scheduling into mac80211

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  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

Re: [RFC] mac80211: budget outstanding airtime for transmission

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> Per frame airtime estimation could be used to track outstanding airtime
> of each txq and can be used to throttle ieee80211_tx_dequeue(). This
> mechanism on its own will get us the queue limiting and latency
> reduction goodness for firmwares with deep queues. And for that it can
> be completely independent of the airtime fairness scheduler, which can
> use the after-tx-compl airtime information to presumably get more
> accurate fairness which includes retransmissions etc.

Very nice! This is pretty much what I would have done as well :)

A few comments below.

>  include/net/mac80211.h  |  2 +-
>  net/mac80211/sta_info.h |  1 +
>  net/mac80211/status.c   | 15 +
>  net/mac80211/tx.c   | 59 
> +++--
>  4 files changed, 70 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 89b192e93ac9..3a9a61fe30b5 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -947,7 +947,7 @@ struct ieee80211_tx_info {
>   u8 use_cts_prot:1;
>   u8 short_preamble:1;
>   u8 skip_table:1;
> - /* 2 bytes free */
> + u16 airtime_est;
>   };
>   /* only needed before rate control */
>   unsigned long jiffies;
> diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
> index b1b0fd6a2e21..ddc2c882c91c 100644
> --- a/net/mac80211/sta_info.h
> +++ b/net/mac80211/sta_info.h
> @@ -135,6 +135,7 @@ struct airtime_info {
>   u64 rx_airtime;
>   u64 tx_airtime;
>   s64 deficit;
> + s32 budget;

Why signed? This should never become negative unless something is wrong
with the accounting somewhere?

Related, are we sure there are no "leaks", i.e., packets that increase
the budget on dequeue, but are never tx_completed?

>  struct sta_info;
> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
> index 664379797c46..529f13cf7b2a 100644
> --- a/net/mac80211/status.c
> +++ b/net/mac80211/status.c
> @@ -718,6 +718,7 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
>   struct ieee80211_bar *bar;
>   int shift = 0;
>   int tid = IEEE80211_NUM_TIDS;
> + u32 ac = IEEE80211_AC_BE;
>  
>   rates_idx = ieee80211_tx_get_rates(hw, info, _count);
>  
> @@ -733,6 +734,16 @@ static void __ieee80211_tx_status(struct ieee80211_hw 
> *hw,
>  
>   acked = !!(info->flags & IEEE80211_TX_STAT_ACK);
>  
> + if (ieee80211_is_data_qos(fc)) {
> + u8 *qc = ieee80211_get_qos_ctl(hdr);
> +
> + tid = qc[0] & 0xf;
> + ac = ieee80211_ac_from_tid(tid);
> + }
> + spin_lock_bh(>fq.lock);
> + sta->airtime[ac].budget -= info->control.airtime_est;
> + spin_unlock_bh(>fq.lock);

What is the argument for accounting non-data frames to the BE AC? And
will this ever happen? Aren't we only putting data frames on the TXQs?

Also, with this locking, we have one field of the airtime struct that is
protected by a different lock than the rest. That is probably going to
become a problem at some point down the line when someone forgets this.
Can we use the active_txq_lock[ac] instead (should probably be renamed
in that case)?

The problem with using the other lock is that we'll need to ensure it is
taken in tx_dequeue(); but if we required schedule_{start,end}() around
all calls to tx_dequeue() it should be fine? (famous last words...)

>   /* mesh Peer Service Period support */
>   if (ieee80211_vif_is_mesh(>sdata->vif) &&
>   ieee80211_is_data_qos(fc))
> @@ -765,10 +776,6 @@ static void __ieee80211_tx_status(struct ieee80211_hw 
> *hw,
>   & IEEE80211_SCTL_SEQ);
>   ieee80211_send_bar(>sdata->vif, hdr->addr1,
>  tid, ssn);
> - } else if (ieee80211_is_data_qos(fc)) {
> - u8 *qc = ieee80211_get_qos_ctl(hdr);
> -
> - tid = qc[0] & 0xf;
>   }
>  
>   if (!acked && ieee80211_is_back_req(fc)) {
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index e4bc43904a8e..fb69af8e9757 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -3485,6 +3485,52 @@ static bool ieee80211_xmit_fast(struct 
> ieee80211_sub_if_data *sdata,
>   return true;
>  }
>  
> +/* The estimated airtime (in microseconds), which is calculated using last
> + * reported TX rate is stored in info context buffer. The budget will be
> + * adjusted upon tx completion.
> + */
> +#define IEEE80211_AIRTIME_BUDGET_MAX4000 /* txq airtime limit: 4ms */
> +#define IEEE80211_AIRTIME_OVERHEAD  100 

Re: [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Kalle Valo  writes:

> Toke Høiland-Jørgensen  writes:
>
>>> Unfortunately ath10k is not reporting last_tx_rate in tx_status(). So
>>> I also applied this "ath10k: report tx rate using ieee80211_tx_status"
>>> change.
>>
>> Yeah, that and the patch that computes the last used rate will probably
>> be necessary; but they can be pretty much applied as-is, right?
>
> Unfortunately not. I think the plan is now to follow Johannes' proposal:
>
>"I'd recommend against doing this and disentangling the necessary
> code in mac80211, e.g. with ieee80211_tx_status_ext() or adding
> similar APIs."
>
>https://patchwork.kernel.org/patch/10353959/

Ahh, right... *that* patch :)

Was thinking on this one with the "as-is" comment:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/588189

-Toke

Re: [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-09-18 13:41, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan  writes:
>> 
>>>>> Also an option to add the node at head or tail would be preferred. 
>>>>> If
>>>>> return_txq adds node at head of list, then it is forcing the driver 
>>>>> to
>>>>> serve same txq until it becomes empty. Also this will not allow the
>>>>> driver to send N frames from each txqs.
>>>> 
>>>> The whole point of this patch set is to move those kinds of decisions
>>>> out of the driver and into mac80211. The airtime scheduler won't
>>>> achieve
>>>> fairness if it allows queues to be queued to the end of the rotation
>>>> before its deficit turns negative. And obviously there's some lag in
>>>> this since we're using after-the-fact airtime information.
>>>> 
>>> Hmm.. As you know ath10k kind of doing fairness by serving fixed 
>>> frames
>>> from each txq. This approach will be removed from ath10k.
>>> 
>>>> For ath9k this has not really been a problem in my tests; if the lag
>>>> turns out to be too great for ath10k (which I suppose is a 
>>>> possibility
>>>> since we don't get airtime information on every TX-compl), I figure 
>>>> we
>>>> can use the same estimated airtime value that is used for throttling
>>>> the
>>>> queues to adjust the deficit immediately...
>>>> 
>>> Thats true. I am porting Kan's changes of airtime estimation for each
>>> msdu for firmware that does not report airtime.
>> 
>> Right. My thinking with this was that we could put the per-frame 
>> airtime
>> estimation into ieee80211_tx_dequeue(), which could track the
>> outstanding airtime and just return NULL if it goes over the threshold.
>> I think this is fairly straight-forward to do on its own; the biggest
>> problem is probably finding the space in the mac80211 cb?
>> 
>> Is this what you are working on porting? Because then I'll wait for 
>> your
>> patch rather than starting to write this code myself :)
>> 
> Kind of.. something like below.
>
> tx_dequeue(){
>  compute airtime_est from last_tx_rate
>  if (sta->airtime[ac].deficit < airtime_est)
>  return NULL;
>  dequeue skb and store airtime_est in cb
> }

I think I would decouple it further and not use the deficit. But rather:

 tx_dequeue(){
  if (sta->airtime[ac].outstanding > AIRTIME_OUTSTANDING_MAX)
return NULL
  compute airtime_est from last_tx_rate
  dequeue skb and store airtime_est in cb
  sta->airtime[ac].outstanding += airtime_est;
 }

> Unfortunately ath10k is not reporting last_tx_rate in tx_status(). So
> I also applied this "ath10k: report tx rate using ieee80211_tx_status"
> change.

Yeah, that and the patch that computes the last used rate will probably
be necessary; but they can be pretty much applied as-is, right?

>> This mechanism on its own will get us the queue limiting and latency
>> reduction goodness for firmwares with deep queues. And for that it can
>> be completely independent of the airtime fairness scheduler, which can
>> use the after-tx-compl airtime information to presumably get more
>> accurate fairness which includes retransmissions etc.
>> 
>> Now, we could *also* use the ahead-of-time airtime estimation for
>> fairness; either just as a fallback for drivers that can't get actual
>> airtime usage information for the hardware, or as an alternative in
>> cases where it works better for other reasons. But I think that
>> separating the two in the initial implementation makes more sense; that
>> will make it easier to experiment with different combinations of the
>> two.
>> 
>> Does that make sense? :)
>> 
> Completely agree. I was thinking of using this as fallback for devices
> that does not report airtime but tx rate.

Great! Seems we are converging on a workable solution, then :)

-Toke

Re: [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

>>> Also an option to add the node at head or tail would be preferred. If
>>> return_txq adds node at head of list, then it is forcing the driver to
>>> serve same txq until it becomes empty. Also this will not allow the
>>> driver to send N frames from each txqs.
>> 
>> The whole point of this patch set is to move those kinds of decisions
>> out of the driver and into mac80211. The airtime scheduler won't 
>> achieve
>> fairness if it allows queues to be queued to the end of the rotation
>> before its deficit turns negative. And obviously there's some lag in
>> this since we're using after-the-fact airtime information.
>> 
> Hmm.. As you know ath10k kind of doing fairness by serving fixed frames
> from each txq. This approach will be removed from ath10k.
>
>> For ath9k this has not really been a problem in my tests; if the lag
>> turns out to be too great for ath10k (which I suppose is a possibility
>> since we don't get airtime information on every TX-compl), I figure we
>> can use the same estimated airtime value that is used for throttling 
>> the
>> queues to adjust the deficit immediately...
>> 
> Thats true. I am porting Kan's changes of airtime estimation for each
> msdu for firmware that does not report airtime.

Right. My thinking with this was that we could put the per-frame airtime
estimation into ieee80211_tx_dequeue(), which could track the
outstanding airtime and just return NULL if it goes over the threshold.
I think this is fairly straight-forward to do on its own; the biggest
problem is probably finding the space in the mac80211 cb?

Is this what you are working on porting? Because then I'll wait for your
patch rather than starting to write this code myself :)

This mechanism on its own will get us the queue limiting and latency
reduction goodness for firmwares with deep queues. And for that it can
be completely independent of the airtime fairness scheduler, which can
use the after-tx-compl airtime information to presumably get more
accurate fairness which includes retransmissions etc.

Now, we could *also* use the ahead-of-time airtime estimation for
fairness; either just as a fallback for drivers that can't get actual
airtime usage information for the hardware, or as an alternative in
cases where it works better for other reasons. But I think that
separating the two in the initial implementation makes more sense; that
will make it easier to experiment with different combinations of the
two.

Does that make sense? :)

-Toke

Re: [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-09-16 10:42, Toke Høiland-Jørgensen wrote:
>> +/**
>> + * ieee80211_return_txq - return a TXQ previously acquired by
>> ieee80211_next_txq()
>> + *
>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>> + * @txq: pointer obtained from station or virtual interface
>> + *
>> + * Should only be called between calls to 
>> ieee80211_txq_schedule_start()
>> + * and ieee80211_txq_schedule_end().
>> + */
>> +void ieee80211_return_txq(struct ieee80211_hw *hw, struct 
>> ieee80211_txq *txq);
>> +
>> 
> return_txq() should return a bool to inform the driver that whether
> txq is queued back or not.

What would the driver do with that return value, exactly?

> Otherwise the same txq will be served indefinitely until txq becomes
> empty. This problem occurs when the driver is running out of hw
> descriptors or driver sends only N frames (< backlog_packets).

No, if it's using next_txq(), the API guarantees that the same TXQ will
not be returned more than once between a set of calls to
schedule_start()/schedule_end() (by way of the seqno mechanism). I
didn't add the same check to may_transmit(), because I assumed the
driver would not be looping in this case. Is that not correct?

> Also an option to add the node at head or tail would be preferred. If
> return_txq adds node at head of list, then it is forcing the driver to
> serve same txq until it becomes empty. Also this will not allow the
> driver to send N frames from each txqs.

The whole point of this patch set is to move those kinds of decisions
out of the driver and into mac80211. The airtime scheduler won't achieve
fairness if it allows queues to be queued to the end of the rotation
before its deficit turns negative. And obviously there's some lag in
this since we're using after-the-fact airtime information.

For ath9k this has not really been a problem in my tests; if the lag
turns out to be too great for ath10k (which I suppose is a possibility
since we don't get airtime information on every TX-compl), I figure we
can use the same estimated airtime value that is used for throttling the
queues to adjust the deficit immediately...

>> +/**
>> + * ieee80211_txq_schedule_start - acquire locks for safe scheduling of 
>> an AC
>> + *
>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>> + * @ac: AC number to acquire locks for
>> + *
>> + * Acquire locks needed to schedule TXQs from the given AC. Should be 
>> called
>> + * before ieee80211_next_txq() or ieee80211_schedule_txq().
>> + */
> Typo error. s/schedule_txq()/return_txq()/.

Yup, will fix :)

-Toke

[PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

This adds an API to mac80211 to handle scheduling of TXQs. The interface
between driver and mac80211 for TXQ handling is changed by adding two new
functions: ieee80211_next_txq(), which will return the next TXQ to schedule
in the current round-robin rotation, and ieee80211_return_txq(), which the
driver uses to indicate that it has finished scheduling a TXQ (which will
then be put back in the scheduling rotation if it isn't empty).

The driver must call ieee80211_txq_schedule_start() at the start of each
scheduling session, and ieee80211_txq_schedule_end() at the end. The API
then guarantees that the same TXQ is not returned twice in the same
session (so a driver can loop on ieee80211_next_txq() without worrying
about breaking the loop.

Usage of the new API is optional, so drivers can be ported one at a time.
In this patch, the actual scheduling performed by mac80211 is simple
round-robin, but a subsequent commit adds airtime fairness awareness to the
scheduler.

Signed-off-by: Toke Høiland-Jørgensen 
---
 include/net/mac80211.h |   62 +---
 net/mac80211/agg-tx.c  |2 +
 net/mac80211/driver-ops.h  |9 ++
 net/mac80211/ieee80211_i.h |9 ++
 net/mac80211/main.c|5 
 net/mac80211/sta_info.c|2 +
 net/mac80211/tx.c  |   59 +-
 7 files changed, 141 insertions(+), 7 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index c4fadbafbf21..5ca1484cba58 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -108,9 +108,16 @@
  * The driver is expected to initialize its private per-queue data for stations
  * and interfaces in the .add_interface and .sta_add ops.
  *
- * The driver can't access the queue directly. To dequeue a frame, it calls
- * ieee80211_tx_dequeue(). Whenever mac80211 adds a new frame to a queue, it
- * calls the .wake_tx_queue driver op.
+ * The driver can't access the queue directly. To dequeue a frame from a
+ * txq, it calls ieee80211_tx_dequeue(). Whenever mac80211 adds a new frame to 
a
+ * queue, it calls the .wake_tx_queue driver op.
+ *
+ * Drivers can optionally delegate responsibility for scheduling queues to
+ * mac80211, to take advantage of airtime fairness accounting. In this case, to
+ * obtain the next queue to pull frames from, the driver calls
+ * ieee80211_next_txq(). The driver is then expected to re-schedule the txq
+ * using ieee80211_schedule_txq() if it is still active after the driver has
+ * finished pulling packets from it.
  *
  * For AP powersave TIM handling, the driver only needs to indicate if it has
  * buffered packets in the driver specific data structures by calling
@@ -6045,13 +6052,60 @@ void ieee80211_unreserve_tid(struct ieee80211_sta *sta, 
u8 tid);
  * ieee80211_tx_dequeue - dequeue a packet from a software tx queue
  *
  * @hw: pointer as obtained from ieee80211_alloc_hw()
- * @txq: pointer obtained from station or virtual interface
+ * @txq: pointer obtained from station or virtual interface, or from
+ *   ieee80211_next_txq()
  *
  * Returns the skb if successful, %NULL if no frame was available.
  */
 struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 struct ieee80211_txq *txq);
 
+/**
+ * ieee80211_next_txq - get next tx queue to pull packets from
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @ac: AC number to return packets from.
+ *
+ * Should only be called between calls to ieee80211_txq_schedule_start()
+ * and ieee80211_txq_schedule_end().
+ * Returns the next txq if successful, %NULL if no queue is eligible. If a txq
+ * is returned, it should be returned with ieee80211_return_txq() after the
+ * driver has finished scheduling it.
+ */
+struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac);
+
+/**
+ * ieee80211_return_txq - return a TXQ previously acquired by 
ieee80211_next_txq()
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ * Should only be called between calls to ieee80211_txq_schedule_start()
+ * and ieee80211_txq_schedule_end().
+ */
+void ieee80211_return_txq(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
+
+/**
+ * ieee80211_txq_schedule_start - acquire locks for safe scheduling of an AC
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @ac: AC number to acquire locks for
+ *
+ * Acquire locks needed to schedule TXQs from the given AC. Should be called
+ * before ieee80211_next_txq() or ieee80211_schedule_txq().
+ */
+void ieee80211_txq_schedule_start(struct ieee80211_hw *hw, u8 ac);
+
+/**
+ * ieee80211_txq_schedule_end - release locks for safe scheduling of an AC
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @ac: AC number to acquire locks for
+ *
+ * Release locks previously acquired by ieee80211_txq_schedule_end().
+ */
+void ieee80211_txq_schedule_end(struct

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

[Toke Høiland-Jørgensen]

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

[PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

This adds airtime accounting and scheduling to the mac80211 TXQ
scheduler. A new callback, ieee80211_sta_register_airtime(), is added
that drivers can call to report airtime usage for stations.

When airtime information is present, mac80211 will schedule TXQs
(through ieee80211_next_txq()) in a way that enforces airtime fairness
between active stations. This scheduling works the same way as the ath9k
in-driver airtime fairness scheduling. If no airtime usage is reported
by the driver, the scheduler will default to round-robin scheduling.

For drivers that don't control TXQ scheduling in software, a new API
function, ieee80211_txq_may_transmit(), is added which the driver can use
to check if the TXQ is eligible for transmission, or should be throttled to
enforce fairness. Calls to this function must also be enclosed in
ieee80211_txq_schedule_{start,end}() calls to ensure proper locking. TXQs
that are throttled by ieee802111_txq_may_transmit() will be woken up again
by a check added to the ieee80211_wake_txqs() tasklet.

Signed-off-by: Toke Høiland-Jørgensen 
---
 include/net/mac80211.h |   52 
 net/mac80211/cfg.c |3 ++
 net/mac80211/debugfs.c |3 ++
 net/mac80211/debugfs_sta.c |   51 +--
 net/mac80211/ieee80211_i.h |3 ++
 net/mac80211/main.c|1 +
 net/mac80211/sta_info.c|   49 ++
 net/mac80211/sta_info.h|   13 +++
 net/mac80211/status.c  |6 +++
 net/mac80211/tx.c  |   83 ++--
 net/mac80211/util.c|   55 +
 11 files changed, 312 insertions(+), 7 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 5ca1484cba58..1e3ee0a2667b 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5350,6 +5350,34 @@ void ieee80211_sta_eosp(struct ieee80211_sta *pubsta);
  */
 void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
 
+/**
+ * ieee80211_sta_register_airtime - register airtime usage for a sta/tid
+ *
+ * Register airtime usage for a given sta on a given tid. The driver can call
+ * this function to notify mac80211 that a station used a certain amount of
+ * airtime. This information will be used by the TXQ scheduler to schedule
+ * stations in a way that ensures airtime fairness.
+ *
+ * The reported airtime should as a minimum include all time that is spent
+ * transmitting to the remote station, including overhead and padding, but not
+ * including time spent waiting for a TXOP. If the time is not reported by the
+ * hardware it can in some cases be calculated from the rate and known frame
+ * composition. When possible, the time should include any failed transmission
+ * attempts.
+ *
+ * The driver can either call this function synchronously for every packet or
+ * aggregate, or asynchronously as airtime usage information becomes available.
+ * TX and RX airtime can be reported together, or separately by setting one of
+ * them to 0.
+ *
+ * @pubsta: the station
+ * @tid: the TID to register airtime for
+ * @tx_airtime: airtime used during TX (in usec)
+ * @rx_airtime: airtime used during RX (in usec)
+ */
+void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
+   u32 tx_airtime, u32 rx_airtime);
+
 /**
  * ieee80211_iter_keys - iterate keys programmed into the device
  * @hw: pointer obtained from ieee80211_alloc_hw()
@@ -6106,6 +6134,30 @@ void ieee80211_txq_schedule_start(struct ieee80211_hw 
*hw, u8 ac);
  */
 void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac);
 
+/**
+ * ieee80211_txq_may_transmit - check whether TXQ is allowed to transmit
+ *
+ * This function is used to check whether given txq is allowed to transmit by
+ * the airtime scheduler, and can be used by drivers to access the airtime
+ * fairness accounting without going using the scheduling order enfored by
+ * next_txq().
+ *
+ * Returns %true if the airtime scheduler thinks the TXQ should be allowed to
+ * transmit, and %false if it should be throttled. This function can also have
+ * the side effect of rotating the TXQ in the scheduler rotation, which will
+ * eventually bring the deficit to positive and allow the station to transmit
+ * again. If a TXQ is throttled, it will be marked and eventually woken up 
again
+ * through drv_wake_tx_queue().
+ *
+ * If this function returns %true, the driver is expected to schedule packets
+ * for transmission, and then return the TXQ through ieee80211_return_txq().
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ */
+bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
+   struct ieee80211_txq *txq);
+
 /**
  * ieee80211_txq_get_depth - get pending frame/byte count of given txq
  *
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 504627e2117f

[PATCH RFC v4 2/4] cfg80211: Add airtime statistics and settings

[Toke Høiland-Jørgensen]

This adds TX airtime statistics to the cfg80211 station dump (to go along
with the RX info already present), and adds a new parameter to set the
airtime weight of each station. The latter allows userspace to implement
policies for different stations by varying their weights.

Signed-off-by: Toke Høiland-Jørgensen 
---
 include/net/cfg80211.h   |   10 +-
 include/uapi/linux/nl80211.h |   15 +++
 net/wireless/nl80211.c   |   29 +
 3 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 9f3ed79c39d7..91d6fc63 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -990,6 +990,7 @@ enum station_parameters_apply_mask {
  * @support_p2p_ps: information if station supports P2P PS mechanism
  * @he_capa: HE capabilities of station
  * @he_capa_len: the length of the HE capabilities
+ * @airtime_weight: airtime scheduler weight for this station
  */
 struct station_parameters {
const u8 *supported_rates;
@@ -1019,6 +1020,7 @@ struct station_parameters {
int support_p2p_ps;
const struct ieee80211_he_cap_elem *he_capa;
u8 he_capa_len;
+   u16 airtime_weight;
 };
 
 /**
@@ -1286,6 +1288,8 @@ struct cfg80211_tid_stats {
  * @rx_beacon_signal_avg: signal strength average (in dBm) for beacons received
  * from this peer
  * @rx_duration: aggregate PPDU duration(usecs) for all the frames from a peer
+ * @tx_duration: aggregate PPDU duration(usecs) for all the frames to a peer
+ * @airtime_weight: current airtime scheduling weight
  * @pertid: per-TID statistics, see  cfg80211_tid_stats, using the last
  * (IEEE80211_NUM_TIDS) index for MSDUs not encapsulated in QoS-MPDUs.
  * Note that this doesn't use the @filled bit, but is used if non-NULL.
@@ -1332,10 +1336,12 @@ struct station_info {
 
u32 expected_throughput;
 
-   u64 rx_beacon;
+   u64 tx_duration;
u64 rx_duration;
+   u64 rx_beacon;
u8 rx_beacon_signal_avg;
struct cfg80211_tid_stats *pertid;
+   u16 airtime_weight;
s8 ack_signal;
s8 avg_ack_signal;
 };
@@ -2363,6 +2369,8 @@ enum wiphy_params_flags {
WIPHY_PARAM_TXQ_QUANTUM = 1 << 8,
 };
 
+#define IEEE80211_DEFAULT_AIRTIME_WEIGHT   256
+
 /**
  * struct cfg80211_pmksa - PMK Security Association
  *
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index cfc94178d608..3664bdc7c8c1 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2241,6 +2241,9 @@ enum nl80211_commands {
  * association request when used with NL80211_CMD_NEW_STATION). Can be set
  * only if %NL80211_STA_FLAG_WME is set.
  *
+ * @NL80211_ATTR_AIRTIME_WEIGHT: Station's weight when scheduled by the airtime
+ *  scheduler.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2682,6 +2685,8 @@ enum nl80211_attrs {
 
NL80211_ATTR_HE_CAPABILITY,
 
+   NL80211_ATTR_AIRTIME_WEIGHT,
+
/* add attributes here, update the policy in nl80211.c */
 
__NL80211_ATTR_AFTER_LAST,
@@ -3051,6 +3056,9 @@ enum nl80211_sta_bss_param {
  * @NL80211_STA_INFO_PAD: attribute used for padding for 64-bit alignment
  * @NL80211_STA_INFO_ACK_SIGNAL: signal strength of the last ACK frame(u8, dBm)
  * @NL80211_STA_INFO_ACK_SIGNAL_AVG: avg signal strength of ACK frames (s8, 
dBm)
+ * @NL80211_STA_INFO_TX_DURATION: aggregate PPDU duration for all frames
+ * sent to the station (u64, usec)
+ * @NL80211_STA_INFO_AIRTIME_WEIGHT: current airtime weight for station (u16)
  * @__NL80211_STA_INFO_AFTER_LAST: internal
  * @NL80211_STA_INFO_MAX: highest possible station info attribute
  */
@@ -3091,6 +3099,8 @@ enum nl80211_sta_info {
NL80211_STA_INFO_PAD,
NL80211_STA_INFO_ACK_SIGNAL,
NL80211_STA_INFO_ACK_SIGNAL_AVG,
+   NL80211_STA_INFO_TX_DURATION,
+   NL80211_STA_INFO_AIRTIME_WEIGHT,
 
/* keep last */
__NL80211_STA_INFO_AFTER_LAST,
@@ -5231,6 +5241,10 @@ enum nl80211_feature_flags {
  *  if this flag is not set. Ignoring this can leak clear text packets 
and/or
  *  freeze the connection.
  *
+ * @NL80211_EXT_FEATURE_AIRTIME_FAIRNESS: Driver supports getting airtime
+ *  fairness for transmitted packets and has enabled airtime fairness
+ *  scheduling.
+ *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
  */
@@ -5269,6 +5283,7 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_SCAN_RANDOM_SN,
NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT,
NL80211_EXT_FEATURE_CAN_REPLACE_PTK0,
+   NL80211_EXT_FEATURE_AIRTIME_FAIRNESS,
 
/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
diff --git a/net/wi

[PATCH RFC v4 4/4] ath9k: Switch to mac80211 TXQ scheduling and airtime APIs

[Toke Høiland-Jørgensen]

This moves the ath9k driver to use the mac80211 TXQ scheduling and
airtime accounting APIs, removing the corresponding state tracking
inside the driver.

Signed-off-by: Toke Høiland-Jørgensen 
---
 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 
 7 files changed, 73 insertions(+), 262 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h 
b/drivers/net/wireless/ath/ath9k/ath9k.h
index 21ba20981a80..90b56766dab1 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -112,8 +112,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct 
ath_descdma *dd,
 #define ATH_TXFIFO_DEPTH   8
 #define ATH_TX_ERROR   0x01
 
-#define ATH_AIRTIME_QUANTUM300 /* usec */
-
 /* Stop tx traffic 1ms before the GO goes away */
 #define ATH_P2P_PS_STOP_TIME   1000
 
@@ -246,10 +244,8 @@ struct ath_atx_tid {
s8 bar_index;
bool active;
bool clear_ps_filter;
-   bool has_queued;
 };
 
-void __ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid);
 void ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid);
 
 struct ath_node {
@@ -263,12 +259,9 @@ struct ath_node {
 
bool sleeping;
bool no_ps_filter;
-   s64 airtime_deficit[IEEE80211_NUM_ACS];
-   u32 airtime_rx_start;
 
 #ifdef CONFIG_ATH9K_STATION_STATISTICS
struct ath_rx_rate_stats rx_rate_stats;
-   struct ath_airtime_stats airtime_stats;
 #endif
u8 key_idx[4];
 
@@ -986,11 +979,6 @@ void ath_ant_comb_scan(struct ath_softc *sc, struct 
ath_rx_status *rs);
 
 #define ATH9K_NUM_CHANCTX  2 /* supports 2 operating channels */
 
-#define AIRTIME_USE_TX BIT(0)
-#define AIRTIME_USE_RX BIT(1)
-#define AIRTIME_USE_NEW_QUEUES BIT(2)
-#define AIRTIME_ACTIVE(flags) (!!(flags & (AIRTIME_USE_TX|AIRTIME_USE_RX)))
-
 struct ath_softc {
struct ieee80211_hw *hw;
struct device *dev;
@@ -1034,8 +1022,6 @@ struct ath_softc {
short nbcnvifs;
unsigned long ps_usecount;
 
-   u16 airtime_flags; /* AIRTIME_* */
-
struct ath_rx rx;
struct ath_tx tx;
struct ath_beacon beacon;
diff --git a/drivers/net/wireless/ath/ath9k/debug.c 
b/drivers/net/wireless/ath/ath9k/debug.c
index 0a6eb8a8c1ed..f928d3a07caa 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -1456,9 +1456,6 @@ int ath9k_init_debug(struct ath_hw *ah)
 #endif
debugfs_create_file("tpc", 0600, sc->debug.debugfs_phy, sc, _tpc);
 
-   debugfs_create_u16("airtime_flags", 0600,
-  sc->debug.debugfs_phy, >airtime_flags);
-
debugfs_create_file("nf_override", 0600,
sc->debug.debugfs_phy, sc, _nf_override);
 
diff --git a/drivers/net/wireless/ath/ath9k/debug.h 
b/drivers/net/wireless/ath/ath9k/debug.h
index 249f8141cd00..559d9628f280 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -319,20 +319,12 @@ ath9k_debug_sync_cause(struct ath_softc *sc, u32 
sync_cause)
 void ath_debug_rate_stats(struct ath_softc *sc,
  struct ath_rx_status *rs,
  struct sk_buff *skb);
-void ath_debug_airtime(struct ath_softc *sc,
-  struct ath_node *an,
-  u32 rx, u32 tx);
 #else
 static inline void ath_debug_rate_stats(struct ath_softc *sc,
struct ath_rx_status *rs,
struct sk_buff *skb)
 {
 }
-static inline void ath_debug_airtime(struct ath_softc *sc,
- struct ath_node *an,
- u32 rx, u32 tx)
-{
-}
 #endif /* CONFIG_ATH9K_STATION_STATISTICS */
 
 #endif /* DEBUG_H */
diff --git a/drivers/net/wireless/ath/ath9k/debug_sta.c 
b/drivers/net/wireless/ath/ath9k/debug_sta.c
index a6f45f1bb5bb..bb6f3250aa30 100644
--- a/drivers/net/wireless/ath/ath9k/debug_sta.c
+++ b/drivers/net/wireless/ath/ath9k/debug_sta.c
@@ -242,59 +242,6 @@ static const struct file_operations fops_node_recv = {
.llseek = default_llseek,
 };
 
-void ath_debug_airtime(struct ath_softc *sc,
-   struct ath_node *an,
-   u32 rx,
-   u32 tx)
-{
-   struct ath_airtime_stats *astats = >airtime_stats;
-
-   astats->rx_airtime += rx;
-   astats->tx_airtime += tx;
-}
-
-static ssize_t read_airtime(struct file *file, char __user *user_buf,
-   size_t count, loff_t *ppos)
-{
-   struct ath_node *an = file-

Re: [PATCH RFC v3 0/4] Move TXQ scheduling into mac80211

[Toke Høiland-Jørgensen]

Kan Yan  writes:

>> I think this is a great approach, that we should definitely adopt in
>> mac80211. However, I'm not sure the outstanding airtime limiting should
>> be integrated into the fairness scheduling, since there are drivers such
>> as ath9k that don't need it.
>>
>> Rather, I'd propose that we figure out the API for fairness scheduling
>> first, and add the BQL-like limiter as a separate layer. They can be
>> integrated in the sense that the limiter's estimate of airtime can be
>> used for fairness scheduling as well in the case where the
>> driver/firmware doesn't have a better source of airtime usage.
>>
>> Does that make sense?
> Sure that make sense. Yes, the airtime based BQL like queue limiter is
> not needed for drivers such as ath9k. We could leave the outstanding
> airtime accounting and queue depth limit to another subject and get
> airtime fairness scheduling API done first.

Cool! I was thinking I would stub out an untested PoC in a separate
patch on my next submission, to show how I think this could be
incorporated. Have some other stuff to finish up this week, but
hopefully I'll have time to do the next version over the weekend :)

-Toke

Re: [PATCH RFC v3 2/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-09-10 04:13, Toke Høiland-Jørgensen wrote:
>> Johannes Berg  writes:
>>>> - txqi->flags & (1<>>> "RUN",
>>>> - txqi->flags & (1<>>> AMPDU" : "",
>>>> - txqi->flags & (1<>>> NO-AMSDU" : 
>>>> "");
>>>> + txqi->flags & (1 << IEEE80211_TXQ_STOP) ? "STOP" 
>>>> : "RUN",
>>>> + txqi->flags & (1 << IEEE80211_TXQ_AMPDU) ? " 
>>>> AMPDU" : "",
>>>> + txqi->flags & (1 << IEEE80211_TXQ_NO_AMSDU) ? " 
>>>> NO-AMSDU" 
>>>> : "");
>>> 
>>> consider BIT() instead as a cleanup? :)
>>> 
>>> (or maybe this is just a leftover from merging some other patches?)
>> 
>> Yeah, this is a merging artifact; Rajkumar's patch added another flag,
>> that I removed again. Didn't notice that there was still a whitespace
>> change in this patch...
>> 
> I added the flag based on our last discussion. The driver needs to check
> txq status for each tx_dequeue(). One time txq check is not sufficient
> as it allows the driver to dequeue all frames from txq.
>
> drv_func() {
>while (ieee80211_airtime_may_transmit(txq) &&
>hw_has_space() &&
>   (pkt = ieee80211_tx_dequeue(hw, txq)))
>push_to_hw(pkt);
> }

Yeah, but with airtime only being recorded on TX completion, the odds of
the value changing within that loop are quite low; so it's not going to
work, which is why I removed it.

However, after reading Kan's patches I get where you're coming from; a
check in tx_dequeue() is needed for the BQL-style queue limiting. Will
try to incorporate a version of that in the next series so you can see
what I mean when I say it should be orthogonal; and I'm still not sure
it needs a flag :)

>>>> +bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
>>>> +  struct ieee80211_txq *txq)
>>>> +{
>>>> +  struct ieee80211_local *local = hw_to_local(hw);
>>>> +  struct txq_info *txqi = to_txq_info(txq);
>>>> +  bool may_tx = false;
>>>> +
>>>> +  spin_lock_bh(>active_txq_lock);
>>>> +
>>>> +  if (ieee80211_txq_check_deficit(local, txqi)) {
>>>> +  may_tx = true;
>>>> +  list_del_init(>schedule_order);
>>> 
>
> To handle above case, may_transmit should remove the node only
> when it is in list.
>
> if (list_empty(>schedule_order))
> list_del_init(>schedule_order);

I assume you missed a ! in that if, right? :)

> So that it can be used to determine whether txq is running negative.

But still not sure what you mean here?

-Toke

Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Mon, 2018-09-10 at 17:00 +0200, Toke Høiland-Jørgensen wrote:
>
>> > Do we even need end_schedule()? It's hard to pass multiple things to a
>> > single call (do you build a list?), so having
>> > 
>> >start_schedule(), get_txq(), return_txq()
>> > 
>> > would be sufficient?
>> 
>> Well, start_schedule() / end_schedule() would be needed if we are going
>> to add locking in mac80211?
>
> [...]
>
>> If we decide mac80211 needs to do locking to prevent two threads from
>> scheduling the same ac, that would also be needed for the hw-managed
>> case?
>
> Yes, good point.
>
>> > It seems like not? Basically it seems to me that in the hw-managed
>> > case all you need is may_tx()? And in fact, once you opt in you don't
>> > even really need *that* since mac80211 can just return NULL from
>> > get_skb()?
>> 
>> Yeah, we could just throttle in get_skb(); the separate call was to
>> avoid the overhead of the check for every packet. Typically, you'll pick
>> a TXQ, then dequeue multiple packets from it in succession; with a
>> separate call to may_tx(), you only do the check once, not for every
>> packet...
>
> Yeah, also a good point.
>
> Still, txq = get_txq(txq) doesn't feel right, so better to keep that
> separate I think.

Right, will do :)

-Toke

Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Mon, 2018-09-10 at 15:18 +0200, Toke Høiland-Jørgensen wrote:
>
>> If we have the start_schedule() / end_schedule() pair anyway, the latter
>> could notify any TXQs that became eligible during the scheduling round.
>
> Do we even need end_schedule()? It's hard to pass multiple things to a
> single call (do you build a list?), so having
>
>   start_schedule(), get_txq(), return_txq()
>
> would be sufficient?

Well, start_schedule() / end_schedule() would be needed if we are going
to add locking in mac80211?

>> Also, instead of having the three different API functions
>> (next_txq()/may_tx()/schedule_txq()), we could  have 
>> get_txq(txq)/put_txq(txq)
>> which would always need to be paired; but the argument to get_txq()
>> could be optional, and if the driver passes NULL it means "give me the
>> next available TXQ".
>
> I can't say I like this. It makes the meaning totally different:
>
>  * with NULL: use the internal scheduler to determine which one is good
>to use next
>  * non-NULL: essentially equivalent to may_tx()

Yeah, it'll be two completely different uses for the same function; but
there wouldn't be two different APIs to keep track of. I'm fine with
keeping them as separately named functions. :)

>> So for ath9k it would be:
>> 
>> 
>> start_schedule(ac);
>> while ((txq = get_txq(NULL)) {
>>   queue_aggregate(txq);
>>   put_txq(txq);
>> }
>> end_schedule(ac);
>> 
>> And for ath10k/iwlwifi it would be:
>> 
>> on_hw_notify(txq) {
>>  start_schedule(ac);
>>  if (txq = get_txq(txq)) {
>>queue_packets(txq);
>>put_txq(txq);
>>  }
>>  end_schedule(ac);
>> }
>> 
>> 
>> I think that would be simpler, API-wise?
>
> I can't say I see much point in overloading get_txq() that way. You'd
> never use it the same way.
>
> Also, would you really start_schedule(ac) in the hw-managed case?

If we decide mac80211 needs to do locking to prevent two threads from
scheduling the same ac, that would also be needed for the hw-managed
case?

> It seems like not? Basically it seems to me that in the hw-managed
> case all you need is may_tx()? And in fact, once you opt in you don't
> even really need *that* since mac80211 can just return NULL from
> get_skb()?

Yeah, we could just throttle in get_skb(); the separate call was to
avoid the overhead of the check for every packet. Typically, you'll pick
a TXQ, then dequeue multiple packets from it in succession; with a
separate call to may_tx(), you only do the check once, not for every
packet...

-Toke

Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Mon, 2018-09-10 at 15:08 +0200, Toke Høiland-Jørgensen wrote:
>> Johannes Berg  writes:
>> 
>> > On Mon, 2018-09-10 at 14:39 +0200, Toke Høiland-Jørgensen wrote:
>> > 
>> > > > From then on, right now we basically just try to refill the hardware
>> > > > queue from the TXQ whenever the hardware releases frames from that
>> > > > queue. If there are none, we fall back to putting them on the hardware
>> > > > queue from the wake signal.
>> > > 
>> > > OK. So if the TXQ is ineligible to dequeue packets, but still have them
>> > > queued, there would need to be a signal (such as wake_txq()) when it
>> > > becomes eligible again, right?
>> > 
>> > Right. It wouldn't have to be wake_txq, but that seems as appropriate as
>> > anything else.
>> > 
>> > If we were to use ieee80211_airtime_may_transmit(), we'd have to have
>> > something that tells us "I previously told you it may not transmit, but
>> > now I changed my mind" :-)
>> 
>> Yes, exactly. Hmm, I'll see what I can come up with :)
>
> No need to look at this right now - let's get this stuff sorted out
> first :)

Right, sure, I'll get the port of ath9k done first; but doesn't hurt to
think about API compatibility with the other drivers at the same time as
well...

If we have the start_schedule() / end_schedule() pair anyway, the latter
could notify any TXQs that became eligible during the scheduling round.

Also, instead of having the three different API functions
(next_txq()/may_tx()/schedule_txq()), we could  have get_txq(txq)/put_txq(txq)
which would always need to be paired; but the argument to get_txq()
could be optional, and if the driver passes NULL it means "give me the
next available TXQ".

So for ath9k it would be:


start_schedule(ac);
while ((txq = get_txq(NULL)) {
  queue_aggregate(txq);
  put_txq(txq);
}
end_schedule(ac);

And for ath10k/iwlwifi it would be:

on_hw_notify(txq) {
 start_schedule(ac);
 if (txq = get_txq(txq)) {
   queue_packets(txq);
   put_txq(txq);
 }
 end_schedule(ac);
}


I think that would be simpler, API-wise?

-Toke

Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Mon, 2018-09-10 at 14:39 +0200, Toke Høiland-Jørgensen wrote:
>
>> > From then on, right now we basically just try to refill the hardware
>> > queue from the TXQ whenever the hardware releases frames from that
>> > queue. If there are none, we fall back to putting them on the hardware
>> > queue from the wake signal.
>> 
>> OK. So if the TXQ is ineligible to dequeue packets, but still have them
>> queued, there would need to be a signal (such as wake_txq()) when it
>> becomes eligible again, right?
>
> Right. It wouldn't have to be wake_txq, but that seems as appropriate as
> anything else.
>
> If we were to use ieee80211_airtime_may_transmit(), we'd have to have
> something that tells us "I previously told you it may not transmit, but
> now I changed my mind" :-)

Yes, exactly. Hmm, I'll see what I can come up with :)

-Toke

Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Mon, 2018-09-10 at 12:57 +0200, Toke Høiland-Jørgensen wrote:
>> Johannes Berg  writes:
>> 
>> > On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote:
>> > > 
>> > > Usage of the new API is optional, so drivers can be ported one at a time.
>> > 
>> > With the 1:1 hardware queue/txq mapping in iwlwifi (we're close to
>> > getting that patch in, though now the Jewish holidays mean a delay),
>> > I'm not sure we'd be able to do this at all in iwlwifi. So this may
>> > not be a case of porting one at a time until we can get rid of it ...
>> 
>> Could you elaborate a bit more on how the hwq/txq stuff works in iwl?
>
> I can try.
>
>> Does the driver just hand off a TXQ to the hardware on wake_txq(), which
>> is then scheduled by the hardware after that? Or how does the mapping to
>> hwqs work, and how is the hardware notified that there are still packets
>> queued / that new packets have arrived for an already mapped txq?
>
> Basically, we use the TXQ driver data to do this. On initialization, we
> set all TXQs to have no hardware queue mapped. Then, when the first wake
> happens for this TXQ, we allocate a hardware queue for this RA/TID.
>
> From then on, right now we basically just try to refill the hardware
> queue from the TXQ whenever the hardware releases frames from that
> queue. If there are none, we fall back to putting them on the hardware
> queue from the wake signal.

OK. So if the TXQ is ineligible to dequeue packets, but still have them
queued, there would need to be a signal (such as wake_txq()) when it
becomes eligible again, right?

-Toke

Re: [PATCH RFC v3 0/4] Move TXQ scheduling into mac80211

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote:
>> 
>> A few things that were discussed in the last round that I did *not* change:
>
> Thanks for this list btw.
>
>> - I did not add any locking around next_txq(); the driver is still supposed
>>   to maintain a lock that prevents two threads from trying to schedule the
>>   same AC at the same time. This is what drivers already do, so I figured it
>>   was easier to just keep it that way rather than do it in mac80211.
>
> I'll look at this in the code, but from a maintainer perspective I'm
> somewhat worried that this will lead to issues that are really the
> driver's fault, but surface in mac80211. I don't know how easy it
> would be to catch that.

Yeah, I get what you mean. The alternative would be to have a
ieee80211_start_schedule(ac) and ieee80211_end_schedule(ac), which
basically just takes a lock. Would mean we could get rid of the 'first'
parameter for next_txq(), so might not be such a bad idea; and if the
driver has its own locking the extra locking in mac80211 would just be
an always-uncontested spinlock, which shouldn't be much overhead, right?

-Toke

Re: [PATCH RFC v3 0/4] Move TXQ scheduling into mac80211

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote:
>
>> - I didn't get rid of the register_airtime() callback. As far as I can tell,
>>   only iwlwifi uses the tx_time field in the struct tx_info. Which means that
>>   we *could* probably use it for this and just make the other drivers set it;
>>   but I'm not sure what effects that would have in relation to WMM-AC for
>>   those drivers, so I chickened out. Will have to try it out, I guess; but it
>>   also depends on whether ath10k needs to be able to report airtime
>>   asynchronously anyway. So I'll hold off on that for a bit more.
>
> I don't think you need to be concerned, the reporting through this has
> no immediate effect as the driver would also have to set the feature
> flag (NL80211_FEATURE_SUPPORTS_WMM_ADMISSION) for userspace to be able
> to use WMM admission TSPECs, and getting tx_tspec->admitted_time to be
> non-zero in ieee80211_sta_tx_wmm_ac_notify().

Great! In that case I'll try moving the reporting go through the tx_info
struct and check that it works for ath9k :)

-Toke

Re: [PATCH RFC v3 3/4] cfg80211: Add airtime statistics and settings

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote:
>> This adds airtime statistics to the cfg80211 station dump, and also adds a 
>> new
>> parameter to set the airtime weight of each station. The latter allows 
>> userspace
>> to implement policies for different stations by varying their weights.
>
> Maybe some documentation on the weights - both for other implementers as
> well as users - would be useful.
>
>> + * @NL80211_STA_INFO_AIRTIME_WEIGHT: current airtime weight for station
>> + *  (u16, usec)
>
> how could the weight be in usec? copy/paste issue?

Well, technically it is a usec value (the amount of airtime added to the
deficit each round); but I agree that it shouldn't be documented as such
if we want freedom to change the mechanism later...

-Toke

Re: [PATCH RFC v3 2/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote:
>> 
>> +/**
>> + * ieee80211_airtime_may_transmit - check whether TXQ is allowed to transmit
>> + *
>> + * This function is used to check whether given txq is allowed to transmit 
>> by
>> + * the airtime scheduler, and can be used by drivers to access the airtime
>> + * fairness accounting without going using the scheduling order enfored by
>> + * next_txq().
>> + *
>> + * Returns %true if the airtime scheduler does not think the TXQ should be
>> + * throttled. This function can also have the side effect of rotating the 
>> TXQ in
>> + * the scheduler rotation, which will eventually bring the deficit to 
>> positive
>> + * and allow the station to transmit again.
>> + *
>> + * If this function returns %true, the TXQ will have been removed from the
>> + * scheduling. It is the driver's responsibility to add it back (by calling
>> + * ieee80211_schedule_txq()) if needed.
>> + *
>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>> + * @txq: pointer obtained from station or virtual interface
>> + *
>> + */
>
> Spurious newline there at the end.
>
> (As an aside from the review, perhaps this would be what we could use in
> iwlwifi, but I'll need to think about _when_ to add it back to the
> scheduling later).

Yeah, this is the API that would be used by drivers where the actual
scheduling of TXQs is done by the hardware (i.e., ath10k in pull/push
mode, and iwlwifi if I understand you correctly); whereas the next_txq()
call is for drivers that do the scheduling in software (ath10k without
pull/push, ath9k and mt76).

Basically, the way I envision this is the drivers doing something like:

function on_hw_notify(hw, txq) {
  if (ieee80211_airtime_may_transmit(txq)) {
 while (hw_has_space() && (pkt = ieee80211_tx_dequeue(hw, txq)))
 push_to_hw(pkt);

 ieee80211_schedule_txq(txq);
  }
}
  
>> + * @NL80211_EXT_FEATURE_AIRTIME_FAIRNESS: Driver supports airtime fairness
>> + *  scheduling.
>> + *
>>   * @NUM_NL80211_EXT_FEATURES: number of extended features.
>>   * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
>>   */
>> @@ -5269,6 +5272,7 @@ enum nl80211_ext_feature_index {
>>  NL80211_EXT_FEATURE_SCAN_RANDOM_SN,
>>  NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT,
>>  NL80211_EXT_FEATURE_CAN_REPLACE_PTK0,
>> +NL80211_EXT_FEATURE_AIRTIME_FAIRNESS,
>
> Why is this necessary in this patch?

It's meant as a way for the driver to signal that it has a source of
airtime information, and thus would like to be scheduled to take this
into account.

> I think that this should probably come with more documentation too,
> particularly what's expected of the driver here.

Right :)

> I'd prefer you reorder patches 2 and 3, and define everything in
> cfg80211 first. That also makes it easier to reason about what happens
> when the feature is not supported (since it will not be supported
> anywhere). Then this can be included in the cfg80211 patch instead,
> which places it better, and the mac80211 bits from the cfg80211 patch 
> can be included here, which also places those better.

Good idea, will do!

>> -   txqi->flags & (1<> "RUN",
>> -   txqi->flags & (1<> AMPDU" : "",
>> -   txqi->flags & (1<> NO-AMSDU" : "");
>> +   txqi->flags & (1 << IEEE80211_TXQ_STOP) ? "STOP" 
>> : "RUN",
>> +   txqi->flags & (1 << IEEE80211_TXQ_AMPDU) ? " 
>> AMPDU" : "",
>> +   txqi->flags & (1 << IEEE80211_TXQ_NO_AMSDU) ? " 
>> NO-AMSDU" : "");
>
> consider BIT() instead as a cleanup? :)
>
> (or maybe this is just a leftover from merging some other patches?)

Yeah, this is a merging artifact; Rajkumar's patch added another flag,
that I removed again. Didn't notice that there was still a whitespace
change in this patch...

>> +static bool ieee80211_txq_check_deficit(struct ieee80211_local *local,
>> +struct txq_info *txqi)
>
> Maybe this should be called "has_deficit" or so - the polarity isn't
> immediately clear to me.

Yup, I suck at naming; gotcha ;)

>> +{
>> +struct sta_info *sta;
>> +
>> +lockdep_assert_held(>active_txq_lock);
>> +
>> +if (!txqi->txq.sta)
>> +return true;
>> +
>> +sta = cont

Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote:
>
>> +/**
>> + * ieee80211_next_txq - get next tx queue to pull packets from
>> + *
>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>> + * @ac: AC number to return packets from.
>> + * @first: Setting this to true signifies the start of a new scheduling 
>> round.
>> + * Each TXQ will only be returned at most exactly once in each 
>> round.
>> + *
>> + * Returns the next txq if successful, %NULL if no queue is eligible. If a 
>> txq
>> + * is returned, it will have been removed from the scheduler queue and 
>> needs to
>> + * be re-scheduled with ieee80211_schedule_txq() to continue to be active. 
>> Note
>> + * that the driver is responsible for locking to ensuring two different 
>> threads
>> + * don't schedule the same AC simultaneously.
>
> "Note that [...] to ensure [...]" would seem better to me?
>
>> + */
>> +struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
>> + bool first);
>
> Why should "ac" be signed? Is there some (undocumented) behaviour that
> allows for passing -1 for "don't care" or "pick highest with frames"?
>
> You said "optionally" in the commit message, but I don't think that's
> true, since you just use ac to index the array in the code.

There was in the previous version, but I removed that; guess I forgot to
change the sign and the commit message ;)

>> +bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
>> +struct ieee80211_txq *txq)
>> +{
>> +struct ieee80211_local *local = hw_to_local(hw);
>> +struct txq_info *txqi = to_txq_info(txq);
>> +bool ret = false;
>> +
>> +spin_lock_bh(>active_txq_lock);
>> +
>> +if (list_empty(>schedule_order)) {
>
> Is there a case where it's allowed to call this while *not* empty? At
> least for the drivers it seems not, so perhaps when called from the
> driver it should WARN() here or so?

Yeah, mac80211 calls this on wake_txq, where it will often be scheduled
already. And since that can happen while drivers schedule stuff, it
could also be the case that a driver re-schedules a queue that is
already scheduled.

> I'm just a bit worried that drivers will get this a bit wrong, and
> we'll never know, but things won't work properly in some weird way.

What, subtle bugs in the data path that causes hangs in hard-to-debug
cases? Nah, that never happens ;)

>> +list_add_tail(>schedule_order,
>> +  >active_txqs[txq->ac]);
>> +ret = true;
>> +}
>> +
>> +spin_unlock_bh(>active_txq_lock);
>> +
>> +return ret;
>> +}
>> +EXPORT_SYMBOL(ieee80211_schedule_txq);
>
> I'd remove the return value - the driver has no need to know that it
> raced with potential rescheduling in mac80211 due to a new packet, and
> you don't use the return value in mac80211 either.

Yeah, good point; that was left over from when the call to wake_txq was
conditional on this actually doing something...

> It also seems to me that you forgot to say when it should be
> rescheduled? As is, following the documentation would sort of makes it
> seem you should take the queue and put it back at the end (without any
> conditions), and that could lead to "scheduling loops" since empty
> queues would always be put back when they shouldn't be?
>
> Also, come to think of it, but I guess the next patch(es) solve(s) that
> - if mac80211 adds a packet while you have this queue scheduled then you
> would take that packet even if it's questionable it belongs to this
> round?
>
> From a driver perspective, I think I'd prefer an API called e.g.
>
>   ieee80211_return_txq()
>
> that does the check internally if we need to schedule the queue (i.e.
> there are packets on it). I was going to say we could even annotate with
> sparse, but no - we can't because ieee80211_next_txq() may return NULL.
> Still, it's easier to ensure that all cleanup paths call
> ieee80211_return_txq() if it's not conditional, IMHO.

Good idea, will do.

>> +struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
>> + bool first)
>> +{
>> +struct ieee80211_local *local = hw_to_local(hw);
>> +struct txq_info *txqi = NULL;
>> +
>> +spin_lock_bh(>active_txq_lock);
>> +
>> +if (first)
>> +local->schedule_round[ac]++;
>
> (here - clearly ac must be 0 <= ac < IEEE802

Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote:
>> 
>> Usage of the new API is optional, so drivers can be ported one at a time.
>
> With the 1:1 hardware queue/txq mapping in iwlwifi (we're close to
> getting that patch in, though now the Jewish holidays mean a delay),
> I'm not sure we'd be able to do this at all in iwlwifi. So this may
> not be a case of porting one at a time until we can get rid of it ...

Could you elaborate a bit more on how the hwq/txq stuff works in iwl?
Does the driver just hand off a TXQ to the hardware on wake_txq(), which
is then scheduled by the hardware after that? Or how does the mapping to
hwqs work, and how is the hardware notified that there are still packets
queued / that new packets have arrived for an already mapped txq?

> It would be nice to be able to use it, for better queue behaviour, but
> it would mean much more accounting in iwlwifi? Not even sure off the
> top of my head how to do that.

I think we'll need to have some kind of fallback airtime estimation in
mac80211 that calculates airtime from packet size and rate information.
Not all drivers can get this from the hardware, it seems. See my reply
to Kan about the BQL-like behaviour as well.

Does iwl get airtime usage information for every packet on tx complete?

-Toke

Re: [PATCH RFC v3 0/4] Move TXQ scheduling into mac80211

[Toke Høiland-Jørgensen]

Kan Yan  writes:

> Hi Toke,
>
> It is great to see finally there will be a general airtime fairness
> support in mac80211. I feel this patch set could still use some
> improvements to make it works better with 11ac drivers like ath10k. I
> did a version of airtime fairness in the ath10k driver that used in
> Google Wifi and I'd like to share a few things I learned from this
> experience.

Hi Kan

Thanks for weighing in!

> The idea is to use airtime accounting to give firmware just enough
> frames to keep it busy and form good aggregations, and keeps the rest
> of frames in mac80211 layer queues so Fq_Codel can work on it to
> control latency.

I think this is a great approach, that we should definitely adopt in
mac80211. However, I'm not sure the outstanding airtime limiting should
be integrated into the fairness scheduling, since there are drivers such
as ath9k that don't need it.

Rather, I'd propose that we figure out the API for fairness scheduling
first, and add the BQL-like limiter as a separate layer. They can be
integrated in the sense that the limiter's estimate of airtime can be
used for fairness scheduling as well in the case where the
driver/firmware doesn't have a better source of airtime usage.

Does that make sense?

-Toke

[PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

This adds an API to mac80211 to handle scheduling of TXQs and changes the
interface between driver and mac80211 for TXQ handling as follows:

- The wake_tx_queue callback interface includes only the ac number
  instead of the TXQ. The driver is expected to retrieve TXQs from
  ieee80211_next_txq()

- Two new mac80211 functions are added: ieee80211_next_txq() and
  ieee80211_schedule_txq(). The former returns the next TXQ that should be
  scheduled, and is how the driver gets a queue to pull packets from. The
  latter is called internally by mac80211 to start scheduling a queue, and
  the driver is supposed to call it to re-schedule the TXQ after it is
  finished pulling packets from it (unless the queue emptied). Drivers
  can optionally filter TXQs on ac to support per-AC hardware queue
  designs.

Usage of the new API is optional, so drivers can be ported one at a time.

Signed-off-by: Toke Høiland-Jørgensen 
---
 include/net/mac80211.h |   45 ++---
 net/mac80211/agg-tx.c  |2 +
 net/mac80211/driver-ops.h  |7 +
 net/mac80211/ieee80211_i.h |9 +++
 net/mac80211/main.c|4 +++
 net/mac80211/sta_info.c|2 +
 net/mac80211/tx.c  |   60 +++-
 7 files changed, 122 insertions(+), 7 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index c4fadbafbf21..ff413d9caa50 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -108,9 +108,16 @@
  * The driver is expected to initialize its private per-queue data for stations
  * and interfaces in the .add_interface and .sta_add ops.
  *
- * The driver can't access the queue directly. To dequeue a frame, it calls
- * ieee80211_tx_dequeue(). Whenever mac80211 adds a new frame to a queue, it
- * calls the .wake_tx_queue driver op.
+ * The driver can't access the queue directly. To dequeue a frame from a
+ * txq, it calls ieee80211_tx_dequeue(). Whenever mac80211 adds a new frame to 
a
+ * queue, it calls the .wake_tx_queue driver op.
+ *
+ * Drivers can optionally delegate responsibility for scheduling queues to
+ * mac80211, to take advantage of airtime fairness accounting. In this case, to
+ * obtain the next queue to pull frames from, the driver calls
+ * ieee80211_next_txq(). The driver is then expected to re-schedule the txq
+ * using ieee80211_schedule_txq() if it is still active after the driver has
+ * finished pulling packets from it.
  *
  * For AP powersave TIM handling, the driver only needs to indicate if it has
  * buffered packets in the driver specific data structures by calling
@@ -6045,13 +6052,43 @@ void ieee80211_unreserve_tid(struct ieee80211_sta *sta, 
u8 tid);
  * ieee80211_tx_dequeue - dequeue a packet from a software tx queue
  *
  * @hw: pointer as obtained from ieee80211_alloc_hw()
- * @txq: pointer obtained from station or virtual interface
+ * @txq: pointer obtained from station or virtual interface, or from
+ *   ieee80211_next_txq()
  *
  * Returns the skb if successful, %NULL if no frame was available.
  */
 struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 struct ieee80211_txq *txq);
 
+/**
+ * ieee80211_schedule_txq - add txq to scheduling loop
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ * Returns %true if the txq was actually added to the scheduling,
+ * %false otherwise.
+ */
+bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
+   struct ieee80211_txq *txq);
+
+/**
+ * ieee80211_next_txq - get next tx queue to pull packets from
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @ac: AC number to return packets from.
+ * @first: Setting this to true signifies the start of a new scheduling round.
+ * Each TXQ will only be returned at most exactly once in each round.
+ *
+ * Returns the next txq if successful, %NULL if no queue is eligible. If a txq
+ * is returned, it will have been removed from the scheduler queue and needs to
+ * be re-scheduled with ieee80211_schedule_txq() to continue to be active. Note
+ * that the driver is responsible for locking to ensuring two different threads
+ * don't schedule the same AC simultaneously.
+ */
+struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
+bool first);
+
 /**
  * ieee80211_txq_get_depth - get pending frame/byte count of given txq
  *
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 69e831bc317b..e94b1a0407af 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -229,7 +229,7 @@ ieee80211_agg_start_txq(struct sta_info *sta, int tid, bool 
enable)
clear_bit(IEEE80211_TXQ_STOP, >flags);
local_bh_disable();
rcu_read_lock();
-   drv_wake_tx_queue(sta->sdata->local, txqi);
+   schedule_and_wake_txq(sta->sdata-

[PATCH RFC v3 4/4] ath9k: Switch to mac80211 TXQ scheduling and airtime APIs

[Toke Høiland-Jørgensen]

This moves the ath9k driver to use the mac80211 TXQ scheduling and
airtime accounting APIs, removing the corresponding state tracking
inside the driver.

Signed-off-by: Toke Høiland-Jørgensen 
---
 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  |  245 
 7 files changed, 74 insertions(+), 262 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h 
b/drivers/net/wireless/ath/ath9k/ath9k.h
index 21ba20981a80..90b56766dab1 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -112,8 +112,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct 
ath_descdma *dd,
 #define ATH_TXFIFO_DEPTH   8
 #define ATH_TX_ERROR   0x01
 
-#define ATH_AIRTIME_QUANTUM300 /* usec */
-
 /* Stop tx traffic 1ms before the GO goes away */
 #define ATH_P2P_PS_STOP_TIME   1000
 
@@ -246,10 +244,8 @@ struct ath_atx_tid {
s8 bar_index;
bool active;
bool clear_ps_filter;
-   bool has_queued;
 };
 
-void __ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid);
 void ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid);
 
 struct ath_node {
@@ -263,12 +259,9 @@ struct ath_node {
 
bool sleeping;
bool no_ps_filter;
-   s64 airtime_deficit[IEEE80211_NUM_ACS];
-   u32 airtime_rx_start;
 
 #ifdef CONFIG_ATH9K_STATION_STATISTICS
struct ath_rx_rate_stats rx_rate_stats;
-   struct ath_airtime_stats airtime_stats;
 #endif
u8 key_idx[4];
 
@@ -986,11 +979,6 @@ void ath_ant_comb_scan(struct ath_softc *sc, struct 
ath_rx_status *rs);
 
 #define ATH9K_NUM_CHANCTX  2 /* supports 2 operating channels */
 
-#define AIRTIME_USE_TX BIT(0)
-#define AIRTIME_USE_RX BIT(1)
-#define AIRTIME_USE_NEW_QUEUES BIT(2)
-#define AIRTIME_ACTIVE(flags) (!!(flags & (AIRTIME_USE_TX|AIRTIME_USE_RX)))
-
 struct ath_softc {
struct ieee80211_hw *hw;
struct device *dev;
@@ -1034,8 +1022,6 @@ struct ath_softc {
short nbcnvifs;
unsigned long ps_usecount;
 
-   u16 airtime_flags; /* AIRTIME_* */
-
struct ath_rx rx;
struct ath_tx tx;
struct ath_beacon beacon;
diff --git a/drivers/net/wireless/ath/ath9k/debug.c 
b/drivers/net/wireless/ath/ath9k/debug.c
index 0a6eb8a8c1ed..f928d3a07caa 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -1456,9 +1456,6 @@ int ath9k_init_debug(struct ath_hw *ah)
 #endif
debugfs_create_file("tpc", 0600, sc->debug.debugfs_phy, sc, _tpc);
 
-   debugfs_create_u16("airtime_flags", 0600,
-  sc->debug.debugfs_phy, >airtime_flags);
-
debugfs_create_file("nf_override", 0600,
sc->debug.debugfs_phy, sc, _nf_override);
 
diff --git a/drivers/net/wireless/ath/ath9k/debug.h 
b/drivers/net/wireless/ath/ath9k/debug.h
index 249f8141cd00..559d9628f280 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -319,20 +319,12 @@ ath9k_debug_sync_cause(struct ath_softc *sc, u32 
sync_cause)
 void ath_debug_rate_stats(struct ath_softc *sc,
  struct ath_rx_status *rs,
  struct sk_buff *skb);
-void ath_debug_airtime(struct ath_softc *sc,
-  struct ath_node *an,
-  u32 rx, u32 tx);
 #else
 static inline void ath_debug_rate_stats(struct ath_softc *sc,
struct ath_rx_status *rs,
struct sk_buff *skb)
 {
 }
-static inline void ath_debug_airtime(struct ath_softc *sc,
- struct ath_node *an,
- u32 rx, u32 tx)
-{
-}
 #endif /* CONFIG_ATH9K_STATION_STATISTICS */
 
 #endif /* DEBUG_H */
diff --git a/drivers/net/wireless/ath/ath9k/debug_sta.c 
b/drivers/net/wireless/ath/ath9k/debug_sta.c
index a6f45f1bb5bb..bb6f3250aa30 100644
--- a/drivers/net/wireless/ath/ath9k/debug_sta.c
+++ b/drivers/net/wireless/ath/ath9k/debug_sta.c
@@ -242,59 +242,6 @@ static const struct file_operations fops_node_recv = {
.llseek = default_llseek,
 };
 
-void ath_debug_airtime(struct ath_softc *sc,
-   struct ath_node *an,
-   u32 rx,
-   u32 tx)
-{
-   struct ath_airtime_stats *astats = >airtime_stats;
-
-   astats->rx_airtime += rx;
-   astats->tx_airtime += tx;
-}
-
-static ssize_t read_airtime(struct file *file, char __user *user_buf,
-   size_t count, loff_t *ppos)
-{
-   struct ath_node *an = file-

[PATCH RFC v3 2/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

This adds airtime accounting and scheduling to the mac80211 TXQ
scheduler. A new callback, ieee80211_sta_register_airtime(), is added
that drivers can call to report airtime usage for stations.

When airtime information is present, mac80211 will schedule TXQs
(through ieee80211_next_txq()) in a way that enforces airtime fairness
between active stations. This scheduling works the same way as the ath9k
in-driver airtime fairness scheduling. If no airtime usage is reported
by the driver, the scheduler will default to round-robin scheduling.

Signed-off-by: Toke Høiland-Jørgensen 
---
 include/net/mac80211.h   |   52 
 include/uapi/linux/nl80211.h |4 ++
 net/mac80211/debugfs.c   |3 ++
 net/mac80211/debugfs_sta.c   |   42 --
 net/mac80211/ieee80211_i.h   |2 +
 net/mac80211/main.c  |1 +
 net/mac80211/sta_info.c  |   32 +++
 net/mac80211/sta_info.h  |   16 ++
 net/mac80211/tx.c|   69 +-
 9 files changed, 216 insertions(+), 5 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index ff413d9caa50..a744ad4f4f59 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5350,6 +5350,34 @@ void ieee80211_sta_eosp(struct ieee80211_sta *pubsta);
  */
 void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
 
+/**
+ * ieee80211_sta_register_airtime - register airtime usage for a sta/tid
+ *
+ * Register airtime usage for a given sta on a given tid. The driver can call
+ * this function to notify mac80211 that a station used a certain amount of
+ * airtime. This information will be used by the TXQ scheduler to schedule
+ * stations in a way that ensures airtime fairness.
+ *
+ * The reported airtime should as a minimum include all time that is spent
+ * transmitting to the remote station, including overhead and padding, but not
+ * including time spent waiting for a TXOP. If the time is not reported by the
+ * hardware it can in some cases be calculated from the rate and known frame
+ * composition. When possible, the time should include any failed transmission
+ * attempts.
+ *
+ * The driver can either call this function synchronously for every packet or
+ * aggregate, or asynchronously as airtime usage information becomes available.
+ * TX and RX airtime can be reported together, or separately by setting one of
+ * them to 0.
+ *
+ * @pubsta: the station
+ * @tid: the TID to register airtime for
+ * @tx_airtime: airtime used during TX (in usec)
+ * @rx_airtime: airtime used during RX (in usec)
+ */
+void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
+   u32 tx_airtime, u32 rx_airtime);
+
 /**
  * ieee80211_iter_keys - iterate keys programmed into the device
  * @hw: pointer obtained from ieee80211_alloc_hw()
@@ -6089,6 +6117,30 @@ bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
 struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
 bool first);
 
+/**
+ * ieee80211_airtime_may_transmit - check whether TXQ is allowed to transmit
+ *
+ * This function is used to check whether given txq is allowed to transmit by
+ * the airtime scheduler, and can be used by drivers to access the airtime
+ * fairness accounting without going using the scheduling order enfored by
+ * next_txq().
+ *
+ * Returns %true if the airtime scheduler does not think the TXQ should be
+ * throttled. This function can also have the side effect of rotating the TXQ 
in
+ * the scheduler rotation, which will eventually bring the deficit to positive
+ * and allow the station to transmit again.
+ *
+ * If this function returns %true, the TXQ will have been removed from the
+ * scheduling. It is the driver's responsibility to add it back (by calling
+ * ieee80211_schedule_txq()) if needed.
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ */
+bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
+   struct ieee80211_txq *txq);
+
 /**
  * ieee80211_txq_get_depth - get pending frame/byte count of given txq
  *
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index cfc94178d608..6b2b61646b6a 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -5231,6 +5231,9 @@ enum nl80211_feature_flags {
  *  if this flag is not set. Ignoring this can leak clear text packets 
and/or
  *  freeze the connection.
  *
+ * @NL80211_EXT_FEATURE_AIRTIME_FAIRNESS: Driver supports airtime fairness
+ *  scheduling.
+ *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
  */
@@ -5269,6 +5272,7 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_SCAN_RANDOM_SN

[PATCH RFC v3 3/4] cfg80211: Add airtime statistics and settings

[Toke Høiland-Jørgensen]

This adds airtime statistics to the cfg80211 station dump, and also adds a new
parameter to set the airtime weight of each station. The latter allows userspace
to implement policies for different stations by varying their weights.

Signed-off-by: Toke Høiland-Jørgensen 
---
 include/net/cfg80211.h   |   12 ++--
 include/uapi/linux/nl80211.h |   11 +++
 net/mac80211/cfg.c   |3 +++
 net/mac80211/sta_info.c  |   15 +++
 net/mac80211/sta_info.h  |3 ---
 net/wireless/nl80211.c   |   25 +
 6 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 9f3ed79c39d7..6fa4a97eefee 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -990,6 +990,7 @@ enum station_parameters_apply_mask {
  * @support_p2p_ps: information if station supports P2P PS mechanism
  * @he_capa: HE capabilities of station
  * @he_capa_len: the length of the HE capabilities
+ * @airtime_weight: airtime scheduler weight for this station
  */
 struct station_parameters {
const u8 *supported_rates;
@@ -1019,6 +1020,7 @@ struct station_parameters {
int support_p2p_ps;
const struct ieee80211_he_cap_elem *he_capa;
u8 he_capa_len;
+   u16 airtime_weight;
 };
 
 /**
@@ -1286,6 +1288,8 @@ struct cfg80211_tid_stats {
  * @rx_beacon_signal_avg: signal strength average (in dBm) for beacons received
  * from this peer
  * @rx_duration: aggregate PPDU duration(usecs) for all the frames from a peer
+ * @tx_duration: aggregate PPDU duration(usecs) for all the frames to a peer
+ * @airtime_weight: current airtime scheduling weight
  * @pertid: per-TID statistics, see  cfg80211_tid_stats, using the last
  * (IEEE80211_NUM_TIDS) index for MSDUs not encapsulated in QoS-MPDUs.
  * Note that this doesn't use the @filled bit, but is used if non-NULL.
@@ -1332,10 +1336,12 @@ struct station_info {
 
u32 expected_throughput;
 
-   u64 rx_beacon;
+   u64 tx_duration;
u64 rx_duration;
+   u64 rx_beacon;
u8 rx_beacon_signal_avg;
struct cfg80211_tid_stats *pertid;
+   u16 airtime_weight;
s8 ack_signal;
s8 avg_ack_signal;
 };
@@ -2349,7 +2355,7 @@ enum cfg80211_connect_params_changed {
  * @WIPHY_PARAM_DYN_ACK: dynack has been enabled
  * @WIPHY_PARAM_TXQ_LIMIT: TXQ packet limit has been changed
  * @WIPHY_PARAM_TXQ_MEMORY_LIMIT: TXQ memory limit has been changed
- * @WIPHY_PARAM_TXQ_QUANTUM: TXQ scheduler quantum
+ * @WIPHY_PARAM_TXQ_QUANTUM: TXQ scheduler quantum (bytes)
  */
 enum wiphy_params_flags {
WIPHY_PARAM_RETRY_SHORT = 1 << 0,
@@ -2363,6 +2369,8 @@ enum wiphy_params_flags {
WIPHY_PARAM_TXQ_QUANTUM = 1 << 8,
 };
 
+#define IEEE80211_DEFAULT_AIRTIME_WEIGHT   256
+
 /**
  * struct cfg80211_pmksa - PMK Security Association
  *
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 6b2b61646b6a..009c2e7b762f 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2241,6 +2241,9 @@ enum nl80211_commands {
  * association request when used with NL80211_CMD_NEW_STATION). Can be set
  * only if %NL80211_STA_FLAG_WME is set.
  *
+ * @NL80211_ATTR_AIRTIME_WEIGHT: Station's weight when scheduled by the airtime
+ *  scheduler.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2682,6 +2685,8 @@ enum nl80211_attrs {
 
NL80211_ATTR_HE_CAPABILITY,
 
+   NL80211_ATTR_AIRTIME_WEIGHT,
+
/* add attributes here, update the policy in nl80211.c */
 
__NL80211_ATTR_AFTER_LAST,
@@ -3051,6 +3056,10 @@ enum nl80211_sta_bss_param {
  * @NL80211_STA_INFO_PAD: attribute used for padding for 64-bit alignment
  * @NL80211_STA_INFO_ACK_SIGNAL: signal strength of the last ACK frame(u8, dBm)
  * @NL80211_STA_INFO_ACK_SIGNAL_AVG: avg signal strength of ACK frames (s8, 
dBm)
+ * @NL80211_STA_INFO_TX_DURATION: aggregate PPDU duration for all frames
+ * sent to the station (u64, usec)
+ * @NL80211_STA_INFO_AIRTIME_WEIGHT: current airtime weight for station
+ *  (u16, usec)
  * @__NL80211_STA_INFO_AFTER_LAST: internal
  * @NL80211_STA_INFO_MAX: highest possible station info attribute
  */
@@ -3091,6 +3100,8 @@ enum nl80211_sta_info {
NL80211_STA_INFO_PAD,
NL80211_STA_INFO_ACK_SIGNAL,
NL80211_STA_INFO_ACK_SIGNAL_AVG,
+   NL80211_STA_INFO_TX_DURATION,
+   NL80211_STA_INFO_AIRTIME_WEIGHT,
 
/* keep last */
__NL80211_STA_INFO_AFTER_LAST,
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 504627e2117f..8bd7c917a7b5 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1388,6 +1388,9 @@ static int sta_apply_parameters(struct ieee80211_local 
*local,
if (ieee80211_vif_

[PATCH RFC v3 0/4] Move TXQ scheduling into mac80211

[Toke Høiland-Jørgensen]

This is an updated version of the patch series to move TXQ scheduling and
airtime fairness scheduling into mac80211. I've only compile tested this
version, but thought it was better to get the conversation moving instead
of being blocked on me.

This addresses most of the comments from the last round. Specifically:

- It folds in Rajkumar's patches to keep per-AC TXQ lists, and to add an
  API that ath10k can use to check if a TXQ may transmit according even
  when not using get_next_txq(). I changed a few names and descriptions,
  but otherwise it's mostly the same. After the discussions we had in the
  last series, I *think* it will work this way, but I'm not entirely sure.

- I got rid of any mention of seqno in next_txq() and schedule_txq() - and
  removed the third parameter to schedule_txq() entirely, so drivers can no
  longer signal that a TXQ should be allowed to re-appear in a scheduling
  round. We can add that back if needed.

- Added a helper function to schedule and wake TXQs in a single call, for
  internal mac80211 use.

- Changed the default station weight to 256 and got rid of the per-phy
  quantum. This makes it possible to lower station weights without having
  to change the weights of every other station.

A few things that were discussed in the last round that I did *not* change:

- I did not add any locking around next_txq(); the driver is still supposed
  to maintain a lock that prevents two threads from trying to schedule the
  same AC at the same time. This is what drivers already do, so I figured it
  was easier to just keep it that way rather than do it in mac80211.

- I didn't get rid of the register_airtime() callback. As far as I can tell,
  only iwlwifi uses the tx_time field in the struct tx_info. Which means that
  we *could* probably use it for this and just make the other drivers set it;
  but I'm not sure what effects that would have in relation to WMM-AC for
  those drivers, so I chickened out. Will have to try it out, I guess; but it
  also depends on whether ath10k needs to be able to report airtime
  asynchronously anyway. So I'll hold off on that for a bit more.

-Toke

---

Toke Høiland-Jørgensen (4):
  mac80211: Add TXQ scheduling API
  mac80211: Add airtime accounting and scheduling to TXQs
  cfg80211: Add airtime statistics and settings
  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  |  245 
 include/net/cfg80211.h |   12 +
 include/net/mac80211.h |   97 +++
 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 |   42 -
 net/mac80211/driver-ops.h  |7 +
 net/mac80211/ieee80211_i.h |   11 +
 net/mac80211/main.c|5 +
 net/mac80211/sta_info.c|   49 +-
 net/mac80211/sta_info.h|   13 +
 net/mac80211/tx.c  |  125 ++
 net/wireless/nl80211.c |   25 +++
 21 files changed, 471 insertions(+), 274 deletions(-)

X-Clacks-Overhead: GNU Terry Pratchett

Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Wed, 2018-09-05 at 14:32 +0200, Toke Høiland-Jørgensen wrote:
>> Johannes Berg  writes:
>> 
>> > On Wed, 2018-09-05 at 13:41 +0200, Johannes Berg wrote:
>> > > On Wed, 2018-09-05 at 13:40 +0200, Toke Høiland-Jørgensen wrote:
>> > > > 
>> > > > Guess we'll have to deal with everything else if we ever move 
>> > > > management
>> > > > frames onto the TXQ path as well...
>> > > 
>> > > Depends on whether we care for management frame priorities or not ... so
>> > > far we haven't really.
>> > 
>> > Actually, for the most part we have implemented that properly. Except
>> > for the TXQ I added for bufferable management ... oh well, I think we're
>> > the only user thereof now.
>> > 
>> > I'm not sure we want to have a TXQ per TID for management, that seems
>> > overkill. But I'm also not sure how to solve this otherwise ...
>> 
>> Graft it to an existing TXQ, similar to how the fragments queue is used
>> now? Saves a TXQ at the expense of having to special-case it...
>
> The problem isn't so much how we handle it in mac80211 for the queueing,
> but how we deal with things like A-MSDU and how we present it to the
> driver ... for iwlwifi at least we'd really like to have only data
> frames so we can map it directly to the hardware queue ...

Ah, I see. No, then just putting them at the head of a different TXQ
probably won't work...

Are you mapping TXQs to hardware queues dynamically as they empty and
re-fill? Presumably you'll have cases where you don't have enough HWQs?

-Toke

Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Wed, 2018-09-05 at 13:34 +0200, Johannes Berg wrote:
>> From: Johannes Berg 
>> 
>> If the TDLS setup happens over a connection to an AP that
>> doesn't have QoS, we nevertheless assign a non-zero TID
>> (skb->priority) and queue mapping, which may confuse us or
>> drivers later.
>> 
>> Fix it by just assigning the special skb->priority and then
>> using ieee80211_select_queue() just like other data frames
>> would go through.
>> 
>> Signed-off-by: Johannes Berg 
>> ---
>> OK, this addresses the case I was worried about - perhaps
>> better take this than the txq patch?

(This is just you talking to yourself, right? :))

> I really only found this. All the other cases aren't doing real data
> frames, only null-data and management. So I guess for those we're OK,
> and this was the only special case after all.

SGTM.

Guess we'll have to deal with everything else if we ever move management
frames onto the TXQ path as well...

-Toke

Re: [PATCH] mac80211: use non-zero TID only for QoS frames

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Wed, 2018-09-05 at 13:07 +0200, Toke Høiland-Jørgensen wrote:
>
>> > Felix wasn't really convinced, I think. He also pointed out some drivers
>> > use skb->priority without checking anything, but I'm not sure we can
>> > really squash all the cases of setting skb priority easily?
>> 
>> ~/build/linux/drivers/net/wireless $ git grep 'skb->priority = '
>> ath/ath9k/channel.c: skb->priority = 7;
>> broadcom/brcm80211/brcmfmac/core.c:  skb->priority = 
>> cfg80211_classify8021d(skb, NULL);
>> broadcom/brcm80211/brcmutil/utils.c: skb->priority = 0;
>> intel/ipw2x00/libipw_tx.c:   skb->priority = libipw_classify(skb);
>> marvell/mwifiex/cfg80211.c:  skb->priority = LOW_PRIO_TID;
>> marvell/mwifiex/main.c:  skb->priority = cfg80211_classify8021d(skb, 
>> NULL);
>> marvell/mwifiex/tdls.c:  skb->priority = MWIFIEX_PRIO_BK;
>> marvell/mwifiex/tdls.c:  skb->priority = MWIFIEX_PRIO_VI;
>> marvell/mwifiex/tdls.c:  skb->priority = MWIFIEX_PRIO_VI;
>> rsi/rsi_91x_core.c:  skb->priority = q_num;
>> rsi/rsi_91x_core.c:  skb->priority = TID_TO_WME_AC(tid);
>> rsi/rsi_91x_core.c:  skb->priority = BE_Q;
>> rsi/rsi_91x_core.c:  skb->priority = q_num;
>> rsi/rsi_91x_hal.c:   skb->priority = VO_Q;
>> rsi/rsi_91x_mgmt.c:  skb->priority = MGMT_SOFT_Q;
>> ti/wlcore/main.c:skb->priority = WL1271_TID_MGMT;
>> 
>> Doesn't seem *that* excessive? Obviously there could be other cases, and
>> I haven't looked closer at any of those...
>
> That's assignments. For assignments, I guess you'd have to look at
> net/mac80211/. It's not that excessive either, but it's not in all
> places trivial to determine ...

Ah, sorry, I read that as "some drivers *set* skb->priority without
checking"...

-Toke

Re: [PATCH] mac80211: use non-zero TID only for QoS frames

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Wed, 2018-09-05 at 11:56 +0200, Toke Høiland-Jørgensen wrote:
>
>> > So basically this gets rid of a corner case that we shouldn't have.
>> > Either we should decide that using different TXQs is *always* correct
>> > for non-QoS, or - what I thought - that this isn't worth it, and then we
>> > should *never* do it.
>> 
>> Yeah, I agree that this is not worth it. The queue is already
>> FQ-CoDel'ed, which gives us most of the benefit of QoS anyway :)
>
> So do I read that as a tentative ack? :)

Yeah, guess so :)

> Felix wasn't really convinced, I think. He also pointed out some drivers
> use skb->priority without checking anything, but I'm not sure we can
> really squash all the cases of setting skb priority easily?

~/build/linux/drivers/net/wireless $ git grep 'skb->priority = '
ath/ath9k/channel.c:skb->priority = 7;
broadcom/brcm80211/brcmfmac/core.c: skb->priority = 
cfg80211_classify8021d(skb, NULL);
broadcom/brcm80211/brcmutil/utils.c:skb->priority = 0;
intel/ipw2x00/libipw_tx.c:  skb->priority = libipw_classify(skb);
marvell/mwifiex/cfg80211.c: skb->priority = LOW_PRIO_TID;
marvell/mwifiex/main.c: skb->priority = cfg80211_classify8021d(skb, NULL);
marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_BK;
marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI;
marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI;
rsi/rsi_91x_core.c: skb->priority = q_num;
rsi/rsi_91x_core.c: skb->priority = TID_TO_WME_AC(tid);
rsi/rsi_91x_core.c: skb->priority = BE_Q;
rsi/rsi_91x_core.c: skb->priority = q_num;
rsi/rsi_91x_hal.c:  skb->priority = VO_Q;
rsi/rsi_91x_mgmt.c: skb->priority = MGMT_SOFT_Q;
ti/wlcore/main.c:   skb->priority = WL1271_TID_MGMT;

Doesn't seem *that* excessive? Obviously there could be other cases, and
I haven't looked closer at any of those...

Does it matter for the drivers that don't use TXQs?

-Toke

Re: [PATCH] mac80211: fix pending queue hang due to TX_DROP

[Toke Høiland-Jørgensen]

Bob Copeland  writes:

> In our environment running lots of mesh nodes, we are seeing the
> pending queue hang periodically, with the debugfs queues file showing
> lines such as:
>
> 00: 0x/348
>
> i.e. there are a large number of frames but no stop reason set.
>
> One way this could happen is if queue processing from the pending
> tasklet exited early without processing all frames, and without having
> some future event (incoming frame, stop reason flag, ...) to reschedule
> it.
>
> Exactly this can occur today if ieee80211_tx() returns false due to
> packet drops or power-save buffering in the tx handlers.  In the
> past, this function would return true in such cases, and the change
> to false doesn't seem to be intentional.

Can confirm that this was not intentional; nice catch! :)

Acked-by: Toke Høiland-Jørgensen 

-Toke

Re: [PATCH] mac80211: use non-zero TID only for QoS frames

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Wed, 2018-09-05 at 11:47 +0200, Toke Høiland-Jørgensen wrote:
>> Johannes Berg  writes:
>> 
>> > From: Johannes Berg 
>> > 
>> > Some frames may have a non-zero skb->priority assigned by
>> > mac80211 internally, e.g. TDLS setup frames, regardless of
>> > support for QoS.
>> > 
>> > Currently, we set skb->priority to 0 for all data frames.
>> > Note that there's a comment that this is "required for
>> > correct WPA/11i MIC", but that doesn't seem true as we use
>> > 
>> > if (ieee80211_is_data_qos(hdr->frame_control))
>> > qos_tid = ieee80211_get_tid(hdr);
>> > else
>> > qos_tid = 0;
>> > 
>> > in the code there. We could therefore reconsider this, but
>> > it seems like unnecessary complexity for the unlikely (and
>> > not very useful) case of not having QoS on the connection.
>> > 
>> > This situation then causes something strange - most data
>> > frames will go on TXQ for TID 0 for non-QoS connections,
>> > but very few exceptions that are internally generated will
>> > go on another TXQ, possibly causing confusion.
>> 
>> What kind of confusion are you seeing? Reordering issues, or something
>> else?
>
> I haven't actually been able to test this...
>
> But with the iwlwifi work we're doing, at the very least we'd waste a
> hardware queue for the case that basically never happens, since you'd
> end up putting these frames (that are very few) on a separate TXQ and
> thus hardware queue.

Ah, right, you're doing 1-to-1 TXQ-to-HWQ mapping. Gotcha.

> You could argue we should explicitly _not_ do this, but then we should
> also set skb->priority to be non-zero for non-QoS stations. Then we
> could benefit from some form of QoS (between the TXQs) for non-QoS
> connections, but that seems pretty complex and doesn't seem worth it
> since all connections that want anything from HT/11n and newer need QoS
> anyway.
>
> So basically this gets rid of a corner case that we shouldn't have.
> Either we should decide that using different TXQs is *always* correct
> for non-QoS, or - what I thought - that this isn't worth it, and then we
> should *never* do it.

Yeah, I agree that this is not worth it. The queue is already
FQ-CoDel'ed, which gives us most of the benefit of QoS anyway :)

-Toke

Re: [PATCH] mac80211: use non-zero TID only for QoS frames

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> From: Johannes Berg 
>
> Some frames may have a non-zero skb->priority assigned by
> mac80211 internally, e.g. TDLS setup frames, regardless of
> support for QoS.
>
> Currently, we set skb->priority to 0 for all data frames.
> Note that there's a comment that this is "required for
> correct WPA/11i MIC", but that doesn't seem true as we use
>
> if (ieee80211_is_data_qos(hdr->frame_control))
> qos_tid = ieee80211_get_tid(hdr);
> else
> qos_tid = 0;
>
> in the code there. We could therefore reconsider this, but
> it seems like unnecessary complexity for the unlikely (and
> not very useful) case of not having QoS on the connection.
>
> This situation then causes something strange - most data
> frames will go on TXQ for TID 0 for non-QoS connections,
> but very few exceptions that are internally generated will
> go on another TXQ, possibly causing confusion.

What kind of confusion are you seeing? Reordering issues, or something
else?

-Toke

Re: [RFC v2 2/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

>> > Perhaps then it should just be fractional? i.e. 8.8 bits or so?, so
>> > the default would be 1.0 (0x0100) and then you could scale down to 0.5
>> > (0x0080) etc?
>> 
>> Hmm, that's an interesting idea. I'll have to run some numbers to see
>> how the precision holds up on various examples; but that would allow us
>> to get rid of the quantum in the userspace API entirely, which is a good
>> thing as far as I'm concerned!
>
> :-)
>
> We can always go to 32 bits and then we have lots to play with? It's 16
> bits right now so I picked 8.8 for no real reason - in fact that seems
> quite pointless since dividing 300us by 256 gives you only just over 1us
> which is really short.
>
> So perhaps 12.4 (minimum 18 usec) is more appropriate?
>
> But perhaps I'm just completely mistaken and you don't really want to be
> able to scale the quantum down further at all, and so you have to play
> with all the stations anyway?
>
> No idea, really, how you'd use this.

I have a patch set for hostapd that will allow a user to configure
priorities (I can send you the paper if you want details). The obvious
thing is just to prioritise a specific station by mac address ("give my
TV twice the airtime so it can stream even though it's too far away from
AP"). But the more interesting thing is the dynamic prioritisation
(e.g., "these two SSIDs should share equally no matter how many clients
they have"). For that, group weights need to be calculated and turned
into station weights by dividing with the number of active stations per
group. I've been using integer weights and scaling in userspace, but if
we just express it as fractions in the netlink API that need would go
away.

Hmm, I'll think about which mode is easier :)
>
>> > > For the drivers that get airtime as part of TX completion, sure; but as
>> > > I understand it, at least ath10k gets airtime information in out of band
>> > > status reports, so there would need to be a callback for that in any
>> > > case...
>> > 
>> > Hmm, ok, but perhaps then we should also tie those to the existing
>> > airtime things?
>> 
>> Eh? Tie what to which existing airtime things?
>
> At least WMM-AC: ieee80211_sta_tx_wmm_ac_notify().

Gotcha; will look at that as well.

-Toke

Re: [RFC v2 2/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Wed, 2018-08-29 at 11:27 +0200, Toke Høiland-Jørgensen wrote:
>
>> Hmm, the problem with a higher weight is that weight*quantum becomes the
>> time each station is scheduled, so having a higher value means higher
>> latency. This could be fixed by replacing the station weights with
>> per-station quantums, but I felt that this was exposing an
>> implementation detail in the API; if we ever change the enforcement
>> mechanism to not be quantum-based (as may be necessary for MU-MIMO for
>> instance), we'll have to convert values in the kernel. Whereas weights
>> are a conceptual measure that is not tied to any particular
>> implementation.
>
> Ok, but that's also an effect you should describe in the API for it.

What's the right place to put that? In the netlink header file?

> Perhaps then it should just be fractional? i.e. 8.8 bits or so?, so
> the default would be 1.0 (0x0100) and then you could scale down to 0.5
> (0x0080) etc?

Hmm, that's an interesting idea. I'll have to run some numbers to see
how the precision holds up on various examples; but that would allow us
to get rid of the quantum in the userspace API entirely, which is a good
thing as far as I'm concerned!

>> For the drivers that get airtime as part of TX completion, sure; but as
>> I understand it, at least ath10k gets airtime information in out of band
>> status reports, so there would need to be a callback for that in any
>> case...
>
> Hmm, ok, but perhaps then we should also tie those to the existing
> airtime things?

Eh? Tie what to which existing airtime things?

-Toke

Re: [RFC v2 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Wed, 2018-08-29 at 11:22 +0200, Toke Høiland-Jørgensen wrote:
>
>> > We're also working on adding a TXQ for (bufferable) management packets
>> > - I wonder how that should interact here? Always be scheduled first?
>> 
>> Ah, cool! It may be that it should be given priority, yeah. Presently,
>> the multicast queue just rotates in the RR with the others, but is never
>> throttled as it doesn't have an airtime measure (though perhaps it
>> should)? But that might not be desirable for management frames, as
>> presumable those need to go out as fast as possible?
>
> Good question. I guess the multicast should have an airtime measure,
> but I don't think we want to do accounting on the management. That
> really should be few frames, and we want them out ASAP in most cases.

Yup, makes sense.

>> Hmm, I seem to recall thinking about just putting the call to
>> schedule_txq() into drv_wake_tx_queue; don't remember why I ended up
>> dropping that; but will take another look at whether it makes sense to
>> combine things.
>
> I was thinking the other way around - but that doesn't work since you'd
> loop from the driver to itself. This way might work, I guess, hadn't
> considered that.
>
> Might be better anyway though to make a new inline that does both, since
> the drv_() calls usually really don't do much on their own (other than
> checks, and in one case backward compatibility code, but still)

ACK, I'll look into that.

-Toke

Re: [RFC v2 2/2] mac80211: manage txq transmission based on airtime deficit

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> Once a txq is selected for transmission by next_txq(), all data from
> txq are dequeued by driver till hardware descriptors are drained.
> i.e the driver is still allowed to dequeue frames from txq even when
> its fairness deficit goes negative during transmission. This breaks
> fairness and also cause inefficient fq-codel in mac80211 when data
> queues are maintained in driver/firmware. To address this problem,
> pause txq transmission immediately upon driver airtime report.

Hmm, interesting observation about the queues moving to mac80211. Not
sure it will actually work that way; depends on how timely the ath10k
airtime report is, I guess. But would be interesting to test! :)

Just one comment on your patch, below.

> +static bool ieee80211_txq_requeued(struct ieee80211_local *local,
> +struct txq_info *txqi)
> +{
> + struct sta_info *sta;
> +
> + lockdep_assert_held(>active_txq_lock);
> +
> + if (!txqi->txq.sta)
> + return false;
> +
> + sta = container_of(txqi->txq.sta, struct sta_info, sta);
> +
> + if (sta->airtime.deficit[txqi->txq.ac] > 0) {
> + clear_bit(IEEE80211_TXQ_PAUSE, >flags);
> + return false;
> + }
> +
> + sta->airtime.deficit[txqi->txq.ac] +=
> + local->airtime_quantum * sta->airtime.weight;
> + list_move_tail(>schedule_order,
> +>active_txqs[txqi->txq.ac]);
> +
> + return true;
> +}
> +
>  struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
>bool inc_seqno)
>  {
> @@ -3655,18 +3682,8 @@ struct ieee80211_txq *ieee80211_next_txq(struct 
> ieee80211_hw *hw, s8 ac,
>   if (!txqi)
>   goto out;
>  
> - if (txqi->txq.sta) {
> - struct sta_info *sta = container_of(txqi->txq.sta,
> - struct sta_info, sta);
> -
> - if (sta->airtime.deficit[txqi->txq.ac] < 0) {
> - sta->airtime.deficit[txqi->txq.ac] +=
> - local->airtime_quantum * sta->airtime.weight;
> - list_move_tail(>schedule_order,
> ->active_txqs[txqi->txq.ac]);
> - goto begin;
> - }
> - }
> + if (ieee80211_txq_requeued(local, txqi))
> + goto begin;
>  
>   /* If seqnos are equal, we've seen this txqi before in this scheduling
>* round, so abort.
> @@ -3687,6 +3704,39 @@ struct ieee80211_txq *ieee80211_next_txq(struct 
> ieee80211_hw *hw, s8 ac,
>  }
>  EXPORT_SYMBOL(ieee80211_next_txq);
>  
> +bool ieee80211_txq_can_transmit(struct ieee80211_hw *hw,
> + struct ieee80211_txq *txq)
> +{
> + struct ieee80211_local *local = hw_to_local(hw);
> + struct txq_info *txqi, *f_txqi;
> + bool can_tx;
> +
> + txqi = to_txq_info(txq);
> + /* Check whether txq is paused or not */
> + if (test_bit(IEEE80211_TXQ_PAUSE, >flags))
> + return false;
> +
> + can_tx = false;
> + spin_lock_bh(>active_txq_lock);
> + f_txqi = find_txqi(local, txq->ac);
> + if (!f_txqi)
> + goto out;
> +
> + /* Allow only head node to ensure fairness */
> + if (f_txqi != txqi)
> + goto out;
> +
> + /* Check if txq is in negative deficit */
> + if (!ieee80211_txq_requeued(local, txqi))
> + can_tx = true;
> +
> + list_del_init(>schedule_order);

Why are you removing the txq from the list here, and how do you expect
it to get added back?

-Toke

Re: [RFC v2 2/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Mon, 2018-07-09 at 18:37 +0200, Toke Høiland-Jørgensen wrote:
>> 
>> @@ -427,6 +428,8 @@ struct sta_info *sta_info_alloc(struct 
>> ieee80211_sub_if_data *sdata,
>>  sta->cparams.interval = MS2TIME(100);
>>  sta->cparams.ecn = true;
>>  
>> +sta->airtime.weight = 1;
>
> Perhaps it might be useful to start with a higher default (even
> something like 1<<8) as that would allow adjusting up/down single
> stations, without having to adjust all stations and listening to new
> additions to adjust them quickly etc?
>
> Theoretically this doesn't really matter, but from a practical POV it
> may be easier to leave them all at the default and just adjust the ones
> that need adjustment for some reason.

Hmm, the problem with a higher weight is that weight*quantum becomes the
time each station is scheduled, so having a higher value means higher
latency. This could be fixed by replacing the station weights with
per-station quantums, but I felt that this was exposing an
implementation detail in the API; if we ever change the enforcement
mechanism to not be quantum-based (as may be necessary for MU-MIMO for
instance), we'll have to convert values in the kernel. Whereas weights
are a conceptual measure that is not tied to any particular
implementation.

>> ieee80211_sta_register_airtime
>
> Do we really need this? We already have at least TX status with
> airtime, for ieee80211_sta_tx_notify() and friends, and the station
> pointer in that context, so couldn't we piggy-back on this? At least
> WMM-AC already requires the driver to provide this.

For the drivers that get airtime as part of TX completion, sure; but as
I understand it, at least ath10k gets airtime information in out of band
status reports, so there would need to be a callback for that in any
case...

-Toke

Re: [RFC v2 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> Rather belatedly reviewing this too ...
>
>> + * The driver can't access the queue directly. To dequeue a frame from a
>> + * txq, it calls ieee80211_tx_dequeue(). Whenever mac80211 adds a new frame 
>> to a
>> + * queue, it calls the .wake_tx_queue driver op.
>> + *
>> + * Drivers can optionally delegate responsibility for scheduling queues to
>> + * mac80211, to take advantage of airtime fairness accounting. In this 
>> case, to
>> + * obtain the next queue to pull frames from, the driver calls
>> + * ieee80211_next_txq(). The driver is then expected to re-schedule the txq
>> + * using ieee80211_schedule_txq() if it is still active after the driver has
>> + * finished pulling packets from it.
>
> Maybe you could clarify what "is still active" means here? I'm
> guessing it means "tx_dequeue() would return non-NULL" but I doubt you
> really want such a strong requirement, perhaps "the last tx_dequeue()
> returned non-NULL" is sufficient?

Yeah, the latter should suffice. Really it should be put back "if it is
not known to be empty", but, well, that doesn't read so well...

> We're also working on adding a TXQ for (bufferable) management packets
> - I wonder how that should interact here? Always be scheduled first?

Ah, cool! It may be that it should be given priority, yeah. Presently,
the multicast queue just rotates in the RR with the others, but is never
throttled as it doesn't have an airtime measure (though perhaps it
should)? But that might not be desirable for management frames, as
presumable those need to go out as fast as possible?

>> +/**
>> + * ieee80211_schedule_txq - add txq to scheduling loop
>> + *
>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>> + * @txq: pointer obtained from station or virtual interface
>> + * @reset_seqno: Whether to reset the internal scheduling sequence number,
>> + *   allowing this txq to appear again in the current scheduling
>> + *   round (see doc for ieee80211_next_txq()).
>> + *
>> + * Returns %true if the txq was actually added to the scheduling,
>> + * %false otherwise.
>> + */
>> +bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
>> +struct ieee80211_txq *txq,
>> +bool reset_seqno);
>
> I concur with Rajkumar in a way that "seqno" is a really bad name for
> this since it's so loaded in the context of wifi. He didn't say it this
> way, but said it was an "internal to mac80211" concept here, and was
> perhaps also/more referring to the manipulations by drivers.
>
> Perhaps calling it something like scheduling_round or something would be
> better? That's not really what it is, schedule_id? Even schedule_seq[no]
> would be clearer.

Yeah, definitely going to change that :)

>> +/**
>> + * ieee80211_next_txq - get next tx queue to pull packets from
>> + *
>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>> + * @ac: filter returned txqs with this AC number. Pass -1 for no filtering.
>> + * @inc_seqno: Whether to increase the scheduling sequence number. Setting 
>> this
>> + * to true signifies the start of a new scheduling round. Each 
>> TXQ
>> + * will only be returned exactly once in each round (unless its
>> + * sequence number is explicitly reset when calling
>> + * ieee80211_schedule_txq()).
>
> Here you already talk about "scheduling sequence number" after all :)
>
>> +ieee80211_schedule_txq(>hw, >txq, true);
>>  drv_wake_tx_queue(local, txqi);
>
> These always seem to come paired - perhaps a helper is useful?
>
> Although it looks like there are sometimes different locking contexts,
> not sure if that's really necessary though.

Hmm, I seem to recall thinking about just putting the call to
schedule_txq() into drv_wake_tx_queue; don't remember why I ended up
dropping that; but will take another look at whether it makes sense to
combine things.

-Toke

Re: [RFC 3/3] mac80211: add ieee80211_reorder_txq

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

>>> As mentioned earlier, next_txq() can not be used for fetching txq
>>> directly. So reorder_txq() needs to take care of refilling txq after
>>> serving them.
>> 
>> Yeah, I got that; but see above. Unless there's a guarantee that the
>> push/pull mechanism will be round-robin scheduled (which as I'm reading
>> the code there isn't), just increasing the deficit on every call to
>> reorder_txq() is not going to ensure fairness (it'll probably help 
>> some,
>> but it won't be completely fair).
>> 
>> However, if we add the check to reorder_txq() so the deficit increase +
>> rotation is only done if the TXQ is at the head of the list, we'll get
>> round-robin-equivalent behaviour since a TXQ will only get to continue
>> when it happens to have rotated to the head of the queue. As I 
>> mentioned
>> previously it will break MIMO, but I we could fix that later once we've
>> verified that the basic mechanism works.
>> 
> Hmm... The pull mechanism operates in round-robin fashion. Agree that
> accessing only head node is right way of ensuring fairness.


Ah, excellent; then it should be no problem to enforce the head node
access; worst case the pull mechanism will be at a different point in
the round-robin, which will sync up quickly.

> Will fix it and rename as ieee80211_txq_can_transmit().

Cool. If you send an updated patchset I can fold it into an updated
version of my RFC and send it as a proper patch once I've verified
everything works with ath9k :)

> Thanks Toke for your feedback.

You're welcome! And thanks for working on this :)

-Toke

Re: [RFC 3/3] mac80211: add ieee80211_reorder_txq

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-08-21 05:24, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan  writes:
>> 
>>> This allows the driver to refill airtime fairness deficit
>>> where the driver will not access txqs by ieee80211_next_txq.
>>> In tx push mode data path, high priority txqs will be scheduled
>>> for data transmission by ieee80211_next_txq and driver will not
>>> prioritize txqs whereas in push-pull mode, the drivers can
>>> prioritize txqs and access them directly. In such mode, airtime
>>> deficit can not filled by ieee80211_next_txq.
>>> 
> [...]
>> 
>> This needs to check that the txq is currently at the head of
>> local->active_txqs[txqi->txq.ac]; otherwise fairness enforcement 
>> doesn't
>> work. And with this check I'm not sure the reorder function is terribly
>> useful for what you want to use it for?
>> 
> Hmm.. I tried to keep the same policy of next_txq(). i.e do not allow
> serving txqs when the deficit is negative. reorder_txq() limits over
> serving the same txq by driver. So that driver is given chance to
> serve other txqs.

Yeah, but the fairness comes from all TXQs being given the *same amount*
of deficit increase. I.e., with reorder_txq() there needs to be a
guarantee that it is called the same number of times for all active
TXQs. Which is what the round-robin scheduling ensures in next_txq().

> As mentioned earlier, next_txq() can not be used for fetching txq
> directly. So reorder_txq() needs to take care of refilling txq after
> serving them.

Yeah, I got that; but see above. Unless there's a guarantee that the
push/pull mechanism will be round-robin scheduled (which as I'm reading
the code there isn't), just increasing the deficit on every call to
reorder_txq() is not going to ensure fairness (it'll probably help some,
but it won't be completely fair).

However, if we add the check to reorder_txq() so the deficit increase +
rotation is only done if the TXQ is at the head of the list, we'll get
round-robin-equivalent behaviour since a TXQ will only get to continue
when it happens to have rotated to the head of the queue. As I mentioned
previously it will break MIMO, but I we could fix that later once we've
verified that the basic mechanism works.

Alternatively, if we keep it the way you've written it in this patch, we
can iterate on the driver API and make sure that airtime reporting
actually works for ath10k, and then change the scheduler once everything
else is in place...

-Toke

Re: [RFC 3/3] mac80211: add ieee80211_reorder_txq

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> This allows the driver to refill airtime fairness deficit
> where the driver will not access txqs by ieee80211_next_txq.
> In tx push mode data path, high priority txqs will be scheduled
> for data transmission by ieee80211_next_txq and driver will not
> prioritize txqs whereas in push-pull mode, the drivers can
> prioritize txqs and access them directly. In such mode, airtime
> deficit can not filled by ieee80211_next_txq.
>
> Signed-off-by: Rajkumar Manoharan 
> ---
>  include/net/mac80211.h | 15 +
>  net/mac80211/tx.c  | 59 
> +++---
>  2 files changed, 57 insertions(+), 17 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index cc16847bd52d..a2f0b6800100 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -6033,6 +6033,21 @@ bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
>  struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac);
>  
>  /**
> + * ieee80211_reorder_txq - change txq position in scheduling loop
> + *
> + * This function is used to reorder txq's position in scheduling loop.
> + * The txq fairness deficit will be refilled. The drivers calling this
> + * function should ensure the txq won't be accessed by ieee80211_next_txq
> + * in the same path.
> + *
> + * @hw: pointer as obtained from ieee80211_alloc_hw()
> + * @txq: pointer obtained from station or virtual interface
> + *
> + */
> +void ieee80211_reorder_txq(struct ieee80211_hw *hw,
> +struct ieee80211_txq *txq);
> +
> +/**
>   * ieee80211_txq_get_depth - get pending frame/byte count of given txq
>   *
>   * The values are not guaranteed to be coherent with regard to each other, 
> i.e.
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 0af35c08e0d9..b7b2f93152f8 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -3634,10 +3634,38 @@ static inline struct txq_info *find_txqi(struct 
> ieee80211_local *local, s8 ac)
>   return txqi;
>  }
>  
> +static bool ieee80211_txq_refill_deficit(struct ieee80211_local *local,
> +  struct txq_info *txqi)
> +{
> + struct fq *fq = >fq;
> + struct sta_info *sta;
> +
> + lockdep_assert_held(>active_txq_lock);
> +
> + if (!txqi->txq.sta)
> + return false;
> +
> + sta = container_of(txqi->txq.sta, struct sta_info, sta);
> +
> + if (sta->airtime.deficit[txqi->txq.ac] > 0)
> + return false;
> +
> + sta->airtime.deficit[txqi->txq.ac] +=
> + IEEE80211_AIRTIME_QUANTUM * sta->airtime.weight;
> + list_move_tail(>schedule_order,
> +>active_txqs[txqi->txq.ac]);

This needs to check that the txq is currently at the head of
local->active_txqs[txqi->txq.ac]; otherwise fairness enforcement doesn't
work. And with this check I'm not sure the reorder function is terribly
useful for what you want to use it for?

-Toke

Re: [PATCH] mac80211: Run TXQ teardown code before de-registering interfaces

[Toke Høiland-Jørgensen]

Arend van Spriel  writes:

> On 8/13/2018 2:16 PM, Toke Høiland-Jørgensen wrote:
>> The TXQ teardown code can reference the vif data structures that are
>> stored in the netdev private memory area if there are still packets on
>> the queue when it is being freed. Since the TXQ teardown code is run
>> after the netdevs are freed, this can lead to a use-after-free. Fix this
>> by moving the TXQ teardown code to earlier in ieee80211_unregister_hw().
>
> Just off the bat, but from reading the above I am wondering whether
> the use-after-free could also happen upon removing an interface?

Hmm, there doesn't appear to be *any* teardown of TXQs when an interface
is removed...? So I guess that if an interface is removed while it still
has frames on the multicast TXQ, that those packets would be left
hanging there? I don't think there would be an explicit use-after-free,
because they will never get dequeued, so they would just constitute a
memory leak?

Am I missing some automatic mechanism that always empties out queues
before an interface is brought down?

-Toke

[PATCH] mac80211: Run TXQ teardown code before de-registering interfaces

[Toke Høiland-Jørgensen]

The TXQ teardown code can reference the vif data structures that are
stored in the netdev private memory area if there are still packets on
the queue when it is being freed. Since the TXQ teardown code is run
after the netdevs are freed, this can lead to a use-after-free. Fix this
by moving the TXQ teardown code to earlier in ieee80211_unregister_hw().

Reported-by: Ben Greear 
Tested-by: Ben Greear 
Signed-off-by: Toke Høiland-Jørgensen 
---
 net/mac80211/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index e054a2fd8d38..98a5c15e8db1 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1170,6 +1170,7 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 #if IS_ENABLED(CONFIG_IPV6)
unregister_inet6addr_notifier(>ifa6_notifier);
 #endif
+   ieee80211_txq_teardown_flows(local);
 
rtnl_lock();
 
@@ -1198,7 +1199,6 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
skb_queue_purge(>skb_queue);
skb_queue_purge(>skb_queue_unreliable);
skb_queue_purge(>skb_queue_tdls_chsw);
-   ieee80211_txq_teardown_flows(local);
 
destroy_workqueue(local->workqueue);
wiphy_unregister(local->hw.wiphy);
-- 
2.18.0

Re: use-after free bug in hacked 4.16 kernel, related to fq_flow_dequeue

[Toke Høiland-Jørgensen]

Ben Greear  writes:

> On 08/02/2018 01:20 PM, Toke Høiland-Jørgensen wrote:
>> Ben Greear  writes:
>>
>>> On 08/02/2018 12:45 PM, Toke Høiland-Jørgensen wrote:
>>>> Ben Greear  writes:
>>>>
>>>>> This is from my hacked kernel, could be my fault. I thought the fq
>>>>> guys might want to know however...
>>>>
>>>> Hmm, nothing obvious comes to mind; fq_flow_dequeue() just dequeues a
>>>> packet from the queue; it only has two memory derefs, to fq->lock and
>>>> flow->queue. Don't see why either of those should be freed at this
>>>> point.
>>>>
>>>> Unless fq_adjust_removal() is being inlined, perhaps? Then I suppose the
>>>> flow->tin reference could be the problem, if the txq_info struct was
>>>> already freed; did you change anything around the handling of TXQs?
>>>
>>> I have worked on some stuff to fix other leaks and corruptions in ath10k 
>>> related
>>> to txqs, maybe that is part of this problem.  My full tree is here:
>>>
>>> https://github.com/greearb/linux-ct-4.16
>>>
>>> This bug in question is fairly repeatable on my current setup, which
>>> is high speed tx + rx on a 9984 NIC, with buggy firmware that crashes
>>> often in the tx path. I think the crash only happens when I rmmod the
>>> driver under load, but possibly some of the fw crash cleanup logic
>>> that ran previously is also involved.
>>
>> Yeah, if it happens under load that is consistent with packets being
>> queued.
>>
>> It seems that mac80211 frees the netdevs of an interface before flushing
>> the TXQs, which may be the cause of the bug you are seeing. Could you
>> try the patch below and see if that fixes the issue?
>
> I've run with this for a few days, and it seems to at least not cause
> any extra problems.  I mostly fixed the firmware crashing I was seeing
> before, so not certain it fixes the root cause of the crashes I
> saw before.  I'm going to roll this into my 4.16 ct kernel for wider
> testing.

Right, thanks for testing. I'll send a proper patch :)

-Toke

Re: use-after free bug in hacked 4.16 kernel, related to fq_flow_dequeue

[Toke Høiland-Jørgensen]

Ben Greear  writes:

> On 08/02/2018 12:45 PM, Toke Høiland-Jørgensen wrote:
>> Ben Greear  writes:
>>
>>> This is from my hacked kernel, could be my fault. I thought the fq
>>> guys might want to know however...
>>
>> Hmm, nothing obvious comes to mind; fq_flow_dequeue() just dequeues a
>> packet from the queue; it only has two memory derefs, to fq->lock and
>> flow->queue. Don't see why either of those should be freed at this
>> point.
>>
>> Unless fq_adjust_removal() is being inlined, perhaps? Then I suppose the
>> flow->tin reference could be the problem, if the txq_info struct was
>> already freed; did you change anything around the handling of TXQs?
>
> I have worked on some stuff to fix other leaks and corruptions in ath10k 
> related
> to txqs, maybe that is part of this problem.  My full tree is here:
>
> https://github.com/greearb/linux-ct-4.16
>
> This bug in question is fairly repeatable on my current setup, which
> is high speed tx + rx on a 9984 NIC, with buggy firmware that crashes
> often in the tx path. I think the crash only happens when I rmmod the
> driver under load, but possibly some of the fw crash cleanup logic
> that ran previously is also involved.

Yeah, if it happens under load that is consistent with packets being
queued.

It seems that mac80211 frees the netdevs of an interface before flushing
the TXQs, which may be the cause of the bug you are seeing. Could you
try the patch below and see if that fixes the issue?

-Toke


diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index e65c2abb2a54..d21ef14d327d 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1213,6 +1213,7 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 #if IS_ENABLED(CONFIG_IPV6)
unregister_inet6addr_notifier(>ifa6_notifier);
 #endif
+   ieee80211_txq_teardown_flows(local);
 
rtnl_lock();
 
@@ -1241,7 +1242,6 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
skb_queue_purge(>skb_queue);
skb_queue_purge(>skb_queue_unreliable);
skb_queue_purge(>skb_queue_tdls_chsw);
-   ieee80211_txq_teardown_flows(local);
 
destroy_workqueue(local->workqueue);
wiphy_unregister(local->hw.wiphy);

Re: use-after free bug in hacked 4.16 kernel, related to fq_flow_dequeue

[Toke Høiland-Jørgensen]

Ben Greear  writes:

> This is from my hacked kernel, could be my fault. I thought the fq
> guys might want to know however...

Hmm, nothing obvious comes to mind; fq_flow_dequeue() just dequeues a
packet from the queue; it only has two memory derefs, to fq->lock and
flow->queue. Don't see why either of those should be freed at this
point.

Unless fq_adjust_removal() is being inlined, perhaps? Then I suppose the
flow->tin reference could be the problem, if the txq_info struct was
already freed; did you change anything around the handling of TXQs?

-Toke

Re: [RFC v2 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

>>> A simple change in argument may break algo. What would be seqno of
>>> first packet of txq if queue_skb() isn't reset seqno?
>> 
>> It would be 0, which would be less than the current seqno in all cases
>> except just when the seqno counter wraps.
>> 
>
> One point should be mentioned in comment section of next_txq() that
> the driver has to ensure that next_txq() iteration is serialized i.e
> no parallel txq traversal are encouraged. Though txqs_list is maintained
> in mac80211, parallel iteration may change the behavior. For example
> I thought of get rid of txqs_lock in ath10k as txqs_list is moved to 
> mac80211.
> But it is still needed. right?

Hmm, the driver really shouldn't need to do any locking apart from using
the next_txq() API. But I think you are right that the seqno mechanism
doesn't play well with unserialised access to through next_txq() from
multiple CPUs. I'll see what I can do about that, and also incorporate
the other changes we've been discussing into a new RFC series.

> Hmm.. reorder_txq() API may not needed. Calling next_txq() takes care
> of reordering list though the driver is accessing txqs directly.

Right, as long as the next_txq() path is still called even while
fetch_ind() is active, that should be fine.

>>> If we don't handle this case, then ath10k driver can not claim
>>> mac80211 ATF support. Agree that MU-MIMO won't work with DDR
>>> scheduler. and it will impact MU-MIMO performace in ath10k when
>>> mac80211 ATF is claimed by ath10k.
>> 
>> Yeah, so the question is if this is an acceptable tradeoff? Do you have
>> any idea how often MU-MIMO actually provides a benefit in the real
>> world?
>> 
> Hmm.. We have some criteria of ~1.9 gain in Mu-MIMO test cases with 50
> 11ac clients.

Hmm, yeah, that would be a shame to lose; although I do think 50-client
deployments are still relatively rare for many people. So maybe we can
make airtime fairness something that can be switched on and off for
ath10k, depending on whether users think they will be needing MU-MIMO?
Until we come up with a scheduler that can handle it, of course...

-Toke

Re: [RFC v2 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-07-21 13:54, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan  writes:
>> 
>>> On 2018-07-19 07:18, Toke Høiland-Jørgensen wrote:
>>>> Rajkumar Manoharan  writes:
>>>> 
>>>>> On 2018-07-13 06:39, Toke Høiland-Jørgensen wrote:
>>>>>> Rajkumar Manoharan  writes:
>>> 
>>>>>> It is not generally predictable how many times this will loop 
>>>>>> before
>>>>>> exiting...
>>>>>> 
>>>>> Agree.. It would be better If the driver does not worry about txq
>>>>> sequence numbering. Perhaps one more API (ieee80211_first_txq) could
>>>>> solve this. Will leave it to you.
>>>> 
>>>> That is basically what the second parameter to next_txq() does in the
>>>> current patchset. It just needs to be renamed...
>>>> 
>>> Agree. As next_txq() increments seqno while starting loop for each AC,
>>> It seems bit confusing. i.e A single ath_txq_schedule_all call will
>>> bump schedule_seqno by 4. right?
>> 
>> Hmm, good point. Guess there should be one seqno per ac...
>> 
> I would prefer to keep separate list for each AC ;) Prepared one on
> top of your earlier merged changes. Now it needs revisit.

Fine with me. Only reason I used a single list + the filtering mechanism
was to keep things compatible with ath10k ;)

>>> Let assume below case where CPU1 is dequeuing skb from isr context and
>>> CPU2 is enqueuing skbs into same txq.
>>> 
>>> CPU1  CPU2
>>>   
>>> ath_txq_schedule
>>>-> ieee80211_next_txq(first)
>>>  -> inc_seq
>>>  -> delete from list
>>>  -> txq->seqno = local->seqno
>>>   ieee80211_tx/fast_xmit
>>>  -> 
>>> ieee80211_queue_skb
>>>  ->
>>> ieee80211_schedule_txq(reset_seqno)
>>>   -> list_add
>>>   -> 
>>> txqi->seqno
>>> = local->seqno - 1
>>> 
>>> In above sequence, it is quite possible that the same txq will be
>>> served repeatedly and other txqs will be starved. am I right? IMHO
>>> seqno mechanism will not guarantee that txqs will be processed only
>>> once in an iteration.
>> 
>> Yeah, ieee80211_queue_skb() shouldn't set reset_seqno; was 
>> experimenting
>> with that, and guess I ended up picking the wrong value. Thanks for
>> pointing that out :)
>> 
> A simple change in argument may break algo. What would be seqno of
> first packet of txq if queue_skb() isn't reset seqno?

It would be 0, which would be less than the current seqno in all cases
except just when the seqno counter wraps.

> I am fine in passing start of loop as arg to next_ntx(). But prefer to
> keep loop detecting within mac80211 itself by tracking txq same as
> ath10k. So that no change is needed in schedule_txq() arg list.
> thoughts?

If we remove the reset argument to schedule_txq we lose the ability for
the driver to signal that it is OK to dequeue another series of packets
from the same TXQ in the current scheduling round. I think that things
would mostly still work, but we may risk the hardware running idle for
short periods of time (in ath9k at least). I am not quite sure which
conditions this would happen under, though; and I haven't been able to
trigger it myself with my test hardware. So one approach would be to try
it out without the parameter and put it back later if it turns out to be
problematic...

>>>>>> Well, it could conceivably be done in a way that doesn't require
>>>>>> taking
>>>>>> the activeq_lock. Adding another STOP flag to the TXQ, for 
>>>>>> instance.
>>>>>> From an API point of view I think that is more consistent with what
>>>>>> we
>>>>>> have already...
>>>>>> 
>>>>> 
>>>>> Make sense. ieee80211_txq_may_pull would be better place to decide
>>>>> whether given txq is allowed for transmission. It also makes drivers
>>>>> do not have to worry about deficit. Still I may need
>>>>> ieee80211_reorder_txq API after processing txq. isn't it?
>>>> 
>>>> The way I was ass

Re: [RFC v2 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-07-19 07:18, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan  writes:
>> 
>>> On 2018-07-13 06:39, Toke Høiland-Jørgensen wrote:
>>>> Rajkumar Manoharan  writes:
>
>>>> It is not generally predictable how many times this will loop before
>>>> exiting...
>>>> 
>>> Agree.. It would be better If the driver does not worry about txq
>>> sequence numbering. Perhaps one more API (ieee80211_first_txq) could
>>> solve this. Will leave it to you.
>> 
>> That is basically what the second parameter to next_txq() does in the
>> current patchset. It just needs to be renamed...
>> 
> Agree. As next_txq() increments seqno while starting loop for each AC,
> It seems bit confusing. i.e A single ath_txq_schedule_all call will
> bump schedule_seqno by 4. right?

Hmm, good point. Guess there should be one seqno per ac...

> Let assume below case where CPU1 is dequeuing skb from isr context and
> CPU2 is enqueuing skbs into same txq.
>
> CPU1  CPU2
>   
> ath_txq_schedule
>-> ieee80211_next_txq(first)
>  -> inc_seq
>  -> delete from list
>  -> txq->seqno = local->seqno
>   ieee80211_tx/fast_xmit
>  -> ieee80211_queue_skb
>  -> 
> ieee80211_schedule_txq(reset_seqno)
>   -> list_add
>   -> txqi->seqno 
> = local->seqno - 1
>
> In above sequence, it is quite possible that the same txq will be
> served repeatedly and other txqs will be starved. am I right? IMHO
> seqno mechanism will not guarantee that txqs will be processed only
> once in an iteration.

Yeah, ieee80211_queue_skb() shouldn't set reset_seqno; was experimenting
with that, and guess I ended up picking the wrong value. Thanks for
pointing that out :)

>>>> Well, it could conceivably be done in a way that doesn't require 
>>>> taking
>>>> the activeq_lock. Adding another STOP flag to the TXQ, for instance.
>>>> From an API point of view I think that is more consistent with what 
>>>> we
>>>> have already...
>>>> 
>>> 
>>> Make sense. ieee80211_txq_may_pull would be better place to decide
>>> whether given txq is allowed for transmission. It also makes drivers
>>> do not have to worry about deficit. Still I may need
>>> ieee80211_reorder_txq API after processing txq. isn't it?
>> 
>> The way I was assuming this would work (and what ath9k does), is that a
>> driver only operates on one TXQ at a time; so it can get that txq,
>> process it, and re-schedule it. In which case no other API is needed;
>> the rotating can be done in next_txq(), and schedule_txq() can insert
>> the txq back into the rotation as needed.
>> 
>> However, it sounds like this is not how ath10k does things? See below.
>> 
> correct. The current rotation works only in push-only mode. i.e when
> firmware is not deciding txqs and the driver picks priority txq from
> active_txqs list. Unfortunately rotation won't work when the driver
> selects txq other than first in the list. In any case separate API
> needed for rotation when the driver is processing few packets from txq
> instead of all pending frames.

Rotation is not dependent on the TXQ draining completely. Dequeueing a
few packets, then rescheduling the TXQ is fine.

>> And if so, how does that interact with ath10k_mac_tx_push_pending()?
>> That function is basically doing the same thing that I explained above
>> for ath9k; in the previous version of this patch series I modified that
>> to use next_txq(). But is that a different TX path, or am I
>> misunderstanding you?
>> 
>> If you could point me to which parts of the ath10k code I should be
>> looking at, that would be helpful as well :)
>> 
> Depends on firmware htt_tx_mode (push/push_pull),
> ath10k_mac_tx_push_pending() downloads all/partial frames to firmware.
> Please take a look at ath10k_mac_tx_can_push() in push_pending(). Let
> me know If you need any other details.

Right, so looking at this, it looks like the driver will loop through
all the available TXQs, trying to dequeue from each of them if
ath10k_mac_tx_can_push() returns true, right? This should actually work
fine with the next_txq() / schedule_txq() API. Without airtime fairness
that will just translate into basically what 

Re: [RFC v2 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-07-13 06:39, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan  writes:
>> 
> [...]
>
>>> Hmm... I thought driver will call ieee80211_schedule_txq when it runs
>>> out of hardware descriptor and break the loop. The serving txq will be
>>> added back to head of activeq list. no?
>> 
>> Yes, and then the next one will be serviced... It's basically:
>> 
>> while (!hwq_is_full()) {
>>  txq = next_txq():
>>  build_one_aggr(txq); // may or may not succeed
>>  if (!empty(txq))
>>schedule_txq(txq);
>> }
>> 
>> It is not generally predictable how many times this will loop before
>> exiting...
>> 
> Agree.. It would be better If the driver does not worry about txq
> sequence numbering. Perhaps one more API (ieee80211_first_txq) could
> solve this. Will leave it to you.

That is basically what the second parameter to next_txq() does in the
current patchset. It just needs to be renamed...

>>>>> 
>>>>> ieee80211_txq_get_depth
>>>>>   - return deficit status along with frm_cnt
>>>>> 
>>>>> ieee80211_reorder_txq
>>>>>   - if txq deficit > 0
>>>>> - return;
>>>>>   - if txq is last
>>>>>  - return
>>>>>   - delete txq from list
>>>>>   - move it to tail
>>>>>   - update deficit by quantum
>>>>> 
>>>>> ath10k_htt_rx_tx_fetch_ind
>>>>>   - get txq deficit status
>>>>>   - if txq deficit > 0
>>>>> - dequeue skb
>>>>>   - else if deficit < 0
>>>>> - return NULL
>>>>> 
>>>>> Please share your thoughts.
>>>> 
>>>> Hmm, not sure exactly how this would work; seems a little 
>>>> complicated?
>>>> Also, I'd rather if drivers were completely oblivious to the deficit;
>>>> that is a bit of an implementation detail...
>>>> 
>>> Agree.. Initially I thought of adding deficit check in
>>> ieee80211_tx_dequeue.
>>> But It will be overhead of taking activeq_lock for every skbs. Perhaps
>>> it can be renamed as allowed_to_dequeue instead of deficit.
>>> 
>>>> We could have an ieee80211_txq_may_pull(); or maybe just have
>>>> ieee80211_tx_dequeue() return NULL if the deficit is negative?
>>>> 
>>> As I said earlier, checking deficit for every skb will be an overhead.
>>> It should be done once before accessing txq.
>> 
>> Well, it could conceivably be done in a way that doesn't require taking
>> the activeq_lock. Adding another STOP flag to the TXQ, for instance.
>> From an API point of view I think that is more consistent with what we
>> have already...
>> 
>
> Make sense. ieee80211_txq_may_pull would be better place to decide
> whether given txq is allowed for transmission. It also makes drivers
> do not have to worry about deficit. Still I may need
> ieee80211_reorder_txq API after processing txq. isn't it?

The way I was assuming this would work (and what ath9k does), is that a
driver only operates on one TXQ at a time; so it can get that txq,
process it, and re-schedule it. In which case no other API is needed;
the rotating can be done in next_txq(), and schedule_txq() can insert
the txq back into the rotation as needed.

However, it sounds like this is not how ath10k does things? See below.

>>>> the reasonable thing for the driver to do, then, would be to ask
>>>> ieee80211_next_txq() for another TXQ to pull from if the current one
>>>> doesn't work for whatever reason.
>>>> 
>>>> Would that work for push-pull mode?
>>>> 
>>> Not really. Driver shouldn't send other txq data instead of asked one.
>> 
>> I didn't necessarily mean immediately. As long as it does it 
>> eventually.
>> If a TXQ's deficit runs negative, that TXQ will not be allowed to send
>> again until its deficit has been restored to positive through enough
>> cycles of the loop in next_txq().
>> 
>
> Thats true. Are you suggesting to run the loop until the txq deficit
> becomes positive?

Yeah, or rather until the first station with a positive deficit is
found.

>>> In MU-MIMO, firmware will query N packets from given set of {STA,TID}.
>>> So the driver not supposed to send other txq's data.
>> 
>> Hmm, it'll actually be interesting to see how the airtime fairness
>> scheduler interacts with MU-MIMO. I

Re: [RFC v2 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-07-12 16:13, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan  writes:
>> 
>>> On 2018-07-11 13:48, Toke Høiland-Jørgensen wrote:
>>>> Rajkumar Manoharan  writes:
>>>> 
>>>>> On 2018-07-09 09:37, Toke Høiland-Jørgensen wrote:
>>>>> [...]
>>>>>> +/**
> [...]
>
>>>> Erm, how would this prevent an infinite loop? With this scheme, at 
>>>> some
>>>> point, ieee80211_next_txq() removes the last txq from activeq, and
>>>> returns that. Then, when it is called again the next time the driver
>>>> loops, it's back to the first case (activeq empty, waitq non-empty); 
>>>> so
>>>> it'll move waitq back as activeq and start over... Only the driver
>>>> really knows when it is starting a logical "loop through all active
>>>> TXQs".
>>>> 
>>> Oops.. My bad.. The idea is that ieee80211_next_txq process txq from
>>> activeq only and keep processed txqs separately. Having single list
>>> eventually needs tracking mechanism. The point is that once activeq
>>> becomes empty, splice waitq list and return NULL. So that driver can
>>> break from the loop.
>>> 
>>> ieee80211_next_txq
>>>   - if activeq empty,
>>>- move waitq list into activeq
>>>- return NULL
>>> 
>>>   - if activeq not empty
>>>- fetch appropriate txq from activeq
>>>- remove txq from activeq list.
>>> 
>>>   - If txq found, return txq else return NULL
>> 
>> Right, so this would ensure the driver only sees each TXQ once *if it
>> keeps looping*. But it doesn't necessarily; if the hardware queues fill
>> up, for instance, it might abort earlier. In that case it would need to
>> signal mac80211 that it is done for now, which is equivalent to
>> signalling when it starts a scheduler round.
>> 
> Hmm... I thought driver will call ieee80211_schedule_txq when it runs
> out of hardware descriptor and break the loop. The serving txq will be
> added back to head of activeq list. no?

Yes, and then the next one will be serviced... It's basically:

while (!hwq_is_full()) {
 txq = next_txq():
 build_one_aggr(txq); // may or may not succeed
 if (!empty(txq))
   schedule_txq(txq);
}

It is not generally predictable how many times this will loop before
exiting...

>>>> Also, for airtime fairness, the queues are not scheduled strictly
>>>> round-robin, so the dual-list scheme wouldn't work there either...
>>>> 
>>> As you know, ath10k driver will operate in two tx modes (push-only,
>>> push-pull). These modes will be switched dynamically depends on
>>> firmware load or resource availability. In push-pull mode, firmware
>>> will query N number of frames for set of STA, TID.
>> 
>> Ah, so it will look up the TXQ without looping through the list of
>> pending queues at all? Didn't realise that is what it's doing; yeah,
>> that would be problematic for airtime fairness :)
>> 
>>> So the driver will directly dequeue N number of frames from given txq.
>>> In this case txq ordering alone wont help. I am planning to add below
>>> changes in exiting API and add new API ieee80211_reorder_txq.
>>> 
>>> ieee80211_txq_get_depth
>>>   - return deficit status along with frm_cnt
>>> 
>>> ieee80211_reorder_txq
>>>   - if txq deficit > 0
>>> - return;
>>>   - if txq is last
>>>  - return
>>>   - delete txq from list
>>>   - move it to tail
>>>   - update deficit by quantum
>>> 
>>> ath10k_htt_rx_tx_fetch_ind
>>>   - get txq deficit status
>>>   - if txq deficit > 0
>>> - dequeue skb
>>>   - else if deficit < 0
>>> - return NULL
>>> 
>>> Please share your thoughts.
>> 
>> Hmm, not sure exactly how this would work; seems a little complicated?
>> Also, I'd rather if drivers were completely oblivious to the deficit;
>> that is a bit of an implementation detail...
>> 
> Agree.. Initially I thought of adding deficit check in 
> ieee80211_tx_dequeue.
> But It will be overhead of taking activeq_lock for every skbs. Perhaps
> it can be renamed as allowed_to_dequeue instead of deficit.
>
>> We could have an ieee80211_txq_may_pull(); or maybe just have
>> ieee80211_tx_dequeue() return NULL if the deficit is negative?
&

Re: [RFC v2 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-07-11 13:48, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan  writes:
>> 
>>> On 2018-07-09 09:37, Toke Høiland-Jørgensen wrote:
>>> [...]
>>>> +/**
>>>> + * ieee80211_schedule_txq - add txq to scheduling loop
>>>> + *
>>>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>>>> + * @txq: pointer obtained from station or virtual interface
>>>> + * @reset_seqno: Whether to reset the internal scheduling sequence
>>>> number,
>>>> + *   allowing this txq to appear again in the current
>>>> scheduling
>>>> + *   round (see doc for ieee80211_next_txq()).
>>>> + *
>>>> + * Returns %true if the txq was actually added to the scheduling,
>>>> + * %false otherwise.
>>>> + */
>>>> +bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
>>>> +  struct ieee80211_txq *txq,
>>>> +  bool reset_seqno);
>>>> +
>>>> +/**
>>>> + * ieee80211_next_txq - get next tx queue to pull packets from
>>>> + *
>>>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>>>> + * @ac: filter returned txqs with this AC number. Pass -1 for no
>>>> filtering.
>>>> + * @inc_seqno: Whether to increase the scheduling sequence number.
>>>> Setting this
>>>> + * to true signifies the start of a new scheduling 
>>>> round.
>>>> Each TXQ
>>>> + * will only be returned exactly once in each round
>>>> (unless its
>>>> + * sequence number is explicitly reset when calling
>>>> + * ieee80211_schedule_txq()).
>>>> + *
>>> Toke,
>>> 
>>> Seems like seqno is internal to mac80211 and meant for active_txq list
>>> manipulation. If so, why would drivers have to worry about increment
>>> or resetting seqno?
>> 
>> The drivers need to be able to signal when they start a new "scheduling
>> round" (which is the parameter to next_txq()), and when a queue should
>> be immediately rescheduled (which is the parameter to schedule_txq()).
>> See the subsequent patch to ath9k for how this is used; the latter is
>> signalled when a TXQ successfully dequeued an aggregate...
>> 
>> Now, you're right that the choice to track this via a sequence number 
>> is
>> an implementation detail internal to mac80211... so maybe the 
>> parameters
>> should be called something different.
>> 
>>> IMHO to avoid over serving same txq, two lists (activeq and waitq) can
>>> be used and always add new txq into waitq list. So that driver will
>>> not worry about mac80211 txq manipulation. Please correct me If Im
>>> wrong.
>>> 
>>> ieee80211_schedule_txq
>>> - if schedule_order empty, add txq into waitq list tail.
>>> 
>>> ieee80211_next_txq
>>> - if activeq empty,
>>>  - move waitq list into activeq
>>> 
>>> - if activeq not empty
>>>  - fetch appropriate txq from activeq
>>>  - remove txq from activeq list.
>>> 
>>> - If txq found, return txq else return NULL
>> 
>> 
>> Erm, how would this prevent an infinite loop? With this scheme, at some
>> point, ieee80211_next_txq() removes the last txq from activeq, and
>> returns that. Then, when it is called again the next time the driver
>> loops, it's back to the first case (activeq empty, waitq non-empty); so
>> it'll move waitq back as activeq and start over... Only the driver
>> really knows when it is starting a logical "loop through all active
>> TXQs".
>> 
> Oops.. My bad.. The idea is that ieee80211_next_txq process txq from
> activeq only and keep processed txqs separately. Having single list
> eventually needs tracking mechanism. The point is that once activeq
> becomes empty, splice waitq list and return NULL. So that driver can
> break from the loop.
>
> ieee80211_next_txq
>   - if activeq empty,
>- move waitq list into activeq
>- return NULL
>
>   - if activeq not empty
>- fetch appropriate txq from activeq
>- remove txq from activeq list.
>
>   - If txq found, return txq else return NULL

Right, so this would ensure the driver only sees each TXQ once *if it
keeps looping*. But it doesn't necessarily; if the hardware queues fill
up, for instance, 

Re: [RFC v2 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Rajkumar Manoharan  writes:

> On 2018-07-09 09:37, Toke Høiland-Jørgensen wrote:
> [...]
>> +/**
>> + * ieee80211_schedule_txq - add txq to scheduling loop
>> + *
>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>> + * @txq: pointer obtained from station or virtual interface
>> + * @reset_seqno: Whether to reset the internal scheduling sequence 
>> number,
>> + *   allowing this txq to appear again in the current 
>> scheduling
>> + *   round (see doc for ieee80211_next_txq()).
>> + *
>> + * Returns %true if the txq was actually added to the scheduling,
>> + * %false otherwise.
>> + */
>> +bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
>> +struct ieee80211_txq *txq,
>> +bool reset_seqno);
>> +
>> +/**
>> + * ieee80211_next_txq - get next tx queue to pull packets from
>> + *
>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>> + * @ac: filter returned txqs with this AC number. Pass -1 for no 
>> filtering.
>> + * @inc_seqno: Whether to increase the scheduling sequence number. 
>> Setting this
>> + * to true signifies the start of a new scheduling round. 
>> Each TXQ
>> + * will only be returned exactly once in each round 
>> (unless its
>> + * sequence number is explicitly reset when calling
>> + * ieee80211_schedule_txq()).
>> + *
> Toke,
>
> Seems like seqno is internal to mac80211 and meant for active_txq list
> manipulation. If so, why would drivers have to worry about increment
> or resetting seqno?

The drivers need to be able to signal when they start a new "scheduling
round" (which is the parameter to next_txq()), and when a queue should
be immediately rescheduled (which is the parameter to schedule_txq()).
See the subsequent patch to ath9k for how this is used; the latter is
signalled when a TXQ successfully dequeued an aggregate...

Now, you're right that the choice to track this via a sequence number is
an implementation detail internal to mac80211... so maybe the parameters
should be called something different.

> IMHO to avoid over serving same txq, two lists (activeq and waitq) can
> be used and always add new txq into waitq list. So that driver will
> not worry about mac80211 txq manipulation. Please correct me If Im
> wrong.
>
> ieee80211_schedule_txq
> - if schedule_order empty, add txq into waitq list tail.
>
> ieee80211_next_txq
> - if activeq empty,
>  - move waitq list into activeq
>
> - if activeq not empty
>  - fetch appropriate txq from activeq
>  - remove txq from activeq list.
>
> - If txq found, return txq else return NULL


Erm, how would this prevent an infinite loop? With this scheme, at some
point, ieee80211_next_txq() removes the last txq from activeq, and
returns that. Then, when it is called again the next time the driver
loops, it's back to the first case (activeq empty, waitq non-empty); so
it'll move waitq back as activeq and start over... Only the driver
really knows when it is starting a logical "loop through all active
TXQs".

Also, for airtime fairness, the queues are not scheduled strictly
round-robin, so the dual-list scheme wouldn't work there either...

-Toke

Re: [PATCHv2] mac80211: add stop/start logic for software TXQs

[Toke Høiland-Jørgensen]

Manikanta Pubbisetty  writes:

> On 7/10/2018 8:52 PM, Toke Høiland-Jørgensen wrote:
>> Manikanta Pubbisetty  writes:
>>
>>> On 7/10/2018 6:28 PM, Toke Høiland-Jørgensen wrote:
>>>
>>>>> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
>>>>> index 172aeae..d07f7f9 100644
>>>>> --- a/net/mac80211/ieee80211_i.h
>>>>> +++ b/net/mac80211/ieee80211_i.h
>>>>> @@ -818,6 +818,7 @@ enum txq_info_flags {
>>>>>   IEEE80211_TXQ_STOP,
>>>>>   IEEE80211_TXQ_AMPDU,
>>>>>   IEEE80211_TXQ_NO_AMSDU,
>>>>> + IEEE80211_TXQ_PAUSED,
>>>>>};
>>>> I think it would be a good idea to either rename the flags, or at least
>>>> add an explanation somewhere of the difference between a paused and a
>>>> stopped queue...
>>> Initially, the idea was to use IEEE80211_TXQ_STOP flag to indicate that
>>> iTXQs are stopped; since this flag was used in the aggregation code, I
>>> was unsure whether the same flag can be used to indicate the iTXQ stop
>>> condition.
>>> I could not find any better name for this:-).
>> Hmm, yeah, not sure whether the two code paths can stomp on each other
>> if you reuse the flag. It would be neat to be able to reuse it, though...
>>
>> Otherwise, how about renaming the old one to _STOP_AGGR and calling the
>> new one _STOP_NETIF or something?
>
> These ones are much better, thanks toke!!
> I would probably like to extend the name to _STOP_NETIF_TX; how about 
> keeping the old one as is  and renaming _TXQ_PAUSED to
> _TXQ_STOP_NETIF_TX ?

Sure, that works as well :)

-Toke

Re: [PATCHv2] mac80211: add stop/start logic for software TXQs

[Toke Høiland-Jørgensen]

> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index 172aeae..d07f7f9 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -818,6 +818,7 @@ enum txq_info_flags {
>   IEEE80211_TXQ_STOP,
>   IEEE80211_TXQ_AMPDU,
>   IEEE80211_TXQ_NO_AMSDU,
> + IEEE80211_TXQ_PAUSED,
>  };

I think it would be a good idea to either rename the flags, or at least
add an explanation somewhere of the difference between a paused and a
stopped queue...

>  /**
> @@ -1226,6 +1227,7 @@ struct ieee80211_local {
>  
>   struct sk_buff_head pending[IEEE80211_MAX_QUEUES];
>   struct tasklet_struct tx_pending_tasklet;
> + struct tasklet_struct wake_txqs_tasklet;

It's not quite clear to me why a tasklet is needed? Couldn't you just
call the ieee80211_wake_txqs() function at the same place where you
currently schedule the tasklet?

-Toke

Re: mac80211: AQM and block_tx

[Toke Høiland-Jørgensen]

Sven Eckelmann  writes:

> On Donnerstag, 5. Juli 2018 10:47:11 CEST Sven Eckelmann wrote:
>> Hi Toke,
>> 
>> we are currently testing DFS with ath10k and noticed that AQM seems to 
>> ignore 
>> cfg80211_csa_settings->block_tx. Problem is now that the channel switch is 
>> started on a detected radar
>> 
>>   echo 1 >/sys/kernel/debug/ieee80211/phy1/ath10k/dfs_simulate_radar
>> 
>> NL80211_ATTR_CH_SWITCH_BLOCK_TX is set for the channel switch by hostapd but 
>> the AP still sends QoS data to the client.
>> 
>> Was there a fix for such a problem which I might have missed? I've just 
>> worked 
>> around that by setting wake_tx_queue TO NULL in ath10k. This still must be 
>> verified but at least I didn't see any packets anymore on a monitor 
>> interface.
>
>
> Just as information, the ath10k Dakota devices went through DFS
> certification

Cool! (I think? Not quite sure what the implications of passing / not
passing certification are?)

> but the AQM had to be disabled. Otherwise we would have had problems
> with the FCC closing time.

Hmm, too bad; but I guess this can be fixed :)

-Toke

[RFC v2 4/4] ath9k: Switch to mac80211 TXQ scheduling and airtime APIs

[Toke Høiland-Jørgensen]

This moves the ath9k driver to use the mac80211 TXQ scheduling and
airtime accounting APIs, removing the corresponding state tracking
inside the driver.

Signed-off-by: Toke Høiland-Jørgensen 
---
 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  |  245 
 7 files changed, 76 insertions(+), 260 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h 
b/drivers/net/wireless/ath/ath9k/ath9k.h
index ef0de4f1312c..f01aedfef24f 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -112,8 +112,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct 
ath_descdma *dd,
 #define ATH_TXFIFO_DEPTH   8
 #define ATH_TX_ERROR   0x01
 
-#define ATH_AIRTIME_QUANTUM300 /* usec */
-
 /* Stop tx traffic 1ms before the GO goes away */
 #define ATH_P2P_PS_STOP_TIME   1000
 
@@ -246,10 +244,8 @@ struct ath_atx_tid {
s8 bar_index;
bool active;
bool clear_ps_filter;
-   bool has_queued;
 };
 
-void __ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid);
 void ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid);
 
 struct ath_node {
@@ -263,12 +259,9 @@ struct ath_node {
 
bool sleeping;
bool no_ps_filter;
-   s64 airtime_deficit[IEEE80211_NUM_ACS];
-   u32 airtime_rx_start;
 
 #ifdef CONFIG_ATH9K_STATION_STATISTICS
struct ath_rx_rate_stats rx_rate_stats;
-   struct ath_airtime_stats airtime_stats;
 #endif
u8 key_idx[4];
 
@@ -986,11 +979,6 @@ void ath_ant_comb_scan(struct ath_softc *sc, struct 
ath_rx_status *rs);
 
 #define ATH9K_NUM_CHANCTX  2 /* supports 2 operating channels */
 
-#define AIRTIME_USE_TX BIT(0)
-#define AIRTIME_USE_RX BIT(1)
-#define AIRTIME_USE_NEW_QUEUES BIT(2)
-#define AIRTIME_ACTIVE(flags) (!!(flags & (AIRTIME_USE_TX|AIRTIME_USE_RX)))
-
 struct ath_softc {
struct ieee80211_hw *hw;
struct device *dev;
@@ -1034,8 +1022,6 @@ struct ath_softc {
short nbcnvifs;
unsigned long ps_usecount;
 
-   u16 airtime_flags; /* AIRTIME_* */
-
struct ath_rx rx;
struct ath_tx tx;
struct ath_beacon beacon;
diff --git a/drivers/net/wireless/ath/ath9k/debug.c 
b/drivers/net/wireless/ath/ath9k/debug.c
index f685843a2ff3..ce62da4840e2 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -1456,9 +1456,6 @@ int ath9k_init_debug(struct ath_hw *ah)
 #endif
debugfs_create_file("tpc", 0600, sc->debug.debugfs_phy, sc, _tpc);
 
-   debugfs_create_u16("airtime_flags", 0600,
-  sc->debug.debugfs_phy, >airtime_flags);
-
debugfs_create_file("nf_override", 0600,
sc->debug.debugfs_phy, sc, _nf_override);
 
diff --git a/drivers/net/wireless/ath/ath9k/debug.h 
b/drivers/net/wireless/ath/ath9k/debug.h
index 249f8141cd00..559d9628f280 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -319,20 +319,12 @@ ath9k_debug_sync_cause(struct ath_softc *sc, u32 
sync_cause)
 void ath_debug_rate_stats(struct ath_softc *sc,
  struct ath_rx_status *rs,
  struct sk_buff *skb);
-void ath_debug_airtime(struct ath_softc *sc,
-  struct ath_node *an,
-  u32 rx, u32 tx);
 #else
 static inline void ath_debug_rate_stats(struct ath_softc *sc,
struct ath_rx_status *rs,
struct sk_buff *skb)
 {
 }
-static inline void ath_debug_airtime(struct ath_softc *sc,
- struct ath_node *an,
- u32 rx, u32 tx)
-{
-}
 #endif /* CONFIG_ATH9K_STATION_STATISTICS */
 
 #endif /* DEBUG_H */
diff --git a/drivers/net/wireless/ath/ath9k/debug_sta.c 
b/drivers/net/wireless/ath/ath9k/debug_sta.c
index a6f45f1bb5bb..bb6f3250aa30 100644
--- a/drivers/net/wireless/ath/ath9k/debug_sta.c
+++ b/drivers/net/wireless/ath/ath9k/debug_sta.c
@@ -242,59 +242,6 @@ static const struct file_operations fops_node_recv = {
.llseek = default_llseek,
 };
 
-void ath_debug_airtime(struct ath_softc *sc,
-   struct ath_node *an,
-   u32 rx,
-   u32 tx)
-{
-   struct ath_airtime_stats *astats = >airtime_stats;
-
-   astats->rx_airtime += rx;
-   astats->tx_airtime += tx;
-}
-
-static ssize_t read_airtime(struct file *file, char __user *user_buf,
-   size_t count, loff_t *ppos)
-{
-   struct ath_node *an = file-

[RFC v2 2/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

This adds airtime accounting and scheduling to the mac80211 TXQ
scheduler. A new callback, ieee80211_sta_register_airtime(), is added
that drivers can call to report airtime usage for stations, and an
extended feature flag is added that drivers can use to opt into airtime
fairness scheduling.

When the airtime fairness feature is enabled, mac80211 will schedule TXQs
(through ieee80211_next_txq()) in a way that enforces airtime fairness
between active stations. This scheduling works the same way as the ath9k
in-driver airtime fairness scheduling, but also adds weighted fairness
to support airtime policies.

If the extended feature is not set, the scheduler will default to
round-robin scheduling.

Signed-off-by: Toke Høiland-Jørgensen 
---
 include/net/mac80211.h   |   28 
 include/uapi/linux/nl80211.h |3 ++
 net/mac80211/debugfs.c   |3 ++
 net/mac80211/debugfs_sta.c   |   35 +
 net/mac80211/ieee80211_i.h   |3 ++
 net/mac80211/main.c  |2 +
 net/mac80211/sta_info.c  |   24 +
 net/mac80211/sta_info.h  |   17 
 net/mac80211/tx.c|   60 --
 9 files changed, 161 insertions(+), 14 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 18e43193b614..17759d55b7d4 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5291,6 +5291,34 @@ void ieee80211_sta_eosp(struct ieee80211_sta *pubsta);
  */
 void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
 
+/**
+ * ieee80211_sta_register_airtime - register airtime usage for a sta/tid
+ *
+ * Register airtime usage for a given sta on a given tid. The driver can call
+ * this function to notify mac80211 that a station used a certain amount of
+ * airtime. This information will be used by the TXQ scheduler to schedule
+ * stations in a way that ensures airtime fairness.
+ *
+ * The reported airtime should as a minimum include all time that is spent
+ * transmitting to the remote station, including overhead and padding, but not
+ * including time spent waiting for a TXOP. If the time is not reported by the
+ * hardware it can in some cases be calculated from the rate and known frame
+ * composition. When possible, the time should include any failed transmission
+ * attempts.
+ *
+ * The driver can either call this function synchronously for every packet or
+ * aggregate, or asynchronously as airtime usage information becomes available.
+ * TX and RX airtime can be reported together, or separately by setting one of
+ * them to 0.
+ *
+ * @pubsta: the station
+ * @tid: the TID to register airtime for
+ * @tx_airtime: airtime used during TX (in usec)
+ * @rx_airtime: airtime used during RX (in usec)
+ */
+void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
+   u32 tx_airtime, u32 rx_airtime);
+
 /**
  * ieee80211_iter_keys - iterate keys programmed into the device
  * @hw: pointer obtained from ieee80211_alloc_hw()
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 7acc16f34942..28550d01948f 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -5223,6 +5223,8 @@ enum nl80211_feature_flags {
  * @NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT: Driver/device can omit all data
  * except for supported rates from the probe request content if requested
  * by the %NL80211_SCAN_FLAG_MIN_PREQ_CONTENT flag.
+ * @NL80211_EXT_FEATURE_AIRTIME_FAIRNESS: Driver supports airtime fairness
+ *  scheduling.
  *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
@@ -5259,6 +5261,7 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_TXQS,
NL80211_EXT_FEATURE_SCAN_RANDOM_SN,
NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT,
+   NL80211_EXT_FEATURE_AIRTIME_FAIRNESS,
 
/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index b5adf3625d16..08d112dc8bd6 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -379,6 +379,9 @@ void debugfs_hw_add(struct ieee80211_local *local)
if (local->ops->wake_tx_queue)
DEBUGFS_ADD_MODE(aqm, 0600);
 
+   debugfs_create_u16("airtime_flags", 0600,
+  phyd, >airtime_flags);
+
statsd = debugfs_create_dir("statistics", phyd);
 
/* if the dir failed, don't put all the other things into the root! */
diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index 4105081dc1df..de4067bc11cd 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -192,6 +192,37 @@ static ssize_t sta_aqm_read(struct file *file, char __user 
*userbuf,
 }
 STA_OPS(aqm);
 
+static ssize_t sta_airtime_read(struct fil

[RFC v2 3/4] cfg80211: Add airtime statistics and settings

[Toke Høiland-Jørgensen]

This adds airtime statistics to the cfg80211 station dump, and also adds
a new parameter to set the airtime weight of each station. The latter
allows userspace to implement policies for different stations by varying
their weights.

Signed-off-by: Toke Høiland-Jørgensen 
---
 include/net/cfg80211.h   |   15 +++--
 include/uapi/linux/nl80211.h |   14 
 net/mac80211/cfg.c   |6 +
 net/mac80211/main.c  |2 +-
 net/mac80211/sta_info.c  |   15 +
 net/mac80211/sta_info.h  |4 ---
 net/wireless/core.c  |2 ++
 net/wireless/nl80211.c   |   50 ++
 8 files changed, 101 insertions(+), 7 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 9ba1f289c439..1480eccbffe9 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -988,6 +988,7 @@ enum station_parameters_apply_mask {
  * @support_p2p_ps: information if station supports P2P PS mechanism
  * @he_capa: HE capabilities of station
  * @he_capa_len: the length of the HE capabilities
+ * @airtime_weight: airtime scheduler weight for this station
  */
 struct station_parameters {
const u8 *supported_rates;
@@ -1017,6 +1018,7 @@ struct station_parameters {
int support_p2p_ps;
const struct ieee80211_he_cap_elem *he_capa;
u8 he_capa_len;
+   u16 airtime_weight;
 };
 
 /**
@@ -1284,6 +1286,8 @@ struct cfg80211_tid_stats {
  * @rx_beacon_signal_avg: signal strength average (in dBm) for beacons received
  * from this peer
  * @rx_duration: aggregate PPDU duration(usecs) for all the frames from a peer
+ * @tx_duration: aggregate PPDU duration(usecs) for all the frames to a peer
+ * @airtime_weight: current airtime scheduling weight
  * @pertid: per-TID statistics, see  cfg80211_tid_stats, using the last
  * (IEEE80211_NUM_TIDS) index for MSDUs not encapsulated in QoS-MPDUs.
  * Note that this doesn't use the @filled bit, but is used if non-NULL.
@@ -1330,10 +1334,12 @@ struct station_info {
 
u32 expected_throughput;
 
-   u64 rx_beacon;
+   u64 tx_duration;
u64 rx_duration;
+   u64 rx_beacon;
u8 rx_beacon_signal_avg;
struct cfg80211_tid_stats *pertid;
+   u16 airtime_weight;
s8 ack_signal;
s8 avg_ack_signal;
 };
@@ -2347,7 +2353,8 @@ enum cfg80211_connect_params_changed {
  * @WIPHY_PARAM_DYN_ACK: dynack has been enabled
  * @WIPHY_PARAM_TXQ_LIMIT: TXQ packet limit has been changed
  * @WIPHY_PARAM_TXQ_MEMORY_LIMIT: TXQ memory limit has been changed
- * @WIPHY_PARAM_TXQ_QUANTUM: TXQ scheduler quantum
+ * @WIPHY_PARAM_TXQ_QUANTUM: TXQ scheduler quantum (bytes)
+ * @WIPHY_PARAM_AIRTIME_QUANTUM: Airtime scheduler quantum (usec)
  */
 enum wiphy_params_flags {
WIPHY_PARAM_RETRY_SHORT = 1 << 0,
@@ -2359,8 +2366,11 @@ enum wiphy_params_flags {
WIPHY_PARAM_TXQ_LIMIT   = 1 << 6,
WIPHY_PARAM_TXQ_MEMORY_LIMIT= 1 << 7,
WIPHY_PARAM_TXQ_QUANTUM = 1 << 8,
+   WIPHY_PARAM_AIRTIME_QUANTUM = 1 << 9,
 };
 
+#define IEEE80211_DEFAULT_AIRTIME_QUANTUM  300 /* usec */
+
 /**
  * struct cfg80211_pmksa - PMK Security Association
  *
@@ -4104,6 +4114,7 @@ struct wiphy {
u32 txq_limit;
u32 txq_memory_limit;
u32 txq_quantum;
+   u16 airtime_quantum;
 
char priv[0] __aligned(NETDEV_ALIGN);
 };
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 28550d01948f..c3ac69640475 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2241,6 +2241,11 @@ enum nl80211_commands {
  * association request when used with NL80211_CMD_NEW_STATION). Can be set
  * only if %NL80211_STA_FLAG_WME is set.
  *
+ * @NL80211_ATTR_AIRTIME_WEIGHT: Station's weight when scheduled by the airtime
+ *  scheduler.
+ * @NL80211_ATTR_AIRTIME_QUANTUM: Airtime scheduler quantum (usec). Airtime
+ *  share given to each station on each round of the airtime scheduler.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2682,6 +2687,9 @@ enum nl80211_attrs {
 
NL80211_ATTR_HE_CAPABILITY,
 
+   NL80211_ATTR_AIRTIME_WEIGHT,
+   NL80211_ATTR_AIRTIME_QUANTUM,
+
/* add attributes here, update the policy in nl80211.c */
 
__NL80211_ATTR_AFTER_LAST,
@@ -3052,6 +3060,10 @@ enum nl80211_sta_bss_param {
  * @NL80211_STA_INFO_ACK_SIGNAL: signal strength of the last ACK frame(u8, dBm)
  * @NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG: avg signal strength of (data)
  * ACK frame (s8, dBm)
+ * @NL80211_STA_INFO_TX_DURATION: aggregate PPDU duration for all frames
+ * sent to the station (u64, usec)
+ * @NL80211_STA_INFO_AIRTIME_WEIGHT: current airtime weight for station
+ *  (u16, usec)
  * @__NL80211

[RFC v2 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

This adds an API to mac80211 to handle scheduling of TXQs. The API
consists of two new functions: ieee80211_next_txq() and
ieee80211_schedule_txq(). The former returns the next TXQ that should be
scheduled, and is how the driver gets a queue to pull packets from. The
latter is called internally by mac80211 to start scheduling a queue, and
the driver is supposed to call it to re-schedule the TXQ after it is
finished pulling packets from it (unless the queue emptied). Drivers can
optionally filter TXQs on ac to support per-AC hardware queue designs,
and a sequence number mechanism is used to support drivers looping over
all available TXQs exactly once.

Using this API allows drivers to take advantage of mac80211 scheduling
features such as airtime fairness (added in a subsequent commit).
However, usage of the new API is optional, so support can be added to
individual drivers one at a time.

Signed-off-by: Toke Høiland-Jørgensen 
---
 include/net/mac80211.h |   50 +---
 net/mac80211/agg-tx.c  |2 +
 net/mac80211/ieee80211_i.h |7 
 net/mac80211/main.c|3 ++
 net/mac80211/sta_info.c|3 ++
 net/mac80211/tx.c  |   69 
 6 files changed, 130 insertions(+), 4 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 5790f55c241d..18e43193b614 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -107,9 +107,16 @@
  * The driver is expected to initialize its private per-queue data for stations
  * and interfaces in the .add_interface and .sta_add ops.
  *
- * The driver can't access the queue directly. To dequeue a frame, it calls
- * ieee80211_tx_dequeue(). Whenever mac80211 adds a new frame to a queue, it
- * calls the .wake_tx_queue driver op.
+ * The driver can't access the queue directly. To dequeue a frame from a
+ * txq, it calls ieee80211_tx_dequeue(). Whenever mac80211 adds a new frame to 
a
+ * queue, it calls the .wake_tx_queue driver op.
+ *
+ * Drivers can optionally delegate responsibility for scheduling queues to
+ * mac80211, to take advantage of airtime fairness accounting. In this case, to
+ * obtain the next queue to pull frames from, the driver calls
+ * ieee80211_next_txq(). The driver is then expected to re-schedule the txq
+ * using ieee80211_schedule_txq() if it is still active after the driver has
+ * finished pulling packets from it.
  *
  * For AP powersave TIM handling, the driver only needs to indicate if it has
  * buffered packets in the driver specific data structures by calling
@@ -5971,13 +5978,48 @@ void ieee80211_unreserve_tid(struct ieee80211_sta *sta, 
u8 tid);
  * ieee80211_tx_dequeue - dequeue a packet from a software tx queue
  *
  * @hw: pointer as obtained from ieee80211_alloc_hw()
- * @txq: pointer obtained from station or virtual interface
+ * @txq: pointer obtained from station or virtual interface, or from
+ *   ieee80211_next_txq()
  *
  * Returns the skb if successful, %NULL if no frame was available.
  */
 struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 struct ieee80211_txq *txq);
 
+/**
+ * ieee80211_schedule_txq - add txq to scheduling loop
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ * @reset_seqno: Whether to reset the internal scheduling sequence number,
+ *   allowing this txq to appear again in the current scheduling
+ *   round (see doc for ieee80211_next_txq()).
+ *
+ * Returns %true if the txq was actually added to the scheduling,
+ * %false otherwise.
+ */
+bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
+   struct ieee80211_txq *txq,
+   bool reset_seqno);
+
+/**
+ * ieee80211_next_txq - get next tx queue to pull packets from
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @ac: filter returned txqs with this AC number. Pass -1 for no filtering.
+ * @inc_seqno: Whether to increase the scheduling sequence number. Setting this
+ * to true signifies the start of a new scheduling round. Each TXQ
+ * will only be returned exactly once in each round (unless its
+ * sequence number is explicitly reset when calling
+ * ieee80211_schedule_txq()).
+ *
+ * Returns the next txq if successful, %NULL if no queue is eligible. If a txq
+ * is returned, it will have been removed from the scheduler queue and needs to
+ * be re-scheduled with ieee80211_schedule_txq() to continue to be active.
+ */
+struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
+bool inc_seqno);
+
 /**
  * ieee80211_txq_get_depth - get pending frame/byte count of given txq
  *
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 69e831bc317b..0a2e0d64fc11 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg

[RFC v2 0/4] Move TXQ scheduling into mac80211

[Toke Høiland-Jørgensen]

This is an updated version of my previous patch series to move TXQ
scheduling into mac80211, allowing more drivers to take advantage of
airtime fairness scheduling. This version keeps compatibility with the
old wake_tx_queue API, so drivers can be ported to the new API one at a
time. For now, only ath9k is changed.

There's also an update to the scheduling to use a sequence number to
keep track of scheduling rounds, so drivers have a clean way to loop
through all queued TXQs exactly once without having to keep track of the
first TXQ seen in a round.

Finally, the airtime fairness scheduler also incorporates weights into
the scheduling, which allows userspace to implement policies for how
airtime is divided between stations. I have a patch series for hostapd
to implement configurable policies, that I will post separately once the
kernel side is in place.

---

Toke Høiland-Jørgensen (4):
  mac80211: Add TXQ scheduling API
  mac80211: Add airtime accounting and scheduling to TXQs
  cfg80211: Add airtime statistics and settings
  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  |  245 
 include/net/cfg80211.h |   15 +-
 include/net/mac80211.h |   78 -
 include/uapi/linux/nl80211.h   |   17 ++
 net/mac80211/agg-tx.c  |2 
 net/mac80211/cfg.c |6 +
 net/mac80211/debugfs.c |3 
 net/mac80211/debugfs_sta.c |   35 
 net/mac80211/ieee80211_i.h |   10 +
 net/mac80211/main.c|5 +
 net/mac80211/sta_info.c|   42 +
 net/mac80211/sta_info.h|   13 +
 net/mac80211/tx.c  |  101 
 net/wireless/core.c|2 
 net/wireless/nl80211.c |   50 ++
 21 files changed, 449 insertions(+), 266 deletions(-)

Re: [PATCH] mac80211: don't put null-data frames on the normal TXQ

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Wed, 2018-07-04 at 09:26 +0200, Toke Høiland-Jørgensen wrote:
>> Johannes Berg  writes:
>> 
>> > I see no evidence of ath9k doing this correctly, so this might fix a
>> > bug there, but I may have missed it.
>> 
>> ath9k does check for this, in ath_tx_sched_aggr() and in
>> ath_tx_form_aggr(); it'll check for the IEEE80211_TX_CTL_AMPDU flag, and
>> stop building the current aggregate if the flag is not set.
>
> Ok, thanks. Nevertheless, I guess it's more efficient to not stop the
> aggregate on encountering a (QoS-)NDP :-)

Oh, absolutely! :)

-Toke

Re: [PATCH] mac80211: don't put null-data frames on the normal TXQ

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> I see no evidence of ath9k doing this correctly, so this might fix a
> bug there, but I may have missed it.

ath9k does check for this, in ath_tx_sched_aggr() and in
ath_tx_form_aggr(); it'll check for the IEEE80211_TX_CTL_AMPDU flag, and
stop building the current aggregate if the flag is not set.

-Toke

Re: [PATCH] mac80211: don't put null-data frames on the normal TXQ

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> From: Johannes Berg 
>
> Since (QoS) NDP frames shouldn't be put into aggregation nor are
> assigned real sequence numbers, etc. it's better to treat them as
> non-data packets and not put them on the normal TXQs, for example
> when building A-MPDUs they need to be treated specially, and they
> are more used for management (e.g. to see if the station is alive)
> anyway.

No objections to this per se; but didn't we want to move towards
everything going through the TXQs? Any progress on that front? :)

-Toke

Re: [PATCH] mac80211: add stop/start logic for software TXQs

[Toke Høiland-Jørgensen]

Manikanta Pubbisetty  writes:

> On 6/26/2018 4:55 PM, Toke Høiland-Jørgensen wrote:
>> Manikanta Pubbisetty  writes:
>>
>>> We can still invoke netif stop/wake APIs when wake_tx_queue is
>>> implemented but this could lead to packet drops in network layer;
>>> adding stop/start logic for software TXQs in mac80211 instead makes
>>> more sense; the change proposed adds the same in mac80211.
>> I agree with the approach; having packets queued in mac80211 while the
>> queues are stopped also means that CoDel can react to them when they are
>> resumed again.
>
> Agreed!!
>
>>
>>> +   list_for_each_entry_rcu(sta, >sta_list, list) {
>>> +   if (!atomic_read(>txqs_paused))
>>> +   continue;
>>> +
>>> +   atomic_set(>txqs_paused, 0);
>> I'm not terribly well-versed in the kernel atomics, but doesn't the
>> split of read and set kinda defeat the point of using them? Also, as
>> this is under RCU, why do you need an atomic in the first place?
> Valid point, txqs_paused is set in ieee80211_tx_dequeue; I was thinking 
> a case where tx_dequeue is called without rcu_read_lock but seems that 
> is not the case. All drivers using wake_tx_queue seems are using 
> rcu_read_lock before tx_dequeue; can there be a case where tx_dequeue is 
> called without rcu_read_lock?

I'm pretty sure we would have other problems if there were... :)

-Toke

Re: [PATCH] mac80211: add stop/start logic for software TXQs

[Toke Høiland-Jørgensen]

Manikanta Pubbisetty  writes:

> We can still invoke netif stop/wake APIs when wake_tx_queue is
> implemented but this could lead to packet drops in network layer;
> adding stop/start logic for software TXQs in mac80211 instead makes
> more sense; the change proposed adds the same in mac80211.

I agree with the approach; having packets queued in mac80211 while the
queues are stopped also means that CoDel can react to them when they are
resumed again.


> + list_for_each_entry_rcu(sta, >sta_list, list) {
> + if (!atomic_read(>txqs_paused))
> + continue;
> +
> + atomic_set(>txqs_paused, 0);

I'm not terribly well-versed in the kernel atomics, but doesn't the
split of read and set kinda defeat the point of using them? Also, as
this is under RCU, why do you need an atomic in the first place?

-Toke

RE: [PATCH 0/4] cfg80211/mac80211: Add support to configure and monitor txrate threshold

[Toke Høiland-Jørgensen]

Tamizh Chelvam Raja  writes:

>> This patchsets introduced new NL command and api to support 
>> configuring txrate threshold for the connected stations and api to 
>> notify userspace application upon crossing the configured txrate threshold.
>> This will be useful for the application which requires station's 
>> current capability change information.
>
> What is the intended use case? Asking mostly out of curiosity :)

> [Tamizh] This is to monitor txrate change for a station. By notifying
> userspace when the txrate for a station goes out of configured
> threshold, It can take steering decisions on the particular station.

Yeah, I got that part. I was curious as to what (userspace) application
you were planning to use this for? I.e., what kind of steering
decisions? :)

-Toke

Re: [PATCH 0/4] cfg80211/mac80211: Add support to configure and monitor txrate threshold

[Toke Høiland-Jørgensen]

Tamizh chelvam  writes:

> This patchsets introduced new NL command and api to support
> configuring txrate threshold for the connected stations and api to
> notify userspace application upon crossing the configured txrate threshold.
> This will be useful for the application which requires
> station's current capability change information.

What is the intended use case? Asking mostly out of curiosity :)

-Toke

Re: [PATCH] mac80211: Move up init of TXQs

[Toke Høiland-Jørgensen]

Niklas Cassel  writes:

> Tested-by: Niklas Cassel 

Great, thanks!

-Toke