Re: [PATCH 1/2] mac80211: refactor sta_info_insert_rcu to 3 main stages

On Tue, Aug 9, 2011 at 3:20 PM, Johannes Berg [off-list ref] wrote:

On Wed, 2011-07-27 at 00:29 +0300, Guy Eilam wrote:

quoted

-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 :)

quoted

+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.

quoted

+/*
+ * 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?

quoted

      }

      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?

quoted

+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