Re: [PATCH 2/2] mac80211: add support for mcs masks

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

[-- Attachment #1: Type: text/plain, Size: 2279 bytes --]

Hello Johannes,

thanks for all the comments, I can agree to all points you made in this mail
and for the other one and will provide a PATCHv2 which should fix this.

One issue is left which I'd like to discuss, please see below.

On Tue, Jan 03, 2012 at 03:24:50PM +0100, Johannes Berg wrote:
> > @@ -358,10 +462,14 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
> >  	 * the common case.
> >  	 */
> >  	mask = sdata->rc_rateidx_mask[info->band];
> > +	memcpy(mcs_mask, sdata->rc_rateidx_mcs_mask[info->band],
> > +	       sizeof(mcs_mask));
> 
> Do we really have to do this? Might not a pointer be better?
> 
> >  	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++)
> > +				mcs_mask[i] &= sta->sta.ht_cap.mcs.rx_mask[i];
> 
> Oh, so it's filtered by station ... hm ok I guess unless we tie
> lifetimes together we have to do this.
> 
> Maybe we could update all stations in the slow-path (changes in the HT
> mask) and then just use the already masked version in the sta entry in
> the fastpath here?

You are right that this definitly adds some load on the fastpath (Although
I don't know how much the compiler optimizes here anyway, its only 10 bytes
which are set and masked).

Currently I see two approaches here:

1.) move the masking into the station handling code (when a new station joins).
    We would also need to iterate over the connected station when changing
    the mcs_mask while the interface is up.
    --> this will probably need to change at quite some palces and might be race-risky
2.) keep the masking in the rate_control_get_rate() function, but mask only
    once (first time) and keep the result in the station struct. Next time we
    enter it, we use the already masked result. We could then use a flag to
    remember that we already computed the mask and just use it, and another
    flag to signalize if the mcs_mask was modified externally
    --> this is not racy and rather simple, but adds some flags

Maybe you have an even better idea, please let me know what you think. :)

Cheers,
	Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]