From: Karl Beldan <[email protected]>
Currently it gets it from the sdata. This uses and updates the ad-hoc
masks of the ieee80211_tx_rate_control instead of copying them.
Signed-off-by: Karl Beldan <[email protected]>
---
net/mac80211/rate.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index dd88381..c1e5f25 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -435,8 +435,8 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
struct ieee80211_sta *ista = NULL;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(txrc->skb);
int i;
- u32 mask;
- u8 mcs_mask[IEEE80211_HT_MCS_MASK_LEN];
+ u32 *mask;
+ u8 *mcs_mask;
if (sta && test_sta_flag(sta, WLAN_STA_RATE_CONTROL)) {
ista = &sta->sta;
@@ -459,14 +459,13 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
* default mask (allow all rates) is used to save some processing for
* the common case.
*/
- mask = sdata->rc_rateidx_mask[info->band];
- memcpy(mcs_mask, sdata->rc_rateidx_mcs_mask[info->band],
- sizeof(mcs_mask));
- if (mask != (1 << txrc->sband->n_bitrates) - 1) {
+ mask = &txrc->rate_idx_mask;
+ mcs_mask = txrc->rate_idx_mcs_mask;
+ if (*mask != (1 << txrc->sband->n_bitrates) - 1) {
if (sta) {
/* Filter out rates that the STA does not support */
- mask &= sta->sta.supp_rates[info->band];
- for (i = 0; i < sizeof(mcs_mask); i++)
+ *mask &= sta->sta.supp_rates[info->band];
+ for (i = 0; i < sizeof(txrc->rate_idx_mcs_mask); i++)
mcs_mask[i] &= sta->sta.ht_cap.mcs.rx_mask[i];
}
/*
@@ -479,7 +478,7 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
if (info->control.rates[i].idx < 0)
break;
rate_idx_match_mask(&info->control.rates[i], txrc,
- mask, mcs_mask);
+ *mask, mcs_mask);
}
}
--
1.7.10.GIT
On 2013-03-04 9:45 PM, john wrote:
> On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote:
>> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote:
>> > From: Karl Beldan <[email protected]>
>> >
>> > Currently it gets it from the sdata. This uses and updates the ad-hoc
>> > masks of the ieee80211_tx_rate_control instead of copying them.
>>
>> Is there any need to update them?
>>
>> The change for "mask" seems to make it less efficient since it could
>> otherwise be put into a register.
>>
> Totally, this commit spares the 10bytes copy of mcs_mask but adds a less
> efficient indirection to mask.
> I thought of it but kept the symmetry with mcs_mask.
> Apparently you wouldn't mind the dissymmetry so I will re-send using mask
> by value, plus I wrote "updates .." where it is more like "lets the
> ad-hoc masks get overwritten".
It seems to me that all of this could be made more efficient by default
if a mcs mask pointer is only passed to rate control if the user
actually configured a MCS mask. Also, filtering out rates from the mask
that the sta does not support seems a bit unnecessary, since the rate
control usually looks at the HT capabilities and the sta's mcs rx mask
anyway.
- Felix
On Mon, Mar 04, 2013 at 09:13:00PM +0100, Johannes Berg wrote:
> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote:
> > From: Karl Beldan <[email protected]>
> >
> > Currently the mcs bitrates mask rate_idx_mcs_mask is only applied when
> > the pre-ht bitrates mask rate_idx_mask of the same band differs from the
> > default mask.
> > Fix it by comparing the rate_idx_mcs_mask with the driver ht caps.
> >
> > Signed-off-by: Karl Beldan <[email protected]>
> > ---
> > net/mac80211/rate.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
> > index c1e5f25..28f1936 100644
> > --- a/net/mac80211/rate.c
> > +++ b/net/mac80211/rate.c
> > @@ -455,13 +455,15 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
> > ref->ops->get_rate(ref->priv, ista, priv_sta, txrc);
> >
> > /*
> > - * Try to enforce the rateidx mask the user wanted. skip this if the
> > - * default mask (allow all rates) is used to save some processing for
> > - * the common case.
> > + * Try to enforce the rateidx masks the user wanted. Skip this if mask
> > + * is the default mask (allow all rates) and mcs_mask does not differ
> > + * from the driver ht caps to save some processing for the common case.
> > */
> > mask = &txrc->rate_idx_mask;
> > mcs_mask = txrc->rate_idx_mcs_mask;
> > - if (*mask != (1 << txrc->sband->n_bitrates) - 1) {
> > + if (*mask != (1 << txrc->sband->n_bitrates) - 1 ||
> > + memcmp(txrc->sband->ht_cap.mcs.rx_mask, txrc->rate_idx_mcs_mask,
> > + sizeof(txrc->rate_idx_mcs_mask))) {
>
> Please fix the indentation, the memcmp should be indented to just after
> the opening parenthesis of "if ("
>
Yes, sorry.
Karl
On Sun, Mar 10, 2013 at 11:27:01PM +0100, Felix Fietkau wrote:
> On 2013-03-10 11:16 PM, Karl Beldan wrote:
> > On Tue, Mar 05, 2013 at 02:29:11PM +0100, Felix Fietkau wrote:
> >> On 2013-03-04 9:45 PM, john wrote:
> >> > On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote:
> >> >> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote:
> >> >> > From: Karl Beldan <[email protected]>
> >> >> >
> >> >> > Currently it gets it from the sdata. This uses and updates the ad-hoc
> >> >> > masks of the ieee80211_tx_rate_control instead of copying them.
> >> >>
> >> >> Is there any need to update them?
> >> >>
> >> >> The change for "mask" seems to make it less efficient since it could
> >> >> otherwise be put into a register.
> >> >>
> >> > Totally, this commit spares the 10bytes copy of mcs_mask but adds a less
> >> > efficient indirection to mask.
> >> > I thought of it but kept the symmetry with mcs_mask.
> >> > Apparently you wouldn't mind the dissymmetry so I will re-send using mask
> >> > by value, plus I wrote "updates .." where it is more like "lets the
> >> > ad-hoc masks get overwritten".
> >> It seems to me that all of this could be made more efficient by default
> >> if a mcs mask pointer is only passed to rate control if the user
> >> actually configured a MCS mask. Also, filtering out rates from the mask
> >> that the sta does not support seems a bit unnecessary, since the rate
> >> control usually looks at the HT capabilities and the sta's mcs rx mask
> >> anyway.
> >>
> > Filtering is necessary to lookup alternative downgrade/upgrade rates.
> Right, but the code could be changed to only do the filtering if
> mac80211 needs to look up an alternative downgrade/upgrade rate.
>
With this I agree.
Do you have strong opinions wrt basic rates ? The current code might tx
mc/bc with non-basic rates.
Karl
On Tue, Mar 05, 2013 at 02:29:11PM +0100, Felix Fietkau wrote:
> On 2013-03-04 9:45 PM, john wrote:
> > On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote:
> >> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote:
> >> > From: Karl Beldan <[email protected]>
> >> >
> >> > Currently it gets it from the sdata. This uses and updates the ad-hoc
> >> > masks of the ieee80211_tx_rate_control instead of copying them.
> >>
> >> Is there any need to update them?
> >>
> >> The change for "mask" seems to make it less efficient since it could
> >> otherwise be put into a register.
> >>
> > Totally, this commit spares the 10bytes copy of mcs_mask but adds a less
> > efficient indirection to mask.
> > I thought of it but kept the symmetry with mcs_mask.
> > Apparently you wouldn't mind the dissymmetry so I will re-send using mask
> > by value, plus I wrote "updates .." where it is more like "lets the
> > ad-hoc masks get overwritten".
> It seems to me that all of this could be made more efficient by default
> if a mcs mask pointer is only passed to rate control if the user
> actually configured a MCS mask. Also, filtering out rates from the mask
> that the sta does not support seems a bit unnecessary, since the rate
> control usually looks at the HT capabilities and the sta's mcs rx mask
> anyway.
>
Filtering is necessary to lookup alternative downgrade/upgrade rates.
Karl
On Mon, 2013-03-11 at 00:06 +0100, Karl Beldan wrote:
> > > Filtering is necessary to lookup alternative downgrade/upgrade rates.
> > Right, but the code could be changed to only do the filtering if
> > mac80211 needs to look up an alternative downgrade/upgrade rate.
> >
> With this I agree.
> Do you have strong opinions wrt basic rates ? The current code might tx
> mc/bc with non-basic rates.
If the users want to shoot themselves in the foot I'd rather give them
enough rope to hang themselves with ;-)
johannes
On Tue, Mar 05, 2013 at 11:27:48PM +0100, john wrote:
> On Tue, Mar 05, 2013 at 07:53:39PM +0100, Johannes Berg wrote:
> > On Tue, 2013-03-05 at 17:10 +0100, Karl Beldan wrote:
> >
> > > > It seems to me that all of this could be made more efficient by default
> > > > if a mcs mask pointer is only passed to rate control if the user
> > > > actually configured a MCS mask. Also, filtering out rates from the mask
> > > > that the sta does not support seems a bit unnecessary, since the rate
> > > > control usually looks at the HT capabilities and the sta's mcs rx mask
> > > > anyway.
> > > >
> > > Yes, some things look a bit overkill in the masks logic.
> >
> > Are you planning to send new patches to improve this?
> >
> I'll see what I can come up with.
>
Now, FWIW, I was looking at how the masks are applied - the code tries
to be thorough wrt the various RC flags - 2 things at least are missing:
handle basic rates with multicast, and protection when downgrading to
pre-ht rates.
Karl
On 2013-03-11 12:06 AM, Karl Beldan wrote:
> On Sun, Mar 10, 2013 at 11:27:01PM +0100, Felix Fietkau wrote:
>> On 2013-03-10 11:16 PM, Karl Beldan wrote:
>> > On Tue, Mar 05, 2013 at 02:29:11PM +0100, Felix Fietkau wrote:
>> >> On 2013-03-04 9:45 PM, john wrote:
>> >> > On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote:
>> >> >> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote:
>> >> >> > From: Karl Beldan <[email protected]>
>> >> >> >
>> >> >> > Currently it gets it from the sdata. This uses and updates the ad-hoc
>> >> >> > masks of the ieee80211_tx_rate_control instead of copying them.
>> >> >>
>> >> >> Is there any need to update them?
>> >> >>
>> >> >> The change for "mask" seems to make it less efficient since it could
>> >> >> otherwise be put into a register.
>> >> >>
>> >> > Totally, this commit spares the 10bytes copy of mcs_mask but adds a less
>> >> > efficient indirection to mask.
>> >> > I thought of it but kept the symmetry with mcs_mask.
>> >> > Apparently you wouldn't mind the dissymmetry so I will re-send using mask
>> >> > by value, plus I wrote "updates .." where it is more like "lets the
>> >> > ad-hoc masks get overwritten".
>> >> It seems to me that all of this could be made more efficient by default
>> >> if a mcs mask pointer is only passed to rate control if the user
>> >> actually configured a MCS mask. Also, filtering out rates from the mask
>> >> that the sta does not support seems a bit unnecessary, since the rate
>> >> control usually looks at the HT capabilities and the sta's mcs rx mask
>> >> anyway.
>> >>
>> > Filtering is necessary to lookup alternative downgrade/upgrade rates.
>> Right, but the code could be changed to only do the filtering if
>> mac80211 needs to look up an alternative downgrade/upgrade rate.
>>
> With this I agree.
> Do you have strong opinions wrt basic rates ? The current code might tx
> mc/bc with non-basic rates.
The only strong opinion I have about the rate masking code is that it
shouldn't waste precious CPU cycles in a hot path ;)
- Felix
On 2013-03-10 11:16 PM, Karl Beldan wrote:
> On Tue, Mar 05, 2013 at 02:29:11PM +0100, Felix Fietkau wrote:
>> On 2013-03-04 9:45 PM, john wrote:
>> > On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote:
>> >> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote:
>> >> > From: Karl Beldan <[email protected]>
>> >> >
>> >> > Currently it gets it from the sdata. This uses and updates the ad-hoc
>> >> > masks of the ieee80211_tx_rate_control instead of copying them.
>> >>
>> >> Is there any need to update them?
>> >>
>> >> The change for "mask" seems to make it less efficient since it could
>> >> otherwise be put into a register.
>> >>
>> > Totally, this commit spares the 10bytes copy of mcs_mask but adds a less
>> > efficient indirection to mask.
>> > I thought of it but kept the symmetry with mcs_mask.
>> > Apparently you wouldn't mind the dissymmetry so I will re-send using mask
>> > by value, plus I wrote "updates .." where it is more like "lets the
>> > ad-hoc masks get overwritten".
>> It seems to me that all of this could be made more efficient by default
>> if a mcs mask pointer is only passed to rate control if the user
>> actually configured a MCS mask. Also, filtering out rates from the mask
>> that the sta does not support seems a bit unnecessary, since the rate
>> control usually looks at the HT capabilities and the sta's mcs rx mask
>> anyway.
>>
> Filtering is necessary to lookup alternative downgrade/upgrade rates.
Right, but the code could be changed to only do the filtering if
mac80211 needs to look up an alternative downgrade/upgrade rate.
- Felix
On Tue, Mar 05, 2013 at 02:29:11PM +0100, Felix Fietkau wrote:
> On 2013-03-04 9:45 PM, john wrote:
> > On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote:
> >> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote:
> >> > From: Karl Beldan <[email protected]>
> >> >
> >> > Currently it gets it from the sdata. This uses and updates the ad-hoc
> >> > masks of the ieee80211_tx_rate_control instead of copying them.
> >>
> >> Is there any need to update them?
> >>
> >> The change for "mask" seems to make it less efficient since it could
> >> otherwise be put into a register.
> >>
> > Totally, this commit spares the 10bytes copy of mcs_mask but adds a less
> > efficient indirection to mask.
> > I thought of it but kept the symmetry with mcs_mask.
> > Apparently you wouldn't mind the dissymmetry so I will re-send using mask
> > by value, plus I wrote "updates .." where it is more like "lets the
> > ad-hoc masks get overwritten".
> It seems to me that all of this could be made more efficient by default
> if a mcs mask pointer is only passed to rate control if the user
> actually configured a MCS mask. Also, filtering out rates from the mask
> that the sta does not support seems a bit unnecessary, since the rate
> control usually looks at the HT capabilities and the sta's mcs rx mask
> anyway.
>
Yes, some things look a bit overkill in the masks logic.
Karl
On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote:
> From: Karl Beldan <[email protected]>
>
> Currently it gets it from the sdata. This uses and updates the ad-hoc
> masks of the ieee80211_tx_rate_control instead of copying them.
Is there any need to update them?
The change for "mask" seems to make it less efficient since it could
otherwise be put into a register.
johannes
On Mon, Mar 04, 2013 at 09:12:04PM +0100, Johannes Berg wrote:
> On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote:
> > From: Karl Beldan <[email protected]>
> >
> > Currently it gets it from the sdata. This uses and updates the ad-hoc
> > masks of the ieee80211_tx_rate_control instead of copying them.
>
> Is there any need to update them?
>
> The change for "mask" seems to make it less efficient since it could
> otherwise be put into a register.
>
Totally, this commit spares the 10bytes copy of mcs_mask but adds a less
efficient indirection to mask.
I thought of it but kept the symmetry with mcs_mask.
Apparently you wouldn't mind the dissymmetry so I will re-send using mask
by value, plus I wrote "updates .." where it is more like "lets the
ad-hoc masks get overwritten".
Karl
On Tue, 2013-03-05 at 17:10 +0100, Karl Beldan wrote:
> > It seems to me that all of this could be made more efficient by default
> > if a mcs mask pointer is only passed to rate control if the user
> > actually configured a MCS mask. Also, filtering out rates from the mask
> > that the sta does not support seems a bit unnecessary, since the rate
> > control usually looks at the HT capabilities and the sta's mcs rx mask
> > anyway.
> >
> Yes, some things look a bit overkill in the masks logic.
Are you planning to send new patches to improve this?
johannes
On Tue, Mar 05, 2013 at 07:53:39PM +0100, Johannes Berg wrote:
> On Tue, 2013-03-05 at 17:10 +0100, Karl Beldan wrote:
>
> > > It seems to me that all of this could be made more efficient by default
> > > if a mcs mask pointer is only passed to rate control if the user
> > > actually configured a MCS mask. Also, filtering out rates from the mask
> > > that the sta does not support seems a bit unnecessary, since the rate
> > > control usually looks at the HT capabilities and the sta's mcs rx mask
> > > anyway.
> > >
> > Yes, some things look a bit overkill in the masks logic.
>
> Are you planning to send new patches to improve this?
>
I'll see what I can come up with.
Karl
From: Karl Beldan <[email protected]>
Currently the mcs bitrates mask rate_idx_mcs_mask is only applied when
the pre-ht bitrates mask rate_idx_mask of the same band differs from the
default mask.
Fix it by comparing the rate_idx_mcs_mask with the driver ht caps.
Signed-off-by: Karl Beldan <[email protected]>
---
net/mac80211/rate.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index c1e5f25..28f1936 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -455,13 +455,15 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
ref->ops->get_rate(ref->priv, ista, priv_sta, txrc);
/*
- * Try to enforce the rateidx mask the user wanted. skip this if the
- * default mask (allow all rates) is used to save some processing for
- * the common case.
+ * Try to enforce the rateidx masks the user wanted. Skip this if mask
+ * is the default mask (allow all rates) and mcs_mask does not differ
+ * from the driver ht caps to save some processing for the common case.
*/
mask = &txrc->rate_idx_mask;
mcs_mask = txrc->rate_idx_mcs_mask;
- if (*mask != (1 << txrc->sband->n_bitrates) - 1) {
+ if (*mask != (1 << txrc->sband->n_bitrates) - 1 ||
+ memcmp(txrc->sband->ht_cap.mcs.rx_mask, txrc->rate_idx_mcs_mask,
+ sizeof(txrc->rate_idx_mcs_mask))) {
if (sta) {
/* Filter out rates that the STA does not support */
*mask &= sta->sta.supp_rates[info->band];
--
1.7.10.GIT
On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote:
> From: Karl Beldan <[email protected]>
>
> Currently the mcs bitrates mask rate_idx_mcs_mask is only applied when
> the pre-ht bitrates mask rate_idx_mask of the same band differs from the
> default mask.
> Fix it by comparing the rate_idx_mcs_mask with the driver ht caps.
>
> Signed-off-by: Karl Beldan <[email protected]>
> ---
> net/mac80211/rate.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
> index c1e5f25..28f1936 100644
> --- a/net/mac80211/rate.c
> +++ b/net/mac80211/rate.c
> @@ -455,13 +455,15 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
> ref->ops->get_rate(ref->priv, ista, priv_sta, txrc);
>
> /*
> - * Try to enforce the rateidx mask the user wanted. skip this if the
> - * default mask (allow all rates) is used to save some processing for
> - * the common case.
> + * Try to enforce the rateidx masks the user wanted. Skip this if mask
> + * is the default mask (allow all rates) and mcs_mask does not differ
> + * from the driver ht caps to save some processing for the common case.
> */
> mask = &txrc->rate_idx_mask;
> mcs_mask = txrc->rate_idx_mcs_mask;
> - if (*mask != (1 << txrc->sband->n_bitrates) - 1) {
> + if (*mask != (1 << txrc->sband->n_bitrates) - 1 ||
> + memcmp(txrc->sband->ht_cap.mcs.rx_mask, txrc->rate_idx_mcs_mask,
> + sizeof(txrc->rate_idx_mcs_mask))) {
Please fix the indentation, the memcmp should be indented to just after
the opening parenthesis of "if ("
johannes