Re: [Make-wifi-fast] [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

["Toke Høiland-Jørgensen" <toke@toke.dk>]

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

> On 2018-09-18 13:41, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan <rmanohar@codeaurora.org> 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