While working on iwm Tx aggregation and revisiting some parts of MiRA,
I have noticed a rate-control problem in iwm and iwx (this also affects
iwn, but I am leaving that for later since iwn already does Tx aggregation
and requires a more elaborate fix).
Rate control algorithms will choose a Tx rate and then gather feedback
from drivers on per-frame basis, with the assumption that feedback
provided by drivers is always based on frames which have been transmitted
at the most recently chosen rate.
During a transmit burst, e.g. while tcpbench is running, iwm and iwx will
queue up to 224 frames on the Tx ring (256 would fit, but our driver stops
queuing new frames after 224 have been queued; an easy way to tell whether
this condition has been reached is to check whether ifconfig reports the
OACTIVE interface flag).
The problem is that rate control will make decisions a lot more often than
just every 224 frames. Which means that results get reported and evaluated
even if they do not correspond to the most recently chosen rate, spoiling
the data available to the rate control algorithm.
This change prevents this problem by only reporting frames which match
the currently chosen Tx rate. Since the algorithms do not advance unless
their choose() function is called this effectively flushes out frames
queued at any previously chosen rates and restarts reporting as soon as
frames with the expected rate are being processed.
ok?
(I am also tweaking how iwx reports retries to AMRR; this is just to reduce
complexity and differences to iwm, and shouldn't make a big difference)
diff 144217ab9d3715f0cf6c417288c8a50500b221fa
addfc4a92b67df7f2aa49c5af241e149413d0015
blob - 2dbd3cac8dc4eddb316034b37b5ce951868b019a
blob + c9d48475e122021d2738076c838a2d8ef03cf48b
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -336,7 +336,7 @@ void iwx_rx_frame(struct iwx_softc *, struct mbuf *,
i
struct ieee80211_rxinfo *, struct mbuf_list *);
void iwx_enable_ht_cck_fallback(struct iwx_softc *, struct iwx_node *);
void iwx_rx_tx_cmd_single(struct iwx_softc *, struct iwx_rx_packet *,
- struct iwx_node *);
+ struct iwx_node *, int, int);
void iwx_rx_tx_cmd(struct iwx_softc *, struct iwx_rx_packet *,
struct iwx_rx_data *);
void iwx_rx_bmiss(struct iwx_softc *, struct iwx_rx_packet *,
@@ -3563,7 +3563,7 @@ iwx_enable_ht_cck_fallback(struct iwx_softc *sc, struc
void
iwx_rx_tx_cmd_single(struct iwx_softc *sc, struct iwx_rx_packet *pkt,
- struct iwx_node *in)
+ struct iwx_node *in, int txmcs, int txrate)
{
struct ieee80211com *ic = &sc->sc_ic;
struct ieee80211_node *ni = &in->in_ni;
@@ -3577,19 +3577,23 @@ iwx_rx_tx_cmd_single(struct iwx_softc *sc, struct iwx_
txfail = (status != IWX_TX_STATUS_SUCCESS &&
status != IWX_TX_STATUS_DIRECT_DONE);
- /* Update rate control statistics. */
+ /*
+ * Update rate control statistics.
+ * Only report frames which were actually queued with the currently
+ * selected Tx rate. Because Tx queues are relatively long we may
+ * encounter previously selected rates here during Tx bursts.
+ * Providing feedback based on such frames can lead to suboptimal
+ * Tx rate control decisions.
+ */
if ((ni->ni_flags & IEEE80211_NODE_HT) == 0 || in->ht_force_cck) {
- in->in_amn.amn_txcnt++;
- if (in->ht_force_cck) {
- /*
- * We want to move back to OFDM quickly if possible.
- * Only show actual Tx failures to AMRR, not retries.
- */
+ if (txrate == ni->ni_txrate) {
+ in->in_amn.amn_txcnt++;
if (txfail)
in->in_amn.amn_retrycnt++;
- } else if (tx_resp->failure_frame > 0)
- in->in_amn.amn_retrycnt++;
- } else if (ic->ic_fixed_mcs == -1) {
+ if (tx_resp->failure_frame > 0)
+ in->in_amn.amn_retrycnt++;
+ }
+ } else if (ic->ic_fixed_mcs == -1 && txmcs == ni->ni_txmcs) {
in->in_mn.frames += tx_resp->frame_count;
in->in_mn.ampdu_size = le16toh(tx_resp->byte_cnt);
in->in_mn.agglen = tx_resp->frame_count;
@@ -3650,7 +3654,7 @@ iwx_rx_tx_cmd(struct iwx_softc *sc, struct iwx_rx_pack
if (txd->m == NULL)
return;
- iwx_rx_tx_cmd_single(sc, pkt, txd->in);
+ iwx_rx_tx_cmd_single(sc, pkt, txd->in, txd->txmcs, txd->txrate);
iwx_txd_done(sc, txd);
/*
@@ -4304,6 +4308,8 @@ iwx_tx(struct iwx_softc *sc, struct mbuf *m, struct ie
}
data->m = m;
data->in = in;
+ data->txmcs = ni->ni_txmcs;
+ data->txrate = ni->ni_txrate;
/* Fill TX descriptor. */
num_tbs = 2 + data->map->dm_nsegs;
blob - e4f0d7a22fb88aee7e55923956e82fd096b4dc81
blob + b606c184937d38d0a4a887ff69b1e599bf399433
--- sys/dev/pci/if_iwxvar.h
+++ sys/dev/pci/if_iwxvar.h
@@ -245,6 +245,8 @@ struct iwx_tx_data {
bus_addr_t cmd_paddr;
struct mbuf *m;
struct iwx_node *in;
+ int txmcs;
+ int txrate;
};
struct iwx_tx_ring {
diff 8ac48d00ee3b52e70fd0fd2de67269d18dbd434b
144217ab9d3715f0cf6c417288c8a50500b221fa
blob - 5902ada2e6527fbb8b7ac5bef70085f410648b2c
blob + c6bd8bf4c37c108bf18050366c221f6ae08158b8
--- sys/dev/pci/if_iwm.c
+++ sys/dev/pci/if_iwm.c
@@ -374,7 +374,7 @@ int iwm_get_noise(const struct iwm_statistics_rx_non_p
void iwm_rx_frame(struct iwm_softc *, struct mbuf *, int, int, int, uint32_t,
struct ieee80211_rxinfo *, struct mbuf_list *);
void iwm_rx_tx_cmd_single(struct iwm_softc *, struct iwm_rx_packet *,
- struct iwm_node *);
+ struct iwm_node *, int, int);
void iwm_rx_tx_cmd(struct iwm_softc *, struct iwm_rx_packet *,
struct iwm_rx_data *);
void iwm_rx_bmiss(struct iwm_softc *, struct iwm_rx_packet *,
@@ -4126,7 +4126,7 @@ iwm_rx_mpdu_mq(struct iwm_softc *sc, struct mbuf *m, v
void
iwm_rx_tx_cmd_single(struct iwm_softc *sc, struct iwm_rx_packet *pkt,
- struct iwm_node *in)
+ struct iwm_node *in, int txmcs, int txrate)
{
struct ieee80211com *ic = &sc->sc_ic;
struct ieee80211_node *ni = &in->in_ni;
@@ -4140,14 +4140,23 @@ iwm_rx_tx_cmd_single(struct iwm_softc *sc, struct iwm_
txfail = (status != IWM_TX_STATUS_SUCCESS &&
status != IWM_TX_STATUS_DIRECT_DONE);
- /* Update rate control statistics. */
+ /*
+ * Update rate control statistics.
+ * Only report frames which were actually queued with the currently
+ * selected Tx rate. Because Tx queues are relatively long we may
+ * encounter previously selected rates here during Tx bursts.
+ * Providing feedback based on such frames can lead to suboptimal
+ * Tx rate control decisions.
+ */
if ((ni->ni_flags & IEEE80211_NODE_HT) == 0) {
- in->in_amn.amn_txcnt++;
- if (txfail)
- in->in_amn.amn_retrycnt++;
- if (tx_resp->failure_frame > 0)
- in->in_amn.amn_retrycnt++;
- } else if (ic->ic_fixed_mcs == -1) {
+ if (txrate == ni->ni_txrate) {
+ in->in_amn.amn_txcnt++;
+ if (txfail)
+ in->in_amn.amn_retrycnt++;
+ if (tx_resp->failure_frame > 0)
+ in->in_amn.amn_retrycnt++;
+ }
+ } else if (ic->ic_fixed_mcs == -1 && txmcs == ni->ni_txmcs) {
in->in_mn.frames += tx_resp->frame_count;
in->in_mn.ampdu_size = le16toh(tx_resp->byte_cnt);
in->in_mn.agglen = tx_resp->frame_count;
@@ -4214,7 +4223,7 @@ iwm_rx_tx_cmd(struct iwm_softc *sc, struct iwm_rx_pack
if (txd->m == NULL)
return;
- iwm_rx_tx_cmd_single(sc, pkt, txd->in);
+ iwm_rx_tx_cmd_single(sc, pkt, txd->in, txd->txmcs, txd->txrate);
iwm_txd_done(sc, txd);
/*
@@ -4944,6 +4953,8 @@ iwm_tx(struct iwm_softc *sc, struct mbuf *m, struct ie
}
data->m = m;
data->in = in;
+ data->txmcs = ni->ni_txmcs;
+ data->txrate = ni->ni_txrate;
/* Fill TX descriptor. */
desc->num_tbs = 2 + data->map->dm_nsegs;
blob - 7d1cc0da51c4fca51dc40382a5a8ed48a2728be8
blob + 869cdb3972a99eea46de7a8347b576d1281732da
--- sys/dev/pci/if_iwmvar.h
+++ sys/dev/pci/if_iwmvar.h
@@ -258,6 +258,8 @@ struct iwm_tx_data {
bus_addr_t scratch_paddr;
struct mbuf *m;
struct iwm_node *in;
+ int txmcs;
+ int txrate;
};
struct iwm_tx_ring {
