[ath9k-devel] queue priority mapping

[ath9k-devel] queue priority mapping

[Hubert Feurstein]

Hi,

in ath9k_init_queues are the AC levels mapped to the hardware-queues by
this loop below. But doesn't this map the priorities in the wrong
direction? The hardware queue 0 has the lowest priority, but is mapped to
IEEE80211_AC_VO. And the hardware queue 3 (with higher priority) is mapped
to IEEE80211_AC_BK. Shouldn't that be the other way around (as changed
below) ?

@@ -323,11 +323,12 @@ static int ath9k_init_queues(struct ath_softc *sc)
  sc->beacon.cabq = ath_txq_setup(sc, ATH9K_TX_QUEUE_CAB, 0);
  ath_cabq_update(sc);

  sc->tx.uapsdq = ath_txq_setup(sc, ATH9K_TX_QUEUE_UAPSD, 0);

- for (i = 0; i < IEEE80211_NUM_ACS; i++) {
+ for (i = IEEE80211_NUM_ACS - 1; i >= 0; i--) {
  sc->tx.txq_map[i] = ath_txq_setup(sc, ATH9K_TX_QUEUE_DATA, i);
  sc->tx.txq_map[i]->mac80211_qnum = i;
  sc->tx.txq_max_pending[i] = ATH_MAX_QDEPTH;
  }
  return 0;

Best regards
Hubert
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ath9k.org/pipermail/ath9k-devel/attachments/20141120/43448825/attachment.htm 

[ath9k-devel] queue priority mapping

[M. Braun]

Hi,

Am 20.11.2014 um 19:00 schrieb Hubert Feurstein:
> in ath9k_init_queues are the AC levels mapped to the hardware-queues by
> this loop below. But doesn't this map the priorities in the wrong
> direction? The hardware queue 0 has the lowest priority, but is mapped to
> IEEE80211_AC_VO. And the hardware queue 3 (with higher priority) is mapped
> to IEEE80211_AC_BK. Shouldn't that be the other way around (as changed
> below) ?

I don't know about the hw priority stuff, but the change you propose
does not change the resulting data structure at all.
You'd likely need to change mac80211_qnum to be IEEE80211_NUM_ACS-1-i
(and maybe others) to change the mapping.

Regards,
 M. Braun

