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

[Rajkumar Manoharan <rmanohar@codeaurora.org>]

On 2018-09-12 04:10, Toke Høiland-Jørgensen wrote:
> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
> 
>> On 2018-09-10 04:13, Toke Høiland-Jørgensen wrote:
>>> Johannes Berg <johannes@sipsolutions.net> writes:
>>>>> -			       txqi->flags & (1<<IEEE80211_TXQ_STOP) ? "STOP" : "RUN",
>>>>> -			       txqi->flags & (1<<IEEE80211_TXQ_AMPDU) ? " AMPDU" : "",
>>>>> -			       txqi->flags & (1<<IEEE80211_TXQ_NO_AMSDU) ? " NO-AMSDU" 
>>>>> :
>>>>> "");
>>>>> +			       txqi->flags & (1 << IEEE80211_TXQ_STOP) ? "STOP" : 
>>>>> "RUN",
>>>>> +			       txqi->flags & (1 << IEEE80211_TXQ_AMPDU) ? " AMPDU" : 
>>>>> "",
>>>>> +			       txqi->flags & (1 << IEEE80211_TXQ_NO_AMSDU) ? " 
>>>>> NO-AMSDU"
>>>>> : "");
>>>> 
>>>> consider BIT() instead as a cleanup? :)
>>>> 
>>>> (or maybe this is just a leftover from merging some other patches?)
>>> 
>>> Yeah, this is a merging artifact; Rajkumar's patch added another 
>>> flag,
>>> that I removed again. Didn't notice that there was still a whitespace
>>> change in this patch...
>>> 
>> I added the flag based on our last discussion. The driver needs to 
>> check
>> txq status for each tx_dequeue(). One time txq check is not sufficient
>> as it allows the driver to dequeue all frames from txq.
>> 
>> drv_func() {
>>        while (ieee80211_airtime_may_transmit(txq) &&
>>                hw_has_space() &&
>>               (pkt = ieee80211_tx_dequeue(hw, txq)))
>>            push_to_hw(pkt);
>> }
> 
> Yeah, but with airtime only being recorded on TX completion, the odds 
> of
> the value changing within that loop are quite low; so it's not going to
> work, which is why I removed it.
> 
> However, after reading Kan's patches I get where you're coming from; a
> check in tx_dequeue() is needed for the BQL-style queue limiting. Will
> try to incorporate a version of that in the next series so you can see
> what I mean when I say it should be orthogonal; and I'm still not sure
> it needs a flag :)
> 
Got it.. Will wait for next version.. thanks.

>>>>> +bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
>>>>> +				struct ieee80211_txq *txq)
>>>>> +{
>>>>> +	struct ieee80211_local *local = hw_to_local(hw);
>>>>> +	struct txq_info *txqi = to_txq_info(txq);
>>>>> +	bool may_tx = false;
>>>>> +
>>>>> +	spin_lock_bh(&local->active_txq_lock);
>>>>> +
>>>>> +	if (ieee80211_txq_check_deficit(local, txqi)) {
>>>>> +		may_tx = true;
>>>>> +		list_del_init(&txqi->schedule_order);
>>>> 
>> 
>> To handle above case, may_transmit should remove the node only
>> when it is in list.
>> 
>> if (list_empty(&txqi->schedule_order))
>>         list_del_init(&txqi->schedule_order);
> 
> I assume you missed a ! in that if, right? :)
> 
Oops.. yes it should be ! :)

-Rajkumar