| Message ID | 20200204120614.28861-1-yhchuang@realtek.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 74c3d72cc13401f9eb3e3c712855e9f8f2d2682b |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtw88: disable TX-AMSDU on 2.4G band | expand |
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 >
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
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
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
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.
> 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
<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
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
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 :)
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,