| Message ID | 20230518091912.181090-1-mail@david-bauer.net |
|---|---|
| State | Deferred |
| Delegated to: | David Bauer |
| Headers | show |
| Series | mac80211: always use mac80211 loss detection | expand |
On 18.05.23 11:19, David Bauer wrote: > ath10k does not report excessive loss in case of broken block-ack > sessions. The loss is communicated to the host-os, but ath10k does not > trigger a low-ack events by itself. > > The mac80211 framework for loss detection however detects this > circumstance well in case of ath10k. So use it regardless of ath10k's > own loss detection mechanism. > > Signed-off-by: David Bauer <mail@david-bauer.net> Please make a patch for ath10k instead of turning the flag into a no-op in mac80211. - Felix
Hi Felix, On 6/23/23 08:55, Felix Fietkau wrote: > On 18.05.23 11:19, David Bauer wrote: >> ath10k does not report excessive loss in case of broken block-ack >> sessions. The loss is communicated to the host-os, but ath10k does not >> trigger a low-ack events by itself. >> >> The mac80211 framework for loss detection however detects this >> circumstance well in case of ath10k. So use it regardless of ath10k's >> own loss detection mechanism. >> >> Signed-off-by: David Bauer <mail@david-bauer.net> > > Please make a patch for ath10k instead of turning the flag into a no-op in mac80211. The rationale for removal was in fact to avoid patching ath1xk separately, given these are the only drivers using this flag. I'm aware this is not the nicest approach, do you have any insight if there's a downside to always keeping mac80211 loss detection active? I however still respect the preferation of keeping this limited to specific drivers, I'm just interested if there's a deeper rationale / problem i did not spot :) Just to outline the issue this is trying to avoid - Intel clients started dropping their BA sessions sometimes in late 2020 without notifying the AP. The ath10k firmware keeps retransmitting exclusive to the device at the lowest rate while not indicating a low-ack situation to the host-os, avoiding station kickout. This results in very low throughput for all connected stations (aql enabled) up to DoS of the AP (aql disabled). Best David > > - Felix
On 23.06.23 12:29, David Bauer wrote: > Hi Felix, > > On 6/23/23 08:55, Felix Fietkau wrote: >> On 18.05.23 11:19, David Bauer wrote: >>> ath10k does not report excessive loss in case of broken block-ack >>> sessions. The loss is communicated to the host-os, but ath10k does not >>> trigger a low-ack events by itself. >>> >>> The mac80211 framework for loss detection however detects this >>> circumstance well in case of ath10k. So use it regardless of ath10k's >>> own loss detection mechanism. >>> >>> Signed-off-by: David Bauer <mail@david-bauer.net> >> >> Please make a patch for ath10k instead of turning the flag into a no-op in mac80211. > > The rationale for removal was in fact to avoid patching ath1xk separately, given these > are the only drivers using this flag. > > I'm aware this is not the nicest approach, do you have any insight if there's a downside > to always keeping mac80211 loss detection active? > > I however still respect the preferation of keeping this limited to specific drivers, I'm > just interested if there's a deeper rationale / problem i did not spot :) > > Just to outline the issue this is trying to avoid - Intel clients started dropping their > BA sessions sometimes in late 2020 without notifying the AP. The ath10k firmware keeps > retransmitting exclusive to the device at the lowest rate while not indicating a low-ack > situation to the host-os, avoiding station kickout. This results in very low throughput > for all connected stations (aql enabled) up to DoS of the AP (aql disabled). My rationale is this: changes to the driver dropping that flag can be upstreamed, because your description implies that it fixes a real bug. A mac80211 patch turning the flag into an no-op will be rejected, since it doesn't make any sense to keep the flag around. If it turns out that all ath drivers can't use this flag because of the issue you're describing, we can remove it upstream from mac80211 entirely instead of turning it into a dummy no-op. - Felix
Hi Felix, On 6/23/23 12:47, Felix Fietkau wrote: > On 23.06.23 12:29, David Bauer wrote: >> Hi Felix, >> >> On 6/23/23 08:55, Felix Fietkau wrote: >>> On 18.05.23 11:19, David Bauer wrote: >>>> ath10k does not report excessive loss in case of broken block-ack >>>> sessions. The loss is communicated to the host-os, but ath10k does not >>>> trigger a low-ack events by itself. >>>> >>>> The mac80211 framework for loss detection however detects this >>>> circumstance well in case of ath10k. So use it regardless of ath10k's >>>> own loss detection mechanism. >>>> >>>> Signed-off-by: David Bauer <mail@david-bauer.net> >>> >>> Please make a patch for ath10k instead of turning the flag into a no-op in mac80211. >> >> The rationale for removal was in fact to avoid patching ath1xk separately, given these >> are the only drivers using this flag. >> >> I'm aware this is not the nicest approach, do you have any insight if there's a downside >> to always keeping mac80211 loss detection active? >> >> I however still respect the preferation of keeping this limited to specific drivers, I'm >> just interested if there's a deeper rationale / problem i did not spot :) >> >> Just to outline the issue this is trying to avoid - Intel clients started dropping their >> BA sessions sometimes in late 2020 without notifying the AP. The ath10k firmware keeps >> retransmitting exclusive to the device at the lowest rate while not indicating a low-ack >> situation to the host-os, avoiding station kickout. This results in very low throughput >> for all connected stations (aql enabled) up to DoS of the AP (aql disabled). > > My rationale is this: changes to the driver dropping that flag can be upstreamed, because your description implies that it fixes a real bug. > A mac80211 patch turning the flag into an no-op will be rejected, since it doesn't make any sense to keep the flag around. Thanks, I haven't looked at it this way. I will update the patch to account for this. > If it turns out that all ath drivers can't use this flag because of the issue you're describing, we can remove it upstream from mac80211 entirely instead of turning it into a dummy no-op. I was not at the point of upstreaming, as from reading the code the root-cause is within the firmware not reporting low-ack situation. So this requires to be evaluated with all existing platform-firmware in mind. As a sidenote - the issue I'm describing also exists with QCAs own driver, however the ramifications there greatly differ from vendor to vendor. Best David > > - Felix > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
diff --git a/package/kernel/mac80211/patches/subsys/340-mac80211-always-use-mac80211-loss-detection.patch b/package/kernel/mac80211/patches/subsys/340-mac80211-always-use-mac80211-loss-detection.patch new file mode 100644 index 0000000000..1e3a41c302 --- /dev/null +++ b/package/kernel/mac80211/patches/subsys/340-mac80211-always-use-mac80211-loss-detection.patch @@ -0,0 +1,33 @@ +From cdf461888f900c3a149b10a04d72b4a590ecdec3 Mon Sep 17 00:00:00 2001 +From: David Bauer <mail@david-bauer.net> +Date: Tue, 16 May 2023 23:11:32 +0200 +Subject: [PATCH] mac80211: always use mac80211 loss detection + +ath10k does not report excessive loss in case of broken block-ack +sessions. The loss is communicated to the host-os, but ath10k does not +trigger a low-ack events by itself. + +The mac80211 framework for loss detection however detects this +circumstance well in case of ath10k. So use it regardless of ath10k's +own loss detection mechanism. + +Signed-off-by: David Bauer <mail@david-bauer.net> +--- + net/mac80211/status.c | 6 ------ + 1 file changed, 6 deletions(-) + +--- a/net/mac80211/status.c ++++ b/net/mac80211/status.c +@@ -794,12 +794,6 @@ static void ieee80211_lost_packet(struct + unsigned long pkt_time = STA_LOST_PKT_TIME; + unsigned int pkt_thr = STA_LOST_PKT_THRESHOLD; + +- /* If driver relies on its own algorithm for station kickout, skip +- * mac80211 packet loss mechanism. +- */ +- if (ieee80211_hw_check(&sta->local->hw, REPORTS_LOW_ACK)) +- return; +- + /* This packet was aggregated but doesn't carry status info */ + if ((info->flags & IEEE80211_TX_CTL_AMPDU) && + !(info->flags & IEEE80211_TX_STAT_AMPDU))
ath10k does not report excessive loss in case of broken block-ack sessions. The loss is communicated to the host-os, but ath10k does not trigger a low-ack events by itself. The mac80211 framework for loss detection however detects this circumstance well in case of ath10k. So use it regardless of ath10k's own loss detection mechanism. Signed-off-by: David Bauer <mail@david-bauer.net> --- ...1-always-use-mac80211-loss-detection.patch | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 package/kernel/mac80211/patches/subsys/340-mac80211-always-use-mac80211-loss-detection.patch