Date: Mon, 6 Aug 2012 01:42:06 -0600 From: PseudoCylon <moonlightakkiy@yahoo.ca> To: Adrian Chadd <adrian.chadd@gmail.com> Cc: freebsd-wireless@freebsd.org, Kim Culhan <w8hdkim@gmail.com> Subject: Re: ath lor Message-ID: <CAFZ_MY%2BmE_%2B2QHB=iVYP6CA_96pt8d=9PNYb2_Y3s6x%2BW2HUyw@mail.gmail.com> In-Reply-To: <CAJ-VmomXVKYodo9Q5Nt1mvWPsOtSHjh5R9RqKcqtrGfrYjh9AA@mail.gmail.com> References: <CAFZ_MYKgUkryy4parts3QahAyPA7FY9xUqC98_E7oFW%2BzarA8A@mail.gmail.com> <CAFZ_MYKeOKxT3k7JWHjdH83vbieZ6JpXe0kbXTJy4neEd5Aqew@mail.gmail.com> <CAJ-VmomGBvgLwFEcXbEuYkAj=g%2By8zVo8cT2nSSMdydCk=OhYQ@mail.gmail.com> <CAFZ_MYJP97aO73zLpJF9%2B8MiQVqAHGNngmtOakYDcaikvyq7og@mail.gmail.com> <CAJ-VmomSTcTFVQovOaGB9_7kTh_R9Z2W4bypknHVrtykYz2SMg@mail.gmail.com> <CAFZ_MYKnKmM2M%2BpcifxWNcp-pXsJGhb2i7aRo94JK3jqUyaNrw@mail.gmail.com> <CAJ-VmokoURqGmD=U_=p0noeGbLBwTbW63fMfo8Teb7iw3U_W-g@mail.gmail.com> <CAFZ_MYL%2BBV0dPP_hxKw%2BWEz-zz2TxwOdYRTqipPuVa6ZEmvx9A@mail.gmail.com> <CAJ-Vmok-TFHyo0JkX_AucDwRj=JX1qwfc_s7nidFSBnrO_Y-jw@mail.gmail.com> <CAFZ_MYLsa1QHHe=U=9=rjFsoXCfv7DB0LMZLxiZekpACSB9nYg@mail.gmail.com> <CAJ-VmonzuvnqE3MsuOac2QXYeamYbCysMhurBmCRsaCCTTpfnA@mail.gmail.com> <CAFZ_MYKmORxoA9utQujdZOHV=Kmfcw=JagSdBRQjBzbTR4bOGg@mail.gmail.com> <CAJ-VmomLwcAkwKhP0YXykJLjobcuSa_=nA4dtdczN8bgixFSCw@mail.gmail.com> <CAFZ_MY%2BD4XptiR6OaLiENo0iU1gG49abXPJ_JksK4_GuHU47Sw@mail.gmail.com> <CAJ-VmomXVKYodo9Q5Nt1mvWPsOtSHjh5R9RqKcqtrGfrYjh9AA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] On Sun, Aug 5, 2012 at 11:24 AM, Adrian Chadd <adrian.chadd@gmail.com> wrote: > On 5 August 2012 06:37, PseudoCylon <moonlightakkiy@yahoo.ca> wrote: > >>> What's the path through the stack -> driver that leads to >>> run_tx_free() being called with the driver lock held? >> >> This one was easy to spot. >> USB stack calls run_bulk_tx_callbackN() with driver lock held, >> ->run_tx_free() ->ieee80211_free_node() then locks node lock. > > Ah. Hm, does the USB stack need to hold any locks held? > That seems like a recipe for disaster for drivers in general, not just wifi. Not 100% sure, but yes. And, it is unsafe to unlock before all usb related tasks have done. In this case, we should unlock somewhere here. http://fxr.watson.org/fxr/source/dev/usb/wlan/if_run.c#L2805 Caller can specify the lock when setting up usb transfers http://fxr.watson.org/fxr/source/dev/usb/wlan/if_run.c#L568 If null is passed rather than ptr to mtx, usb stack will use giant lock. http://fxr.watson.org/fxr/source/dev/usb/usb_transfer.c?im=10#L841 > >>> And on the flip side, what's the path through the driver -> stack >>> that's called with the driver lock held? The comlock shouldn't be held >>> when TX/RX'ing packets up to the stack. >> >> I have not found it yet. I've been stuck here, but I'll keep looking >> for this. Then I can patch usb/150189. > > Cool. > >>> Hm, maybe we should start a wiki page with all of the net80211/wifi >>> driver LORs. Do you have wiki.freebsd.org access? If not, create an >>> account and tell me what username you choose. If you do, create a page >>> under http://wiki.freebsd.org/WiFi and let's start documenting the >>> LORs that we see. >> >> run(4) calls ieee80211_runtask() because thread is non-sleepable or >> avoid LOR. I think I can still track back LOR. Also, I saved LOR >> outputs I saw long time ago. See if I can find them on old hard drive. > > Yes. Please, let's start a wiki page with all the wifi stack / driver > related LORs that you find. I really want to find/fix/document/squish > them as soon as possible. Will do. > > Also, would you please push a patch to the mailing list for your > current ieee80211_iterate_nodes() work, so Kim and I can try it out? > That way we can provide testing and feedback (respectively.) Attached iter.patch (with if_print) iter2.patch (with reverting change functions) both diff against CURRENT > > Thanks for your work with this! I'm glad to have someone else working > on improving things. (Ray, Bernard and Monthadar included as well. :-) Grad to help some. AK [-- Attachment #2 --] diff --git a/ieee80211_node.c b/ieee80211_node.c index 861fa85..a609cb1 100644 --- a/ieee80211_node.c +++ b/ieee80211_node.c @@ -2156,32 +2156,79 @@ ieee80211_node_timeout(void *arg) ieee80211_node_timeout, ic); } +/* + * May directly be called and do customized iterate functions. + * Only requirement is to decrement each node's ref count. + */ void -ieee80211_iterate_nodes(struct ieee80211_node_table *nt, - ieee80211_iter_func *f, void *arg) +ieee80211_iterate_nt(struct ieee80211_node_table *nt, + struct ieee80211_node **ni_arr, uint16_t max_aid) { struct ieee80211_node *ni; u_int gen; + int i = 0; IEEE80211_NODE_ITERATE_LOCK(nt); + IEEE80211_NODE_LOCK(nt); + gen = ++nt->nt_scangen; + restart: - 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_UNLOCK(nt); - (*f)(arg, ni); - ieee80211_free_node(ni); - goto restart; + if (ni->ni_scangen == gen) + continue; + + if (i >= max_aid) { + if_printf(nt->nt_ic->ic_ifp, + "Node array overflow: max=%u", max_aid); + break; } + + ni->ni_scangen = gen; + (*(ni_arr + i++)) = ieee80211_ref_node(ni); + goto restart; } - IEEE80211_NODE_UNLOCK(nt); + IEEE80211_NODE_UNLOCK(nt); IEEE80211_NODE_ITERATE_UNLOCK(nt); } +/* + * Just a wrapper, so we don't have to change every ieee80211_iterate_nodes() + * reference in the source. + */ +void +ieee80211_iterate_nodes(struct ieee80211_node_table *nt, + ieee80211_iter_func *f, void *arg) +{ + struct ieee80211_node **ni_arr; + struct ieee80211_node *ni; + unsigned long size; + int i; + uint16_t max_aid; + + max_aid = TAILQ_FIRST(&nt->nt_ic->ic_vaps)->iv_max_aid; + size = max_aid * sizeof(*ni_arr); + ni_arr = (struct ieee80211_node **)malloc(size, M_80211_NODE, + M_NOWAIT | M_ZERO); + if (ni_arr == NULL) + return; + + ieee80211_iterate_nt(nt, ni_arr, max_aid); + + for (i = 0; i < max_aid; i++) { + ni = *(ni_arr + i); + if (ni == NULL) /* end of the list */ + break; + + (*f)(arg, ni); + /* ieee80211_free_node() locks by itself */ + ieee80211_free_node(ni); + } + + free(ni_arr, M_80211_NODE); +} + void ieee80211_dump_node(struct ieee80211_node_table *nt, struct ieee80211_node *ni) { diff --git a/ieee80211_node.h b/ieee80211_node.h index 83b108b..1fbc892 100644 --- a/ieee80211_node.h +++ b/ieee80211_node.h @@ -438,6 +438,8 @@ int ieee80211_node_delucastkey(struct ieee80211_node *); void ieee80211_node_timeout(void *arg); typedef void ieee80211_iter_func(void *, struct ieee80211_node *); +void ieee80211_iterate_nt(struct ieee80211_node_table *, + struct ieee80211_node **, uint16_t); void ieee80211_iterate_nodes(struct ieee80211_node_table *, ieee80211_iter_func *, void *); [-- Attachment #3 --] diff --git a/ieee80211_node.c b/ieee80211_node.c index 861fa85..a15fc91 100644 --- a/ieee80211_node.c +++ b/ieee80211_node.c @@ -2156,30 +2156,93 @@ ieee80211_node_timeout(void *arg) ieee80211_node_timeout, ic); } -void -ieee80211_iterate_nodes(struct ieee80211_node_table *nt, - ieee80211_iter_func *f, void *arg) +/* + * May directly be called and do customized iterate functions. + * Only requirement is to decrement each node's ref count. + */ +int +ieee80211_iterate_nt(struct ieee80211_node_table *nt, + struct ieee80211_node **ni_arr, uint16_t max_aid) { struct ieee80211_node *ni; u_int gen; + int i, ret; IEEE80211_NODE_ITERATE_LOCK(nt); + IEEE80211_NODE_LOCK(nt); + gen = ++nt->nt_scangen; + i = ret = 0; + restart: - 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_UNLOCK(nt); - (*f)(arg, ni); + if (ni->ni_scangen == gen) + continue; + + if (i >= max_aid) { + ret = E2BIG; + if_printf(nt->nt_ic->ic_ifp, + "Node array overflow: max=%u", max_aid); + break; + } + + ni->ni_scangen = gen; + (*(ni_arr + i++)) = ieee80211_ref_node(ni); + goto restart; + } + + if (ret) { + nt->nt_scangen--; + for (i = 0; i < max_aid; i++) { + ni = *(ni_arr + i); + ni->ni_scangen--; + /* node lock is recursive */ ieee80211_free_node(ni); - goto restart; } + free(ni_arr, M_80211_NODE); } - IEEE80211_NODE_UNLOCK(nt); + IEEE80211_NODE_UNLOCK(nt); IEEE80211_NODE_ITERATE_UNLOCK(nt); + + return (ret); +} + +/* + * Just a wrapper, so we don't have to change every ieee80211_iterate_nodes() + * reference in the source. + */ +void +ieee80211_iterate_nodes(struct ieee80211_node_table *nt, + ieee80211_iter_func *f, void *arg) +{ + struct ieee80211_node **ni_arr; + struct ieee80211_node *ni; + unsigned long size; + int i; + uint16_t max_aid; + + max_aid = TAILQ_FIRST(&nt->nt_ic->ic_vaps)->iv_max_aid; + size = max_aid * sizeof(*ni_arr); + ni_arr = (struct ieee80211_node **)malloc(size, M_80211_NODE, + M_NOWAIT | M_ZERO); + if (ni_arr == NULL) + return; + + if (!ieee80211_iterate_nt(nt, ni_arr, max_aid)) + return; + + for (i = 0; i < max_aid; i++) { + ni = *(ni_arr + i); + if (ni == NULL) /* end of the list */ + break; + + (*f)(arg, ni); + /* ieee80211_free_node() locks by itself */ + ieee80211_free_node(ni); + } + + free(ni_arr, M_80211_NODE); } void diff --git a/ieee80211_node.h b/ieee80211_node.h index 83b108b..48eae2d 100644 --- a/ieee80211_node.h +++ b/ieee80211_node.h @@ -438,6 +438,8 @@ int ieee80211_node_delucastkey(struct ieee80211_node *); void ieee80211_node_timeout(void *arg); typedef void ieee80211_iter_func(void *, struct ieee80211_node *); +int ieee80211_iterate_nt(struct ieee80211_node_table *, + struct ieee80211_node **, uint16_t); void ieee80211_iterate_nodes(struct ieee80211_node_table *, ieee80211_iter_func *, void *);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFZ_MY%2BmE_%2B2QHB=iVYP6CA_96pt8d=9PNYb2_Y3s6x%2BW2HUyw>