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 {