On Tue, Sep 20, 2016 at 02:35:18AM +0200, Stefan Sperling wrote: > I'd like to know if this diff helps with iwm(4) performance issues > some people have been reporting. > > This is not done yet and some details don't really make sense, especially > the #if 0 hiding of what should be correct code -- yet, enabling that code > makes the problem come back. > > But hopefully this is generally going in the right direction. > > Thank you, Edward Wandasiewicz, for pointing out that reverting if_iwm.c > r1.85 restores performance. That was the door to this rabbit hole :-)
Here's a new version. Requires bleeding edge -current and also this uncommitted patch I posted earlier today: http://marc.info/?l=openbsd-tech&m=147440006608035&q=raw It already seems to behave better. But the problem seems to be load dependent and I can't trigger it very reliably. Thanks to all who provided feedback for the previous version. Index: if_iwm.c =================================================================== RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v retrieving revision 1.132 diff -u -p -r1.132 if_iwm.c --- if_iwm.c 12 Sep 2016 10:18:26 -0000 1.132 +++ if_iwm.c 20 Sep 2016 21:52:57 -0000 @@ -301,6 +301,7 @@ void iwm_init_channel_map(struct iwm_sof void iwm_setup_ht_rates(struct iwm_softc *); void iwm_htprot_task(void *); void iwm_update_htprot(struct ieee80211com *, struct ieee80211_node *); +void iwm_updateedca(struct ieee80211com *); int iwm_ampdu_rx_start(struct ieee80211com *, struct ieee80211_node *, uint8_t); void iwm_ampdu_rx_stop(struct ieee80211com *, struct ieee80211_node *, @@ -402,10 +403,10 @@ int iwm_config_umac_scan(struct iwm_soft int iwm_umac_scan(struct iwm_softc *); void iwm_ack_rates(struct iwm_softc *, struct iwm_node *, int *, int *); void iwm_mac_ctxt_cmd_common(struct iwm_softc *, struct iwm_node *, - struct iwm_mac_ctx_cmd *, uint32_t); + struct iwm_mac_ctx_cmd *, uint32_t, int); void iwm_mac_ctxt_cmd_fill_sta(struct iwm_softc *, struct iwm_node *, struct iwm_mac_data_sta *, int); -int iwm_mac_ctxt_cmd(struct iwm_softc *, struct iwm_node *, uint32_t); +int iwm_mac_ctxt_cmd(struct iwm_softc *, struct iwm_node *, uint32_t, int); int iwm_update_quotas(struct iwm_softc *, struct iwm_node *); int iwm_auth(struct iwm_softc *); int iwm_assoc(struct iwm_softc *); @@ -2379,13 +2380,16 @@ void iwm_sta_rx_agg(struct iwm_softc *sc, struct ieee80211_node *ni, uint8_t tid, uint16_t ssn, int start) { + struct ieee80211com *ic = &sc->sc_ic; struct iwm_add_sta_cmd_v7 cmd; struct iwm_node *in = (void *)ni; int err, s; uint32_t status; - if (start && sc->sc_rx_ba_sessions >= IWM_MAX_RX_BA_SESSIONS) + if (start && sc->sc_rx_ba_sessions >= IWM_MAX_RX_BA_SESSIONS) { + ieee80211_addba_req_refuse(ic, ni, tid); return; + } memset(&cmd, 0, sizeof(cmd)); @@ -2406,17 +2410,18 @@ iwm_sta_rx_agg(struct iwm_softc *sc, str status = IWM_ADD_STA_SUCCESS; err = iwm_send_cmd_pdu_status(sc, IWM_ADD_STA, sizeof(cmd), &cmd, &status); - if (err) - return; - if (status == IWM_ADD_STA_SUCCESS) { - s = splnet(); - if (start) + s = splnet(); + if (err == 0 && status == IWM_ADD_STA_SUCCESS) { + if (start) { sc->sc_rx_ba_sessions++; - else if (sc->sc_rx_ba_sessions > 0) + ieee80211_addba_req_accept(ic, ni, tid); + } else if (sc->sc_rx_ba_sessions > 0) sc->sc_rx_ba_sessions--; - splx(s); - } + } else if (start) + ieee80211_addba_req_refuse(ic, ni, tid); + + splx(s); } void @@ -2428,13 +2433,29 @@ iwm_htprot_task(void *arg) int err; /* This call updates HT protection based on in->in_ni.ni_htop1. */ - err = iwm_mac_ctxt_cmd(sc, in, IWM_FW_CTXT_ACTION_MODIFY); + err = iwm_mac_ctxt_cmd(sc, in, IWM_FW_CTXT_ACTION_MODIFY, 1); if (err) printf("%s: could not change HT protection: error %d\n", DEVNAME(sc), err); } /* + * This function is called by upper layer when EDCA settings in + * beacons have changed. + */ +void +iwm_updateedca(struct ieee80211com *ic) +{ + struct iwm_softc *sc = ic->ic_softc; + + if (ic->ic_state != IEEE80211_S_RUN) + return; + + /* XXX htprot_task is also used here and should be renamed */ + task_add(systq, &sc->htprot_task); +} + +/* * This function is called by upper layer when HT protection settings in * beacons have changed. */ @@ -2479,7 +2500,7 @@ iwm_ampdu_rx_start(struct ieee80211com * sc->ba_ssn = htole16(ba->ba_winstart); task_add(systq, &sc->ba_task); - return 0; /* XXX firmware may still fail to add BA agreement... */ + return EBUSY; } /* @@ -4203,14 +4224,15 @@ void iwm_power_build_cmd(struct iwm_softc *sc, struct iwm_node *in, struct iwm_mac_power_cmd *cmd) { - struct ieee80211com *ic = &sc->sc_ic; struct ieee80211_node *ni = &in->in_ni; - int dtimper, dtimper_msec; - int keep_alive; + int dtim_period, dtim_msec, keep_alive; cmd->id_and_color = htole32(IWM_FW_CMD_ID_AND_COLOR(in->in_id, in->in_color)); - dtimper = ic->ic_dtim_period ?: 1; + if (ni->ni_dtimperiod) + dtim_period = ni->ni_dtimperiod; + else + dtim_period = 1; /* * Regardless of power management state the driver must set @@ -4218,11 +4240,16 @@ iwm_power_build_cmd(struct iwm_softc *sc * immediately after association. Check that keep alive period * is at least 3 * DTIM. */ - dtimper_msec = dtimper * ni->ni_intval; - keep_alive - = MAX(3 * dtimper_msec, 1000 * IWM_POWER_KEEP_ALIVE_PERIOD_SEC); + dtim_msec = dtim_period * ni->ni_intval; + keep_alive = MAX(3 * dtim_msec, 1000 * IWM_POWER_KEEP_ALIVE_PERIOD_SEC); keep_alive = roundup(keep_alive, 1000) / 1000; cmd->keep_alive_seconds = htole16(keep_alive); + +#ifdef notyet + cmd->flags = htole16(IWM_POWER_FLAGS_POWER_SAVE_ENA_MSK); + cmd->rx_data_timeout = IWM_DEFAULT_PS_RX_DATA_TIMEOUT; + cmd->tx_data_timeout = IWM_DEFAULT_PS_TX_DATA_TIMEOUT; +#endif } int @@ -4250,7 +4277,9 @@ int iwm_power_update_device(struct iwm_softc *sc) { struct iwm_device_power_cmd cmd = { +#ifdef notyet .flags = htole16(IWM_DEVICE_POWER_FLAGS_POWER_SAVE_ENA_MSK), +#endif }; if (!(sc->sc_capaflags & IWM_UCODE_TLV_FLAGS_DEVICE_PS_CMD)) @@ -4975,8 +5004,9 @@ iwm_ack_rates(struct iwm_softc *sc, stru void iwm_mac_ctxt_cmd_common(struct iwm_softc *sc, struct iwm_node *in, - struct iwm_mac_ctx_cmd *cmd, uint32_t action) + struct iwm_mac_ctx_cmd *cmd, uint32_t action, int assoc) { +#define IWM_EXP2(x) ((1 << (x)) - 1) /* CWmin = 2^ECWmin - 1 */ struct ieee80211com *ic = &sc->sc_ic; struct ieee80211_node *ni = ic->ic_bss; int cck_ack_rates, ofdm_ack_rates; @@ -4987,14 +5017,11 @@ iwm_mac_ctxt_cmd_common(struct iwm_softc cmd->action = htole32(action); cmd->mac_type = htole32(IWM_FW_MAC_TYPE_BSS_STA); - cmd->tsf_id = htole32(in->in_tsfid); + cmd->tsf_id = htole32(IWM_TSF_ID_A); IEEE80211_ADDR_COPY(cmd->node_addr, ic->ic_myaddr); - if (in->in_assoc) { - IEEE80211_ADDR_COPY(cmd->bssid_addr, ni->ni_bssid); - } else { - IEEE80211_ADDR_COPY(cmd->bssid_addr, etherbroadcastaddr); - } + IEEE80211_ADDR_COPY(cmd->bssid_addr, ni->ni_bssid); + iwm_ack_rates(sc, in, &cck_ack_rates, &ofdm_ack_rates); cmd->cck_rates = htole32(cck_ack_rates); cmd->ofdm_rates = htole32(ofdm_ack_rates); @@ -5006,15 +5033,18 @@ iwm_mac_ctxt_cmd_common(struct iwm_softc = htole32((ic->ic_flags & IEEE80211_F_SHSLOT) ? IWM_MAC_FLG_SHORT_SLOT : 0); - for (i = 0; i < IWM_AC_NUM+1; i++) { - int txf = i; - - cmd->ac[txf].cw_min = htole16(0x0f); - cmd->ac[txf].cw_max = htole16(0x3f); - cmd->ac[txf].aifsn = 1; + for (i = 0; i < EDCA_NUM_AC; i++) { + struct ieee80211_edca_ac_params *ac = &ic->ic_edca_ac[i]; + int txf = iwm_ac_to_tx_fifo[i]; + + cmd->ac[txf].cw_min = htole16(IWM_EXP2(ac->ac_ecwmin)); + cmd->ac[txf].cw_max = htole16(IWM_EXP2(ac->ac_ecwmax)); + cmd->ac[txf].aifsn = ac->ac_aifsn; cmd->ac[txf].fifos_mask = (1 << txf); - cmd->ac[txf].edca_txop = 0; + cmd->ac[txf].edca_txop = htole16(ac->ac_txoplimit * 32); } + if (ni->ni_flags & IEEE80211_NODE_QOS) + cmd->qos_flags |= htole32(IWM_MAC_QOS_FLG_UPDATE_EDCA); if (ni->ni_flags & IEEE80211_NODE_HT) { enum ieee80211_htprot htprot = @@ -5034,56 +5064,58 @@ iwm_mac_ctxt_cmd_common(struct iwm_softc default: break; } - cmd->qos_flags |= htole32(IWM_MAC_QOS_FLG_TGN); } if (ic->ic_flags & IEEE80211_F_USEPROT) cmd->protection_flags |= htole32(IWM_MAC_PROT_FLG_TGG_PROTECT); - cmd->filter_flags = htole32(IWM_MAC_FILTER_ACCEPT_GRP); + cmd->filter_flags |= htole32(IWM_MAC_FILTER_ACCEPT_GRP); +#undef IWM_EXP2 } void iwm_mac_ctxt_cmd_fill_sta(struct iwm_softc *sc, struct iwm_node *in, - struct iwm_mac_data_sta *ctxt_sta, int force_assoc_off) + struct iwm_mac_data_sta *sta, int assoc) { struct ieee80211_node *ni = &in->in_ni; - struct ieee80211com *ic = &sc->sc_ic; + uint32_t dtim_off; + uint64_t tsf; - ctxt_sta->is_assoc = htole32(0); - ctxt_sta->bi = htole32(ni->ni_intval); - ctxt_sta->bi_reciprocal = htole32(iwm_reciprocal(ni->ni_intval)); - ctxt_sta->dtim_interval = htole32(ni->ni_intval * ic->ic_dtim_period); - ctxt_sta->dtim_reciprocal = - htole32(iwm_reciprocal(ni->ni_intval * ic->ic_dtim_period)); - - /* 10 = CONN_MAX_LISTEN_INTERVAL */ - ctxt_sta->listen_interval = htole32(10); - ctxt_sta->assoc_id = htole32(ni->ni_associd); + dtim_off = ni->ni_dtimcount * ni->ni_intval * IEEE80211_DUR_TU; + memcpy(&tsf, ni->ni_tstamp, sizeof(tsf)); + tsf = letoh64(tsf); + + sta->is_assoc = htole32(assoc); + sta->dtim_time = htole32(ni->ni_rstamp + dtim_off); + sta->dtim_tsf = htole64(tsf + dtim_off); + sta->bi = htole32(ni->ni_intval); + sta->bi_reciprocal = htole32(iwm_reciprocal(ni->ni_intval)); + sta->dtim_interval = htole32(ni->ni_intval * ni->ni_dtimperiod); + sta->dtim_reciprocal = htole32(iwm_reciprocal(sta->dtim_interval)); + sta->listen_interval = htole32(10); + sta->assoc_id = htole32(ni->ni_associd); + sta->assoc_beacon_arrive_time = htole32(ni->ni_rstamp); } int -iwm_mac_ctxt_cmd(struct iwm_softc *sc, struct iwm_node *in, uint32_t action) +iwm_mac_ctxt_cmd(struct iwm_softc *sc, struct iwm_node *in, uint32_t action, + int assoc) { + struct ieee80211_node *ni = &in->in_ni; struct iwm_mac_ctx_cmd cmd; memset(&cmd, 0, sizeof(cmd)); - iwm_mac_ctxt_cmd_common(sc, in, &cmd, action); + iwm_mac_ctxt_cmd_common(sc, in, &cmd, action, assoc); /* Allow beacons to pass through as long as we are not associated or we * do not have dtim period information */ - if (!in->in_assoc || !sc->sc_ic.ic_dtim_period) + if (!assoc || !ni->ni_associd || !ni->ni_dtimperiod) cmd.filter_flags |= htole32(IWM_MAC_FILTER_IN_BEACON); else - cmd.filter_flags &= ~htole32(IWM_MAC_FILTER_IN_BEACON); + iwm_mac_ctxt_cmd_fill_sta(sc, in, &cmd.sta, assoc); - /* Fill the data specific for station mode */ - iwm_mac_ctxt_cmd_fill_sta(sc, in, - &cmd.sta, action == IWM_FW_CTXT_ACTION_ADD); - - return iwm_send_cmd_pdu(sc, IWM_MAC_CONTEXT_CMD, 0, sizeof(cmd), - &cmd); + return iwm_send_cmd_pdu(sc, IWM_MAC_CONTEXT_CMD, 0, sizeof(cmd), &cmd); } int @@ -5145,8 +5177,7 @@ iwm_update_quotas(struct iwm_softc *sc, /* Give the remainder of the session to the first binding */ cmd.quotas[0].quota = htole32(le32toh(cmd.quotas[0].quota) + quota_rem); - return iwm_send_cmd_pdu(sc, IWM_TIME_QUOTA_CMD, 0, - sizeof(cmd), &cmd); + return iwm_send_cmd_pdu(sc, IWM_TIME_QUOTA_CMD, 0, sizeof(cmd), &cmd); } int @@ -5157,8 +5188,6 @@ iwm_auth(struct iwm_softc *sc) uint32_t duration; int err; - in->in_assoc = 0; - err = iwm_sf_config(sc, IWM_SF_FULL_ON); if (err) return err; @@ -5182,7 +5211,7 @@ iwm_auth(struct iwm_softc *sc) if (err) return err; - err = iwm_mac_ctxt_cmd(sc, in, IWM_FW_CTXT_ACTION_MODIFY); + err = iwm_mac_ctxt_cmd(sc, in, IWM_FW_CTXT_ACTION_MODIFY, 0); if (err) { printf("%s: failed to update MAC\n", DEVNAME(sc)); return err; @@ -5192,9 +5221,11 @@ iwm_auth(struct iwm_softc *sc) * Prevent the FW from wandering off channel during association * by "protecting" the session with a time event. */ - /* XXX duration is in units of TU, not MS */ - duration = IWM_TE_SESSION_PROTECTION_MAX_TIME_MS; - iwm_protect_session(sc, in, duration, 500 /* XXX magic number */); + if (in->in_ni.ni_intval) + duration = in->in_ni.ni_intval * 2; + else + duration = IEEE80211_DUR_TU; + iwm_protect_session(sc, in, duration, in->in_ni.ni_intval / 2); DELAY(100); return 0; @@ -5211,14 +5242,6 @@ iwm_assoc(struct iwm_softc *sc) if (err) return err; - in->in_assoc = 1; - - err = iwm_mac_ctxt_cmd(sc, in, IWM_FW_CTXT_ACTION_MODIFY); - if (err) { - printf("%s: failed to update MAC\n", DEVNAME(sc)); - return err; - } - return 0; } @@ -5284,17 +5307,15 @@ iwm_setrates(struct iwm_node *in) struct iwm_host_cmd cmd = { .id = IWM_LQ_CMD, .len = { sizeof(in->in_lq), }, - .flags = 0, + .flags = IWM_LQ_FLAG_USE_RTS_MSK, }; memset(lq, 0, sizeof(*lq)); lq->sta_id = IWM_STATION_ID; - /* For HT, enable RTS/CTS, and SGI (if supported). */ - if (ni->ni_flags & IEEE80211_NODE_HT) { - lq->flags |= IWM_LQ_FLAG_USE_RTS_MSK; + if (ni->ni_flags & IEEE80211_NODE_HT) sgi_ok = (ni->ni_htcaps & IEEE80211_HTCAP_SGI20); - } else + else sgi_ok = 0; /* @@ -5422,10 +5443,6 @@ iwm_newstate_task(void *psc) /* Reset the device if moving out of AUTH, ASSOC, or RUN. */ /* XXX Is there a way to switch states without a full reset? */ if (ostate > IEEE80211_S_SCAN && nstate < ostate) { - in = (struct iwm_node *)ic->ic_bss; - if (in) - in->in_assoc = 0; - iwm_stop_device(sc); iwm_init_hw(sc); @@ -5479,16 +5496,46 @@ iwm_newstate_task(void *psc) case IEEE80211_S_RUN: in = (struct iwm_node *)ic->ic_bss; - iwm_power_mac_update_mode(sc, in); + + /* We have now been assigned an associd by the AP. */ + err = iwm_mac_ctxt_cmd(sc, in, IWM_FW_CTXT_ACTION_MODIFY, 1); + if (err) { + printf("%s: failed to update MAC\n", DEVNAME(sc)); + return; + } + + err = iwm_power_update_device(sc); + if (err) { + printf("%s: could send power command (error %d)\n", + DEVNAME(sc), err); + return; + } #ifdef notyet /* * Disabled for now. Default beacon filter settings * prevent net80211 from getting ERP and HT protection * updates from beacons. */ - iwm_enable_beacon_filter(sc, in); + err = iwm_enable_beacon_filter(sc, in); + if (err) { + printf("%s: could not enable beacon filter\n", + DEVNAME(sc)); + return; + } #endif - iwm_update_quotas(sc, in); + err = iwm_power_mac_update_mode(sc, in); + if (err) { + printf("%s: could not update MAC power (error %d)\n", + DEVNAME(sc), err); + return; + } + + err = iwm_update_quotas(sc, in); + if (err) { + printf("%s: could not update quotas (error %d)\n", + DEVNAME(sc), err); + return; + } ieee80211_amrr_node_init(&sc->sc_amrr, &in->in_amn); @@ -5820,7 +5867,7 @@ iwm_init_hw(struct iwm_softc *sc) err = iwm_power_update_device(sc); if (err) { - printf("%s: could send power update command (error %d)\n", + printf("%s: could send power command (error %d)\n", DEVNAME(sc), err); goto err; } @@ -5853,7 +5900,7 @@ iwm_init_hw(struct iwm_softc *sc) } } - err = iwm_mac_ctxt_cmd(sc, in, IWM_FW_CTXT_ACTION_ADD); + err = iwm_mac_ctxt_cmd(sc, in, IWM_FW_CTXT_ACTION_ADD, 0); if (err) { printf("%s: could not add MAC context (error %d)\n", DEVNAME(sc), err); @@ -6009,7 +6056,6 @@ iwm_stop(struct ifnet *ifp, int disable) ifq_clr_oactive(&ifp->if_snd); in->in_phyctxt = NULL; - in->in_assoc = 0; task_del(systq, &sc->init_task); task_del(sc->sc_nswq, &sc->newstate_task); @@ -7158,6 +7204,7 @@ iwm_attach(struct device *parent, struct /* Override 802.11 state transition machine. */ sc->sc_newstate = ic->ic_newstate; ic->ic_newstate = iwm_newstate; + ic->ic_updateedca = iwm_updateedca; ic->ic_update_htprot = iwm_update_htprot; ic->ic_ampdu_rx_start = iwm_ampdu_rx_start; ic->ic_ampdu_rx_stop = iwm_ampdu_rx_stop; Index: if_iwmvar.h =================================================================== RCS file: /cvs/src/sys/dev/pci/if_iwmvar.h,v retrieving revision 1.23 diff -u -p -r1.23 if_iwmvar.h --- if_iwmvar.h 12 Sep 2016 10:18:26 -0000 1.23 +++ if_iwmvar.h 20 Sep 2016 20:25:22 -0000 @@ -501,10 +501,6 @@ struct iwm_node { uint16_t in_id; uint16_t in_color; - int in_tsfid; - - /* status "bits" */ - int in_assoc; struct iwm_lq_cmd in_lq; struct ieee80211_amrr_node in_amn;