> 
> @@ -323,11 +323,12 @@ static int ath9k_init_queues(struct ath_softc *sc)
>   sc->beacon.cabq = ath_txq_setup(sc, ATH9K_TX_QUEUE_CAB, 0);
>   ath_cabq_update(sc);
> 
>   sc->tx.uapsdq = ath_txq_setup(sc, ATH9K_TX_QUEUE_UAPSD, 0);
> 
> - for (i = 0; i < IEEE80211_NUM_ACS; i++) {
> + for (i = IEEE80211_NUM_ACS - 1; i >= 0; i--) {
>   sc->tx.txq_map[i] = ath_txq_setup(sc, ATH9K_TX_QUEUE_DATA, i);
>   sc->tx.txq_map[i]->mac80211_qnum = i;
>   sc->tx.txq_max_pending[i] = ATH_MAX_QDEPTH;
>   }
>   return 0;
> 
> Best regards
> Hubert
> 
> 
> 
> _______________________________________________
> ath9k-devel mailing list
> ath9k-devel at lists.ath9k.org
> https://lists.ath9k.org/mailman/listinfo/ath9k-devel
> 

[ath9k-devel] queue priority mapping

[Hubert Feurstein]

Hi,

2014-11-24 10:10 GMT+01:00 M. Braun <michael-dev@fami-braun.de>:
[...]
>
> I don't know about the hw priority stuff, but the change you propose
> does not change the resulting data structure at all.
> You'd likely need to change mac80211_qnum to be IEEE80211_NUM_ACS-1-i
> (and maybe others) to change the mapping.

Well, in fact it does. To understand my post you have to know the
behaviour of ath_txq_setup. The point here is that on the first call
of ath_txq_setup(..ATH9K_TX_QUEUE_DATA..), hw-queue 0 is returned. On
the second call hw-queue 1 is returned, ... . And IEEE80211_AC_VO = 0,
so hw-queue 0 is assigned, which is wrong in my opinion. But the right
place to fix this would be ath_txq_setup itself.

Regards
Hubert

[ath9k-devel] queue priority mapping

[Dave Taht]

802.11e style priority mappings are just so wrong for 802.11n, anyway.
All they do is add bufferbloat and eat txops where you are much better
off aggregating more packets.

On Tue, Nov 25, 2014 at 1:14 PM, Hubert Feurstein <h.feurstein@gmail.com> wrote:
> Hi,
>
> 2014-11-24 10:10 GMT+01:00 M. Braun <michael-dev@fami-braun.de>:
> [...]
>>
>> I don't know about the hw priority stuff, but the change you propose
>> does not change the resulting data structure at all.
>> You'd likely need to change mac80211_qnum to be IEEE80211_NUM_ACS-1-i
>> (and maybe others) to change the mapping.
>
> Well, in fact it does. To understand my post you have to know the
> behaviour of ath_txq_setup. The point here is that on the first call
> of ath_txq_setup(..ATH9K_TX_QUEUE_DATA..), hw-queue 0 is returned. On
> the second call hw-queue 1 is returned, ... . And IEEE80211_AC_VO = 0,
> so hw-queue 0 is assigned, which is wrong in my opinion. But the right
> place to fix this would be ath_txq_setup itself.
>
> Regards
> Hubert
> _______________________________________________
> ath9k-devel mailing list
> ath9k-devel at lists.ath9k.org
> https://lists.ath9k.org/mailman/listinfo/ath9k-devel



-- 
Dave T?ht

thttp://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks

[ath9k-devel] queue priority mapping

[M. Braun]

Hi,

Am 25.11.2014 um 22:14 schrieb Hubert Feurstein:
> [...]
> Well, in fact it does. To understand my post you have to know the
> behaviour of ath_txq_setup. The point here is that on the first call
> of ath_txq_setup(..ATH9K_TX_QUEUE_DATA..), hw-queue 0 is returned. On
> the second call hw-queue 1 is returned, ... . And IEEE80211_AC_VO = 0,
> so hw-queue 0 is assigned, which is wrong in my opinion. But the right
> place to fix this would be ath_txq_setup itself.

sorry, I looked at the ATH_TXQ_SETUP macro in ath9k.h instead of the
ath_txq_setup function in xmit.c.

Regards,
 M. Braun

[ath9k-devel] queue priority mapping

[Sujith Manoharan]

Hubert Feurstein wrote:
> Well, in fact it does. To understand my post you have to know the
> behaviour of ath_txq_setup. The point here is that on the first call
> of ath_txq_setup(..ATH9K_TX_QUEUE_DATA..), hw-queue 0 is returned. On
> the second call hw-queue 1 is returned, ... . And IEEE80211_AC_VO = 0,
> so hw-queue 0 is assigned, which is wrong in my opinion. But the right
> place to fix this would be ath_txq_setup itself.

The mapping has been changed recently and I think it might be a problem,
since this is what the datasheet says:

"The mapping of physical DCUs to absolute channel access priorities is fixed and
cannot be altered by software."

Sujith

[ath9k-devel] queue priority mapping

[Hubert Feurstein]

Hi Sujith,

2014-11-26 13:25 GMT+01:00 Sujith Manoharan <sujith@msujith.org>:
> Hubert Feurstein wrote:
>> Well, in fact it does. To understand my post you have to know the
>> behaviour of ath_txq_setup. The point here is that on the first call
>> of ath_txq_setup(..ATH9K_TX_QUEUE_DATA..), hw-queue 0 is returned. On
>> the second call hw-queue 1 is returned, ... . And IEEE80211_AC_VO = 0,
>> so hw-queue 0 is assigned, which is wrong in my opinion. But the right
>> place to fix this would be ath_txq_setup itself.
>
> The mapping has been changed recently and I think it might be a problem,
> since this is what the datasheet says:
>
> "The mapping of physical DCUs to absolute channel access priorities is fixed and
> cannot be altered by software."

Yes I know, and isn't this the reason why there is
sc->tx.txq_map[<AC>] in place, to map the AC to the proper HW queue in
software. So sc->tx.txq_map[IEEE80211_AC_VO] should return the
higher-prio-queue 3 and not the low-prio-queue 0. In FreeBSD it is
that way, in Linux it is the opposite, because in Linux
IEEE80211_AC_VO=0 and in FreeBSD WME_AC_VO=3.

Hubert

[ath9k-devel] queue priority mapping

[Thomas Hühn]

Hi Hubert,

From my point of view the call-path of the functions look like this:

ath9k_init_queues <http://lxr.free-electrons.com/ident?i=ath9k_init_queues>(struct ath_softc <http://lxr.free-electrons.com/ident?i=ath_softc> *sc <http://lxr.free-electrons.com/ident?i=sc>) 
       -> ath_txq_setup <http://lxr.free-electrons.com/ident?i=ath_txq_setup>(sc <http://lxr.free-electrons.com/ident?i=sc>, ATH9K_TX_QUEUE_DATA, i <http://lxr.free-electrons.com/ident?i=i>) with subtype-remapping via 
		qi.tqi_subtype = subtype_txq_to_hwq <http://lxr.free-electrons.com/ident?i=subtype_txq_to_hwq>[subtype];
			-> ath9k_hw_setuptxqueue <http://lxr.free-electrons.com/ident?i=ath9k_hw_setuptxqueue>(ah, qtype, &qi);

And the subtyperemapping reflects the correct AC class mapping to ath9k hw queues.
In xmit.c function ath_txq_setup <http://lxr.free-electrons.com/ident?i=ath_txq_setup>() defines and initializes the mapping with:

static const int subtype_txq_to_hwq <http://lxr.free-electrons.com/ident?i=subtype_txq_to_hwq>[] = {
	[IEEE80211_AC_BE] = ATH_TXQ_AC_BE,
	[IEEE80211_AC_BK] = ATH_TXQ_AC_BK,
	[IEEE80211_AC_VI] = ATH_TXQ_AC_VI,
	[IEEE80211_AC_VO] = ATH_TXQ_AC_VO,
};
from mac80211.h:
101 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L101> /**
102 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L102>  * enum ieee80211_ac_numbers - AC numbers as used in mac80211
103 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L103>  * @IEEE80211_AC_VO: voice
104 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L104>  * @IEEE80211_AC_VI: video
105 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L105>  * @IEEE80211_AC_BE: best effort
106 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L106>  * @IEEE80211_AC_BK: background
107 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L107>  */
108 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L108> enum ieee80211_ac_numbers <http://lxr.free-electrons.com/ident?i=ieee80211_ac_numbers> {
109 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L109>         IEEE80211_AC_VO         = 0,
110 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L110>         IEEE80211_AC_VI         = 1,
111 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L111>         IEEE80211_AC_BE         = 2,
112 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L112>         IEEE80211_AC_BK         = 3,
113 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L113> };
114 <http://lxr.free-electrons.com/source/include/net/mac80211.h#L114> #define IEEE80211_NUM_ACS <http://lxr.free-electrons.com/ident?i=IEEE80211_NUM_ACS>       4
from ath9k/hw.h:
218 <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath9k/hw.h#L218> enum ath_hw_txq_subtype <http://lxr.free-electrons.com/ident?i=ath_hw_txq_subtype> {
219 <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath9k/hw.h#L219>         ATH_TXQ_AC_BE = 0,
220 <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath9k/hw.h#L220>         ATH_TXQ_AC_BK = 1,
221 <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath9k/hw.h#L221>         ATH_TXQ_AC_VI = 2,
222 <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath9k/hw.h#L222>         ATH_TXQ_AC_VO = 3,
223 <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath9k/hw.h#L223> };
this translates to the following mapping of AC to hw queues:
static const int subtype_txq_to_hwq <http://lxr.free-electrons.com/ident?i=subtype_txq_to_hwq>[] = {
	[2] = 0,
	[3] = 1,
	[1] = 2,
	[0] = 3,
};

