On Tue, Aug 9, 2011 at 3:20 PM, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
> 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?
I'll make sure that the _ibss and _non_ibss code paths will have
similar freeing (the freeinng in both cases will be handled outside
the function like it is in the _non_ibss now).
The reason I have the might_sleep/locking outside the _non_ibss
function is because the additions in the next patch. The whole point
of this refactoring from my point of view was the option to have a
similar insert function that won't have the locking inside of it.
>
> 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
