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

[Adam Leventhal]

Hey Garrett,

usr/src/uts/common/sys/taskq_impl.h
153: missing extern prefix
should taskq_dispatch_ent() return a taskqid_t like taskq_dispatch()?
did you think about making a #define for tqent_un.tqent_bucket or did
you want to force yourself to consider all referring places?

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.

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
>



-- 
Adam Leventhal, Delphix
http://dtrace.org/blogs/ahl

275 Middlefield Road, Suite 50
Menlo Park, CA 94025
http://www.delphix.com