That is how I understand the source code and yes it looks a bit complicated but function wise seems to be correct.
Or do I miss something here ?

Greetings from Berlin
Thomas




> Am 27.11.2014 um 13:04 schrieb Hubert Feurstein <h.feurstein@gmail.com>:
> 
> Hi Sujith,
> 
> 2014-11-26 13:25 GMT+01:00 Sujith Manoharan <sujith at msujith.org <mailto:sujith@msujith.org>>:
>> Hubert Feurstein wrote:
>>> Well, in fact it does. To understand my post you have to know the
>>> behaviour of ath_txq_setup. The point here is that on the first call
>>> of ath_txq_setup(..ATH9K_TX_QUEUE_DATA..), hw-queue 0 is returned. On
>>> the second call hw-queue 1 is returned, ... . And IEEE80211_AC_VO = 0,
>>> so hw-queue 0 is assigned, which is wrong in my opinion. But the right
>>> place to fix this would be ath_txq_setup itself.
>> 
>> The mapping has been changed recently and I think it might be a problem,
>> since this is what the datasheet says:
>> 
>> "The mapping of physical DCUs to absolute channel access priorities is fixed and
>> cannot be altered by software."
> 
> Yes I know, and isn't this the reason why there is
> sc->tx.txq_map[<AC>] in place, to map the AC to the proper HW queue in
> software. So sc->tx.txq_map[IEEE80211_AC_VO] should return the
> higher-prio-queue 3 and not the low-prio-queue 0. In FreeBSD it is
> that way, in Linux it is the opposite, because in Linux
> IEEE80211_AC_VO=0 and in FreeBSD WME_AC_VO=3.
> 
> Hubert
> _______________________________________________
> ath9k-devel mailing list
> ath9k-devel at lists.ath9k.org <mailto:ath9k-devel@lists.ath9k.org>
> https://lists.ath9k.org/mailman/listinfo/ath9k-devel <https://lists.ath9k.org/mailman/listinfo/ath9k-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ath9k.org/pipermail/ath9k-devel/attachments/20141130/aa6eeb3b/attachment.htm 

