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

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

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?

-Toke