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;

Reply via email to