[ath9k-devel] queue priority mapping

[Sergey Ryazanov]

Hi Thomas,

2014-11-30 14:30 GMT+03:00 Thomas H?hn <thomas@net.t-labs.tu-berlin.de>:
> From my point of view the call-path of the functions look like this:
>
> ath9k_init_queues(struct ath_softc *sc)
>
>        -> ath_txq_setup(sc, ATH9K_TX_QUEUE_DATA, i) with subtype-remapping
> via
>
> qi.tqi_subtype = subtype_txq_to_hwq[subtype];
>
> -> ath9k_hw_setuptxqueue(ah, qtype, &qi);
>
> And the subtyperemapping reflects the correct AC class mapping to ath9k hw
> queues.

tqi_subtype field doesn't define hw queue. Quick grep show only 6
lines in the whole driver, where this field is accessed. Actual hw
queue number stored in axq_qnum field of ath_txq as documented in
ath9k.h

> In xmit.c function ath_txq_setup() defines and initializes the mapping with:
>
> static const int subtype_txq_to_hwq[] = {
>
> [IEEE80211_AC_BE] = ATH_TXQ_AC_BE,
> [IEEE80211_AC_BK] = ATH_TXQ_AC_BK,
> [IEEE80211_AC_VI] = ATH_TXQ_AC_VI,
> [IEEE80211_AC_VO] = ATH_TXQ_AC_VO,
> };
>
> from mac80211.h:
>
> 101 /**
> 102  * enum ieee80211_ac_numbers - AC numbers as used in mac80211
> 103  * @IEEE80211_AC_VO: voice
> 104  * @IEEE80211_AC_VI: video
> 105  * @IEEE80211_AC_BE: best effort
> 106  * @IEEE80211_AC_BK: background
> 107  */
> 108 enum ieee80211_ac_numbers {
> 109         IEEE80211_AC_VO         = 0,
> 110         IEEE80211_AC_VI         = 1,
> 111         IEEE80211_AC_BE         = 2,
> 112         IEEE80211_AC_BK         = 3,
> 113 };
> 114 #define IEEE80211_NUM_ACS       4
>
> from ath9k/hw.h:
>
> 218 enum ath_hw_txq_subtype {
> 219         ATH_TXQ_AC_BE = 0,
> 220         ATH_TXQ_AC_BK = 1,
> 221         ATH_TXQ_AC_VI = 2,
> 222         ATH_TXQ_AC_VO = 3,
> 223 };
>
> this translates to the following mapping of AC to hw queues:
>
> static const int subtype_txq_to_hwq[] = {
> [2] = 0,
> [3] = 1,
> [1] = 2,
> [0] = 3,
> };
>

