Re: [PATCH RFC v5 3/4] mac80211: Add airtime accounting and scheduling to TXQs
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
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.
-Rajkumardiff --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);
ieee80211_txq_schedule_end(hw, txq->ac);
record->num_msdus = cpu_to_le16(num_msdus);
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index cf64d9e02a24..d39bc841ea04 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4206,7 +4206,7 @@ static int ath10k_mac_schedule_txq(struct ieee80211_hw *hw, u32 ac)
if (ret < 0)
break;
}
- ieee80211_return_txq(hw, txq);
+ ieee80211_return_txq(hw, txq, true);
ath10k_htt_tx_txq_update(hw, txq);
if (ret == -EBUSY)
break;
@@ -4475,18 +4475,21 @@ static void ath10k_mac_op_wake_tx_queue(struct ieee80211_hw *hw,
{
u8 ac = txq->ac;
int ret = 0;
+ bool pushed = false;
+ ath10k_htt_tx_txq_update(hw, txq);
ieee80211_txq_schedule_start(hw, ac);
txq = ieee80211_next_txq(hw, ac);
if (!txq)
goto out;
while (ath10k_mac_tx_can_push(hw, txq)) {
+ pushed = true;
ret = ath10k_mac_tx_push_txq(hw, txq);
if (ret < 0)
break;
}
- ieee80211_return_txq(hw, txq);
+ ieee80211_return_txq(hw, txq, pushed);
ath10k_htt_tx_txq_update(hw, txq);
out:
ieee80211_txq_schedule_end(hw, ac);
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 6aab06909e76..40ff0bdbf7c9 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -117,7 +117,7 @@ void ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid)
struct ieee80211_txq *queue = container_of(
(void *)tid, struct ieee80211_txq, drv_priv);
- ieee80211_return_txq(sc->hw, queue);
+ ieee80211_return_txq(sc->hw, queue, false);
}
void ath9k_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq *queue)
@@ -1913,7 +1913,7 @@ void ath_txq_schedule(struct ath_softc *sc, struct ath_txq *txq)
ret = ath_tx_sched_aggr(sc, txq, tid);
ath_dbg(common, QUEUE, "ath_tx_sched_aggr returned %d\n", ret);
- ieee80211_return_txq(hw, queue);
+ ieee80211_return_txq(hw, queue, false);
}
out:
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 9cadfa408f50..995e19e29d9e 100644
---
Re: [Make-wifi-fast] [PATCH RFC v5 3/4] mac80211: Add airtime accounting and scheduling to TXQs
On Fri, Oct 12, 2018 at 12:38 AM Rajkumar Manoharan
wrote:
>
> 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.
> 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. 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?
I'll have some time in the coming weeks to be able to test this stuff.
I'm mostly interested
in algorithmic correctness more than the API changes...
Is there a version of these patches that is stable enough on ath9 or ath10k?
Do I foresee any issues? Jeeze, no, we *never* have any issues with wifi.
"fetch indication may pull ~4ms/8ms of packets from each tid"
made me really twitchy.
>
> #define IEEE80211_TXQ_MAY_TX_QUANTUM 20
> 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);
> struct sta_info *sta;
> u8 ac = txq->ac;
>
> lockdep_assert_held(>active_txq_lock[ac]);
>
> if (!txqi->txq.sta)
> goto out;
>
> sta = container_of(txqi->txq.sta, struct sta_info, sta);
> 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);
> }
>
> return false;
>
> out:
> list_del_init(>schedule_order);
> return true;
> }
>
> -Rajkumar
> ___
> Make-wifi-fast mailing list
> make-wifi-f...@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/make-wifi-fast
--
Dave Täht
CTO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-831-205-9740
Re: [PATCH RFC v5 3/4] mac80211: Add airtime accounting and scheduling to TXQs
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
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.
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. 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?
#define IEEE80211_TXQ_MAY_TX_QUANTUM 20
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);
struct sta_info *sta;
u8 ac = txq->ac;
lockdep_assert_held(>active_txq_lock[ac]);
if (!txqi->txq.sta)
goto out;
sta = container_of(txqi->txq.sta, struct sta_info, sta);
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);
}
return false;
out:
list_del_init(>schedule_order);
return true;
}
-Rajkumar
Re: [PATCH RFC v5 3/4] mac80211: Add airtime accounting and scheduling to TXQs
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
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.
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);
struct sta_info *sta;
u8 ac = txq->ac;
lockdep_assert_held(>active_txq_lock[ac]);
if (!txqi->txq.sta)
goto out;
sta = container_of(txqi->txq.sta, struct sta_info, sta);
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 += sta->airtime_weight;
}
return false;
out:
list_del_init(>schedule_order);
return true;
}
-Rajkumar
Re: [PATCH RFC v5 3/4] mac80211: Add airtime accounting and scheduling to TXQs
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
Re: [PATCH RFC v5 3/4] mac80211: Add airtime accounting and scheduling to TXQs
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. 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.
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.
-Rajkumar
[PATCH RFC v5 3/4] mac80211: Add airtime accounting and scheduling to TXQs
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
