[PATCH] rtw88: disable TX-AMSDU on 2.4G band

[PATCH] rtw88: disable TX-AMSDU on 2.4G band

[yhchuang]

From: Yan-Hsuan Chuang <yhchuang@realtek.com>

Some tests shows that using AMSDU to aggregate TCP ACKs to specific
APs will degrade the throughput on 2.4G band in 20MHz bandwidth
(< 10 Mbps, should be ~100 Mbps for 2x2). Also found that there's
barely no negative impact if we disable TX AMSDU on 2.4G to connect
to other APs. So it seems like we can just tell mac80211 to not to
aggregate MSDUs when transmitting on 2.4G band.

Note that we still can TX AMSDU on 5G band and benefit from it by
having 50 ~ 70 Mbps throughput improvement.

Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
---
 drivers/net/wireless/realtek/rtw88/mac80211.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c b/drivers/net/wireless/realtek/rtw88/mac80211.c
index 6fc33e11d08c..21b56db16916 100644
--- a/drivers/net/wireless/realtek/rtw88/mac80211.c
+++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
@@ -592,6 +592,20 @@ static int rtw_ops_ampdu_action(struct ieee80211_hw *hw,
 	return 0;
 }
 
+static bool rtw_ops_can_aggregate_in_amsdu(struct ieee80211_hw *hw,
+					   struct sk_buff *head,
+					   struct sk_buff *skb)
+{
+	struct rtw_dev *rtwdev = hw->priv;
+	struct rtw_hal *hal = &rtwdev->hal;
+
+	/* we don't want to enable TX AMSDU on 2.4G */
+	if (hal->current_band_type == RTW_BAND_2G)
+		return false;
+
+	return true;
+}
+
 static void rtw_ops_sw_scan_start(struct ieee80211_hw *hw,
 				  struct ieee80211_vif *vif,
 				  const u8 *mac_addr)
