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.)
>
> - 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.
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]
