[PATCH 1/2] mac80211: do not use low data rates for data frames with no ack flag

[PATCH 1/2] mac80211: do not use low data rates for data frames with no ack flag

[Philipp Borgers]

Data Frames with no ack flag set should be handle by the rate controler.
Make sure we reach the rate controler by returning early from
rate_control_send_low if the frame is a data frame with no ack flag.

Signed-off-by: Philipp Borgers <borgers@mi.fu-berlin.de>
---
 net/mac80211/rate.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 63652c39c8e0..4f5b35a0ea28 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -391,11 +391,16 @@ static bool rate_control_send_low(struct ieee80211_sta *pubsta,
 				  struct ieee80211_tx_rate_control *txrc)
 {
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(txrc->skb);
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) txrc->skb->data;
 	struct ieee80211_supported_band *sband = txrc->sband;
 	struct sta_info *sta;
 	int mcast_rate;
 	bool use_basicrate = false;
 
+	if (ieee80211_is_data(hdr->frame_control) &&
+			(info->flags & IEEE80211_TX_CTL_NO_ACK))
+		return false;
+
 	if (!pubsta || rc_no_data_or_no_ack_use_min(txrc)) {
 		__rate_control_send_low(txrc->hw, sband, pubsta, info,
 					txrc->rate_idx_mask);
-- 
2.31.1

[PATCH 2/2] mac80211: refactor rc_no_data_or_no_ack_use_min function

[Philipp Borgers]

Signed-off-by: Philipp Borgers <borgers@mi.fu-berlin.de>
---
 net/mac80211/rate.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 4f5b35a0ea28..ae8d595aa2c2 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -294,18 +294,11 @@ void ieee80211_check_rate_mask(struct ieee80211_sub_if_data *sdata)
 	sdata->rc_rateidx_mask[band] = (1 << sband->n_bitrates) - 1;
 }
 
