| Message ID | 20180111070932.9929-6-pkshih@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On 01/11/2018 01:09 AM, pkshih@realtek.com wrote: > From: Ping-Ke Shih <pkshih@realtek.com> > > To enable the mac80211 fast-tx feature, the hw/driver needs to support > dynamic power saving and fragmentation. Since our driver does not > need to fragment packet into smaller pieces, we just hook an empty > callback of set_frag_threshold to avoid fragmentation in mac80211. > > After this, the mac80211 will not fragment the packets and can transmit > them faster by cache the header information. > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > --- > drivers/net/wireless/realtek/rtlwifi/base.c | 2 ++ > drivers/net/wireless/realtek/rtlwifi/core.c | 6 ++++++ > 2 files changed, 8 insertions(+) Acked-by: Larry Finger <Larry.Finger@lwfinger.net> > > diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c > index 89ec318598ea..e902cd7adbfe 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/base.c > +++ b/drivers/net/wireless/realtek/rtlwifi/base.c > @@ -395,6 +395,8 @@ static void _rtl_init_mac80211(struct ieee80211_hw *hw) > ieee80211_hw_set(hw, CONNECTION_MONITOR); > ieee80211_hw_set(hw, MFP_CAPABLE); > ieee80211_hw_set(hw, REPORTS_TX_ACK_STATUS); > + ieee80211_hw_set(hw, SUPPORTS_TX_FRAG); > + ieee80211_hw_set(hw, SUPPORT_FAST_XMIT); > > /* swlps or hwlps has been set in diff chip in init_sw_vars */ > if (rtlpriv->psc.swctrl_lps) { > diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c > index 6c698123ac07..d454c38fc9cd 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/core.c > +++ b/drivers/net/wireless/realtek/rtlwifi/core.c > @@ -1001,6 +1001,11 @@ static void rtl_op_sta_statistics(struct ieee80211_hw *hw, > sinfo->filled = 0; > } > > +static int rtl_op_set_frag_threshold(struct ieee80211_hw *hw, u32 value) > +{ > + return -EOPNOTSUPP; > +} > + > /* > *for mac80211 VO = 0, VI = 1, BE = 2, BK = 3 > *for rtl819x BE = 0, BK = 1, VI = 2, VO = 3 > @@ -1906,6 +1911,7 @@ const struct ieee80211_ops rtl_ops = { > .configure_filter = rtl_op_configure_filter, > .set_key = rtl_op_set_key, > .sta_statistics = rtl_op_sta_statistics, > + .set_frag_threshold = rtl_op_set_frag_threshold, > .conf_tx = rtl_op_conf_tx, > .bss_info_changed = rtl_op_bss_info_changed, > .get_tsf = rtl_op_get_tsf, >
<pkshih@realtek.com> writes: > From: Ping-Ke Shih <pkshih@realtek.com> > > To enable the mac80211 fast-tx feature, the hw/driver needs to support > dynamic power saving and fragmentation. Since our driver does not > need to fragment packet into smaller pieces, we just hook an empty > callback of set_frag_threshold to avoid fragmentation in mac80211. > > After this, the mac80211 will not fragment the packets and can transmit > them faster by cache the header information. > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> [...] > --- a/drivers/net/wireless/realtek/rtlwifi/core.c > +++ b/drivers/net/wireless/realtek/rtlwifi/core.c > @@ -1001,6 +1001,11 @@ static void rtl_op_sta_statistics(struct ieee80211_hw *hw, > sinfo->filled = 0; > } > > +static int rtl_op_set_frag_threshold(struct ieee80211_hw *hw, u32 value) > +{ > + return -EOPNOTSUPP; > +} > + > /* > *for mac80211 VO = 0, VI = 1, BE = 2, BK = 3 > *for rtl819x BE = 0, BK = 1, VI = 2, VO = 3 > @@ -1906,6 +1911,7 @@ const struct ieee80211_ops rtl_ops = { > .configure_filter = rtl_op_configure_filter, > .set_key = rtl_op_set_key, > .sta_statistics = rtl_op_sta_statistics, > + .set_frag_threshold = rtl_op_set_frag_threshold, This also looks fishy, I guess there's a good reason why mac80211 requires set_frag_threshold()? To me this is just a workaround for a mac80211 test.
On 1/16/2018 4:45 PM, Kalle Valo wrote: > <pkshih@realtek.com> writes: > >> From: Ping-Ke Shih <pkshih@realtek.com> >> >> To enable the mac80211 fast-tx feature, the hw/driver needs to support >> dynamic power saving and fragmentation. Since our driver does not >> need to fragment packet into smaller pieces, we just hook an empty >> callback of set_frag_threshold to avoid fragmentation in mac80211. >> >> After this, the mac80211 will not fragment the packets and can transmit >> them faster by cache the header information. >> >> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> >> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > > [...] > >> --- a/drivers/net/wireless/realtek/rtlwifi/core.c >> +++ b/drivers/net/wireless/realtek/rtlwifi/core.c >> @@ -1001,6 +1001,11 @@ static void rtl_op_sta_statistics(struct ieee80211_hw *hw, >> sinfo->filled = 0; >> } >> >> +static int rtl_op_set_frag_threshold(struct ieee80211_hw *hw, u32 value) >> +{ >> + return -EOPNOTSUPP; >> +} >> + >> /* >> *for mac80211 VO = 0, VI = 1, BE = 2, BK = 3 >> *for rtl819x BE = 0, BK = 1, VI = 2, VO = 3 >> @@ -1906,6 +1911,7 @@ const struct ieee80211_ops rtl_ops = { >> .configure_filter = rtl_op_configure_filter, >> .set_key = rtl_op_set_key, >> .sta_statistics = rtl_op_sta_statistics, >> + .set_frag_threshold = rtl_op_set_frag_threshold, > > This also looks fishy, I guess there's a good reason why mac80211 > requires set_frag_threshold()? To me this is just a workaround for a > mac80211 test. When I saw this flying by I had the same feeling. This is clearly not how it was intended although you could interpret the comments of the .set_frag_threshold() callback and IEEE80211_HW_SUPPORTS_TX_FRAG. However, the fragmentation threshold is a user-configurable stack parameter as per the standard. This patch effectively kill that option for the user although there may be RF conditions in which fragmentation can help. Having the user configure a fragmentation threshold of 2346 also disables fragmentation and allows mac80211 to use cached fastpath. In short this should be NACK'ed. Regards, Arend
On Tue, 2018-01-16 at 21:12 +0100, Arend van Spriel wrote: > > When I saw this flying by I had the same feeling. This is clearly not > how it was intended although you could interpret the comments of the > .set_frag_threshold() callback and IEEE80211_HW_SUPPORTS_TX_FRAG. > However, the fragmentation threshold is a user-configurable stack > parameter as per the standard. This patch effectively kill that option > for the user although there may be RF conditions in which fragmentation > can help. Having the user configure a fragmentation threshold of 2346 > also disables fragmentation and allows mac80211 to use cached fastpath. > /* fast-xmit doesn't handle fragmentation at all */ if (local->hw.wiphy->frag_threshold != (u32)-1 && !ieee80211_hw_check(&local->hw, SUPPORTS_TX_FRAG)) goto out; so internally at least it has to be -1, does 2346 really do that? remember, but it might well :-) johannes
On 1/16/2018 11:24 PM, Johannes Berg wrote: > On Tue, 2018-01-16 at 21:12 +0100, Arend van Spriel wrote: >> >> When I saw this flying by I had the same feeling. This is clearly not >> how it was intended although you could interpret the comments of the >> .set_frag_threshold() callback and IEEE80211_HW_SUPPORTS_TX_FRAG. >> However, the fragmentation threshold is a user-configurable stack >> parameter as per the standard. This patch effectively kill that option >> for the user although there may be RF conditions in which fragmentation >> can help. Having the user configure a fragmentation threshold of 2346 >> also disables fragmentation and allows mac80211 to use cached fastpath. >> > > /* fast-xmit doesn't handle fragmentation at all */ > if (local->hw.wiphy->frag_threshold != (u32)-1 && > !ieee80211_hw_check(&local->hw, SUPPORTS_TX_FRAG)) > goto out; > > so internally at least it has to be -1, does 2346 really do that? > remember, but it might well :-) I am getting old. In my recollection the fragmentation threshold would be set to maximum MPDU size, which I believed to be 2346. Not sure if that still is true today. To be sure taken a look in the 2016 spec: """ dot11FragmentationThreshold OBJECT-TYPE SYNTAX Unsigned32 (256..65535) UNITS "octets" MAX-ACCESS read-write STATUS current DESCRIPTION "This is a control variable. It is written by an external management entity. Changes take effect as soon as practical in the implementation. This attribute specifies the maximum size of an individually addressed MPDU beyond which the corresponding MSDU or MMPDU is fragmented, except when an MSDU is transmitted under an HT-immediate or HT-delayed block ack agreement, or when an MSDU is carried in an A-MSDU, or when an MSDU or MMPDU is carried in an A-MPDU that does not contain a VHT single MPDU. Fields added to the MPDU by security encapsulation are not counted against the limit specified by this attribute. An MSDU or MMPDU might be fragmented even if it is smaller." DEFVAL { 65535 } ::= { dot11OperationEntry 5 } """ Anyway, the fact remains that we should leave it up to the user to control the value. Regards, Arend
On Wed, 2018-01-17 at 09:37 +0100, Arend van Spriel wrote: > > I am getting old. In my recollection the fragmentation threshold would > be set to maximum MPDU size, which I believed to be 2346. Not sure if > that still is true today. To be sure taken a look in the 2016 spec: > > """ > dot11FragmentationThreshold OBJECT-TYPE > SYNTAX Unsigned32 (256..65535) I think the larger sizes are for 60 GHz. > Anyway, the fact remains that we should leave it up to the user to > control the value. Indeed. johannes
On Tue, 2018-01-16 at 23:24 +0100, Johannes Berg wrote: > On Tue, 2018-01-16 at 21:12 +0100, Arend van Spriel wrote: > > > > > > When I saw this flying by I had the same feeling. This is clearly > > not > > how it was intended although you could interpret the comments of > > the > > .set_frag_threshold() callback and IEEE80211_HW_SUPPORTS_TX_FRAG. > > However, the fragmentation threshold is a user-configurable stack > > parameter as per the standard. This patch effectively kill that > > option > > for the user although there may be RF conditions in which > > fragmentation > > can help. Having the user configure a fragmentation threshold of > > 2346 > > also disables fragmentation and allows mac80211 to use cached > > fastpath. > > > /* fast-xmit doesn't handle fragmentation at all */ > if (local->hw.wiphy->frag_threshold != (u32)-1 && > !ieee80211_hw_check(&local->hw, SUPPORTS_TX_FRAG)) > goto out; > Thank you for this point. This commit has not to set SUPPORTS_TX_FRAG, but only set SUPPORT_FAST_XMIT. I'll test and prepare for next submission. PK
On Thu, 2018-01-18 at 09:05 +0000, Pkshih wrote: > > > /* fast-xmit doesn't handle fragmentation at all */ > > if (local->hw.wiphy->frag_threshold != (u32)-1 && > > !ieee80211_hw_check(&local->hw, SUPPORTS_TX_FRAG)) > > goto out; > > > > Thank you for this point. This commit has not to set SUPPORTS_TX_FRAG, > but only set SUPPORT_FAST_XMIT. I'll test and prepare for next > submission. But the point is that you *shouldn't* set SUPPORTS_TX_FRAG, and then fast-xmit will only get used for non-fragmented setups, which realistically is all anyone cares about anyway. johannes
On Thu, 2018-01-18 at 11:09 +0100, Johannes Berg wrote: > On Thu, 2018-01-18 at 09:05 +0000, Pkshih wrote: > > > > > > > > > > /* fast-xmit doesn't handle fragmentation at all */ > > > if (local->hw.wiphy->frag_threshold != (u32)-1 && > > > !ieee80211_hw_check(&local->hw, SUPPORTS_TX_FRAG)) > > > goto out; > > > > > Thank you for this point. This commit has not to set SUPPORTS_TX_FRAG, > > but only set SUPPORT_FAST_XMIT. I'll test and prepare for next > > submission. > But the point is that you *shouldn't* set SUPPORTS_TX_FRAG, and then > fast-xmit will only get used for non-fragmented setups, which > realistically is all anyone cares about anyway. > My fault. Since the goal of this commit is to enable fast-tx, I shouldn't set SUPPORTS_TX_FRAG and the op set_frag_threshold. PK
diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c index 89ec318598ea..e902cd7adbfe 100644 --- a/drivers/net/wireless/realtek/rtlwifi/base.c +++ b/drivers/net/wireless/realtek/rtlwifi/base.c @@ -395,6 +395,8 @@ static void _rtl_init_mac80211(struct ieee80211_hw *hw) ieee80211_hw_set(hw, CONNECTION_MONITOR); ieee80211_hw_set(hw, MFP_CAPABLE); ieee80211_hw_set(hw, REPORTS_TX_ACK_STATUS); + ieee80211_hw_set(hw, SUPPORTS_TX_FRAG); + ieee80211_hw_set(hw, SUPPORT_FAST_XMIT); /* swlps or hwlps has been set in diff chip in init_sw_vars */ if (rtlpriv->psc.swctrl_lps) { diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c index 6c698123ac07..d454c38fc9cd 100644 --- a/drivers/net/wireless/realtek/rtlwifi/core.c +++ b/drivers/net/wireless/realtek/rtlwifi/core.c @@ -1001,6 +1001,11 @@ static void rtl_op_sta_statistics(struct ieee80211_hw *hw, sinfo->filled = 0; } +static int rtl_op_set_frag_threshold(struct ieee80211_hw *hw, u32 value) +{ + return -EOPNOTSUPP; +} + /* *for mac80211 VO = 0, VI = 1, BE = 2, BK = 3 *for rtl819x BE = 0, BK = 1, VI = 2, VO = 3 @@ -1906,6 +1911,7 @@ const struct ieee80211_ops rtl_ops = { .configure_filter = rtl_op_configure_filter, .set_key = rtl_op_set_key, .sta_statistics = rtl_op_sta_statistics, + .set_frag_threshold = rtl_op_set_frag_threshold, .conf_tx = rtl_op_conf_tx, .bss_info_changed = rtl_op_bss_info_changed, .get_tsf = rtl_op_get_tsf,