[PATCH 1/2] mac80211: minstrels: spare numerous useless calls to get_random_bytes

[PATCH 1/2] mac80211: minstrels: spare numerous useless calls to get_random_bytes

[Karl Beldan]

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

These are called to init the sample tables.
This occurs once in minstrel_ht upon its registration (since it uses one
single common sample table for all STAs), and once per sta addition in
minstrel.

Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
---
 net/mac80211/rc80211_minstrel.c    | 3 +--
 net/mac80211/rc80211_minstrel_ht.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/rc80211_minstrel.c b/net/mac80211/rc80211_minstrel.c
index 7fa1b36..a40e4e3 100644
--- a/net/mac80211/rc80211_minstrel.c
+++ b/net/mac80211/rc80211_minstrel.c
@@ -422,10 +422,9 @@ init_sample_table(struct minstrel_sta_info *mi)
 	memset(mi->sample_table, 0xff, SAMPLE_COLUMNS * mi->n_rates);
 
 	for (col = 0; col < SAMPLE_COLUMNS; col++) {
+		get_random_bytes(rnd, sizeof(rnd));
 		for (i = 0; i < mi->n_rates; i++) {
-			get_random_bytes(rnd, sizeof(rnd));
 			new_idx = (i + rnd[i & 7]) % mi->n_rates;
-
 			while (SAMPLE_TBL(mi, new_idx, col) != 0xff)
 				new_idx = (new_idx + 1) % mi->n_rates;
 
diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
index 5d60779..aeec401 100644
--- a/net/mac80211/rc80211_minstrel_ht.c
+++ b/net/mac80211/rc80211_minstrel_ht.c
@@ -1052,10 +1052,9 @@ init_sample_table(void)
 
 	memset(sample_table, 0xff, sizeof(sample_table));
 	for (col = 0; col < SAMPLE_COLUMNS; col++) {
+		get_random_bytes(rnd, sizeof(rnd));
 		for (i = 0; i < MCS_GROUP_RATES; i++) {
-			get_random_bytes(rnd, sizeof(rnd));
 			new_idx = (i + rnd[i]) % MCS_GROUP_RATES;
-
 			while (sample_table[col][new_idx] != 0xff)
 				new_idx = (new_idx + 1) % MCS_GROUP_RATES;
 
-- 
1.8.2

[PATCH 2/2] mac80211: minstrel_ht: do not sample unsupported rates

[Karl Beldan]

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

ATM minstrel_ht does not check whether the sampling rate is supported.
Unsupported rates attempts can trigger when there are holes between
supported MCSes belonging to the same group (e.g many devices are
capable of MCS32 without being capable of MCS33->MCS39).
This change replaces an unsupported sample index with the fls of the
bitfield of supported indexes.
This is not a problem in minstrel which fills a per STA sample table
with only supported rates (though only at init).

Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
---
 net/mac80211/rc80211_minstrel_ht.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
index aeec401..1b835ad 100644
--- a/net/mac80211/rc80211_minstrel_ht.c
+++ b/net/mac80211/rc80211_minstrel_ht.c
@@ -703,6 +703,8 @@ minstrel_get_sample_rate(struct minstrel_priv *mp, struct minstrel_ht_sta *mi)
 
 	mg = &mi->groups[mi->sample_group];
 	sample_idx = sample_table[mg->column][mg->index];
+	if (!(mg->supported & BIT(sample_idx)))
+		sample_idx = fls(sample_idx) - 1;
 	mr = &mg->rates[sample_idx];
 	sample_group = mi->sample_group;
 	sample_idx += sample_group * MCS_GROUP_RATES;
-- 
1.8.2

Re: [PATCH 1/2] mac80211: minstrels: spare numerous useless calls to get_random_bytes

[Felix Fietkau]

On 2013-11-08 17:34, Karl Beldan wrote:
> From: Karl Beldan <karl.beldan@rivierawaves.com>
> 
> These are called to init the sample tables.
> This occurs once in minstrel_ht upon its registration (since it uses one
> single common sample table for all STAs), and once per sta addition in
> minstrel.
> 
> Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
Acked-by: Felix Fietkau <nbd@openwrt.org>

Re: [PATCH 2/2] mac80211: minstrel_ht: do not sample unsupported rates

[Felix Fietkau]

On 2013-11-08 17:34, Karl Beldan wrote:
> From: Karl Beldan <karl.beldan@rivierawaves.com>
> 
> ATM minstrel_ht does not check whether the sampling rate is supported.
> Unsupported rates attempts can trigger when there are holes between
> supported MCSes belonging to the same group (e.g many devices are
> capable of MCS32 without being capable of MCS33->MCS39).
> This change replaces an unsupported sample index with the fls of the
> bitfield of supported indexes.
> This is not a problem in minstrel which fills a per STA sample table
> with only supported rates (though only at init).
> 
> Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
> ---
>  net/mac80211/rc80211_minstrel_ht.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
> index aeec401..1b835ad 100644
> --- a/net/mac80211/rc80211_minstrel_ht.c
> +++ b/net/mac80211/rc80211_minstrel_ht.c
> @@ -703,6 +703,8 @@ minstrel_get_sample_rate(struct minstrel_priv *mp, struct minstrel_ht_sta *mi)
>  
>  	mg = &mi->groups[mi->sample_group];
>  	sample_idx = sample_table[mg->column][mg->index];
> +	if (!(mg->supported & BIT(sample_idx)))
> +		sample_idx = fls(sample_idx) - 1;
You probably meant fls(mg->supported) - 1 here, however I think a better
approach would be to just skip the sample attempt. If there are fewer
rates in a group, we can run fewer sample attempts on it.

- Felix

Re: [PATCH 2/2] mac80211: minstrel_ht: do not sample unsupported rates

[Karl Beldan]

On Fri, Nov 08, 2013 at 05:43:35PM +0100, Felix Fietkau wrote:
> On 2013-11-08 17:34, Karl Beldan wrote:
> > From: Karl Beldan <karl.beldan@rivierawaves.com>
> > 
> > ATM minstrel_ht does not check whether the sampling rate is supported.
> > Unsupported rates attempts can trigger when there are holes between
> > supported MCSes belonging to the same group (e.g many devices are
> > capable of MCS32 without being capable of MCS33->MCS39).
> > This change replaces an unsupported sample index with the fls of the
> > bitfield of supported indexes.
> > This is not a problem in minstrel which fills a per STA sample table
> > with only supported rates (though only at init).
> > 
> > Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
> > ---
> >  net/mac80211/rc80211_minstrel_ht.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
> > index aeec401..1b835ad 100644
> > --- a/net/mac80211/rc80211_minstrel_ht.c
> > +++ b/net/mac80211/rc80211_minstrel_ht.c
> > @@ -703,6 +703,8 @@ minstrel_get_sample_rate(struct minstrel_priv *mp, struct minstrel_ht_sta *mi)
> >  
> >  	mg = &mi->groups[mi->sample_group];
> >  	sample_idx = sample_table[mg->column][mg->index];
> > +	if (!(mg->supported & BIT(sample_idx)))
> > +		sample_idx = fls(sample_idx) - 1;
> You probably meant fls(mg->supported) - 1 here, however I think a better
Arf, yes.

> approach would be to just skip the sample attempt. If there are fewer
> rates in a group, we can run fewer sample attempts on it.
> 
Agreed :)

I was skimming through minstrel before leaving for the week-end because
I had planned to put in shape a hack I did a while ago for vht in
minstrel monday is a holiday here and I have absolutely nothing planned.
 
Karl

[PATCH v2] mac80211: minstrel_ht: do not sample unsupported rates

[Karl Beldan]

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

ATM minstrel_ht does not check whether a sampling rate is supported.
Unsupported rates attempts can trigger when there are holes in bitfields
of supported MCSes belonging to the same group (e.g many devices are
MCS32 capable without MCS33->39 capable, also we systematically have a
hole for CCK rates).
I originally replaced an unsupported sample index with the fls of the
bitfield of supported indexes of the sta current sample group, instead,
this change simply drops the sample attempt, as suggested by Felix.

This is not a problem in minstrel which fills a per STA sample table
with only supported rates (though only at init).

Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
---
 net/mac80211/rc80211_minstrel_ht.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
index aeec401..1076bca 100644
--- a/net/mac80211/rc80211_minstrel_ht.c
+++ b/net/mac80211/rc80211_minstrel_ht.c
@@ -701,12 +701,16 @@ minstrel_get_sample_rate(struct minstrel_priv *mp, struct minstrel_ht_sta *mi)
 	if (!mi->sample_tries)
 		return -1;
 
-	mg = &mi->groups[mi->sample_group];
+	sample_group = mi->sample_group;
+	mg = &mi->groups[sample_group];
 	sample_idx = sample_table[mg->column][mg->index];
+	minstrel_next_sample_idx(mi);
+
+	if (!(mg->supported & BIT(sample_idx)))
+		return -1;
+
 	mr = &mg->rates[sample_idx];
-	sample_group = mi->sample_group;
 	sample_idx += sample_group * MCS_GROUP_RATES;
-	minstrel_next_sample_idx(mi);
 
 	/*
 	 * Sampling might add some overhead (RTS, no aggregation)
-- 
1.8.2

Re: [PATCH 1/2] mac80211: minstrels: spare numerous useless calls to get_random_bytes

[Johannes Berg]

On Fri, 2013-11-08 at 17:34 +0100, Karl Beldan wrote:
> From: Karl Beldan <karl.beldan@rivierawaves.com>
> 
> These are called to init the sample tables.
> This occurs once in minstrel_ht upon its registration (since it uses one
> single common sample table for all STAs), and once per sta addition in
> minstrel.

This commit log is suboptimal, it doesn't even seem to describe a change
but rather the behaviour after (or before?) the change?

johannes

Re: [PATCH v2] mac80211: minstrel_ht: do not sample unsupported rates

[Johannes Berg]

On Mon, 2013-11-11 at 13:10 +0100, Karl Beldan wrote:
> From: Karl Beldan <karl.beldan@rivierawaves.com>
> 
> ATM minstrel_ht does not check whether a sampling rate is supported.
> Unsupported rates attempts can trigger when there are holes in bitfields
> of supported MCSes belonging to the same group (e.g many devices are
> MCS32 capable without MCS33->39 capable, also we systematically have a
> hole for CCK rates).
> I originally replaced an unsupported sample index with the fls of the
> bitfield of supported indexes of the sta current sample group, instead,
> this change simply drops the sample attempt, as suggested by Felix.

That paragraph doesn't really belong here - you should describe what
this change is doing (now). There may be some value in describing how
you arrived at the solution, but this particular description seems
unnecessary?

johannes

Re: [PATCH 1/2] mac80211: minstrels: spare numerous useless calls to get_random_bytes

[Karl Beldan]

On Mon, Nov 11, 2013 at 04:45:45PM +0100, Johannes Berg wrote:
> On Fri, 2013-11-08 at 17:34 +0100, Karl Beldan wrote:
> > From: Karl Beldan <karl.beldan@rivierawaves.com>
> > 
> > These are called to init the sample tables.
> > This occurs once in minstrel_ht upon its registration (since it uses one
> > single common sample table for all STAs), and once per sta addition in
> > minstrel.
> 
> This commit log is suboptimal, it doesn't even seem to describe a change
> but rather the behaviour after (or before?) the change?
> 
Other than replacing:
"These are called to init the sample tables.\nThis occurs once in"
with:
"These are called to init the sample tables, which occurs once in",
plus this ridiculously small diff in a non convoluted localized code
path .. 

FYI, this lowers the prng bytes demands from about 10*12*8bytes to
10*8bytes per non-ht sta addition and at minstrel_ht init. This is to
relate to the email posted just after I posted this change:
http://marc.info/?l=linux-wireless&m=138393239432434&w=2
 
Karl

Re: [PATCH v2] mac80211: minstrel_ht: do not sample unsupported rates

[Karl Beldan]

On Mon, Nov 11, 2013 at 04:46:36PM +0100, Johannes Berg wrote:
> On Mon, 2013-11-11 at 13:10 +0100, Karl Beldan wrote:
> > From: Karl Beldan <karl.beldan@rivierawaves.com>
> > 
> > ATM minstrel_ht does not check whether a sampling rate is supported.
> > Unsupported rates attempts can trigger when there are holes in bitfields
> > of supported MCSes belonging to the same group (e.g many devices are
> > MCS32 capable without MCS33->39 capable, also we systematically have a
> > hole for CCK rates).
> > I originally replaced an unsupported sample index with the fls of the
> > bitfield of supported indexes of the sta current sample group, instead,
> > this change simply drops the sample attempt, as suggested by Felix.
> 
> That paragraph doesn't really belong here - you should describe what
> this change is doing (now). There may be some value in describing how
> you arrived at the solution, but this particular description seems
> unnecessary?
> 
Feel free to get rid of the superfluous comments.
 
Karl