Peter Memishian wrote:
>Thiru,
>
>I think my new DLPI ACK checking logic has unearthed some longstanding IP
>brokenness at teardown. For instance, in the normal case, IP attempts to
>send a DL_DETACH_REQ as part of ill_delete_tail(), but this DL_DETACH_REQ
>is never actually sent. This is because the processing of the
>DL_UNBIND_REQ that's sent before it is never completed, because when we
>call ip_rput_dlpi() with a DL_OK_ACK for the DL_UNBIND_REQ, we are already
>inside the IPSQ -- but it then tries to do:
>
> case DL_OK_ACK:
> ip1dbg(("ip_rput: DL_OK_ACK for %s\n",
> dlpi_prim_str((int)dloa->dl_correct_primitive)));
> switch (dloa->dl_correct_primitive) {
> case DL_UNBIND_REQ:
> mutex_enter(&ill->ill_lock);
> ill->ill_state_flags &= ~ILL_DL_UNBIND_IN_PROGRESS;
> cv_signal(&ill->ill_cv);
> mutex_exit(&ill->ill_lock);
>--> /* FALLTHRU */
> case DL_ATTACH_REQ:
> case DL_DETACH_REQ:
> /*
> * Refhold the ill to match qwriter_ip which does a
> * refrele. Since this is on the ill stream we
> * unconditionally bump up the refcount
> */
> ill_refhold(ill);
>--> qwriter_ip(NULL, ill, q, mp, ip_rput_dlpi_writer,
> CUR_OP, B_FALSE);
> return;
>
>Since this qwriter_ip() is for CUR_OP and reentry_ok (the last parameter)
>is set to B_FALSE, ip_rput_dlpi_writer() will never be called. Since it's
>not called, ill_dlpi_done() is never called, and thus we will never send
>the deferred DL_DETACH_REQ.
>
>
In reality the unplumb thread never releases the ipsq once it enters
the ipq in ip_modclose(). The cv_wait is done while holding the ipsq,
and after the qprocsoff, ipsq_flush() gets rid of all enqueued messages.
trying to become writer. So irrespective of the 'reentry_ok' we are not
going to process the DL_OK_ACK !
One possibility is to call ill_dlpi_done() after the cv_wait() in
ill_delete_tail(). The DL_OK_ACK message itself should just be freed
by ip_rput_dlpi() (instead of calling qwriter_ip) after clearing the
ILL_DL_UNBIND_IN_PROGRESS.
>There is a related problem here: if the interface isn't "up" (that is,
>DL_BIND_REQ has not been sent), then we *will* send the DL_DETACH_REQ
>(since the DL_UNBIND_REQ that would clog the DLPI pipe as above is
>skipped), processing the ack will also call qwriter_ip() (as above) and
>also be unable to call ip_rput_dlpi_writer(). However, note that even if
>it was able to call ip_rput_dlpi_writer(), the resulting ill_dlpi_done()
>would generate an "unsolicited ack" warning because ill_dlpi_pending will
>be set to DL_PRIM_INVAL, since ill_dlpi_dispatch() skips setting
>ill_dlpi_pending when ILL_CONDEMNED is set (except for DL_UNBIND_REQs).
>
>
I think the intent is just to at most send the DL_DETACH to the driver.
ip_rput allows only M_PCPROTO after the unplumb has started (ILL_CONDEMNED
is set). Maybe ip_rput_dlpi can free the OK_ACK for the detach case
if you don't want to see the "unsolicited ack" warning.
But again all this is timing dependent. ill_delete_tail() itself does a
qprocsoff() immediately after sending down the detach. So whether we
get an OK_ACK or not itself is unknown depending on whether the driver
does an inline processing of the detach request or not.
>Seems like the simplest fix for a lot of this brokenness is to simply not
>try to send the DL_DETACH_REQ. As per the bugs above, it seems like we've
>accidentally not been sending it for a while, and no one has complained.
>Further, the DLPI specification does not require that one do a detach
>prior to close -- so I think IP is within its rights to skip the detach.
>Thoughts?
>
>
I don't have the DLPI spec handy. Is the optional part because some
drivers could be type 1 or is it optional even for type 2 drivers ?
Anyway I would not worry too much about it, making things simpler is better.
As more and more drivers become GLDv3, this is even less of an issue.
P.S. If you see on10, the result is still the same, we probably don't
send detach. We just do a sequence of ill_dlpi_send an then do a qprocsoff.
So only the 1st request may go down. The same is true even in S9 and
earlier...
Thirumalai
