[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
>
> 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.

Then let's either change them everywhere or be stylistically consistent.

>> 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?

Looks good.

> Thanks for the review!

Thanks for making the change!

Adam

>        - 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
>> >
>>
>>
>>
>
>
>



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

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