by Rajkumar Manoharan

[permalink] [raw]

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

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.

Two major differences b/w ath9k and ath10k

1) Serving txqs
The ath9k always serves txq by next_txq and so that the txqs_list is
rotated to serve
other txq. But in ath10k (pull-mode), first node becomes sticky one
until it is
picked by firmware via fetch indication and it becomes negative deficit.
The sequence is followed in wake_tx_queue

- dequeue first node
- push is not allowed
- enqueue same txq back to head

2) Refill rate of deficit.

The ath9k refills deficit mostly in hot path by next_txq() in tx & isr
routine.
In case of ath10k, due to above problem, deficits wont be filled in hot
path.
Either it should be filled in fetch_ind itself or by scheduling another
task.
Both the approaches are slower compared to hot path when the driver is
bursting
aggregation. On an idle condition a single fetch indication can dequeue
~190 msdus
from each tid of give stn list. This drains the deficit quickly and
becomes too low.
To speed up this, either refill the station by multiples of stn airtime
weight or
allows the txqs_list rotation. So that next_txq will be used for
refilling deficit.

Attaching return_txq() change that helps to get rid of quantum multiple.

-Rajkumar

Attachments:

return_tail.patch (5.14 kB)

2018-10-16 10:20:52

by Toke Høiland-Jørgensen

[permalink] [raw]

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

Rajkumar Manoharan <[email protected]> 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, &local->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(&txqi->schedule_order, &local->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