Bugs in wake-queue logic.

[Michal Kazior]

On 05/12/2015, Ben Greear <greearb at candelatech.com> wrote:
> On 12/05/2015 08:42 AM, Michal Kazior wrote:
>> On 05/12/2015, Ben Greear <greearb at candelatech.com> wrote:
>>> On 12/05/2015 05:42 AM, Michal Kazior wrote:
>>>> On 04/12/2015, Ben Greear <greearb at candelatech.com> wrote:
>>>>> So, after tweaking a firmware image to actually be able to use
>>>>> all tx-buffers, then queues can actually be stopped on the host
>>>>> now.
>>>>>
>>>>> I'm now getting splats related to tx-queue being
>>>>> out of range.
>>>>>
>>>>> Why are we using vdev_id as the queue-id below?
>>>>>
>>>>> void ath10k_mac_vif_tx_unlock(struct ath10k_vif *arvif, int reason)
>>>>> {
>>>>>            struct ath10k *ar = arvif->ar;
>>>>>
>>>>>            lockdep_assert_held(&ar->htt.tx_lock);
>>>>>
>>>>>            WARN_ON(reason >= BITS_PER_LONG);
>>>>>            arvif->tx_paused &= ~BIT(reason);
>>>>>
>>>>>            if (ar->tx_paused)
>>>>>                    return;
>>>>>
>>>>>            if (arvif->tx_paused)
>>>>>                    return;
>>>>>
>>>>>            ieee80211_wake_queue(ar->hw, arvif->vdev_id);
>>>>
>>>> I guess you could try replacing arvif->vdev_id with
>>>> arvif->vif->cab_queue. This along with vif->hw_queue[] share the same
>>>> queue number associated with vdev_id. Refer to the add_interface()
>>>> implementation, please.
>>>>
>>>> You don't need to increase mac80211's max_queues unless you intend to
>>>> support a similar host queue control via firmware events like qca6174.
>>>
>>> What about the comments about tx-locking?  Does that only apply to
>>> qca6174?
>>
>> The code applies the logic to all chips but only qca6174 generates the
>> events that can stop queues per vif. The qca6174 doesn't have any
>> iface combinations with 16 vdevs so the logic works fine in all cases
>> as far as upstream is concerned.
>
> So, for non qca6174, do we even need more than one tx-queue?
>
>>> For host queue control, are firmware events really needed, or can we
>>> just keep track of buffered pkts per peer and/or per vdev and use
>>> that on the host?
>>
>> It's possible. However you probably have your 16+ vdev setup in mind.
>> This in itself won't work as-is because of the modulo/overlap.
>>
>> You'll probably need to add some kind of mechanism to avoid waking up
>> a queue when multiple vdevs share one.
>
> Seems like fake hardware tx-queues might not be the way to do this...upper
> levels
> could make the decisions needed on a per-peer (and maybe per-tid?) basis
> based on the
> peer's tx-rate (my firmware can report this properly).  If the firmware
> ever exhausts it's tx-buffers, stopping all TX is as good as anything
> else I think...

There's not enough data for upper layers to do this in multi-channel
case. It's not just about rate control. It's also about channel
scheduling which isn't known to the driver. Relying on HTT chan-change
(assuming it even works on wmi-tlv firmware) won't be straightforward
because it's an after-the-fact which is a problem because firmware
takes care of powersaving which itself is considered for channel
scheduling in firmware.

If you switch channel and clog up all HTT tx descriptors with traffic
to a vdev which sits on a different channel you'll just waste time.
All in all you'd end up with suboptimal performance.


Michal