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-16 10:42, 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.
>> 
>> +bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
>> +				struct ieee80211_txq *txq)
>> +{
>
> [...]
>
>> +	if (ret) {
>> +		clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags);
>> +		list_del_init(&txqi->schedule_order);
>> +	} else
>> +		set_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags);
>> +
>> 
> This looks wrong to me. txqi->flags are protected by fq->lock but here
> it is by active_txq_lock. no?

Both set_bit() and clear_bit() are atomic operations, so they don't need
separate locking. See Documentation/atomic_bitops.txt

>> @@ -3677,6 +3751,7 @@ void ieee80211_txq_schedule_end(struct
>> ieee80211_hw *hw, u8 ac)
>>  	struct ieee80211_local *local = hw_to_local(hw);
>> 
>>  	spin_unlock_bh(&local->active_txq_lock[ac]);
>> +	tasklet_schedule(&local->wake_txqs_tasklet);
>>  }
>> 
> It is an overload to schedule wake_txqs_tasklet for each txq unlock.
> Instead I would prefer to call __ieee80211_kick_airtime from 
> schedule_end.
> Thoughts?

Yeah, I realise scheduling the whole wake_txqs_tasklet is maybe a bit
heavy-handed here. However just calling into __ieee80211_kick_airtime()
means we'll end up recursing back to the same place, which is not good
either (we could in theory flip-flop between two queues until we run out
of stack space).

My "backup plan" if the wake_txqs_tasklet turns out to be too heavy was
to define a new tasklet just for this use; but wanted to see if this
actually turned out to be a problem first...

-Toke