All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate
@ 2013-03-04 18:27 Karl Beldan
  2013-03-04 18:27 ` [PATCH v2 2/2] mac80211: fix the check for mcs rates masking Karl Beldan
  2013-03-04 20:12 ` [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate Johannes Berg
  0 siblings, 2 replies; 16+ messages in thread
From: Karl Beldan @ 2013-03-04 18:27 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless, Karl Beldan, Karl Beldan

From: Karl Beldan <karl.beldan@rivierawaves.com>

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 <karl.beldan@rivierawaves.com>
---
 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 2/2] mac80211: fix the check for mcs rates masking
  2013-03-04 18:27 [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate Karl Beldan
@ 2013-03-04 18:27 ` Karl Beldan
  2013-03-04 20:13   ` Johannes Berg
  2013-03-04 20:12 ` [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate Johannes Berg
  1 sibling, 1 reply; 16+ messages in thread
From: Karl Beldan @ 2013-03-04 18:27 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless, Karl Beldan, Karl Beldan

From: Karl Beldan <karl.beldan@rivierawaves.com>

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 <karl.beldan@rivierawaves.com>
---
 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate
  2013-03-04 18:27 [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate Karl Beldan
  2013-03-04 18:27 ` [PATCH v2 2/2] mac80211: fix the check for mcs rates masking Karl Beldan
@ 2013-03-04 20:12 ` Johannes Berg
  2013-03-04 20:45   ` john
  1 sibling, 1 reply; 16+ messages in thread
From: Johannes Berg @ 2013-03-04 20:12 UTC (permalink / raw)
  To: Karl Beldan; +Cc: linux-wireless, Karl Beldan

On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote:
> From: Karl Beldan <karl.beldan@rivierawaves.com>
> 
> 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/2] mac80211: fix the check for mcs rates masking
  2013-03-04 18:27 ` [PATCH v2 2/2] mac80211: fix the check for mcs rates masking Karl Beldan
@ 2013-03-04 20:13   ` Johannes Berg
  2013-03-04 20:51     ` john
  0 siblings, 1 reply; 16+ messages in thread
From: Johannes Berg @ 2013-03-04 20:13 UTC (permalink / raw)
  To: Karl Beldan; +Cc: linux-wireless, Karl Beldan

On Mon, 2013-03-04 at 19:27 +0100, Karl Beldan wrote:
> From: Karl Beldan <karl.beldan@rivierawaves.com>
> 
> 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 <karl.beldan@rivierawaves.com>
> ---
>  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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate
  2013-03-04 20:12 ` [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate Johannes Berg
@ 2013-03-04 20:45   ` john
  2013-03-05 13:29     ` Felix Fietkau
  0 siblings, 1 reply; 16+ messages in thread
From: john @ 2013-03-04 20:45 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Karl Beldan, linux-wireless, Karl Beldan

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 <karl.beldan@rivierawaves.com>
> > 
> > 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/2] mac80211: fix the check for mcs rates masking
  2013-03-04 20:13   ` Johannes Berg
@ 2013-03-04 20:51     ` john
  0 siblings, 0 replies; 16+ messages in thread
From: john @ 2013-03-04 20:51 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Karl Beldan, linux-wireless, Karl Beldan

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 <karl.beldan@rivierawaves.com>
> > 
> > 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 <karl.beldan@rivierawaves.com>
> > ---
> >  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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate
  2013-03-04 20:45   ` john
@ 2013-03-05 13:29     ` Felix Fietkau
  2013-03-05 16:10       ` Karl Beldan
  2013-03-10 22:16       ` Karl Beldan
  0 siblings, 2 replies; 16+ messages in thread
From: Felix Fietkau @ 2013-03-05 13:29 UTC (permalink / raw)
  To: john; +Cc: Johannes Berg, linux-wireless, Karl Beldan

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 <karl.beldan@rivierawaves.com>
>> > 
>> > 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate
  2013-03-05 13:29     ` Felix Fietkau
@ 2013-03-05 16:10       ` Karl Beldan
  2013-03-05 18:53         ` Johannes Berg
  2013-03-10 22:16       ` Karl Beldan
  1 sibling, 1 reply; 16+ messages in thread
