Re: [PATCH v4] mac80211: Optimize sta lookup for many VIFs

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2013-04-03 at 06:18 -0700, Ben Greear wrote:
> On 04/03/2013 05:58 AM, Johannes Berg wrote:
> > On Fri, 2013-03-29 at 09:00 -0700, greearb@candelatech.com wrote:
> >
> >> --- a/net/mac80211/cfg.c
> >> +++ b/net/mac80211/cfg.c
> >> @@ -1287,6 +1287,7 @@ static int ieee80211_change_station(struct wiphy *wiphy,
> >>   	if (params->vlan && params->vlan != sta->sdata->dev) {
> >>   		bool prev_4addr = false;
> >>   		bool new_4addr = false;
> >> +		struct sta_info *some_sta;
> >>
> >>   		vlansdata = IEEE80211_DEV_TO_SUB_IF(params->vlan);
> >>
> >> @@ -1312,7 +1313,12 @@ static int ieee80211_change_station(struct wiphy *wiphy,
> >>   			prev_4addr = true;
> >>   		}
> >>
> >> +		some_sta = rcu_dereference_protected(sta->sdata->some_sta,
> >> +					lockdep_is_held(&local->sta_mtx));
> >> +		if (some_sta == sta)
> >> +			rcu_assign_pointer(sta->sdata->some_sta, NULL);
> >>   		sta->sdata = vlansdata;
> >> +		rcu_assign_pointer(sta->sdata->some_sta, sta);
> >>
> >>   		if (sta->sta_state == IEEE80211_STA_AUTHORIZED &&
> >>   		    prev_4addr != new_4addr) {
> >
> > I think you can drop all this code since you don't allow this for
> > non-station interfaces, and stations here can only be reassigned between
> > AP/AP_VLAN, no?
> 
> Well, it seems more fragile to skip this code.  If someone ever changes
> how reassignment can happen it may be a while before anyone notices
> the bug.  This code is not in any sort of hot path, so there isn't much
> overhead...

True.

> But, if you still prefer this code removed then I'll make the change.

No, mostly just wondering.

> >> @@ -278,6 +297,8 @@ static void sta_info_hash_add(struct ieee80211_local *local,
> >>   	lockdep_assert_held(&local->sta_mtx);
> >>   	sta->hnext = local->sta_hash[STA_HASH(sta->sta.addr)];
> >>   	rcu_assign_pointer(local->sta_hash[STA_HASH(sta->sta.addr)], sta);
> >> +
> >> +	rcu_assign_pointer(sta->sdata->some_sta, sta);
> >
> > Maybe also assign it only for station to start with?
> 
> That sounds fine, though I doubt it matters any performance wise.

It won't, it's really not used for lookup anyway.

I'll just apply it as is.

johannes