Garrett D'Amore wrote: > On Mon, 2007-09-03 at 16:50 +0800, Cathy Zhou wrote: >> Garrett D'Amore wrote: >>> I'm planning on putting back the fixes in the following webrev. This >>> changes the notification in Nemo somewhat, hopefully to be a lot more >>> robust. >>> >>> It fixes in particular 6508900, which basically states that i_mac_notify >>> can fail, and when it does, it spams the log. (Worse, it can fail to >>> call qenable() which could lead to an apparent hang on transmit, etc.) >>> >>> (It can fail due to resource shortage.) My solution is to use a >>> separate thread, bit mask, and condition variable. >>> >>> The webrev is at http://cr.opensolaris.org/~gdamore/mac-notify/ >>> >>> Have a look if you're interested. (I think I have my 2 needed >>> reviewers, but I won't complain if others give me feedback.) >>> >>> -- Garrett >>> >> Garrett, >> >> I had a look of your change, and I have several questions: >> >> - What prevent new mac notifications come in between Line 1429 and Line >> 1441? If new mac notifications can come in, the thread will exit because the >> check on Line 217 would passes, then the system will wait for ever on Line >> 1444-1445? > > Its a good observation, but it is prevented because i_mac_notify checks > the mip->mi_disabled field. So once the mac_unregister() starts, no new > notifications get delivered to the thread. > > >> I think more reasonable way is for i_mac_notify() to do nothing if ((bits & >> (1 << MAC_NNOTE)) != 0), and to have i_mac_notify_thread() to only exit >> after it finishes to process each notification bits. > > The mip->mi_disabled check (which was already there) serves that > purpose. But I suppose I could easily change it to this check. (The > boolean test against mi_disabled is faster than doing it a bitfield > check though, and i_mac_notify() is potentially a hot path. So unless > you feel really strongly about this, I'd prefer to leave it as is.) > That's fine. But I think it worths a comment though, or an assertion "ASSERT(mip->mi_disabled == B_TRUE) on line 218. Also, don't you need a "cv_broadcast(&mip->mi_notify_cv);" before line 1370?
> >> - Why we need the notification quiesce twice? One on Line 1426-1429, another >> on Line 1439-1448? > > The first one isn't strictly needed, and I could remove it with no real > harm, but it does allow for any pending notifications to finish delivery > before shutting down the thread. This isn't on a critical hot path, so > I figured it was more polite to allow them to complete than to nuke 'em > from orbit. Can this be achieved by changing i_mac_notify_thread() to first process all pending notifications before it exits the thread? Thanks - Cathy > > If you feel strongly about it, I can remove the first one at 1426. > > It is interesting that you're the fourth reviewer. Each reviewer has > had slightly different feedback about the structure of the thread > function, but it seems that acceptance of the conversion to a thread is > unanimously acceptable. So, aprt from different opinions about how to > structure the test cases (and some of which I will accept... in > particular the test for MAC_NOTE_LINK will be moved outside of the > middle for loop) I am pleased that everyone is happy with it. > > -- Garrett > >> Thanks >> - Cathy > _______________________________________________ networking-discuss mailing list [email protected]
