Rajkumar Manoharan <rmanohar@xxxxxxxxxxxxxx> 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