From: Karl Beldan @ 2013-03-05 16:10 UTC (permalink / raw)
  To: Felix Fietkau; +Cc: Johannes Berg, linux-wireless, Karl Beldan

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 <karl.beldan@rivierawaves.com>
> >> > 
> >> > 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate
  2013-03-05 16:10       ` Karl Beldan
@ 2013-03-05 18:53         ` Johannes Berg
  2013-03-05 22:27           ` john
  0 siblings, 1 reply; 16+ messages in thread
From: Johannes Berg @ 2013-03-05 18:53 UTC (permalink / raw)
  To: Karl Beldan; +Cc: Felix Fietkau, linux-wireless, Karl Beldan

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate
  2013-03-05 18:53         ` Johannes Berg
@ 2013-03-05 22:27           ` john
  2013-03-10 22:35             ` Karl Beldan
  0 siblings, 1 reply; 16+ messages in thread
From: john @ 2013-03-05 22:27 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Karl Beldan, Felix Fietkau, linux-wireless, Karl Beldan

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate
  2013-03-05 13:29     ` Felix Fietkau
  2013-03-05 16:10       ` Karl Beldan
@ 2013-03-10 22:16       ` Karl Beldan
  2013-03-10 22:27         ` Felix Fietkau
  1 sibling, 1 reply; 16+ messages in thread
From: Karl Beldan @ 2013-03-10 22:16 UTC (permalink / raw)
  To: Felix Fietkau; +Cc: Johannes Berg, linux-wireless, Karl Beldan

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 <karl.beldan@rivierawaves.com>
> >> > 
> >> > 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate
  2013-03-10 22:16       ` Karl Beldan
@ 2013-03-10 22:27         ` Felix Fietkau
  2013-03-10 23:06           ` Karl Beldan
  0 siblings, 1 reply; 16+ messages in thread
From: Felix Fietkau @ 2013-03-10 22:27 UTC (permalink / raw)
  To: Karl Beldan; +Cc: Johannes Berg, linux-wireless, Karl Beldan

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 <karl.beldan@rivierawaves.com>
>> >> > 
>> >> > 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate
  2013-03-05 22:27           ` john
@ 2013-03-10 22:35             ` Karl Beldan
  0 siblings, 0 replies; 16+ messages in thread
From: Karl Beldan @ 2013-03-10 22:35 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Felix Fietkau, linux-wireless, Karl Beldan

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate
  2013-03-10 22:27         ` Felix Fietkau
@ 2013-03-10 23:06           ` Karl Beldan
  2013-03-10 23:33             ` Felix Fietkau
  2013-03-15 15:46             ` Johannes Berg
  0 siblings, 2 replies; 16+ messages in thread
From: Karl Beldan @ 2013-03-10 23:06 UTC (permalink / raw)
  To: Felix Fietkau; +Cc: Johannes Berg, linux-wireless, Karl Beldan

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 <karl.beldan@rivierawaves.com>
> >> >> > 
> >> >> > 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate
  2013-03-10 23:06           ` Karl Beldan
@ 2013-03-10 23:33             ` Felix Fietkau
  2013-03-15 15:46             ` Johannes Berg
  1 sibling, 0 replies; 16+ messages in thread
From: Felix Fietkau @ 2013-03-10 23:33 UTC (permalink / raw)
  To: Karl Beldan; +Cc: Johannes Berg, linux-wireless, Karl Beldan

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 <karl.beldan@rivierawaves.com>
>> >> >> > 
>> >> >> > 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate
  2013-03-10 23:06           ` Karl Beldan
  2013-03-10 23:33             ` Felix Fietkau
@ 2013-03-15 15:46             ` Johannes Berg
  1 sibling, 0 replies; 16+ messages in thread
From: Johannes Berg @ 2013-03-15 15:46 UTC (permalink / raw)
  To: Karl Beldan; +Cc: Felix Fietkau, linux-wireless, Karl Beldan

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2013-03-16 12:57 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-04 18:27 [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate Karl Beldan
2013-03-04 18:27 ` [PATCH v2 2/2] mac80211: fix the check for mcs rates masking Karl Beldan
2013-03-04 20:13   ` Johannes Berg
2013-03-04 20:51     ` john
2013-03-04 20:12 ` [PATCH v2 1/2] mac80211: get the rates masks from the txrc in rate_control_get_rate Johannes Berg
2013-03-04 20:45   ` john
2013-03-05 13:29     ` Felix Fietkau
2013-03-05 16:10       ` Karl Beldan
2013-03-05 18:53         ` Johannes Berg
2013-03-05 22:27           ` john
2013-03-10 22:35             ` Karl Beldan
2013-03-10 22:16       ` Karl Beldan
2013-03-10 22:27         ` Felix Fietkau
2013-03-10 23:06           ` Karl Beldan
2013-03-10 23:33             ` Felix Fietkau
2013-03-15 15:46             ` Johannes Berg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.