Rajkumar Manoharan <rmanohar@xxxxxxxxxxxxxx> writes:
> On 2018-09-10 04:13, Toke Høiland-Jørgensen wrote:
>> Johannes Berg <johannes@xxxxxxxxxxxxxxxx> 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 :)
>>>> +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? :)
> So that it can be used to determine whether txq is running negative.
But still not sure what you mean here?
-Toke
