Re: re-enable A-MSDU support with iwm(4) and iwx(4) fixed
Stefan Sperling wrote: > This patch adds A-MSDU rx offloading support for both iwm(4) and iwx(4) > and re-enables net80211's software A-MSDU Rx support for all 11n drivers. > > Meaning iwn(4) and athn(4) will also be receiving A-MSDUs again. > This feature has been turned off since July 2019: > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net80211/ieee80211_input.c#rev1.207 > The root cause of the problem I saw at that time was related to dlg's Rx > queue back-pressure mechanism. Once we figured this out, all wireless drivers > were fixed to call if_input() only once per interrupt so this is no longer > an issue. > > For a very brief period we tried to enable A-MSDUs again in -current but > we found out that iwm/iwx needed additional work for the new devices which > received support while A-MSDU was disabled in-tree: > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net80211/ieee80211_input.c#rev1.226 > The patch below completes the missing bits to make A-MSDUs work on these > new devices. > > My previous iwm-only patch and related test reports are here: > https://marc.info/?t=16170390711=1=2 > The iwm changes have already been extensively tested. > For iwm, this new patch only adds an additional range check: > @@ -4640,7 +4640,8 @@ iwm_rx_reorder(struct iwm_softc *sc, struct mbuf > *m, i > > baid = (reorder_data & IWM_RX_MPDU_REORDER_BAID_MASK) >> > IWM_RX_MPDU_REORDER_BAID_SHIFT; > - if (baid == IWM_RX_REORDER_DATA_INVALID_BAID) > + if (baid == IWM_RX_REORDER_DATA_INVALID_BAID || > + baid >= nitems(sc->sc_rxba_data)) > return 0; > > rxba = >sc_rxba_data[baid]; > > > I have tested on the following devices: > > iwm0 at pci2 dev 0 function 0 "Intel Dual Band Wireless-AC 8265" rev 0x78, msi > iwm0: hw rev 0x230, fw ver 34.0.1, address 7c:11:xx:xx:xx:xx > > iwx0 at pci5 dev 0 function 0 "Intel Wi-Fi 6 AX200" rev 0x1a, msix > iwx0: hw rev 0x340, fw ver 48.1335886879.0, address d0:ab:xx:xx:xx:xx > > iwx0 at pci0 dev 20 function 3 "Intel Wi-Fi 6 AX201" rev 0x00, msix > iwx0: hw rev 0x350, fw ver 48.1335886879.0, address 0c:7a:xx:xx:xx:xx > > ok? > No regressions under normal use and a little bit of stress testing. iwm0 at pci2 dev 0 function 0 "Intel AC 7265" rev 0x59, msi iwm0: hw rev 0x210, fw ver 17.3216344376.0, address dc:53:xx:xx:xx:xx Tim.
Re: re-enable A-MSDU support with iwm(4) and iwx(4) fixed
Hi, * Stefan Sperling wrote: > On Fri, Apr 23, 2021 at 12:28:42PM +0200, Matthias Schmidt wrote: > > I had a new kernel with only your following patch running all day and > > never encountered the situation as described in my last email. > > Connection was stable and transfer rates remained around 3MB/s. This is > > less then the rates mentioned in your recent email but I rarely get > > higher speeds. So I assume it is to my environment and has nothing to > > do with the patch. > > Ok. Thanks for spending the time to check this thoroughly. I have already > made myself spend many hours debugging ghosts after test observations like > this, many many times :-) So I've become careful about quickly drawing > conclusions from observations like this. Wifi is complicated and as far > as I know nobody is ever testing OpenBSD wifi patches in an RF-isolated lab. > > Many problem reports I receive (and many of those arrive in private > email, unfortunately) boil down to "it doesn't work when the cafe downstairs > fills up with laptops and phones at lunch time" or "it only becomes slow > when two or more laptops are doing video calls at the same time". > The problem is that sometimes there are bugs to fix or missing features to > implement in situations like this, so I cannot just dismiss such reports > outright. But dealing with them can take a huge amount of time. > To be honest, I wouldn't consider your problem a blocking issue. I would > first wait to see if the exact same issue pops up elsewhere after the > patch lands in the tree. Seems I hit a byzantine situation like that. I had the patch running on the T450s with the 8265 and additionally on a X250 with the following hardware all evening and morning (including suspend/resume cycles) and it worked as expected. iwm0 at pci2 dev 0 function 0 "Intel AC 7265" rev 0x59, msi iwm0: hw rev 0x210, fw ver 17.3216344376.0, address Cheers and thanks for the good work and your efforts Matthias
Re: re-enable A-MSDU support with iwm(4) and iwx(4) fixed
On Fri, Apr 23, 2021 at 12:28:42PM +0200, Matthias Schmidt wrote: > I had a new kernel with only your following patch running all day and > never encountered the situation as described in my last email. > Connection was stable and transfer rates remained around 3MB/s. This is > less then the rates mentioned in your recent email but I rarely get > higher speeds. So I assume it is to my environment and has nothing to > do with the patch. Ok. Thanks for spending the time to check this thoroughly. I have already made myself spend many hours debugging ghosts after test observations like this, many many times :-) So I've become careful about quickly drawing conclusions from observations like this. Wifi is complicated and as far as I know nobody is ever testing OpenBSD wifi patches in an RF-isolated lab. Many problem reports I receive (and many of those arrive in private email, unfortunately) boil down to "it doesn't work when the cafe downstairs fills up with laptops and phones at lunch time" or "it only becomes slow when two or more laptops are doing video calls at the same time". The problem is that sometimes there are bugs to fix or missing features to implement in situations like this, so I cannot just dismiss such reports outright. But dealing with them can take a huge amount of time. To be honest, I wouldn't consider your problem a blocking issue. I would first wait to see if the exact same issue pops up elsewhere after the patch lands in the tree. > I'll now reboot to your other patch, again, and start testing again. Great, thanks! I would be surprised if it was 100% related, but if it actually is then I'm certainly interested in trying to debug it.
Re: re-enable A-MSDU support with iwm(4) and iwx(4) fixed
Hi Stefan, * Stefan Sperling wrote: > On Thu, Apr 22, 2021 at 07:47:29PM +0200, Matthias Schmidt wrote: > > I have a kernel with your patch running since several hours and > > noticed a regression. My usual "test case" is copying several large > > files from my file server via NFSv3 to my laptop. In the beginning the > > transfer rate was about 2-3M/s and after some time it dropped to around > > 50-300K/s and never recovered (transfer is now running for 2.5h). > > > > I have the following device in a Thinkpad T450s connect to a Fritzbox > > AP. > > > > iwm0 at pci2 dev 0 function 0 "Intel Dual Band Wireless-AC 8265" rev 0x78, > > msi > > iwm0: hw rev 0x230, fw ver 34.0.1, address > > The patch doesn't actually change anything in the driver's receive path for > the 8265 chip. You might have seen a side-effect of something else that is > unrelated to the patch, such as the wifi channel suddently becoming very > busy with unrelated traffic. We should rule that out with a fair amount > of certainty before trying to debug the issue. So my question would be if > this problem is 100% repeatable with the patch and not at all repeatable > without the patch? > > One possibility is that you've managed to trigger a problem in the receive > path of net80211 with A-MSDUs enabled. Such a bug would already have existed > but it would have been dormant since July 2019 when A-MSDUs were disabled. > > For 8265 devices the entire patch boils down to this change so you could > simply revert all the other changes and try this much smaller patch instead > to test for this regression: I had a new kernel with only your following patch running all day and never encountered the situation as described in my last email. Connection was stable and transfer rates remained around 3MB/s. This is less then the rates mentioned in your recent email but I rarely get higher speeds. So I assume it is to my environment and has nothing to do with the patch. I'll now reboot to your other patch, again, and start testing again. Cheers Matthias > diff 804ff40445876fb6652323b00b9804f826133e70 /tmp/net80211 > blob - a2de00f7bcdef99ced5d09da5e9b4bc8615156bd > file + ieee80211_input.c > --- ieee80211_input.c > +++ ieee80211_input.c > @@ -2758,7 +2758,7 @@ ieee80211_recv_addba_req(struct ieee80211com *ic, stru > ba->ba_params = (params & IEEE80211_ADDBA_BA_POLICY); > ba->ba_params |= ((ba->ba_winsize << IEEE80211_ADDBA_BUFSZ_SHIFT) | > (tid << IEEE80211_ADDBA_TID_SHIFT)); > -#if 0 > +#if 1 > /* iwm(4) 9k and iwx(4) need more work before AMSDU can be enabled. */ > ba->ba_params |= IEEE80211_ADDBA_AMSDU; > #endif > blob - 5bd45d993b558bac50a513c1c4422508d96f44ba > file + ieee80211_proto.c > --- ieee80211_proto.c > +++ ieee80211_proto.c > @@ -695,7 +695,7 @@ ieee80211_addba_request(struct ieee80211com *ic, struc > ba->ba_params = > (ba->ba_winsize << IEEE80211_ADDBA_BUFSZ_SHIFT) | > (tid << IEEE80211_ADDBA_TID_SHIFT); > -#if 0 > +#if 1 > /* iwm(4) 9k and iwx(4) need more work before AMSDU can be enabled. */ > ba->ba_params |= IEEE80211_ADDBA_AMSDU; > #endif
Re: re-enable A-MSDU support with iwm(4) and iwx(4) fixed
On Thu, Apr 22, 2021 at 07:47:29PM +0200, Matthias Schmidt wrote: > I have a kernel with your patch running since several hours and > noticed a regression. My usual "test case" is copying several large > files from my file server via NFSv3 to my laptop. In the beginning the > transfer rate was about 2-3M/s and after some time it dropped to around > 50-300K/s and never recovered (transfer is now running for 2.5h). > > I have the following device in a Thinkpad T450s connect to a Fritzbox > AP. > > iwm0 at pci2 dev 0 function 0 "Intel Dual Band Wireless-AC 8265" rev 0x78, msi > iwm0: hw rev 0x230, fw ver 34.0.1, address Just to put your measurements in perspective, here are results I see with tcpbench sending from the LAN through a pepwave 11AC access point, which transmits A-MSDUs, towards a laptop with iwm 8265 with the full iwm+iwx A-MSDU patch applied. I haven't noticed any stability issues, and will keep running the patch in case they do pop up. This is ~60 Mbps i.e. 7.5 megabytes/s in peak, not 2-3 metabytes/s. Your NFS test's transfer speed is below what this patch makes possible. It will depend on the AP and the wifi environment. My test was using an otherwise idle channel with a single iwm client. Conn: 1 Mbps: 59.171 Peak Mbps: 62.283 Avg Mbps: 59.171 1061227573040 60.584 100.00% Conn: 1 Mbps: 60.584 Peak Mbps: 62.283 Avg Mbps: 60.584 1071237755488 61.982 100.00% Conn: 1 Mbps: 61.982 Peak Mbps: 62.283 Avg Mbps: 61.982 1081237487608 59.901 100.00% Conn: 1 Mbps: 59.901 Peak Mbps: 62.283 Avg Mbps: 59.901 1091257665712 61.264 100.00% Conn: 1 Mbps: 61.264 Peak Mbps: 62.283 Avg Mbps: 61.264 1101287416656 59.215 100.00% Conn: 1 Mbps: 59.215 Peak Mbps: 62.283 Avg Mbps: 59.215 287619376 60.955 100.00% Conn: 1 Mbps: 60.955 Peak Mbps: 62.283 Avg Mbps: 60.955 1121297028592 56.229 100.00% Conn: 1 Mbps: 56.229 Peak Mbps: 62.283 Avg Mbps: 56.229 1131316908408 55.212 100.00% Conn: 1 Mbps: 55.212 Peak Mbps: 62.283 Avg Mbps: 55.212 1141317267512 58.140 100.00% Conn: 1 Mbps: 58.140 Peak Mbps: 62.283 Avg Mbps: 58.140
Re: re-enable A-MSDU support with iwm(4) and iwx(4) fixed
On Thu, Apr 22, 2021 at 07:47:29PM +0200, Matthias Schmidt wrote: > I have a kernel with your patch running since several hours and > noticed a regression. My usual "test case" is copying several large > files from my file server via NFSv3 to my laptop. In the beginning the > transfer rate was about 2-3M/s and after some time it dropped to around > 50-300K/s and never recovered (transfer is now running for 2.5h). > > I have the following device in a Thinkpad T450s connect to a Fritzbox > AP. > > iwm0 at pci2 dev 0 function 0 "Intel Dual Band Wireless-AC 8265" rev 0x78, msi > iwm0: hw rev 0x230, fw ver 34.0.1, address The patch doesn't actually change anything in the driver's receive path for the 8265 chip. You might have seen a side-effect of something else that is unrelated to the patch, such as the wifi channel suddently becoming very busy with unrelated traffic. We should rule that out with a fair amount of certainty before trying to debug the issue. So my question would be if this problem is 100% repeatable with the patch and not at all repeatable without the patch? One possibility is that you've managed to trigger a problem in the receive path of net80211 with A-MSDUs enabled. Such a bug would already have existed but it would have been dormant since July 2019 when A-MSDUs were disabled. For 8265 devices the entire patch boils down to this change so you could simply revert all the other changes and try this much smaller patch instead to test for this regression: diff 804ff40445876fb6652323b00b9804f826133e70 /tmp/net80211 blob - a2de00f7bcdef99ced5d09da5e9b4bc8615156bd file + ieee80211_input.c --- ieee80211_input.c +++ ieee80211_input.c @@ -2758,7 +2758,7 @@ ieee80211_recv_addba_req(struct ieee80211com *ic, stru ba->ba_params = (params & IEEE80211_ADDBA_BA_POLICY); ba->ba_params |= ((ba->ba_winsize << IEEE80211_ADDBA_BUFSZ_SHIFT) | (tid << IEEE80211_ADDBA_TID_SHIFT)); -#if 0 +#if 1 /* iwm(4) 9k and iwx(4) need more work before AMSDU can be enabled. */ ba->ba_params |= IEEE80211_ADDBA_AMSDU; #endif blob - 5bd45d993b558bac50a513c1c4422508d96f44ba file + ieee80211_proto.c --- ieee80211_proto.c +++ ieee80211_proto.c @@ -695,7 +695,7 @@ ieee80211_addba_request(struct ieee80211com *ic, struc ba->ba_params = (ba->ba_winsize << IEEE80211_ADDBA_BUFSZ_SHIFT) | (tid << IEEE80211_ADDBA_TID_SHIFT); -#if 0 +#if 1 /* iwm(4) 9k and iwx(4) need more work before AMSDU can be enabled. */ ba->ba_params |= IEEE80211_ADDBA_AMSDU; #endif
Re: re-enable A-MSDU support with iwm(4) and iwx(4) fixed
Hi Stefan, * Stefan Sperling wrote: > This patch adds A-MSDU rx offloading support for both iwm(4) and iwx(4) > and re-enables net80211's software A-MSDU Rx support for all 11n drivers. > > Meaning iwn(4) and athn(4) will also be receiving A-MSDUs again. > This feature has been turned off since July 2019: > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net80211/ieee80211_input.c#rev1.207 > The root cause of the problem I saw at that time was related to dlg's Rx > queue back-pressure mechanism. Once we figured this out, all wireless drivers > were fixed to call if_input() only once per interrupt so this is no longer > an issue. > > For a very brief period we tried to enable A-MSDUs again in -current but > we found out that iwm/iwx needed additional work for the new devices which > received support while A-MSDU was disabled in-tree: > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net80211/ieee80211_input.c#rev1.226 > The patch below completes the missing bits to make A-MSDUs work on these > new devices. I have a kernel with your patch running since several hours and noticed a regression. My usual "test case" is copying several large files from my file server via NFSv3 to my laptop. In the beginning the transfer rate was about 2-3M/s and after some time it dropped to around 50-300K/s and never recovered (transfer is now running for 2.5h). I have the following device in a Thinkpad T450s connect to a Fritzbox AP. iwm0 at pci2 dev 0 function 0 "Intel Dual Band Wireless-AC 8265" rev 0x78, msi iwm0: hw rev 0x230, fw ver 34.0.1, address Cheers Matthias
re-enable A-MSDU support with iwm(4) and iwx(4) fixed
This patch adds A-MSDU rx offloading support for both iwm(4) and iwx(4) and re-enables net80211's software A-MSDU Rx support for all 11n drivers. Meaning iwn(4) and athn(4) will also be receiving A-MSDUs again. This feature has been turned off since July 2019: https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net80211/ieee80211_input.c#rev1.207 The root cause of the problem I saw at that time was related to dlg's Rx queue back-pressure mechanism. Once we figured this out, all wireless drivers were fixed to call if_input() only once per interrupt so this is no longer an issue. For a very brief period we tried to enable A-MSDUs again in -current but we found out that iwm/iwx needed additional work for the new devices which received support while A-MSDU was disabled in-tree: https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net80211/ieee80211_input.c#rev1.226 The patch below completes the missing bits to make A-MSDUs work on these new devices. My previous iwm-only patch and related test reports are here: https://marc.info/?t=16170390711=1=2 The iwm changes have already been extensively tested. For iwm, this new patch only adds an additional range check: @@ -4640,7 +4640,8 @@ iwm_rx_reorder(struct iwm_softc *sc, struct mbuf *m, i baid = (reorder_data & IWM_RX_MPDU_REORDER_BAID_MASK) >> IWM_RX_MPDU_REORDER_BAID_SHIFT; - if (baid == IWM_RX_REORDER_DATA_INVALID_BAID) + if (baid == IWM_RX_REORDER_DATA_INVALID_BAID || + baid >= nitems(sc->sc_rxba_data)) return 0; rxba = >sc_rxba_data[baid]; I have tested on the following devices: iwm0 at pci2 dev 0 function 0 "Intel Dual Band Wireless-AC 8265" rev 0x78, msi iwm0: hw rev 0x230, fw ver 34.0.1, address 7c:11:xx:xx:xx:xx iwx0 at pci5 dev 0 function 0 "Intel Wi-Fi 6 AX200" rev 0x1a, msix iwx0: hw rev 0x340, fw ver 48.1335886879.0, address d0:ab:xx:xx:xx:xx iwx0 at pci0 dev 20 function 3 "Intel Wi-Fi 6 AX201" rev 0x00, msix iwx0: hw rev 0x350, fw ver 48.1335886879.0, address 0c:7a:xx:xx:xx:xx ok? diff refs/heads/master refs/heads/amsdu blob - 00bf20b37ed33a652232885349c2f3dfa0d666d3 blob + 05cc01334b522b7b537fba6df4b4fd3e0c5d8eb9 --- sys/dev/pci/if_iwm.c +++ sys/dev/pci/if_iwm.c @@ -144,6 +144,8 @@ #include #include #include +#include /* for SEQ_LT */ +#undef DPRINTF /* defined in ieee80211_priv.h */ #define DEVNAME(_s)((_s)->sc_dev.dv_xname) @@ -328,12 +330,17 @@ int iwm_mimo_enabled(struct iwm_softc *); void iwm_setup_ht_rates(struct iwm_softc *); void iwm_htprot_task(void *); void iwm_update_htprot(struct ieee80211com *, struct ieee80211_node *); +void iwm_init_reorder_buffer(struct iwm_reorder_buffer *, uint16_t, + uint16_t); +void iwm_clear_reorder_buffer(struct iwm_softc *, struct iwm_rxba_data *); intiwm_ampdu_rx_start(struct ieee80211com *, struct ieee80211_node *, uint8_t); void iwm_ampdu_rx_stop(struct ieee80211com *, struct ieee80211_node *, uint8_t); +void iwm_rx_ba_session_expired(void *); +void iwm_reorder_timer_expired(void *); void iwm_sta_rx_agg(struct iwm_softc *, struct ieee80211_node *, uint8_t, - uint16_t, uint16_t, int); + uint16_t, uint16_t, int, int); #ifdef notyet intiwm_ampdu_tx_start(struct ieee80211com *, struct ieee80211_node *, uint8_t); @@ -372,8 +379,10 @@ intiwm_rxmq_get_signal_strength(struct iwm_softc *, s void iwm_rx_rx_phy_cmd(struct iwm_softc *, struct iwm_rx_packet *, struct iwm_rx_data *); intiwm_get_noise(const struct iwm_statistics_rx_non_phy *); +intiwm_rx_hwdecrypt(struct iwm_softc *, struct mbuf *, uint32_t, + struct ieee80211_rxinfo *); intiwm_ccmp_decap(struct iwm_softc *, struct mbuf *, - struct ieee80211_node *); + struct ieee80211_node *, struct ieee80211_rxinfo *); void iwm_rx_frame(struct iwm_softc *, struct mbuf *, int, uint32_t, int, int, uint32_t, struct ieee80211_rxinfo *, struct mbuf_list *); void iwm_rx_tx_cmd_single(struct iwm_softc *, struct iwm_rx_packet *, @@ -490,6 +499,20 @@ void iwm_nic_umac_error(struct iwm_softc *); #endif void iwm_rx_mpdu(struct iwm_softc *, struct mbuf *, void *, size_t, struct mbuf_list *); +void iwm_flip_address(uint8_t *); +intiwm_detect_duplicate(struct iwm_softc *, struct mbuf *, + struct iwm_rx_mpdu_desc *, struct ieee80211_rxinfo *); +intiwm_is_sn_less(uint16_t, uint16_t, uint16_t); +void iwm_release_frames(struct iwm_softc *, struct ieee80211_node *, + struct iwm_rxba_data *, struct iwm_reorder_buffer *, uint16_t, + struct mbuf_list *); +intiwm_oldsn_workaround(struct iwm_softc *, struct ieee80211_node *, + int, struct iwm_reorder_buffer *, uint32_t, uint32_t); +intiwm_rx_reorder(struct iwm_softc *, struct mbuf *, int, + struct iwm_rx_mpdu_desc *, int, int, uint32_t, +