This table confuse me too. But if you trace where the table and the
tqi_subtype field is used, then you face, that this table means
nothing for hw queue selection.

>
> That is how I understand the source code and yes it looks a bit complicated
> but function wise seems to be correct.
> Or do I miss something here ?
>

Actual hw queue selection occurs inside the ath9k_hw_setuptxqueue()
function (in mac.c). Stripped version looks like:

int ath9k_hw_setuptxqueue(struct ath_hw *ah, enum ath9k_tx_queue type,
                          const struct ath9k_tx_queue_info *qinfo)
{
        int q;
[stripped]
                for (q = 0; q < ATH9K_NUM_TX_QUEUES; q++)
                        if (ah->txq[q].tqi_type ==
                            ATH9K_TX_QUEUE_INACTIVE)
                                break;
[stripped]
        return q;
}

So, as Huber wrote, hw queue number really depends on call order. And
existing call order gives us the following result:

~# cat /sys/kernel/debug/ieee80211/phy0/ath9k/queues
(VO):  qnum: 0 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
(VI):  qnum: 1 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
(BE):  qnum: 2 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
(BK):  qnum: 3 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
(CAB): qnum: 8 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0

Just in case, I checked ath5k driver and it contains exactly the same code.

Let's wait a comment from maintainers.

> Greetings from Berlin
> Thomas
>
>
>
>
> Am 27.11.2014 um 13:04 schrieb Hubert Feurstein <h.feurstein@gmail.com>:
>
> Hi Sujith,
>
> 2014-11-26 13:25 GMT+01:00 Sujith Manoharan <sujith@msujith.org>:
>
> Hubert Feurstein wrote:
>
> Well, in fact it does. To understand my post you have to know the
> behaviour of ath_txq_setup. The point here is that on the first call
> of ath_txq_setup(..ATH9K_TX_QUEUE_DATA..), hw-queue 0 is returned. On
> the second call hw-queue 1 is returned, ... . And IEEE80211_AC_VO = 0,
> so hw-queue 0 is assigned, which is wrong in my opinion. But the right
> place to fix this would be ath_txq_setup itself.
>
>
> The mapping has been changed recently and I think it might be a problem,
> since this is what the datasheet says:
>
> "The mapping of physical DCUs to absolute channel access priorities is fixed
> and
> cannot be altered by software."
>
>
> Yes I know, and isn't this the reason why there is
> sc->tx.txq_map[<AC>] in place, to map the AC to the proper HW queue in
> software. So sc->tx.txq_map[IEEE80211_AC_VO] should return the
> higher-prio-queue 3 and not the low-prio-queue 0. In FreeBSD it is
> that way, in Linux it is the opposite, because in Linux
> IEEE80211_AC_VO=0 and in FreeBSD WME_AC_VO=3.
>
> Hubert

-- 
BR,
Sergey

[ath9k-devel] queue priority mapping

[Thomas Hühn]

Hi Sergey,

You are right, the wrong mapping was caused by the call order.
Felix has already sent 3 patches to fix that bug. So lets test.

Greetings from Berlin
Thomas