-static bool rc_no_data_or_no_ack_use_min(struct ieee80211_tx_rate_control *txrc)
+static bool rc_no_data_or_no_ack_use_min(u32 flags, __le16 frame_control)
 {
-	struct sk_buff *skb = txrc->skb;
-	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
-	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
-	__le16 fc;
-
-	fc = hdr->frame_control;
-
-	return (info->flags & (IEEE80211_TX_CTL_NO_ACK |
+	return (flags & (IEEE80211_TX_CTL_NO_ACK |
 			       IEEE80211_TX_CTL_USE_MINRATE)) ||
-		!ieee80211_is_data(fc);
+		!ieee80211_is_data(frame_control);
 }
 
 static void rc_send_low_basicrate(struct ieee80211_tx_rate *rate,
@@ -396,12 +389,13 @@ static bool rate_control_send_low(struct ieee80211_sta *pubsta,
 	struct sta_info *sta;
 	int mcast_rate;
 	bool use_basicrate = false;
+	__le16 frame_control = hdr->frame_control;
 
-	if (ieee80211_is_data(hdr->frame_control) &&
+	if (ieee80211_is_data(frame_control) &&
 			(info->flags & IEEE80211_TX_CTL_NO_ACK))
 		return false;
 
-	if (!pubsta || rc_no_data_or_no_ack_use_min(txrc)) {
+	if (!pubsta || rc_no_data_or_no_ack_use_min(info->flags, frame_control)) {
 		__rate_control_send_low(txrc->hw, sband, pubsta, info,
 					txrc->rate_idx_mask);
 
-- 
2.31.1

Re: [PATCH 1/2] mac80211: do not use low data rates for data frames with no ack flag

[Felix Fietkau]

On 2021-05-18 13:07, Philipp Borgers wrote:
> Data Frames with no ack flag set should be handle by the rate controler.
> Make sure we reach the rate controler by returning early from
> rate_control_send_low if the frame is a data frame with no ack flag.
> 
> Signed-off-by: Philipp Borgers <borgers@mi.fu-berlin.de>
> ---
>  net/mac80211/rate.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
> index 63652c39c8e0..4f5b35a0ea28 100644
> --- a/net/mac80211/rate.c
> +++ b/net/mac80211/rate.c
> @@ -391,11 +391,16 @@ static bool rate_control_send_low(struct ieee80211_sta *pubsta,
>  				  struct ieee80211_tx_rate_control *txrc)
>  {
>  	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(txrc->skb);
> +	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) txrc->skb->data;
>  	struct ieee80211_supported_band *sband = txrc->sband;
>  	struct sta_info *sta;
>  	int mcast_rate;
>  	bool use_basicrate = false;
>  
> +	if (ieee80211_is_data(hdr->frame_control) &&
> +			(info->flags & IEEE80211_TX_CTL_NO_ACK))
> +		return false;
Frames with IEEE80211_TX_CTL_HW_80211_ENCAP set have no 802.11 header,
so please change the check something like this:

	if ((info->flags & IEEE80211_TX_CTL_NO_ACK) &&
	    ((info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP) ||
	     ieee80211_is_data(hdr->frame_control)))

Same applies to the other patch.
There are other places in the code where this kind of change needs to be
done, but it would be good if at least newly added code handles it properly.

- Felix

Re: [PATCH 1/2] mac80211: do not use low data rates for data frames with no ack flag

[Johannes Berg]

On Tue, 2021-05-18 at 13:17 +0200, Felix Fietkau wrote:
> 
> Frames with IEEE80211_TX_CTL_HW_80211_ENCAP set have no 802.11 header,
> so please change the check something like this:
> 
> 	if ((info->flags & IEEE80211_TX_CTL_NO_ACK) &&
> 	    ((info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP) ||
> 	     ieee80211_is_data(hdr->frame_control)))

Maybe we should consider some kind of inline helper?

static inline bool ieee80211_is_tx_data(struct sk_buff *skb)
{
	... *info = ...
	... *hdr = (void *)skb->data;

	return (info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP) ||
               ieee80211_is_data(hdr->frame_control);
}


or so?

johannes

Re: [PATCH 1/2] mac80211: do not use low data rates for data frames with no ack flag

[Felix Fietkau]

On 2021-05-18 13:19, Johannes Berg wrote:
> On Tue, 2021-05-18 at 13:17 +0200, Felix Fietkau wrote:
>> 
>> Frames with IEEE80211_TX_CTL_HW_80211_ENCAP set have no 802.11 header,
>> so please change the check something like this:
>> 
>> 	if ((info->flags & IEEE80211_TX_CTL_NO_ACK) &&
>> 	    ((info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP) ||
>> 	     ieee80211_is_data(hdr->frame_control)))
> 
> Maybe we should consider some kind of inline helper?
> 
> static inline bool ieee80211_is_tx_data(struct sk_buff *skb)
> {
> 	... *info = ...
> 	... *hdr = (void *)skb->data;
> 
> 	return (info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP) ||
>                ieee80211_is_data(hdr->frame_control);
> }
> 
> 
> or so?
Yes, I think that's a good idea. There will definitely be more places
that need this.

- Felix

Re: [PATCH 2/2] mac80211: refactor rc_no_data_or_no_ack_use_min function

[Kalle Valo]

Philipp Borgers <borgers@mi.fu-berlin.de> writes:

> Signed-off-by: Philipp Borgers <borgers@mi.fu-berlin.de>

Why? Empty commit logs is a bad idea, even if the reason is trivial to
you it might not be for others.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH 1/2] mac80211: do not use low data rates for data frames with no ack flag

[Philipp Borgers]

On Tue, May 18, 2021 at 01:19:46PM +0200, Johannes Berg wrote:
> On Tue, 2021-05-18 at 13:17 +0200, Felix Fietkau wrote:
> > 
> > Frames with IEEE80211_TX_CTL_HW_80211_ENCAP set have no 802.11 header,
> > so please change the check something like this:
> > 
> > 	if ((info->flags & IEEE80211_TX_CTL_NO_ACK) &&
> > 	    ((info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP) ||
> > 	     ieee80211_is_data(hdr->frame_control)))
> 
> Maybe we should consider some kind of inline helper?
> 
> static inline bool ieee80211_is_tx_data(struct sk_buff *skb)
> {
> 	... *info = ...
> 	... *hdr = (void *)skb->data;
> 
> 	return (info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP) ||
>                ieee80211_is_data(hdr->frame_control);
> }

A frame with IEEE80211_TX_CTL_HW_80211_ENCAP set is always a data frame?

Should I put the definition of the function into include/net/mac80211.h?

Re: [PATCH 1/2] mac80211: do not use low data rates for data frames with no ack flag

[Johannes Berg]

On Wed, 2021-05-19 at 12:47 +0200, Philipp Borgers wrote:
> On Tue, May 18, 2021 at 01:19:46PM +0200, Johannes Berg wrote:
> > On Tue, 2021-05-18 at 13:17 +0200, Felix Fietkau wrote:
> > > 
> > > Frames with IEEE80211_TX_CTL_HW_80211_ENCAP set have no 802.11 header,
> > > so please change the check something like this:
> > > 
> > > 	if ((info->flags & IEEE80211_TX_CTL_NO_ACK) &&
> > > 	    ((info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP) ||
> > > 	     ieee80211_is_data(hdr->frame_control)))
> > 
> > Maybe we should consider some kind of inline helper?
> > 
> > static inline bool ieee80211_is_tx_data(struct sk_buff *skb)
> > {
> > 	... *info = ...
> > 	... *hdr = (void *)skb->data;
> > 
> > 	return (info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP) ||
> >                ieee80211_is_data(hdr->frame_control);
> > }
> 
> A frame with IEEE80211_TX_CTL_HW_80211_ENCAP set is always a data frame?

Yes, other frames can't be HW-encap'ed, nor would it make sense to
offload that.

> Should I put the definition of the function into include/net/mac80211.h?
> 
Seems reasonable, yeah.

johannes