@@ -787,6 +801,7 @@ const struct ieee80211_ops rtw_ops = {
 	.sta_remove		= rtw_ops_sta_remove,
 	.set_key		= rtw_ops_set_key,
 	.ampdu_action		= rtw_ops_ampdu_action,
+	.can_aggregate_in_amsdu	= rtw_ops_can_aggregate_in_amsdu,
 	.sw_scan_start		= rtw_ops_sw_scan_start,
 	.sw_scan_complete	= rtw_ops_sw_scan_complete,
 	.mgd_prepare_tx		= rtw_ops_mgd_prepare_tx,
-- 
2.17.1

Re: [PATCH] rtw88: disable TX-AMSDU on 2.4G band

[Chris Chiu]

On Tue, Feb 4, 2020 at 8:06 PM <yhchuang@realtek.com> wrote:
>
> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> Some tests shows that using AMSDU to aggregate TCP ACKs to specific
> APs will degrade the throughput on 2.4G band in 20MHz bandwidth
> (< 10 Mbps, should be ~100 Mbps for 2x2). Also found that there's
> barely no negative impact if we disable TX AMSDU on 2.4G to connect
> to other APs. So it seems like we can just tell mac80211 to not to
> aggregate MSDUs when transmitting on 2.4G band.
>
> Note that we still can TX AMSDU on 5G band and benefit from it by
> having 50 ~ 70 Mbps throughput improvement.
>
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
> ---
Reviewed-by: Chris Chiu <chiu@endlessm.com>


>  drivers/net/wireless/realtek/rtw88/mac80211.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c b/drivers/net/wireless/realtek/rtw88/mac80211.c
> index 6fc33e11d08c..21b56db16916 100644
> --- a/drivers/net/wireless/realtek/rtw88/mac80211.c
> +++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
> @@ -592,6 +592,20 @@ static int rtw_ops_ampdu_action(struct ieee80211_hw *hw,
>         return 0;
>  }
>
> +static bool rtw_ops_can_aggregate_in_amsdu(struct ieee80211_hw *hw,
> +                                          struct sk_buff *head,
> +                                          struct sk_buff *skb)
> +{
> +       struct rtw_dev *rtwdev = hw->priv;
> +       struct rtw_hal *hal = &rtwdev->hal;
> +
> +       /* we don't want to enable TX AMSDU on 2.4G */
> +       if (hal->current_band_type == RTW_BAND_2G)
> +               return false;
> +
> +       return true;
> +}
> +
>  static void rtw_ops_sw_scan_start(struct ieee80211_hw *hw,
>                                   struct ieee80211_vif *vif,
>                                   const u8 *mac_addr)
> @@ -787,6 +801,7 @@ const struct ieee80211_ops rtw_ops = {
>         .sta_remove             = rtw_ops_sta_remove,
>         .set_key                = rtw_ops_set_key,
>         .ampdu_action           = rtw_ops_ampdu_action,
> +       .can_aggregate_in_amsdu = rtw_ops_can_aggregate_in_amsdu,
>         .sw_scan_start          = rtw_ops_sw_scan_start,
>         .sw_scan_complete       = rtw_ops_sw_scan_complete,
>         .mgd_prepare_tx         = rtw_ops_mgd_prepare_tx,
> --
> 2.17.1
>

Re: [PATCH] rtw88: disable TX-AMSDU on 2.4G band

[Justin Capella]

Instead of permanently disabling could a module parameter or other
configurable be used? I appreciate the performance enhancements but
don't like the idea of disabling functionality

Re: [PATCH] rtw88: disable TX-AMSDU on 2.4G band

[Brian Norris]

On Fri, Feb 7, 2020 at 11:41 AM Justin Capella <justincapella@gmail.com> wrote:
> Instead of permanently disabling could a module parameter or other
> configurable be used? I appreciate the performance enhancements but
> don't like the idea of disabling functionality

It feels like you have that backwards: Tony is claiming that
(a) TX AMSDU does not give any performance benefit on 2.4GHz
(b) TX AMSDU gives a severe performance degradation on 2.4GHz with certain APs

That sounds like a case where the feature should be disabled by
default. (A separate module parameter to re-enable it experimentally
sounds like it could be OK, although it's not likely I would use it or
recommend doing so. But that doesn't sound like what you're
suggesting.)

I say "claiming" above, but I have fielded evidence for (b) at least.
I don't know much about (a), but limited tests haven't showed any real
loss for me.

HTH,
Brian

Re: [PATCH] rtw88: disable TX-AMSDU on 2.4G band

[Brian Norris]

On Fri, Feb 7, 2020 at 12:48 PM Justin Capella <justincapella@gmail.com> wrote:
> I understand, I'm suggesting disable by default but option to re-enable

Ah, OK. Seems reasonable, I suppose, although I don't recall Kalle
having a particularly-high opinion of module parameters for tweaking
core 802.11 protocol behaviors.

Brian

Re: [PATCH] rtw88: disable TX-AMSDU on 2.4G band

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

> On Fri, Feb 7, 2020 at 12:48 PM Justin Capella <justincapella@gmail.com> wrote:
>> I understand, I'm suggesting disable by default but option to re-enable
>
> Ah, OK. Seems reasonable, I suppose, although I don't recall Kalle
> having a particularly-high opinion of module parameters for tweaking
> core 802.11 protocol behaviors.

Yeah, exactly. And the number of module parameters a driver has should
be minimised. I know out-of-tree vendor drivers have ini files with 100
different knobs, but I don't think module parameters should be
equivalent to ini files.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

RE: [PATCH] rtw88: disable TX-AMSDU on 2.4G band

[Tony Chuang]

> Would some other piece of the stack could decide to use or not use aggregation for a given band/vif/sta? Maybe just semantics but my thought was the driver returning false makes it seem incapable of it.
> I agree about not polluting the module parameters. I'll have a look at the out of tree stuff. Thoughts on debugfs knobs, not necessarily patch specific just in general? 

> On Sat, Feb 8, 2020, 2:09 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>> Brian Norris <briannorris@chromium.org> writes:
>>> On Fri, Feb 7, 2020 at 12:48 PM Justin Capella <justincapella@gmail.com> wrote:
>>>> I understand, I'm suggesting disable by default but option to re-enable
>>>
>>> Ah, OK. Seems reasonable, I suppose, although I don't recall Kalle
>>> having a particularly-high opinion of module parameters for tweaking
>>> core 802.11 protocol behaviors.

>> Yeah, exactly. And the number of module parameters a driver has should
>> be minimised. I know out-of-tree vendor drivers have ini files with 100
>> different knobs, but I don't think module parameters should be
>> equivalent to ini files.

Module parameters are really good for me, too. But we've had discussion
before with Kalle and Brian, they both were trying hard to avoid module
parameters.

So, I think Kalle and Brian don't recommend using module parameters.
And I think just disable it on 2.4G is OK, there's no need to enable it for
those supported 2x2 devices, unless we are going to support 3x3/4x4
devices. If that happens, we can add conditions for those 3x3/4x4.

Yan-Hsuan

Re: [PATCH] rtw88: disable TX-AMSDU on 2.4G band

[Kalle Valo]

<yhchuang@realtek.com> wrote:

> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> 
> Some tests shows that using AMSDU to aggregate TCP ACKs to specific
> APs will degrade the throughput on 2.4G band in 20MHz bandwidth
> (< 10 Mbps, should be ~100 Mbps for 2x2). Also found that there's
> barely no negative impact if we disable TX AMSDU on 2.4G to connect
> to other APs. So it seems like we can just tell mac80211 to not to
> aggregate MSDUs when transmitting on 2.4G band.
> 
> Note that we still can TX AMSDU on 5G band and benefit from it by
> having 50 ~ 70 Mbps throughput improvement.
> 
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
> Reviewed-by: Chris Chiu <chiu@endlessm.com>

Patch applied to wireless-drivers-next.git, thanks.

74c3d72cc134 rtw88: disable TX-AMSDU on 2.4G band

-- 
https://patchwork.kernel.org/patch/11364515/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] rtw88: disable TX-AMSDU on 2.4G band

[Brian Norris]

On Tue, Feb 11, 2020 at 6:56 PM Tony Chuang <yhchuang@realtek.com> wrote:
> Module parameters are really good for me, too. But we've had discussion
> before with Kalle and Brian, they both were trying hard to avoid module
> parameters.

My personal preference is to avoid module parameters when you can fix
the defaults, and that module parameters should never be a workaround
for fixing the default behavior.

I don't think my above preference precludes module parameters: they
can be useful as "extra debug knobs."

But Kalle had a little more nuanced opinion here -- he didn't even
want "debug knobs" for core 802.11 functionality, because (I may be
paraphrasing) one person's "debug knob" easily becomes the next
person's "required knob." Additionally, a mess of disorganized knobs
can make maintenance difficult -- one can't really expect the average
distribution to make a good selection on 100 different parameters; and
for those that do tweak the parameters, it now creates a combinatoric
mess to debug and triage user reports of "it's broken". I can respect
all of those reasons too.

Regards,
Brian

Re: [PATCH] rtw88: disable TX-AMSDU on 2.4G band

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

> On Tue, Feb 11, 2020 at 6:56 PM Tony Chuang <yhchuang@realtek.com> wrote:
>> Module parameters are really good for me, too. But we've had discussion
>> before with Kalle and Brian, they both were trying hard to avoid module
>> parameters.
>
> My personal preference is to avoid module parameters when you can fix
> the defaults, and that module parameters should never be a workaround
> for fixing the default behavior.
>
> I don't think my above preference precludes module parameters: they
> can be useful as "extra debug knobs."
>
> But Kalle had a little more nuanced opinion here -- he didn't even
> want "debug knobs" for core 802.11 functionality, because (I may be
> paraphrasing) one person's "debug knob" easily becomes the next
> person's "required knob." Additionally, a mess of disorganized knobs
> can make maintenance difficult -- one can't really expect the average
> distribution to make a good selection on 100 different parameters; and
> for those that do tweak the parameters, it now creates a combinatoric
> mess to debug and triage user reports of "it's broken". I can respect
> all of those reasons too.

Exactly my thinking as well, thanks Brian for writing these out. We
should add this description "why module parameters are bad" to the wiki
to make it more visible :)

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches