Date: Fri, 18 Apr 2008 20:07:45 GMT From: Sam Leffler <sam@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 140224 for review Message-ID: <200804182007.m3IK7jtr093266@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=140224 Change 140224 by sam@sam_ebb on 2008/04/18 20:07:02 o back out use of rwlock for node table and iterator; it was wrong for several reasons. Restore the 2 mtx's; one for the node table contents and one for the scan generation #; we must have two since we need to drop the node table lock over the callback and must guard against scangen being changed by colliding iterators. o cleanup the lock name stuff by encapsulating the char buffer that holds the constructed lock name in the lock types. o eliminate some unnecessary recursive locking in mlme ops o correct the LOR in the mlme op to deauth/diassoc station(s) by holding the node table lock only for the single-address case and using ieee80211_iterate_nodes otherwise Affected files ... .. //depot/projects/vap/sys/net80211/ieee80211_freebsd.h#25 edit .. //depot/projects/vap/sys/net80211/ieee80211_ioctl.c#55 edit .. //depot/projects/vap/sys/net80211/ieee80211_node.c#29 edit .. //depot/projects/vap/sys/net80211/ieee80211_node.h#19 edit Differences ... ==== //depot/projects/vap/sys/net80211/ieee80211_freebsd.h#25 (text+ko) ==== @@ -49,22 +49,46 @@ /* * Node locking definitions. */ -typedef struct rwlock ieee80211_node_lock_t; -#define IEEE80211_NODE_LOCK_INIT(_nt, _name) \ - rw_init_flags(&(_nt)->nt_nodelock, _name, RW_RECURSE) -#define IEEE80211_NODE_LOCK_DESTROY(_nt) rw_destroy(&(_nt)->nt_nodelock) -#define IEEE80211_NODE_LOCK(_nt) rw_wlock(&(_nt)->nt_nodelock) -#define IEEE80211_NODE_IS_LOCKED(_nt) rw_wowned(&(_nt)->nt_nodelock) -#define IEEE80211_NODE_UNLOCK(_nt) rw_wunlock(&(_nt)->nt_nodelock) +typedef struct { + char name[16]; /* e.g. "ath0_node_lock" */ + struct mtx mtx; +} ieee80211_node_lock_t; +#define IEEE80211_NODE_LOCK_INIT(_nt, _name) do { \ + ieee80211_node_lock_t *nl = &(_nt)->nt_nodelock; \ + snprintf(nl->name, sizeof(nl->name), "%s_node_lock", _name); \ + mtx_init(&nl->mtx, NULL, nl->name, MTX_DEF | MTX_RECURSE); \ +} while (0) +#define IEEE80211_NODE_LOCK_DESTROY(_nt) \ + mtx_destroy(&(_nt)->nt_nodelock.mtx) +#define IEEE80211_NODE_LOCK(_nt) \ + mtx_lock(&(_nt)->nt_nodelock.mtx) +#define IEEE80211_NODE_IS_LOCKED(_nt) \ + mtx_owned(&(_nt)->nt_nodelock.mtx) +#define IEEE80211_NODE_UNLOCK(_nt) \ + mtx_unlock(&(_nt)->nt_nodelock.mtx) #define IEEE80211_NODE_LOCK_ASSERT(_nt) \ - rw_assert(&(_nt)->nt_nodelock, RA_WLOCKED) + mtx_assert(&(_nt)->nt_nodelock.mtx, MA_OWNED) /* - * Node table iteration locking definitions; we piggyback on the node - * table lock by using a read/shared acquisition. + * Node table iteration locking definitions; this protects the + * scan generation # used to iterate over the station table + * while grabbing+releasing the node lock. */ -#define IEEE80211_NODE_ITERATE_LOCK(_nt) rw_rlock(&(_nt)->nt_nodelock) -#define IEEE80211_NODE_ITERATE_UNLOCK(_nt) rw_runlock(&(_nt)->nt_nodelock) +typedef struct { + char name[16]; /* e.g. "ath0_scan_lock" */ + struct mtx mtx; +} ieee80211_scan_lock_t; +#define IEEE80211_NODE_ITERATE_LOCK_INIT(_nt, _name) do { \ + ieee80211_scan_lock_t *sl = &(_nt)->nt_scanlock; \ + snprintf(sl->name, sizeof(sl->name), "%s_scan_lock", _name); \ + mtx_init(&sl->mtx, NULL, sl->name, MTX_DEF); \ +} while (0) +#define IEEE80211_NODE_ITERATE_LOCK_DESTROY(_nt) \ + mtx_destroy(&(_nt)->nt_scanlock.mtx) +#define IEEE80211_NODE_ITERATE_LOCK(_nt) \ + mtx_lock(&(_nt)->nt_scanlock.mtx) +#define IEEE80211_NODE_ITERATE_UNLOCK(_nt) \ + mtx_unlock(&(_nt)->nt_scanlock.mtx) #define _AGEQ_ENQUEUE(_ifq, _m, _qlen, _age) do { \ (_m)->m_nextpkt = NULL; \ ==== //depot/projects/vap/sys/net80211/ieee80211_ioctl.c#55 (text+ko) ==== @@ -1261,6 +1261,31 @@ ieee80211_node_leave(ni); } +static int +setmlme_dropsta(struct ieee80211vap *vap, + const uint8_t mac[IEEE80211_ADDR_LEN], struct mlmeop *mlmeop) +{ + struct ieee80211com *ic = vap->iv_ic; + struct ieee80211_node_table *nt = &ic->ic_sta; + struct ieee80211_node *ni; + int error = 0; + + /* NB: the broadcast address means do 'em all */ + if (!IEEE80211_ADDR_EQ(mac, ic->ic_ifp->if_broadcastaddr)) { + IEEE80211_NODE_LOCK(nt); + ni = ieee80211_find_node_locked(nt, mac); + if (ni != NULL) { + domlme(mlmeop, ni); + ieee80211_free_node(ni); + } else + error = ENOENT; + IEEE80211_NODE_UNLOCK(nt); + } else { + ieee80211_iterate_nodes(nt, domlme, mlmeop); + } + return error; +} + static __noinline int setmlme_common(struct ieee80211vap *vap, int op, const uint8_t mac[IEEE80211_ADDR_LEN], int reason) @@ -1285,20 +1310,7 @@ mlmeop.vap = vap; mlmeop.op = op; mlmeop.reason = reason; - IEEE80211_NODE_LOCK(nt); - /* NB: the broadcast address means do 'em all */ - if (!IEEE80211_ADDR_EQ(mac, ic->ic_ifp->if_broadcastaddr)) { - ni = ieee80211_find_node_locked(nt, mac); - if (ni != NULL) { - domlme(&mlmeop, ni); - ieee80211_free_node(ni); - } else - error = ENOENT; - } else { - ieee80211_iterate_nodes(&ic->ic_sta, - domlme, &mlmeop); - } - IEEE80211_NODE_UNLOCK(nt); + error = setmlme_dropsta(vap, mac, &mlmeop); break; case IEEE80211_M_WDS: /* XXX user app should send raw frame? */ @@ -1332,7 +1344,7 @@ break; } IEEE80211_NODE_LOCK(nt); - ni = ieee80211_find_vap_node(&ic->ic_sta, vap, mac); + ni = ieee80211_find_vap_node_locked(nt, vap, mac); if (ni != NULL) { mlmedebug(vap, mac, op, reason); if (op == IEEE80211_MLME_AUTHORIZE) @@ -1350,7 +1362,7 @@ break; } IEEE80211_NODE_LOCK(nt); - ni = ieee80211_find_vap_node(&ic->ic_sta, vap, mac); + ni = ieee80211_find_vap_node_locked(nt, vap, mac); if (ni != NULL) { mlmedebug(vap, mac, op, reason); if (reason == IEEE80211_STATUS_SUCCESS) { ==== //depot/projects/vap/sys/net80211/ieee80211_node.c#29 (text+ko) ==== @@ -1698,10 +1698,11 @@ struct ieee80211_node_table *nt, const char *name, int inact, int keyixmax) { + struct ifnet *ifp = ic->ic_ifp; + nt->nt_ic = ic; - snprintf(nt->nt_lockname, sizeof(nt->nt_lockname), "%s_node_lock", - ic->ic_ifp->if_xname); - IEEE80211_NODE_LOCK_INIT(nt, nt->nt_lockname); + IEEE80211_NODE_LOCK_INIT(nt, ifp->if_xname); + IEEE80211_NODE_ITERATE_LOCK_INIT(nt, ifp->if_xname); TAILQ_INIT(&nt->nt_node); nt->nt_name = name; nt->nt_scangen = 1; @@ -1772,6 +1773,7 @@ FREE(nt->nt_keyixmap, M_80211_NODE); nt->nt_keyixmap = NULL; } + IEEE80211_NODE_ITERATE_LOCK_DESTROY(nt); IEEE80211_NODE_LOCK_DESTROY(nt); } @@ -1793,13 +1795,10 @@ struct ieee80211_node *ni; int gen = 0; + IEEE80211_NODE_ITERATE_LOCK(nt); + gen = ++nt->nt_scangen; restart: IEEE80211_NODE_LOCK(nt); - if (gen == 0) { - gen = ++nt->nt_scangen; - if (nt->nt_scangen == 0) /* NB: 0 is never used */ - nt->nt_scangen++; - } TAILQ_FOREACH(ni, &nt->nt_node, ni_list) { if (ni->ni_scangen == gen) /* previously handled */ continue; @@ -1920,6 +1919,8 @@ } } IEEE80211_NODE_UNLOCK(nt); + + IEEE80211_NODE_ITERATE_UNLOCK(nt); } /* @@ -2003,28 +2004,28 @@ } void -ieee80211_iterate_nodes(struct ieee80211_node_table *nt, ieee80211_iter_func *f, void *arg) +ieee80211_iterate_nodes(struct ieee80211_node_table *nt, + ieee80211_iter_func *f, void *arg) { struct ieee80211_node *ni; - u_int gen = 0; + u_int gen; + IEEE80211_NODE_ITERATE_LOCK(nt); + gen = ++nt->nt_scangen; restart: - IEEE80211_NODE_ITERATE_LOCK(nt); - if (gen == 0) { - gen = ++nt->nt_scangen; - if (nt->nt_scangen == 0) /* NB: 0 is never used */ - nt->nt_scangen++; - } + IEEE80211_NODE_LOCK(nt); TAILQ_FOREACH(ni, &nt->nt_node, ni_list) { if (ni->ni_scangen != gen) { ni->ni_scangen = gen; (void) ieee80211_ref_node(ni); - IEEE80211_NODE_ITERATE_UNLOCK(nt); + IEEE80211_NODE_UNLOCK(nt); (*f)(arg, ni); ieee80211_free_node(ni); goto restart; } } + IEEE80211_NODE_UNLOCK(nt); + IEEE80211_NODE_ITERATE_UNLOCK(nt); } @@ -2070,6 +2071,8 @@ { struct ieee80211vap *vap; + IEEE80211_LOCK_ASSERT(ic); + TAILQ_FOREACH(vap, &ic->ic_vaps, iv_next) if (vap->iv_opmode == IEEE80211_M_HOSTAP) ieee80211_beacon_notify(vap, IEEE80211_BEACON_ERP); ==== //depot/projects/vap/sys/net80211/ieee80211_node.h#19 (text+ko) ==== @@ -295,14 +295,14 @@ */ struct ieee80211_node_table { struct ieee80211com *nt_ic; /* back reference */ - char nt_lockname[16];/* e.g. "ath0_node_lock" */ ieee80211_node_lock_t nt_nodelock; /* on node table */ TAILQ_HEAD(, ieee80211_node) nt_node; /* information of all nodes */ LIST_HEAD(, ieee80211_node) nt_hash[IEEE80211_NODE_HASHSIZE]; struct ieee80211_node **nt_keyixmap; /* key ix -> node map */ int nt_keyixmax; /* keyixmap size */ - const char *nt_name; /* for debugging */ - u_int nt_scangen; /* gen# for timeout scan */ + const char *nt_name; /* table name for debug msgs */ + ieee80211_scan_lock_t nt_scanlock; /* on nt_scangen */ + u_int nt_scangen; /* gen# for iterators */ int nt_inact_init; /* initial node inact setting */ };
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200804182007.m3IK7jtr093266>