This is an interesting case, let us go through this. (The discussion is bsed on the new GLDv3 locking framework which is in the current Crossbow gate, but it is yet to merge with ClearView UV. The source should get pushed to OpenSolaris shortly.)
* In the new model, DLD/DLS/MAC hold locks only locally to protect data structures. They are not held across a call to another layer. In addition di_lock is gone. So that gets rid of any deadlocks involving di_lock. * The framework however allows mac clients to hold locks across mac calls with certain restrictions. Client locks may be held across mac calls only in the control path and they may end up cv_waiting in the mac. To avoid deadlock, these locks must not be used in the data path. Going to our example substitute some arbitrary mac client instead of DLS. Now thread 1 holds the di_lock across the call to the MAC in the DL_DISABMULTI_REQ call and blocks in the mac waiting for a response. To avoid deadlock di_lock must never be acquired in the data path and hence thread 2 won't attempt to acquire this lock. > They are generally asking for trouble, but I recall drivers that do this > under the assumption that it won't be an issue as long as they ensure that > the lock in question will never be acquired by the same thread on reentry. > For instance, they might hold foo_lock across putnext() on the read side, > but then ensure that foo_lock is only held in srv(9E) context on the write > side. The current GLDv3 locking design makes that assumption insufficient. It is assuming more. It works only if all the other STREAMS modules are well behaved and don't hold locks across layers with ce being the only privileged exception. With GLD every module gets that privilege and all hell breaks loose. Now trying to come up with a list of rules of when to allow holding locks across layers and when not to, makes the rule set non-trivial. On the other hand, a single rule that says 'Locks must not be held across layers' which is the equivalent of 'no locks across putnext' makes the inter-module interaction very simple. Inter-layer deadlock avoidance and lock ordering become trivial. In the long run, I believe this is the move viable option, though for now we have to live with mac clients holding locks across layers. Thirumalai Peter Memishian wrote: > During IPMP testing, I hit an interesting deadlock between softmac/GLDv3 > and ce. Thread 1 grabbed di_lock as RW_WRITER (via dls_multicst_remove()), > sent a DL_DISABMULTI_REQ downstream, and is blocked waiting for an ACK: > > stack pointer for thread 2a10046fca0: 2a10046eda1 > [ 000002a10046eda1 cv_timedwait+0x8c() ] > 000002a10046ee51 softmac_output+0x80() > 000002a10046ef01 mac_multicst_remove+0xc4() > 000002a10046efb1 dls_multicst_remove+0x60() > 000002a10046f061 proto_disabmulti_req+0xbc() > 000002a10046f111 dld_wput_nondata_task+0xf0() > 000002a10046f1c1 taskq_d_thread+0xbc() > 000002a10046f291 thread_start+4() > > Thread 2 is an interrupt that happened to come in after thread 1 grabbed > di_lock but before the DL_DISABMULTI_REQ was handled by ce. Inside the > ce_intr() logic, it grabbed a lock as RW_READER and called putnext(). > It's blocked in dls_accept() trying to acquire di_lock as RW_READER: > > stack pointer for thread 2a10007fca0: 2a10007e191 > [ 000002a10007e191 turnstile_block+0x5a4() ] > 000002a10007e241 rw_enter_sleep+0x168() > 000002a10007e2f1 dls_accept+0x1c() > 000002a10007e3a1 i_dls_link_rx+0x260() > 000002a10007e4d1 mac_do_rx+0xb0() > 000002a10007e581 putnext+0x3f4() > 000002a10007e631 ce_intr+0x1a8c() > 000002a10007f1d1 pci_intr_wrapper+0xe8() > 000002a10007f291 intr_thread+0x2b8() > > Thread 3 is the taskq handling the DL_DISABMULTI_REQ. It's trying to > acquire the aforementioned ce lock as RW_WRITER, but is blocked because > thread 2 holds it as RW_READER: > > stack pointer for thread 2a100157ca0: 2a100156691 > [ 000002a100156691 turnstile_block+0x5a4() ] > 000002a100156741 rw_enter_sleep+0x1b0() > 000002a1001567f1 ce_dmreq+0xc8() > 000002a1001568b1 ce_proto+0x1d8() > 000002a100156961 ce_wsrv+0x2d30() > 000002a100157061 runservice+0x6c() > 000002a100157111 stream_service+0x190() > 000002a1001571c1 taskq_d_thread+0xbc() > 000002a100157291 thread_start+4() > > So, T1 is blocked waiting for T3, T3 is blocked waiting for T2, and T2 is > blocked waiting for T1. Seems like the right fix is to change ce not to > hold a lock across putnext(), but that may be a high-risk change and there > may be other legacy drivers that have a similar flaw. So I'm interested > to hear from Thiru on whether his new GLDv3 locking design would also > resolve this deadlock. > >
