> http://atlantic.east/ws/meem/physaddr/webrev > > > > * ip_reprocess_ioctl() The check at L25359 is redundant. > > > ipsq_current_ipif is bound to be NULL here. So we can call > > > ipsq_current_start directly. Right ? > > > >This is an interesting case. Is your assertion that ipsq_current_ipif > >must be NULL because if it's non-NULL, the it must still have the original > >IPSQ value and thus the ipsq_try_enter() in ipif_set_values() will always > >trivially succeed (by reentering itself)? If so, I think I agree, but the > >current code in Nevada is not so bold -- that is, ip_reprocess_ioctl() > >does not ASSERT() that ipsq_current_ipif is NULL prior to setting it to > >ill->ill_ipif -- so it ends up working in both cases. The wording of the > >comment in ip_reprocess_ioctl() also suggests that there are cases where > >ipsq_current_ipif is already set. > > > >For now, I've made ip_reprocess_ioctl() mirror the ipsq_current_start() > >code in ipif_set_values() -- but I could be convinced to be more bold. > > > ip_reprocess_ioctl is called only from ipsq_exit() during the unwind > from the ipsq and not directly. ipsq_dq() picks the next message from > the ipsq. ipsq_dq() won't pick up the next message from > ipsq_xopq_mphead unless ipsq_current_ipif is NULL. In the case of > ipif_set_values -> ipsq_try_enter it specifies NEW_OP. Based on all the > above you can assert that ipsq_current_ipif must be NULL in this case.
OK -- I'm curious though: what's wrong with my explanation above that ipsq_try_enter() will always trivially succeed because ipsq_writer == curthread && reentry_ok? > >Is there an explicit problem with always treating DL_NOTIFY_IND as a new > >operation? Since IP cannot be sure that driver support DL_NOTIFY, it does > >not wait for the response for the drivers when it sends the DL_NOTIFY_REQ > >probe -- so why would it be appropriate to consider the resulting > >DL_NOTIFY_IND's part of the current operation? > > There may not be any problem today. But consider the following. > We start some operation say a plumb which consists of multiple DLPI message > exchanges between IP and the driver. Some message say DL_BIND triggers a > DL_NOTE_LINK_UP. Now we don't process this message immediately and > queue it as a NEW_OP and continue the DLPI message exchange process. > After the plumb is over, this DL_NOTE_LINK_UP is irrelevant, but we pick it > up now and attempt to set the RUNNING flag, but it should already be set. I still don't see the problem -- e.g., when ip_rput_dlpi_writer() processes a DL_NOTE_LINK_UP, it checks to see if it actually represents a link state change. If not, it's ignored. > Do you need the ill_fastpath_flush() in ill_set_phys_addr_tail() ? > Hasn't bringing down and bringing up the interface achieved > that already ? I think so, but I'm not sure I understand where the IRE_BROADCAST and IRE_MIPRTUN entries that are currently cleaned up by the the fastpath flush code are done in the down/up dance. Do you know? -- meem
