On Tue, 2007-09-04 at 11:13 +0800, Cathy Zhou wrote:
> 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.
Accept. I've added that assertion.
> Also, don't you need a
> "cv_broadcast(&mip->mi_notify_cv);" before line 1370?
Yes, I do! Thanks! Accept.
>
> >
> >> - 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?
Well, there is also the problem that I'd really prefer the notifications
to finish *before* commiting to destroy the thread, but I'd prefer not
to commit to destroy the thread until after the dls_destroy() is called.
This helps with a more graceful recovery if dls_destroy() fails for some
reason.
Right now it Just Works, so unless you really think it should be
altered, I'd prefer to leave it alone.
-- Garrett
>
> 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]
