Re: intr_event_destroy(9)

On Tue, Oct 26, 2010 at 9:46 AM, John Baldwin <jhb_at_freebsd.org> wrote:
> On Tuesday, October 26, 2010 11:55:10 am Matthew Fleming wrote:
>> It looks like a bug in intr_event_destroy(9): I'm trying to unload a
>> new driver being developed internally for NVRAM, and I get this
>> WITNESS warning and hang:
>>
>>
>> # kldunload rnv
>> Sleeping on "ithdty" with the following non-sleepable locks held:
>> exclusive sleep mutex intr event list (intr event list) r = 0
>> (0xffffffff806f9560) locked _at_
>> /data/sb/BR_BONNEVILLE_HW/src/sys/kern/kern_intr.c:404
>> KDB: stack backtrace:
>> [ffffffff801a544d] db_trace_self_wrapper+0x3d
>> [ffffffff802e7b26] witness_warn+0x2f6
>> [ffffffff802a1a43] _sleep+0xc3
>> [ffffffff8026dad5] intr_event_destroy+0xe5
>> [ffffff87b05ba805] rnv_pci_detach+0xc5
>> [ffffffff802c9414] device_detach+0xb4
>> [ffffffff802c974f] devclass_delete_driver+0xdf
>> [ffffffff802c991d] driver_module_handler+0x11d
>> [ffffffff802843a2] module_unload+0x42
>> [ffffffff80279f4b] linker_file_unload+0x19b
>> [ffffffff8027aa1b] kern_kldunload+0x10b
>> [ffffffff802a2609] isi_syscall+0x99
>> [ffffffff804dee3e] ia32_syscall+0x1ce
>> [ffffffff804a7e50] Xint0x80_syscall+0x60
>> --- syscall (444, FreeBSD ELF32, kldunloadf), rip = 0x280c1aff, rsp =
>> 0xffffd44c, rbp = 0xffffdc98 ---
>>
>> Looking at intr_event_destroy, I see this snippet from r157728:
>>
>>
>> #ifndef notyet
>>       if (ie->ie_thread != NULL) {
>>               ithread_destroy(ie->ie_thread);
>>               ie->ie_thread = NULL;
>>       }
>> #endif
>>
>> There is an msleep(9) in ithread_destroy(9).  And everywhere else that
>> uses notyet has #ifdef, not #ifndef.  So... is this a typo?
>
> No, it's actually on purpose I think as the other bits under notyet destroy
> the thread when the last handler for it goes away.
>
> However, ithread_destroy() does not block in any of 7.x or later:
>
> static void
> ithread_destroy(struct intr_thread *ithread)
> {
>        struct thread *td;
>
>        CTR2(KTR_INTR, "%s: killing %s", __func__, ithread->it_event->ie_name);
>        td = ithread->it_thread;
>        thread_lock(td);
>        ithread->it_flags |= IT_DEAD;
>        if (TD_AWAITING_INTR(td)) {
>                TD_CLR_IWAIT(td);
>                sched_add(td, SRQ_INTR);
>        }
>        thread_unlock(td);
> }
>
> Maybe you have a local change?  If so, you can probably unlock the global
> event_list lock before calling ithread_destroy() (but after the
> TAILQ_REMOVE()) in intr_event_destroy().

Gah, yes, it looks like a local change we can probably do without.

And as it turns out the driver can pass NULL to siw_add() and skip the
intr_event_destroy() anyways.

Thanks for the help!
Sorry for the noise.
matthew