[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: workqueue in if_bnx: static struct work * seems wrong
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
variable. But, this struct is used as a linked list, and reuse of
struct risks creating a circular list.
bnx's workqueue callback does not free the struct work *. Other
workqueue uses seem to alloc before enqueue and then free after the
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
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
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.
Main Index |
Thread Index |