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:
signature.asc
Description: Digital signature
