[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 Sun, 26 Feb 2012 09:47:16 -0500, Greg Troxel wrote:
For obvious reasons, I could not defer allocation to a thread
context while having to allocate a work struct from handler
I don't follow that. One only needs to be able to allocate a small
struct, but I guess you are saying that there might not be memory
available? Or are you saying that all means of allocating memory
require a thread context.
The former. I could allocate from intterupt handler, but had to handle
the case where memory is scarced and allocation fails (pool_cache(9)
allows that, for example). I regularly read that we should avoid
allocating from interrupt and defer that step to thread context, so I
took down this path.
Note that this choice was influenced because I wanted my patch to
remain close to the one in OpenBSD. Besides, the wk struct is unused in
the current code so allocating it to free it a few seconds later is just
extra overhead for no real purpose.
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
passing NULL arguments, so I had to restrict the variable scope to
smallest block, hence the static declaration in the function. Ugly,
but the allocation deferal fixed the LOCKDEBUG kernel with this
I saw that it wasn't used, but subr_workqueue will, if
were called twice, write the forward pointers on the work struct.
The code is missing an explanation of why this apparently unsafe code
is ok, and the critical point is the flag that prevents a second
From being made while the first one is still on the workqueue proper
(rather than the workqueue's runlist). I have a commit in progress
adds comments and a KASSERT.
Indeed. Go ahead.
A much cleaner approach would be an API where we can arbitrarily
continuations to a thread with /optional/ arguments, and a way to
wakeup a thread without having to implement the locking in the
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.
I don't think the problem isn't the optional argument; it's that
workqueue uses struct work to chain the pending requests.
So we are moving to a world where allocation of memory in interrupt
context is basically not allowed? I think then we need a primitive
which is like workqueue but only allows it to be signalled, and has
argument, basically a way to request the allocate_stuff() function to
It is allowed, but for that we have to use allocators like
pool_cache(9). And like all "high priority" allocations they can fail,
so the handler has to cope with this. Last time I checked OpenBSD's
bnx(4) driver this code path was not needed, because of the way their
workqueue implementation works.
So this would be workqueue, minus the queue, and hence minus struct
work. Perhaps that's where Matt ended up.
Very likely. IMHO workqueues, callouts and softint scheduling have the
same idea behind: pass down work to be processed in another context. So
they follow the same logic behind, perhaps a general implementation
could be factored out.
Main Index |
Thread Index |