Rajkumar Manoharan <rmano...@codeaurora.org> writes:
> On 2018-09-28 00:51, Toke Høiland-Jørgensen wrote:
>> On 28 September 2018 07:29:03 CEST, Rajkumar Manoharan
>> <rmano...@codeaurora.org> wrote:
>>> On 2018-09-26 17:09, Rajkumar Manoharan wrote:
>>>> On 2018-09-26 02:22, Toke Høiland-Jørgensen wrote:
>>>>> Rajkumar Manoharan <rmano...@codeaurora.org> 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(&local->active_txq_lock[ac]);
>
> - begin:
> if (list_empty(&local->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,
> - &txqi->flags))
> + if (test_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags)) {
> + clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->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
