Rajkumar Manoharan <rmanohar@xxxxxxxxxxxxxx> writes:
> On 2018-07-13 06:39, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan <rmanohar@xxxxxxxxxxxxxx> writes:
>>
> [...]
>
>>> Hmm... I thought driver will call ieee80211_schedule_txq when it runs
>>> out of hardware descriptor and break the loop. The serving txq will be
>>> added back to head of activeq list. no?
>>
>> Yes, and then the next one will be serviced... It's basically:
>>
>> while (!hwq_is_full()) {
>> txq = next_txq():
>> build_one_aggr(txq); // may or may not succeed
>> if (!empty(txq))
>> schedule_txq(txq);
>> }
>>
>> It is not generally predictable how many times this will loop before
>> exiting...
>>
> Agree.. It would be better If the driver does not worry about txq
> sequence numbering. Perhaps one more API (ieee80211_first_txq) could
> solve this. Will leave it to you.
That is basically what the second parameter to next_txq() does in the
current patchset. It just needs to be renamed...
>>>>>
>>>>> ieee80211_txq_get_depth
>>>>> - return deficit status along with frm_cnt
>>>>>
>>>>> ieee80211_reorder_txq
>>>>> - if txq deficit > 0
>>>>> - return;
>>>>> - if txq is last
>>>>> - return
>>>>> - delete txq from list
>>>>> - move it to tail
>>>>> - update deficit by quantum
>>>>>
>>>>> ath10k_htt_rx_tx_fetch_ind
>>>>> - get txq deficit status
>>>>> - if txq deficit > 0
>>>>> - dequeue skb
>>>>> - else if deficit < 0
>>>>> - return NULL
>>>>>
>>>>> Please share your thoughts.
>>>>
>>>> Hmm, not sure exactly how this would work; seems a little
>>>> complicated?
>>>> Also, I'd rather if drivers were completely oblivious to the deficit;
>>>> that is a bit of an implementation detail...
>>>>
>>> Agree.. Initially I thought of adding deficit check in
>>> ieee80211_tx_dequeue.
>>> But It will be overhead of taking activeq_lock for every skbs. Perhaps
>>> it can be renamed as allowed_to_dequeue instead of deficit.
>>>
>>>> We could have an ieee80211_txq_may_pull(); or maybe just have
>>>> ieee80211_tx_dequeue() return NULL if the deficit is negative?
>>>>
>>> As I said earlier, checking deficit for every skb will be an overhead.
>>> It should be done once before accessing txq.
>>
>> Well, it could conceivably be done in a way that doesn't require taking
>> the activeq_lock. Adding another STOP flag to the TXQ, for instance.
>> From an API point of view I think that is more consistent with what we
>> have already...
>>
>
> Make sense. ieee80211_txq_may_pull would be better place to decide
> whether given txq is allowed for transmission. It also makes drivers
> do not have to worry about deficit. Still I may need
> ieee80211_reorder_txq API after processing txq. isn't it?
The way I was assuming this would work (and what ath9k does), is that a
driver only operates on one TXQ at a time; so it can get that txq,
process it, and re-schedule it. In which case no other API is needed;
the rotating can be done in next_txq(), and schedule_txq() can insert
the txq back into the rotation as needed.
However, it sounds like this is not how ath10k does things? See below.
>>>> the reasonable thing for the driver to do, then, would be to ask
>>>> ieee80211_next_txq() for another TXQ to pull from if the current one
>>>> doesn't work for whatever reason.
>>>>
>>>> Would that work for push-pull mode?
>>>>
>>> Not really. Driver shouldn't send other txq data instead of asked one.
>>
>> I didn't necessarily mean immediately. As long as it does it
>> eventually.
>> If a TXQ's deficit runs negative, that TXQ will not be allowed to send
>> again until its deficit has been restored to positive through enough
>> cycles of the loop in next_txq().
>>
>
> Thats true. Are you suggesting to run the loop until the txq deficit
> becomes positive?
Yeah, or rather until the first station with a positive deficit is
found.
>>> In MU-MIMO, firmware will query N packets from given set of {STA,TID}.
>>> So the driver not supposed to send other txq's data.
>>
>> Hmm, it'll actually be interesting to see how the airtime fairness
>> scheduler interacts with MU-MIMO. I'm not quite sure that it'll be in a
>> good way; the DRR scheduler generally only restores one TXQ to positive
>> deficit at a time, so it may be that MU-MIMO will break completely and
>> we'll have to come up with another scheduling algorithm.
>>
>
> In push-pull method, driver reports to firmware that number of frames
> queued for each tid per station by wake_tx_queue. Later firmware will
> query N frames from each TID and after dequeue driver will update
> remaining frames for that tid. In ATF case, when driver is not able to
> dequeue frames, driver will simply update remaining frames. The
> consecutive fetch_ind get opportunity to dequeue the frames. By This
> way, transmission for serving client will be paused for a while and
> opportunity will be given to others.
This sounds like the driver doesn't do anything other than notify the
firmware that a txq has pending frames, and everything else is handled
by the firmware? Is this the case?
And if so, how does that interact with ath10k_mac_tx_push_pending()?
That function is basically doing the same thing that I explained above
for ath9k; in the previous version of this patch series I modified that
to use next_txq(). But is that a different TX path, or am I
misunderstanding you?
If you could point me to which parts of the ath10k code I should be
looking at, that would be helpful as well :)
-Toke
