Bugs in wake-queue logic.

Bugs in wake-queue logic.

[Ben Greear]

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);
}


Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: Bugs in wake-queue logic.

[Janusz Dziedzic]

On 3 December 2015 at 20:52, Ben Greear <greearb@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?
>
This is comment in the code:

        /* Using vdev_id as queue number will make it very easy to do per-vif
         * tx queue locking. This shouldn't wrap due to interface combinations
         * but do a modulo for correctness sake and prevent using offchannel tx
         * queues for regular vif tx.
         */

BR
Janusz

> 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);
> }
>
>
> Thanks,
> Ben
>
> --
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc  http://www.candelatech.com
>
>
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: Bugs in wake-queue logic.

[Janusz Dziedzic]

On 4 December 2015 at 07:06, Janusz Dziedzic <janusz.dziedzic@tieto.com> wrote:
> On 3 December 2015 at 20:52, Ben Greear <greearb@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?
>>
> This is comment in the code:
>
>         /* Using vdev_id as queue number will make it very easy to do per-vif
>          * tx queue locking. This shouldn't wrap due to interface combinations
>          * but do a modulo for correctness sake and prevent using offchannel tx
>          * queues for regular vif tx.
>          */
>
BTW, I think Michal will send soon "new" design for that because of
MU-MIMO implementation.


> BR
> Janusz
>
>> 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);
>> }
>>
>>
>> Thanks,
>> Ben
>>
>> --
>> Ben Greear <greearb@candelatech.com>
>> Candela Technologies Inc  http://www.candelatech.com
>>
>>
>> _______________________________________________
>> ath10k mailing list
>> ath10k@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/ath10k

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: Bugs in wake-queue logic.

[Ben Greear]

On 12/03/2015 10:11 PM, Janusz Dziedzic wrote:
> On 4 December 2015 at 07:06, Janusz Dziedzic <janusz.dziedzic@tieto.com> wrote:
>> On 3 December 2015 at 20:52, Ben Greear <greearb@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?
>>>
>> This is comment in the code:
>>
>>          /* Using vdev_id as queue number will make it very easy to do per-vif
>>           * tx queue locking. This shouldn't wrap due to interface combinations
>>           * but do a modulo for correctness sake and prevent using offchannel tx
>>           * queues for regular vif tx.
>>           */
>>
> BTW, I think Michal will send soon "new" design for that because of
> MU-MIMO implementation.

It would be nice if ath10k could gracefully deal with 64 vdevs using only
the default 16 maximum tx-queues supported by mac80211.

I wrote a patch that extended max-queues to 65, but not sure it will
make it upstream...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: Bugs in wake-queue logic.

[Janusz Dziedzic]

On 4 December 2015 at 07:30, Ben Greear <greearb@candelatech.com> wrote:
>
>
> On 12/03/2015 10:11 PM, Janusz Dziedzic wrote:
>>
>> On 4 December 2015 at 07:06, Janusz Dziedzic <janusz.dziedzic@tieto.com>
>> wrote:
>>>
>>> On 3 December 2015 at 20:52, Ben Greear <greearb@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?
>>>>
>>> This is comment in the code:
>>>
>>>          /* Using vdev_id as queue number will make it very easy to do
>>> per-vif
>>>           * tx queue locking. This shouldn't wrap due to interface
>>> combinations
>>>           * but do a modulo for correctness sake and prevent using
>>> offchannel tx
>>>           * queues for regular vif tx.
>>>           */
>>>
>> BTW, I think Michal will send soon "new" design for that because of
>> MU-MIMO implementation.
>
>
> It would be nice if ath10k could gracefully deal with 64 vdevs using only
> the default 16 maximum tx-queues supported by mac80211.
>
I think Michal changed also mac80211 (netdev) queues - that can be
used/locked per station for MU-MIMO case.
I think he will send paches soon, while now he stuck in Chennai do to
flooding ...

BR
Janusz

> I wrote a patch that extended max-queues to 65, but not sure it will
> make it upstream...
>
>
> Thanks,
> Ben
>
> --
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc  http://www.candelatech.com

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: Bugs in wake-queue logic.

[Michal Kazior]

On 04/12/2015, Ben Greear <greearb@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.


Michal

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: Bugs in wake-queue logic.

[Ben Greear]

On 12/05/2015 05:42 AM, Michal Kazior wrote:
> On 04/12/2015, Ben Greear <greearb@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?

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?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: Bugs in wake-queue logic.

[Michal Kazior]

On 05/12/2015, Ben Greear <greearb@candelatech.com> wrote:
> On 12/05/2015 05:42 AM, Michal Kazior wrote:
>> On 04/12/2015, Ben Greear <greearb@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.


> 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.


Michal

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: Bugs in wake-queue logic.

[Ben Greear]

On 12/05/2015 08:42 AM, Michal Kazior wrote:
> On 05/12/2015, Ben Greear <greearb@candelatech.com> wrote:
>> On 12/05/2015 05:42 AM, Michal Kazior wrote:
>>> On 04/12/2015, Ben Greear <greearb@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...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: Bugs in wake-queue logic.

[Michal Kazior]

On 05/12/2015, Ben Greear <greearb@candelatech.com> wrote:
> On 12/05/2015 08:42 AM, Michal Kazior wrote:
>> On 05/12/2015, Ben Greear <greearb@candelatech.com> wrote:
>>> On 12/05/2015 05:42 AM, Michal Kazior wrote:
>>>> On 04/12/2015, Ben Greear <greearb@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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k