| Message ID | 7c3f72eac1c34921cd84a462e60d71e125862152.1676616450.git.ryder.lee@mediatek.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [1/2] wifi: mac80211: introduce ieee80211_refresh_tx_agg_session_timer() | expand |
On Sat, 2023-02-18 at 01:50 +0800, Ryder Lee wrote: > This allows low level drivers to refresh the tx agg session timer, based on > querying stats from the firmware usually. Especially for some mt76 devices > support .net_fill_forward_path would bypass mac80211, which leads to tx BA > session timeout for certain clients. > Does it even matter? We could just request sessions without a timeout in the first place. Or do you have a strong reason to need the timeout, such as limited hardware resources for (TX) aggregation sessions? But then maybe you should just time them out based on FW statistics directly, rather than having to periodically refresh the timer in mac80211? I don't mind the patch, and I'll happily take it if it's needed, I'm just wondering if that isn't a very roundabout way of achieving things. johannes
On Fri, 2023-02-17 at 19:01 +0100, Johannes Berg wrote: > On Sat, 2023-02-18 at 01:50 +0800, Ryder Lee wrote: > > This allows low level drivers to refresh the tx agg session timer, > > based on > > querying stats from the firmware usually. Especially for some mt76 > > devices > > support .net_fill_forward_path would bypass mac80211, which leads > > to tx BA > > session timeout for certain clients. > > > > Does it even matter? We could just request sessions without a timeout > in > the first place. > I think we're already. Our main issue is performance periodically drops every few seconds when .net_fill_forward_path is enabled. Wireless client have normal 500+ Mb/s iperf3 download speed for several seconds. Then it drops less than 100 Mb/s for several seconds. Then everything repeats. Issue occurs only on certain clients. (i.e. Intel cards AX200, AX1675, Advanced-N 6235 in Win11) > Or do you have a strong reason to need the timeout, such as limited > hardware resources for (TX) aggregation sessions? > > But then maybe you should just time them out based on FW statistics > directly, rather than having to periodically refresh the timer in > mac80211? > > I don't mind the patch, and I'll happily take it if it's needed, I'm > just wondering if that isn't a very roundabout way of achieving > things. > > johannes
On Fri, 2023-02-17 at 18:43 +0000, Ryder Lee wrote: > On Fri, 2023-02-17 at 19:01 +0100, Johannes Berg wrote: > > On Sat, 2023-02-18 at 01:50 +0800, Ryder Lee wrote: > > > This allows low level drivers to refresh the tx agg session timer, > > > based on > > > querying stats from the firmware usually. Especially for some mt76 > > > devices > > > support .net_fill_forward_path would bypass mac80211, which leads > > > to tx BA > > > session timeout for certain clients. > > > > > > > Does it even matter? We could just request sessions without a timeout > > in > > the first place. > > > > I think we're already. Our main issue is performance periodically drops > every few seconds when .net_fill_forward_path is enabled. Wireless > client have normal 500+ Mb/s iperf3 download speed for several seconds. > Then it drops less than 100 Mb/s for several seconds. Then everything > repeats. Issue occurs only on certain clients. (i.e. Intel cards AX200, > AX1675, Advanced-N 6235 in Win11) > Strange. But how does this patch do anything about it, that should be completely client agnostic? johannes
On Fri, 2023-02-17 at 19:53 +0100, Johannes Berg wrote: > On Fri, 2023-02-17 at 18:43 +0000, Ryder Lee wrote: > > On Fri, 2023-02-17 at 19:01 +0100, Johannes Berg wrote: > > > On Sat, 2023-02-18 at 01:50 +0800, Ryder Lee wrote: > > > > This allows low level drivers to refresh the tx agg session > > > > timer, > > > > based on > > > > querying stats from the firmware usually. Especially for some > > > > mt76 > > > > devices > > > > support .net_fill_forward_path would bypass mac80211, which > > > > leads > > > > to tx BA > > > > session timeout for certain clients. > > > > > > > > > > Does it even matter? We could just request sessions without a > > > timeout > > > in > > > the first place. > > > > > > > I think we're already. Our main issue is performance periodically > > drops > > every few seconds when .net_fill_forward_path is enabled. Wireless > > client have normal 500+ Mb/s iperf3 download speed for several > > seconds. > > Then it drops less than 100 Mb/s for several seconds. Then > > everything > > repeats. Issue occurs only on certain clients. (i.e. Intel cards > > AX200, > > AX1675, Advanced-N 6235 in Win11) > > > > Strange. But how does this patch do anything about it, that should be > completely client agnostic? > > Since there's no any keep alive packet being received by host stack, leads to mac80211 destrory BA sesion. Ax200 series needs to update timer for each 5s period to maintain ba session. We originally did this to workaround issue, but obviouly this hack will not be accepted upstream, since it effectively completely disables the session expiry timer without removing the code. --- a/net/mac80211/agg-tx.c +++ b/net/mac80211/agg-tx.c @@ -568,10 +568,9 @@ static void sta_tx_agg_session_timer_expired(struct timer_list *t) } timeout = tid_tx->last_tx + TU_TO_JIFFIES(tid_tx->timeout); - if (time_is_after_jiffies(timeout)) { - mod_timer(&tid_tx->session_timer, timeout); - return; - } + /* remove timerout handle for ax210 interoperability issue */ + mod_timer(&tid_tx->session_timer, timeout); + return; I'm not sure if there's a better way to fix this though. Ryder
On Fri, 2023-02-17 at 19:02 +0000, Ryder Lee wrote: > On Fri, 2023-02-17 at 19:53 +0100, Johannes Berg wrote: > > On Fri, 2023-02-17 at 18:43 +0000, Ryder Lee wrote: > > > On Fri, 2023-02-17 at 19:01 +0100, Johannes Berg wrote: > > > > On Sat, 2023-02-18 at 01:50 +0800, Ryder Lee wrote: > > > > > This allows low level drivers to refresh the tx agg session > > > > > timer, > > > > > based on > > > > > querying stats from the firmware usually. Especially for some > > > > > mt76 > > > > > devices > > > > > support .net_fill_forward_path would bypass mac80211, which > > > > > leads > > > > > to tx BA > > > > > session timeout for certain clients. > > > > > > > > > > > > > Does it even matter? We could just request sessions without a > > > > timeout > > > > in > > > > the first place. > > > > > > > > > > I think we're already. Our main issue is performance periodically > > > drops > > > every few seconds when .net_fill_forward_path is enabled. > > > Wireless > > > client have normal 500+ Mb/s iperf3 download speed for several > > > seconds. > > > Then it drops less than 100 Mb/s for several seconds. Then > > > everything > > > repeats. Issue occurs only on certain clients. (i.e. Intel cards > > > AX200, > > > AX1675, Advanced-N 6235 in Win11) > > > > > > > Strange. But how does this patch do anything about it, that should > > be > > completely client agnostic? > > > > > > Since there's no any keep alive packet being received by host stack, > leads to mac80211 destrory BA sesion. > More specifically, the BA session relies on client side's Tx data to maintain ... but the point is mac80211 can't get any data after .net_fill_forward_path being enabled (HW path). So, we need a way to notify mac80211 to refresh last_tx... I think my patch is needed for that case. Ryder
On Mon, 2023-02-20 at 02:55 +0000, Ryder Lee wrote: > On Fri, 2023-02-17 at 19:02 +0000, Ryder Lee wrote: > > On Fri, 2023-02-17 at 19:53 +0100, Johannes Berg wrote: > > > On Fri, 2023-02-17 at 18:43 +0000, Ryder Lee wrote: > > > > On Fri, 2023-02-17 at 19:01 +0100, Johannes Berg wrote: > > > > > On Sat, 2023-02-18 at 01:50 +0800, Ryder Lee wrote: > > > > > > This allows low level drivers to refresh the tx agg session > > > > > > timer, > > > > > > based on > > > > > > querying stats from the firmware usually. Especially for > > > > > > some > > > > > > mt76 > > > > > > devices > > > > > > support .net_fill_forward_path would bypass mac80211, which > > > > > > leads > > > > > > to tx BA > > > > > > session timeout for certain clients. > > > > > > > > > > > > > > > > Does it even matter? We could just request sessions without a > > > > > timeout > > > > > in > > > > > the first place. > > > > > > > > > > > > > I think we're already. Our main issue is performance > > > > periodically > > > > drops > > > > every few seconds when .net_fill_forward_path is enabled. > > > > Wireless > > > > client have normal 500+ Mb/s iperf3 download speed for several > > > > seconds. > > > > Then it drops less than 100 Mb/s for several seconds. Then > > > > everything > > > > repeats. Issue occurs only on certain clients. (i.e. Intel > > > > cards > > > > AX200, > > > > AX1675, Advanced-N 6235 in Win11) > > > > > > > > > > Strange. But how does this patch do anything about it, that > > > should > > > be > > > completely client agnostic? > > > > > > > > > > Since there's no any keep alive packet being received by host > > stack, > > leads to mac80211 destrory BA sesion. > > > > More specifically, the BA session relies on client side's Tx data to Typo... I mean *our side*. Something like this ieee80211_8023_xmit() if (tid_tx->timeout) tid_tx->last_tx = jiffies; Ryder
Hi, > > > Since there's no any keep alive packet being received by host > > > stack, leads to mac80211 destrory BA sesion. > > > > > > > More specifically, the BA session relies on client side's Tx data to > > Typo... I mean *our side*. Something like this Sorry. I'm just totally confused - I thought the initiator only set the timeout, but I see now that it's negotiated and the actual value used is from the client. Which explains basically everything. johannes
On Tue, 2023-02-21 at 10:57 +0100, Johannes Berg wrote: > Hi, > > > > > Since there's no any keep alive packet being received by host > > > > stack, leads to mac80211 destrory BA sesion. > > > > > > > > > > More specifically, the BA session relies on client side's Tx data > > > to > > > > Typo... I mean *our side*. Something like this > > Sorry. I'm just totally confused - I thought the initiator only set > the > timeout, but I see now that it's negotiated and the actual value used > is > from the client. > > Which explains basically everything. > > Yup ... after accepting the AddBA Response we activated a timer, *resetting it after each frame that we send* - sta_tx_agg_session_timer_expired(). The .net_fill_forward_path() offloads tx path to HW, so it can only rely on other way to reset as mac80211 isn't aware of that. Ryder
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 76a12bec71d5..920ea31620cd 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -5993,6 +5993,18 @@ void ieee80211_queue_delayed_work(struct ieee80211_hw *hw, struct delayed_work *dwork, unsigned long delay); +/** + * ieee80211_refresh_tx_agg_session_timer - Refresh a tx agg session timer. + * @sta: the station for which to start a BA session + * @tid: the TID to BA on. + * + * This function allows low level driver to refresh tx agg session timer + * to maintain BA session, the session level will still be managed by the + * mac80211. + */ +void ieee80211_refresh_tx_agg_session_timer(struct ieee80211_sta *sta, + u16 tid); + /** * ieee80211_start_tx_ba_session - Start a tx Block Ack session. * @sta: the station for which to start a BA session diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c index f9514bacbd4a..3b651e7f5a73 100644 --- a/net/mac80211/agg-tx.c +++ b/net/mac80211/agg-tx.c @@ -554,6 +554,23 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid) ieee80211_send_addba_with_timeout(sta, tid_tx); } +void ieee80211_refresh_tx_agg_session_timer(struct ieee80211_sta *pubsta, + u16 tid) +{ + struct sta_info *sta = container_of(pubsta, struct sta_info, sta); + struct tid_ampdu_tx *tid_tx; + + if (WARN_ON_ONCE(tid >= IEEE80211_NUM_TIDS)) + return; + + tid_tx = rcu_dereference(sta->ampdu_mlme.tid_tx[tid]); + if (!tid_tx) + return; + + tid_tx->last_tx = jiffies; +} +EXPORT_SYMBOL(ieee80211_refresh_tx_agg_session_timer); + /* * After accepting the AddBA Response we activated a timer, * resetting it after each frame that we send.
This allows low level drivers to refresh the tx agg session timer, based on querying stats from the firmware usually. Especially for some mt76 devices support .net_fill_forward_path would bypass mac80211, which leads to tx BA session timeout for certain clients. Signed-off-by: Ryder Lee <ryder.lee@mediatek.com> --- include/net/mac80211.h | 12 ++++++++++++ net/mac80211/agg-tx.c | 17 +++++++++++++++++ 2 files changed, 29 insertions(+)