> Am 30.11.2014 um 19:18 schrieb Sergey Ryazanov <ryazanov.s.a@gmail.com>:
> 
> Hi Thomas,
> 
> 2014-11-30 14:30 GMT+03:00 Thomas H?hn <thomas@net.t-labs.tu-berlin.de>:
>> From my point of view the call-path of the functions look like this:
>> 
>> ath9k_init_queues(struct ath_softc *sc)
>> 
>>       -> ath_txq_setup(sc, ATH9K_TX_QUEUE_DATA, i) with subtype-remapping
>> via
>> 
>> qi.tqi_subtype = subtype_txq_to_hwq[subtype];
>> 
>> -> ath9k_hw_setuptxqueue(ah, qtype, &qi);
>> 
>> And the subtyperemapping reflects the correct AC class mapping to ath9k hw
>> queues.
> 
> tqi_subtype field doesn't define hw queue. Quick grep show only 6
> lines in the whole driver, where this field is accessed. Actual hw
> queue number stored in axq_qnum field of ath_txq as documented in
> ath9k.h
> 
>> In xmit.c function ath_txq_setup() defines and initializes the mapping with:
>> 
>> static const int subtype_txq_to_hwq[] = {
>> 
>> [IEEE80211_AC_BE] = ATH_TXQ_AC_BE,
>> [IEEE80211_AC_BK] = ATH_TXQ_AC_BK,
>> [IEEE80211_AC_VI] = ATH_TXQ_AC_VI,
>> [IEEE80211_AC_VO] = ATH_TXQ_AC_VO,
>> };
>> 
>> from mac80211.h:
>> 
>> 101 /**
>> 102  * enum ieee80211_ac_numbers - AC numbers as used in mac80211
>> 103  * @IEEE80211_AC_VO: voice
>> 104  * @IEEE80211_AC_VI: video
>> 105  * @IEEE80211_AC_BE: best effort
>> 106  * @IEEE80211_AC_BK: background
>> 107  */
>> 108 enum ieee80211_ac_numbers {
>> 109         IEEE80211_AC_VO         = 0,
>> 110         IEEE80211_AC_VI         = 1,
>> 111         IEEE80211_AC_BE         = 2,
>> 112         IEEE80211_AC_BK         = 3,
>> 113 };
>> 114 #define IEEE80211_NUM_ACS       4
>> 
>> from ath9k/hw.h:
>> 
>> 218 enum ath_hw_txq_subtype {
>> 219         ATH_TXQ_AC_BE = 0,
>> 220         ATH_TXQ_AC_BK = 1,
>> 221         ATH_TXQ_AC_VI = 2,
>> 222         ATH_TXQ_AC_VO = 3,
>> 223 };
>> 
>> this translates to the following mapping of AC to hw queues:
>> 
>> static const int subtype_txq_to_hwq[] = {
>> [2] = 0,
>> [3] = 1,
>> [1] = 2,
>> [0] = 3,
>> };
>> 
> 
> This table confuse me too. But if you trace where the table and the
> tqi_subtype field is used, then you face, that this table means
> nothing for hw queue selection.
> 
>> 
>> That is how I understand the source code and yes it looks a bit complicated
>> but function wise seems to be correct.
>> Or do I miss something here ?
>> 
> 
> Actual hw queue selection occurs inside the ath9k_hw_setuptxqueue()
> function (in mac.c). Stripped version looks like:
> 
> int ath9k_hw_setuptxqueue(struct ath_hw *ah, enum ath9k_tx_queue type,
>                          const struct ath9k_tx_queue_info *qinfo)
> {
>        int q;
> [stripped]
>                for (q = 0; q < ATH9K_NUM_TX_QUEUES; q++)
>                        if (ah->txq[q].tqi_type ==
>                            ATH9K_TX_QUEUE_INACTIVE)
>                                break;
> [stripped]
>        return q;
> }
> 
> So, as Huber wrote, hw queue number really depends on call order. And
> existing call order gives us the following result:
> 
> ~# cat /sys/kernel/debug/ieee80211/phy0/ath9k/queues
> (VO):  qnum: 0 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
> (VI):  qnum: 1 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
> (BE):  qnum: 2 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
> (BK):  qnum: 3 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
> (CAB): qnum: 8 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
> 
> Just in case, I checked ath5k driver and it contains exactly the same code.
> 
> Let's wait a comment from maintainers.
> 
>> Greetings from Berlin
>> Thomas
>> 
>> 
>> 
>> 
>> Am 27.11.2014 um 13:04 schrieb Hubert Feurstein <h.feurstein@gmail.com>:
>> 
>> Hi Sujith,
>> 
>> 2014-11-26 13:25 GMT+01:00 Sujith Manoharan <sujith@msujith.org>:
>> 
>> Hubert Feurstein wrote:
>> 
>> Well, in fact it does. To understand my post you have to know the
>> behaviour of ath_txq_setup. The point here is that on the first call
>> of ath_txq_setup(..ATH9K_TX_QUEUE_DATA..), hw-queue 0 is returned. On
>> the second call hw-queue 1 is returned, ... . And IEEE80211_AC_VO = 0,
>> so hw-queue 0 is assigned, which is wrong in my opinion. But the right
>> place to fix this would be ath_txq_setup itself.
>> 
>> 
>> The mapping has been changed recently and I think it might be a problem,
>> since this is what the datasheet says:
>> 
>> "The mapping of physical DCUs to absolute channel access priorities is fixed
>> and
>> cannot be altered by software."
>> 
>> 
>> Yes I know, and isn't this the reason why there is
>> sc->tx.txq_map[<AC>] in place, to map the AC to the proper HW queue in
>> software. So sc->tx.txq_map[IEEE80211_AC_VO] should return the
>> higher-prio-queue 3 and not the low-prio-queue 0. In FreeBSD it is
>> that way, in Linux it is the opposite, because in Linux
>> IEEE80211_AC_VO=0 and in FreeBSD WME_AC_VO=3.
>> 
>> Hubert
> 
> -- 
> BR,
> Sergey
> _______________________________________________
> ath9k-devel mailing list
> ath9k-devel at lists.ath9k.org
> https://lists.ath9k.org/mailman/listinfo/ath9k-devel

