On Tue, 21 Feb 2012 12:30:56 -0500, Greg Troxel wrote:
My colleague Bev Schwartz found that in src/sys/dev/pci/if_bnx.c,workqueue_enqueue is called with a struct work * that points to a static variable. But, this struct is used as a linked list, and reuse of thestruct risks creating a circular list. bnx's workqueue callback does not free the struct work *. Otherworkqueue uses seem to alloc before enqueue and then free after the workis done. It turns out that under our workloads we have yet to see a case where wk->wk_entry == wk.Can anyone explain why the current code is ok, or how it should change?
I made this change to defer allocation of bnx packets outside the interrupt handler while staying close to what was done in OpenBSD's bnx(4). For obvious reasons, I could not defer allocation to a thread context while having to allocate a work struct from handler (chicken/egg problem).
The work struct is not used per-see, the workqueue_enqueue() step is solely used to wake the wq thread. IIRC workqueue(9) does not support passing NULL arguments, so I had to restrict the variable scope to its smallest block, hence the static declaration in the function. Ugly, but the allocation deferal fixed the LOCKDEBUG kernel with this driver.
A much cleaner approach would be an API where we can arbitrarily pass continuations to a thread with /optional/ arguments, and a way to wakeup a thread without having to implement the locking in the caller every time (workqueue(9) is close to that but allocating wk structs prevent API use from interrupt code). Matt's API proposal was interesting in this regard, but I did not have time to look into it.
-- Jean-Yves Migeon jym%NetBSD.org@localhost