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

[Rajkumar Manoharan <rmanohar@codeaurora.org>]

On 2018-09-28 02:58, Toke Høiland-Jørgensen wrote:
> Rajkumar Manoharan <rmanohar@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
>>> <rmanohar@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 <rmanohar@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?
> 

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.

-Rajkumar