[ath9k-devel] queue priority mapping

[Sergey Ryazanov]

2014-12-01 0:57 GMT+03:00 Thomas H?hn <thomas@net.t-labs.tu-berlin.de>:
> Hi Sergey,
>
Hi Thomas,

> You are right, the wrong mapping was caused by the call order.
> Felix has already sent 3 patches to fix that bug. So lets test.
>
Do you know any issues caused by this queues misuse? I actually do not
know what to test.

> Greetings from Berlin
> Thomas
>
>
>
>
>> Am 30.11.2014 um 19:18 schrieb Sergey Ryazanov <ryazanov.s.a@gmail.com>:
>>
>> Hi Thomas,
>>
>> 2014-11-30 14:30 GMT+03:00 Thomas H?hn <thomas@net.t-labs.tu-berlin.de>:
>>> From my point of view the call-path of the functions look like this:
>>>
>>> ath9k_init_queues(struct ath_softc *sc)
>>>
>>>       -> ath_txq_setup(sc, ATH9K_TX_QUEUE_DATA, i) with subtype-remapping
>>> via
>>>
>>> qi.tqi_subtype = subtype_txq_to_hwq[subtype];
>>>
>>> -> ath9k_hw_setuptxqueue(ah, qtype, &qi);
>>>
>>> And the subtyperemapping reflects the correct AC class mapping to ath9k hw
>>> queues.
>>
>> tqi_subtype field doesn't define hw queue. Quick grep show only 6
>> lines in the whole driver, where this field is accessed. Actual hw
>> queue number stored in axq_qnum field of ath_txq as documented in
>> ath9k.h
>>
>>> In xmit.c function ath_txq_setup() defines and initializes the mapping with:
>>>
>>> static const int subtype_txq_to_hwq[] = {
>>>
>>> [IEEE80211_AC_BE] = ATH_TXQ_AC_BE,
>>> [IEEE80211_AC_BK] = ATH_TXQ_AC_BK,
>>> [IEEE80211_AC_VI] = ATH_TXQ_AC_VI,
>>> [IEEE80211_AC_VO] = ATH_TXQ_AC_VO,
>>> };
>>>
>>> from mac80211.h:
>>>
>>> 101 /**
>>> 102  * enum ieee80211_ac_numbers - AC numbers as used in mac80211
>>> 103  * @IEEE80211_AC_VO: voice
>>> 104  * @IEEE80211_AC_VI: video
>>> 105  * @IEEE80211_AC_BE: best effort
>>> 106  * @IEEE80211_AC_BK: background
>>> 107  */
>>> 108 enum ieee80211_ac_numbers {
>>> 109         IEEE80211_AC_VO         = 0,
>>> 110         IEEE80211_AC_VI         = 1,
>>> 111         IEEE80211_AC_BE         = 2,
>>> 112         IEEE80211_AC_BK         = 3,
>>> 113 };
>>> 114 #define IEEE80211_NUM_ACS       4
>>>
>>> from ath9k/hw.h:
>>>
>>> 218 enum ath_hw_txq_subtype {
>>> 219         ATH_TXQ_AC_BE = 0,
>>> 220         ATH_TXQ_AC_BK = 1,
>>> 221         ATH_TXQ_AC_VI = 2,
>>> 222         ATH_TXQ_AC_VO = 3,
>>> 223 };
>>>
>>> this translates to the following mapping of AC to hw queues:
>>>
>>> static const int subtype_txq_to_hwq[] = {
>>> [2] = 0,
>>> [3] = 1,
>>> [1] = 2,
>>> [0] = 3,
>>> };
>>>
>>
>> This table confuse me too. But if you trace where the table and the
>> tqi_subtype field is used, then you face, that this table means
>> nothing for hw queue selection.
>>
>>>
>>> That is how I understand the source code and yes it looks a bit complicated
>>> but function wise seems to be correct.
>>> Or do I miss something here ?
>>>
>>
>> Actual hw queue selection occurs inside the ath9k_hw_setuptxqueue()
>> function (in mac.c). Stripped version looks like:
>>
>> int ath9k_hw_setuptxqueue(struct ath_hw *ah, enum ath9k_tx_queue type,
>>                          const struct ath9k_tx_queue_info *qinfo)
>> {
>>        int q;
>> [stripped]
>>                for (q = 0; q < ATH9K_NUM_TX_QUEUES; q++)
>>                        if (ah->txq[q].tqi_type ==
>>                            ATH9K_TX_QUEUE_INACTIVE)
>>                                break;
>> [stripped]
>>        return q;
>> }
>>
>> So, as Huber wrote, hw queue number really depends on call order. And
>> existing call order gives us the following result:
>>
>> ~# cat /sys/kernel/debug/ieee80211/phy0/ath9k/queues
>> (VO):  qnum: 0 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
>> (VI):  qnum: 1 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
>> (BE):  qnum: 2 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
>> (BK):  qnum: 3 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
>> (CAB): qnum: 8 qdepth:  0 ampdu-depth:  0 pending:   0 stopped: 0
>>
>> Just in case, I checked ath5k driver and it contains exactly the same code.
>>
>> Let's wait a comment from maintainers.
>>
>>> Greetings from Berlin
>>> Thomas
>>>
>>>
>>>
>>>
>>> Am 27.11.2014 um 13:04 schrieb Hubert Feurstein <h.feurstein@gmail.com>:
>>>
>>> Hi Sujith,
>>>
>>> 2014-11-26 13:25 GMT+01:00 Sujith Manoharan <sujith@msujith.org>:
>>>
>>> Hubert Feurstein wrote:
>>>
>>> Well, in fact it does. To understand my post you have to know the
>>> behaviour of ath_txq_setup. The point here is that on the first call
>>> of ath_txq_setup(..ATH9K_TX_QUEUE_DATA..), hw-queue 0 is returned. On
>>> the second call hw-queue 1 is returned, ... . And IEEE80211_AC_VO = 0,
>>> so hw-queue 0 is assigned, which is wrong in my opinion. But the right
>>> place to fix this would be ath_txq_setup itself.
>>>
>>>
>>> The mapping has been changed recently and I think it might be a problem,
>>> since this is what the datasheet says:
>>>
>>> "The mapping of physical DCUs to absolute channel access priorities is fixed
>>> and
>>> cannot be altered by software."
>>>
>>>
>>> Yes I know, and isn't this the reason why there is
>>> sc->tx.txq_map[<AC>] in place, to map the AC to the proper HW queue in
>>> software. So sc->tx.txq_map[IEEE80211_AC_VO] should return the
>>> higher-prio-queue 3 and not the low-prio-queue 0. In FreeBSD it is
>>> that way, in Linux it is the opposite, because in Linux
>>> IEEE80211_AC_VO=0 and in FreeBSD WME_AC_VO=3.
>>>
>>> Hubert
>>
>> --
>> BR,
>> Sergey
>> _______________________________________________
>> ath9k-devel mailing list
>> ath9k-devel at lists.ath9k.org
>> https://lists.ath9k.org/mailman/listinfo/ath9k-devel
>



-- 
BR,
Sergey

? ?????????? ???????????
??????? ??????