[illumos-Developer] Final (?) review for light-weight task dispatch

[Garrett D'Amore]

On Sun, 2011-07-24 at 17:51 -0700, Adam Leventhal wrote:
> Hey Garrett,
> 
> usr/src/uts/common/sys/taskq_impl.h
> 153: missing extern prefix

Nope.  Function prototypes are *always* extern unless explicitly
declared static.  In fact, all the extra instances of "extern" scattered
around headers is one of my personal pet peeves.

> should taskq_dispatch_ent() return a taskqid_t like taskq_dispatch()?

I didn't think this was particularly useful.  taskq_dispatch() really
just uses its return value for success or failure, and
taskq_dispatch_ent cannot fail.  (There are external functions that take
a taskq_id_t, so the value is really not useful to the caller except to
test against 0 for failure.)

> did you think about making a #define for tqent_un.tqent_bucket or did
> you want to force yourself to consider all referring places?

I wanted to find all the places to make sure I got them all, and that
the use of the union here was safe.  Call me paranoid. :-)

> 
> While the code looks good, what I think is missing is an update to the
> big theory statement at the top of usr/src/uts/common/os/taskq.c. It
> would be good to update that to include some discussion of this new
> mechanism you've added.

I added this:

 * void taskq_dispatch_ent(tq, func, arg, flags, tqent)
 *
 *	This is a light-weight form of taskq_dispatch(), that uses a
 *	preallocated taskq_ent_t structure for scheduling.  As a
 *	result, it does not perform allocations and cannot ever fail.
 *	Note especially that it cannot be used with TASKQ_DYNAMIC
 *	taskqs.  The memory for the tqent must not be modified or used
 *	until the function (func) is called.  (However, func itself
 *	may safely modify or free this memory, once it is called.)
 *	Note that the taskq framework will NOT free this memory.

Is that sufficient?

Thanks for the review!

	- Garrett

> 
> Adam
> 
> On Tue, Jul 19, 2011 at 2:14 PM, Garrett D'Amore <garrett at damore.org> wrote:
> > The review is here:
> > http://mexico.purplecow.org/gdamore/webrev/taskq-redux/
> >
> > This is the fixes for
> >
> > 734 taskq_dispatch_prealloc() desired
> > 943 zio_interrupt ends up calling taskq_dispatch with TQ_SLEEP
> >
> >
> > The main difference with earlier reviews is that I've considered
> > George's feedback, and have implemented "userland" equivalent support
> > taskq_dispatch_ent().  (Sorry it took so long -- I was hitting an
> > assertion in zfstest, which I thought was an indication that my
> > assumptions about zio scheduling were wrong.  Turns out that no, it was
> > just an annoying use-after-free bug in my userland implementation of
> > taskq_dispatch_ent.)
> >
> > So now this code has also been tested with ztest.  Passes beautifully.
> > And it has fewer #ifdef KERNEL hacks than the prior version.  Yay.
> >
> > I did not try to change the API to support dynamic allocation of
> > taskq_ent_t's; I view that as a potential future enhancement, and
> > probably something that certainly ought to be done as part of any future
> > work to raise this to a public API.  I also kept the TQENT_FLAG_PREALLOC
> > name (it turns out the supposed collision was with TASKQ_PREPOPULATE),
> > and I left it in the union.  Mostly because I did not want to have to
> > redo the performance tests -- which I would have if I had increased the
> > size of the taskq_ent_t structure. (There has been significant
> > performance testing done on this code, already.)
> >
> > Anyway, I'm looking forward to feedback... especially George, if you can
> > review the new changes, that would be awesome.  I'd like a very short
> > timer on this if possible, because I have another change that I'd like
> > to integrate which depends on these changes.  And trust me, you *want*
> > that fix if you use COMSTAR. :-)
> >
> >        - Garrett
> >
> >
> > _______________________________________________
> > Developer mailing list
> > Developer at lists.illumos.org
> > http://lists.illumos.org/m/listinfo/developer
> >
> 
> 
>