Peter Memishian wrote: > > 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? > > I think you said the same thing in a different way. I tried to explain the reasoning in a different way to come to the same conclusion.
> > >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. > > In this case there may be no harm, at worst we may end up doing some extra work, for example if the driver sent up a DL_NOTE_LINK_DOWN followed by DL_NOTE_LINK_UP on getting a DL_BIND. To me, the test is whether the DL_NOTE is truly asynchronous or in response to some IP request. If it is the former then it is a NEW_OP. Otherwise it is CUR_OP. I guess the difficulty is that we don't really know. The DL_NOTE may be sent truly asynchronously when a link goes down as pulling a cable, or in response to IP asking the driver to turn the link on or off. > > 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? > > > ipif_down() -> ipif_down_delete_ire, ire_walk_ill_mrtun should take care of it, since you are downing all the ipifs of the ill. Thirumalai
