On Wed, 2011-07-27 at 00:29 +0300, Guy Eilam wrote:
> -int sta_info_insert_rcu(struct sta_info *sta) __acquires(RCU)
> +static int sta_info_insert_check(struct sta_info *sta)
> {
> - struct ieee80211_local *local = sta->local;
> struct ieee80211_sub_if_data *sdata = sta->sdata;
> - unsigned long flags;
> - int err = 0;
>
> /*
> * Can't be a WARN_ON because it can be triggered through a race:
> * something inserts a STA (on one CPU) without holding the RTNL
> * and another CPU turns off the net device.
> */
> - if (unlikely(!ieee80211_sdata_running(sdata))) {
> - err = -ENETDOWN;
> - rcu_read_lock();
> - goto out_free;
> - }
> + if (unlikely(!ieee80211_sdata_running(sdata)))
> + return -ENETDOWN;
>
> if (WARN_ON(compare_ether_addr(sta->sta.addr, sdata->vif.addr) == 0 ||
> - is_multicast_ether_addr(sta->sta.addr))) {
> - err = -EINVAL;
> + is_multicast_ether_addr(sta->sta.addr)))
> + return -EINVAL;
> +
> + return 0;
> +}
That seems nice, even simplifying the code :)
> +static int sta_info_insert_adhoc(struct sta_info *sta) __acquires(RCU)
> +{
> + struct ieee80211_local *local = sta->local;
> + struct ieee80211_sub_if_data *sdata = sta->sdata;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&local->sta_lock, flags);
> + /* check if STA exists already */
> + if (sta_info_get_bss(sdata, sta->sta.addr)) {
> + spin_unlock_irqrestore(&local->sta_lock, flags);
> rcu_read_lock();
> - goto out_free;
> + __sta_info_free(local, sta);
> + return -EEXIST;
> }
>
> - /*
> - * In ad-hoc mode, we sometimes need to insert stations
> - * from tasklet context from the RX path. To avoid races,
> - * always do so in that case -- see the comment below.
> - */
> - if (sdata->vif.type == NL80211_IFTYPE_ADHOC) {
> - spin_lock_irqsave(&local->sta_lock, flags);
> - /* check if STA exists already */
> - if (sta_info_get_bss(sdata, sta->sta.addr)) {
> - spin_unlock_irqrestore(&local->sta_lock, flags);
> - rcu_read_lock();
> - err = -EEXIST;
> - goto out_free;
> - }
> -
> - local->num_sta++;
> - local->sta_generation++;
> - smp_mb();
> - sta_info_hash_add(local, sta);
> + local->num_sta++;
> + local->sta_generation++;
> + smp_mb();
> + sta_info_hash_add(local, sta);
>
> - list_add_tail(&sta->list, &local->sta_pending_list);
> + list_add_tail(&sta->list, &local->sta_pending_list);
>
> - rcu_read_lock();
> - spin_unlock_irqrestore(&local->sta_lock, flags);
> + rcu_read_lock();
> + spin_unlock_irqrestore(&local->sta_lock, flags);
>
> #ifdef CONFIG_MAC80211_VERBOSE_DEBUG
> - wiphy_debug(local->hw.wiphy, "Added IBSS STA %pM\n",
> - sta->sta.addr);
> + wiphy_debug(local->hw.wiphy, "Added IBSS STA %pM\n",
> + sta->sta.addr);
> #endif /* CONFIG_MAC80211_VERBOSE_DEBUG */
>
> - ieee80211_queue_work(&local->hw, &local->sta_finish_work);
> + ieee80211_queue_work(&local->hw, &local->sta_finish_work);
>
> - return 0;
> - }
> + return 0;
> +}
Yup, that looks good too, just the patch is a bit odd due to the code
indentation change.
> +/*
> + * should be called with sta_mtx locked
> + * this function replaces the mutex lock
> + * with a RCU lock
> + */
> +static int sta_info_insert_mgd(struct sta_info *sta) __acquires(RCU)
> +{
> + struct ieee80211_local *local = sta->local;
> + struct ieee80211_sub_if_data *sdata = sta->sdata;
> + unsigned long flags;
> + int err = 0;
> +
> + lockdep_assert_held(&local->sta_mtx);
>
> /*
> * On first glance, this will look racy, because the code
> - * below this point, which inserts a station with sleeping,
> + * in this function, which inserts a station with sleeping,
> * unlocks the sta_lock between checking existence in the
> * hash table and inserting into it.
> *
> * However, it is not racy against itself because it keeps
> - * the mutex locked. It still seems to race against the
> - * above code that atomically inserts the station... That,
> - * however, is not true because the above code can only
> - * be invoked for IBSS interfaces, and the below code will
> - * not be -- and the two do not race against each other as
> - * the hash table also keys off the interface.
> + * the mutex locked.
> */
>
> - might_sleep();
> -
> - mutex_lock(&local->sta_mtx);
> -
> spin_lock_irqsave(&local->sta_lock, flags);
> /* check if STA exists already */
> if (sta_info_get_bss(sdata, sta->sta.addr)) {
> spin_unlock_irqrestore(&local->sta_lock, flags);
> mutex_unlock(&local->sta_mtx);
> rcu_read_lock();
> - err = -EEXIST;
> - goto out_free;
> + return -EEXIST;
Isn't that missing the free now?
> }
>
> spin_unlock_irqrestore(&local->sta_lock, flags);
> @@ -466,7 +464,7 @@ int sta_info_insert_rcu(struct sta_info *sta) __acquires(RCU)
> if (err) {
> mutex_unlock(&local->sta_mtx);
> rcu_read_lock();
> - goto out_free;
> + return err;
and here?
> +int sta_info_insert_rcu(struct sta_info *sta) __acquires(RCU)
> +{
> + struct ieee80211_local *local = sta->local;
> + struct ieee80211_sub_if_data *sdata = sta->sdata;
> + int err = 0;
> +
> + err = sta_info_insert_check(sta);
> + if (err) {
> + rcu_read_lock();
> + goto out_free;
> + }
> +
> + /*
> + * In ad-hoc mode, we sometimes need to insert stations
> + * from tasklet context from the RX path. To avoid races,
> + * always do so in that case -- see the comment below.
> + */
> + if (sdata->vif.type == NL80211_IFTYPE_ADHOC)
> + return sta_info_insert_adhoc(sta);
> +
> + /*
> + * It might seem that the function called below is in race against
> + * the function call above that atomically inserts the station... That,
> + * however, is not true because the above code can only
> + * be invoked for IBSS interfaces, and the below code will
> + * not be -- and the two do not race against each other as
> + * the hash table also keys off the interface.
> + */
> +
> + might_sleep();
> +
> + mutex_lock(&local->sta_mtx);
> +
> + err = sta_info_insert_mgd(sta);
> + if (err)
> + goto out_free;
> +
> + return 0;
> out_free:
> BUG_ON(!err);
> __sta_info_free(local, sta);
Ah, ok, I guess not.
Hmm. Can we make the ibss vs. non-IBSS code paths more similar wrt.
freeing? The _ibss one will free it, but the _mgd one won't, that seems
a bit strange to me.
Also, I'd prefer to move the might_sleep/locking into _mgd() if
possible?
johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
