jean-Yves Migeon <jym%NetBSD.org@localhost> writes: > 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 the 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 >> work is 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). Understood, and that seems fine. > For obvious reasons, I could not defer allocation to a thread > context while having to allocate a work struct from handler > (chicken/egg problem). 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 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. I saw that it wasn't used, but subr_workqueue will, if workqueue_enqueue 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 request 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 that adds comments and a KASSERT. > 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. 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 no argument, basically a way to request the allocate_stuff() function to be run. So this would be workqueue, minus the queue, and hence minus struct work. Perhaps that's where Matt ended up.
Attachment:
pgpA3kFVJxMDJ.pgp
Description: PGP signature