On Thu, Mar 18, 2021 at 10:32:54PM +0800, zxystd wrote:
> Hi Stefan,
> I agree, we had better using the ac array directly.
> According to our tests, this actually takes effect for speed improving,
> before this fix, iwx can run up to about 60mbps download and 20mbps upload
> which is slower than iwm, after applying this fix, they are the same. I don't
> know why, but I think you can try it. Thanks for your reply.
> zxystd.
Below is an updated patch. It uses an array and also fixes the mapping.
Your patch has the mapping inversed. It is effectively sending every
frame as class VO (highest priority). Perhaps that's why you are seeing
some difference in throughput? Your patch maps the values like this:
static const uint8_t iwx_ac_to_ucode_ac[] = {
IWX_AC_VO,
IWX_AC_VI,
IWX_AC_BE,
IWX_AC_BK
};
This means your mapping is as follows:
BE -> VO
BK -> VI
VI -> BE
VO -> BK
The two ranges are as follows:
range 1:
enum ieee80211_edca_ac {
EDCA_AC_BK = 1, /* Background */
EDCA_AC_BE = 0, /* Best Effort */
EDCA_AC_VI = 2, /* Video */
EDCA_AC_VO = 3 /* Voice */
};
#define EDCA_NUM_AC 4
(I don't know why ieee80211_edca_ac is defined out of order.
This could be an error).
range 2:
#define IWX_AC_BK 0
#define IWX_AC_BE 1
#define IWX_AC_VI 2
#define IWX_AC_VO 3
#define IWX_AC_NUM 4
And note that IWX_AC is actually the same as IWX_TX_FIFO:
#define IWX_TX_FIFO_BK 0
#define IWX_TX_FIFO_BE 1
#define IWX_TX_FIFO_VI 2
#define IWX_TX_FIFO_VO 3
It is better to have a specific map for IWX_AC for clarity.
But it becomes a purely cosmetic change.
diff 1a45b5b6e341aafc249501a50d7a0f9ed64b7957 /usr/src
blob - 1f619296ab144ed0e286ebb37611b6f21f733f66
file + sys/dev/pci/if_iwx.c
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -2372,6 +2372,14 @@ const uint8_t iwx_ac_to_tx_fifo[] = {
IWX_GEN2_EDCA_TX_FIFO_VO,
};
+/* Map ieee80211_edca_ac categories to an iwx_ac_qos array index. */
+const uint8_t iwx_ac_to_ucode_ac[] = {
+ IWX_AC_BE,
+ IWX_AC_BK,
+ IWX_AC_VI,
+ IWX_AC_VO
+};
+
int
iwx_enable_txq(struct iwx_softc *sc, int sta_id, int qid, int tid,
int num_slots)
@@ -5181,12 +5189,13 @@ iwx_mac_ctxt_cmd_common(struct iwx_softc *sc, struct i
for (i = 0; i < EDCA_NUM_AC; i++) {
struct ieee80211_edca_ac_params *ac = &ic->ic_edca_ac[i];
int txf = iwx_ac_to_tx_fifo[i];
+ uint8_t ucode_ac = iwx_ac_to_ucode_ac[i];
- cmd->ac[txf].cw_min = htole16(IWX_EXP2(ac->ac_ecwmin));
- cmd->ac[txf].cw_max = htole16(IWX_EXP2(ac->ac_ecwmax));
- cmd->ac[txf].aifsn = ac->ac_aifsn;
- cmd->ac[txf].fifos_mask = (1 << txf);
- cmd->ac[txf].edca_txop = htole16(ac->ac_txoplimit * 32);
+ cmd->ac[ucode_ac].cw_min = htole16(IWX_EXP2(ac->ac_ecwmin));
+ cmd->ac[ucode_ac].cw_max = htole16(IWX_EXP2(ac->ac_ecwmax));
+ cmd->ac[ucode_ac].aifsn = ac->ac_aifsn;
+ cmd->ac[ucode_ac].fifos_mask = (1 << txf);
+ cmd->ac[ucode_ac].edca_txop = htole16(ac->ac_txoplimit * 32);
}
if (ni->ni_flags & IEEE80211_NODE_QOS)
cmd->qos_flags |= htole32(IWX_MAC_QOS_FLG_UPDATE_EDCA);
> ---Original---
> From: "Stefan Sperling"<s...@stsp.name>
> Date: Thu, Mar 18, 2021 17:07 PM
> To: "zxystd"<zxy...@foxmail.com>;
> Cc: "tech"<tech@openbsd.org>;
> Subject: Re: Fix iwx wrong ac indexs.
>
>
> On Thu, Mar 18, 2021 at 11:41:33AM +0800, zxystd wrote:
> > The indexes into the ac array in the iwx_mac_ctxt_cmd_common are from the
> > iwx_ac enum and not the txfs. The current code therefore puts the edca
> params
> > in the wrong indexes of the array, causing wrong priority for
> data-streams of
> > different ACs. This bug is not exists in iwm as its txf number is equal
> to
> > the corresponding ac in the iwx_ac enum.
>
> Thank you!
> This doesn't really cause any problem as far as I can see, since we
> don't actually make use of QoS. But I agree it should be fixed.
>
> > Index: src/sys/dev/pci/if_iwx.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/pci/if_iwx.c,v
> > retrieving revision 1.50
> > diff -u -p -u -r1.50 if_iwx.c
> > --- src/sys/dev/pci/if_iwx.c17 Mar 2021 15:59:27 -00001.50
> > +++ src/sys/dev/pci/if_iwx.c18 Mar 2021 03:28:04 -0000
> > @@ -5135,6 +5135,17 @@ iwx_ack_rates(struct iwx_softc *sc, stru
> > *ofdm_rates = ofdm;
> > }
> >
> > +static uint8_t iwx_mac80211_ac_to_ucode_ac(enum ieee80211_edca_ac ac)
> > +{
> > +static const uint8_t iwx_ac_to_ucode_ac[] = {
> > +IWX_AC_VO,
> > +IWX_AC_VI,
> > +IWX_AC_BE,
> > +IWX_AC_BK
> > +};
>
> I would prefer to use a global array without wrapping it inside a function.
> Other code can simply index the array directly, see below.
>
> > +return iwx_ac_to_ucode_ac[ac];
> > +}
> > +
> > void
> > iwx_mac_ctxt_cmd_common(struct iwx_softc *sc, struct iwx_node *in,
> > struct iwx_mac_ctx_cmd *cmd, uint32_t
> action)
> > @@ -5181,12 +5192,13 @@ iwx_mac_ctxt_cmd_common(struct iwx_softc
> > for (i = 0; i < EDCA_NUM_AC; i++) {
> > struct ieee80211_edca_ac_params *ac = &ic->ic_edca_ac[i];
> > int txf = iwx_ac_to_tx_fifo[i];
> > +uint8_t ucode_ac = iwx_mac80211_ac_to_ucode_ac(i);
>
> Instead of calling a function, we could do an array lookup:
>
> uint8_t ucode_ac = iwx_ac_to_ucode_ac[ac];
> >
> > -cmd->ac[txf].cw_min = htole16(IWX_EXP2(ac->ac_ecwmin));
> > -cmd->ac[txf].cw_max = htole16(IWX_EXP2(ac->ac_ecwmax));
> > -cmd->ac[txf].aifsn = ac->ac_aifsn;
> > -cmd->ac[txf].fifos_mask = (1 << txf);
> > -cmd->ac[txf].edca_txop = htole16(ac->ac_txoplimit * 32);
> > +cmd->ac[ucode_ac].cw_min = htole16(IWX_EXP2(ac->ac_ecwmin));
> > +cmd->ac[ucode_ac].cw_max = htole16(IWX_EXP2(ac->ac_ecwmax));
> > +cmd->ac[ucode_ac].aifsn = ac->ac_aifsn;
> > +cmd->ac[ucode_ac].fifos_mask = (1 << txf);
> > +cmd->ac[ucode_ac].edca_txop = htole16(ac->ac_txoplimit * 32);
> > }
> > if (ni->ni_flags & IEEE80211_NODE_QOS)
> > cmd->qos_flags |= htole32(IWX_MAC_QOS_FLG_UPDATE_EDCA);
