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

[Garrett D'Amore]

On Jul 25, 2011, at 10:28 PM, Adam Leventhal <ahl at delphix.com> 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.
> 
> Then let's either change them everywhere or be stylistically consistent.

You may note that this is the first prototype in this file, so there is no intra file consistency problem.  Further, the use of extern keywords on prototypes is not at all consistent in the system headers.  So I felt that eliding it here was a reasonable choice.  Had there already been a preexisting style within this file, I would either have followed it or changed the other prototypes.

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

No problem, thanks again!  I will file the RTI tommorrow.

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