Re: [RFC 2/2] mac80211: Optimize sta lookup for many VIFs

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2013-03-15 at 14:13 -0700, greearb@candelatech.com wrote:
 
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -669,6 +669,12 @@ struct ieee80211_sub_if_data {
>  	/* count for keys needing tailroom space allocation */
>  	int crypto_tx_tailroom_needed_cnt;
>  
> +	/* A pointer to some station associated with this interface, or
> +	 * NULL.  This aids oportunistic lookup for sta_info objects when

typo: opportunistic. Also it should probably say "allows opportunistic
lookup" :-)

> +	/* Shortcut for finding station entries when sdata is a station */
> +	some_sta = rcu_dereference(sdata->some_sta);
> +	if (some_sta) {
> +		if (WARN_ON(some_sta->sdata != sdata))
> +			rcu_assign_pointer(sdata->some_sta, NULL);
> +		else
> +			if (ether_addr_equal(some_sta->sta.addr, addr))
> +				return some_sta;

I worry a little bit about the overhead in the "always cache miss" case.
Is this really helpful for AP interfaces? Maybe it should be limited to
managed virtual interfaces.

>  
>  	sta = rcu_dereference_check(local->sta_hash[STA_HASH(addr)],
>  				    lockdep_is_held(&local->sta_mtx));
> @@ -263,10 +273,14 @@ struct sta_info *sta_info_get_by_idx(struct ieee80211_sub_if_data *sdata,
>   */
>  void sta_info_free(struct ieee80211_local *local, struct sta_info *sta)
>  {
> +	struct sta_info* some_sta;
>  	if (sta->rate_ctrl)

missing blank line

>  		rate_control_free_sta(sta);
>  
>  	sta_dbg(sta->sdata, "Destroyed STA %pM\n", sta->sta.addr);
> +	some_sta = rcu_dereference(sta->sdata->some_sta);
> +	if (some_sta == sta)
> +		rcu_assign_pointer(sta->sdata->some_sta, NULL);

This clearing is WAY too late. You can look up the station way after you
must be allowed to, this will invariably crash eventually. It absolutely
must be when the station is unhashed, not when it's freed.

> @@ -373,6 +387,8 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>  	for (i = 0; i < NUM_RX_DATA_QUEUES; i++)
>  		sta->last_seq_ctrl[i] = cpu_to_le16(USHRT_MAX);
>  
> +	rcu_assign_pointer(sta->sdata->some_sta, sta);

This is too early, the station might not even be used, this must only be
done when the station is hashed.

johannes