On Mon, Dec 07, 2020 at 02:36:05PM +0100, Tobias Heider wrote:
> Hi,
>
> our net80211 gapwait accounting implementation seems to have several
> problems:
> - If we lose packets with serial numbers 0 und 2 but receive the
> packet with serial number 1, the first gap wait timeout will
> skip serial number 0, flush out serial number 1 and then wait
> for serial number 2. However, at this time the timeout is not
> re-armed and we have to wait for a window slide.
> - The logic that does gap skip if too many packets are in the reorder
> buffer does not kick in if we lose two packets (as gapwait cannot
> reach windowsize - 1. Additionally, what the logic does is mostly
> equivalent to a normal window slide if we receive a packet that is
> beyond the window. So remove this logic completely.
>
> To fix this use ba_gapwait to actually count all the packets
> currently in the reorder buffer and restart the gap timeout if the
> buffer is not empty after we flush out some of the packets.
>
> Found and fix by Christian Erhardt (CC).
>
> ok?
I am happy with this if it works well during testing.
Could someone convince phessler@ to take this for a flight in Minecraft?
> Index: ieee80211_input.c
> ===================================================================
> RCS file: /cvs/src/sys/net80211/ieee80211_input.c,v
> retrieving revision 1.221
> diff -u -p -r1.221 ieee80211_input.c
> --- ieee80211_input.c 28 Aug 2020 12:01:48 -0000 1.221
> +++ ieee80211_input.c 7 Dec 2020 12:58:31 -0000
> @@ -839,30 +839,10 @@ ieee80211_input_ba(struct ieee80211com *
> /* store Rx meta-data too */
> rxi->rxi_flags |= IEEE80211_RXI_AMPDU_DONE;
> ba->ba_buf[idx].rxi = *rxi;
> + ba->ba_gapwait++;
>
> - if (ba->ba_buf[ba->ba_head].m == NULL) {
> - if (ba->ba_gapwait < (ba->ba_winsize - 1)) {
> - if (ba->ba_gapwait == 0) {
> - timeout_add_msec(&ba->ba_gap_to,
> - IEEE80211_BA_GAP_TIMEOUT);
> - }
> - ba->ba_gapwait++;
> - } else {
> - /*
> - * A full BA window worth of frames is now waiting.
> - * Skip the missing frame at the head of the window.
> - */
> - int skipped = ieee80211_input_ba_gap_skip(ba);
> - ic->ic_stats.is_ht_rx_ba_frame_lost += skipped;
> - ba->ba_gapwait = 0;
> - if (timeout_pending(&ba->ba_gap_to))
> - timeout_del(&ba->ba_gap_to);
> - }
> - } else {
> - ba->ba_gapwait = 0;
> - if (timeout_pending(&ba->ba_gap_to))
> - timeout_del(&ba->ba_gap_to);
> - }
> + if (ba->ba_buf[ba->ba_head].m == NULL && ba->ba_gapwait == 1)
> + timeout_add_msec(&ba->ba_gap_to, IEEE80211_BA_GAP_TIMEOUT);
>
> ieee80211_input_ba_flush(ic, ni, ba, ml);
> }
> @@ -894,6 +874,7 @@ ieee80211_input_ba_seq(struct ieee80211c
> ieee80211_inputm(ifp, ba->ba_buf[ba->ba_head].m,
> ni, &ba->ba_buf[ba->ba_head].rxi, ml);
> ba->ba_buf[ba->ba_head].m = NULL;
> + ba->ba_gapwait--;
> } else
> ic->ic_stats.is_ht_rx_ba_frame_lost++;
> ba->ba_head = (ba->ba_head + 1) % IEEE80211_BA_MAX_WINSZ;
> @@ -916,12 +897,18 @@ ieee80211_input_ba_flush(struct ieee8021
> ieee80211_inputm(ifp, ba->ba_buf[ba->ba_head].m, ni,
> &ba->ba_buf[ba->ba_head].rxi, ml);
> ba->ba_buf[ba->ba_head].m = NULL;
> + ba->ba_gapwait--;
>
> ba->ba_head = (ba->ba_head + 1) % IEEE80211_BA_MAX_WINSZ;
> /* move window forward */
> ba->ba_winstart = (ba->ba_winstart + 1) & 0xfff;
> }
> ba->ba_winend = (ba->ba_winstart + ba->ba_winsize - 1) & 0xfff;
> +
> + if (timeout_pending(&ba->ba_gap_to))
> + timeout_del(&ba->ba_gap_to);
> + if (ba->ba_gapwait)
> + timeout_add_msec(&ba->ba_gap_to, IEEE80211_BA_GAP_TIMEOUT);
> }
>
> /*
> @@ -989,6 +976,7 @@ ieee80211_ba_move_window(struct ieee8021
> ieee80211_inputm(ifp, ba->ba_buf[ba->ba_head].m, ni,
> &ba->ba_buf[ba->ba_head].rxi, ml);
> ba->ba_buf[ba->ba_head].m = NULL;
> + ba->ba_gapwait--;
> } else
> ic->ic_stats.is_ht_rx_ba_frame_lost++;
> ba->ba_head = (ba->ba_head + 1) % IEEE80211_BA_MAX_WINSZ;
>
