[illumos-Developer] initial review: bombproof taskq dispatch

[George Wilson]

On 7/7/11 9:47 AM, Albert Lee wrote:
> On Thu, Jul 7, 2011 at 11:58 AM, Garrett D'Amore<garrett at damore.org>  wrote:
>> On Jul 7, 2011, at 7:21 AM, George Wilson<gwilson at zfsmail.com>  wrote:
>>
>>> Garrett,
>>>
>>> A few comments:
>>>
>>> - With your change do we still need to use TASKQ_PREPOPULATE in spa_taskq_create()?
>> Probably not, at least not in the kernel build.  Userland might still need or want it.
>>
>>> - I would really love for the #ifdef _KERNEL lines to go away before integrating this, any chance that can happen? Let me know if you need help with that.
>> In order for that to happen, I'd have to implement the taskq_dispatch_ent() for user space in libzpool.  I guess I can make those changes.  I despise the _KERNEL ifdefs too.
>>
>>> - In taskq_dispatch(), lines 1136 - 1162 is it possible to make this a common routine so that both taskq_dispatch() and taskq_dispatch_ent() can both use?
>> I thought about this, but ultimately I decided it was pointless, and would lengthen code paths.  For example, random dispatch failures should never happen for a taskq_dispatch_ent(), since that cannot (by definition) ever fail.  I decided it was ultimately cleanest and simplest to just add the dozen or so new lines of code.  Really the only code that could be shared is the code from 1299 to 1303.  The rest is unique to the specific version.  Not worth adding extra branches to share 4 lines of code, IMO.
>>
>>> - In taskq_thread(), line# 1582, this comment is confusing since we don't really talk about prealloc'd tasks and may get confused with prealloced taskqs which are mentioned in this file.
>> Agreed the terminology is confusing.  Any naming suggestions?
>>
> "static tasks"? Although that wouldn't necessarily be correct... more
> "user allocated/provided/managed".

What if we used TQE_TASKQ_MANAGED vs TQE_USER_MANAGED (feel free to 
change the names). Then instead of lines 1152-1154 in taskq_dispatch() 
you could set TQE_TASKQ_MANAGED at line #1009. This also gives you the 
ability to ASSERT at line#1287 that tqe is not TQE_TASKQ_MANAGED. I'm 
going to assume that you would need to change taskq_ent_t so that 
tqent_flags are no longer part of the union. What do you think?

In taskq_dispatch_ent(), line #1293, this should probably not be an 
or-ed in value just in case someone were to pass in a tqe which has a 
valid tqent_bucket then it would end up pointing to the wrong location. 
This also goes away if tqent_flags are no longer part of a union.

One more question, the comment says that we can't touch the tqent after 
this point, can you explain that further? Is this only true for the user 
managed taskq_ent?  The reason I ask is I was wondering if this logic 
could actually live in taskq_ent_free()?

>>> I have to admit that this interface is a little funky and I'm wondering if we shouldn't try to better define this. Maybe something like taskq_ent_create() and taskq_ent_destroy() that can be called by external consumers?
>> As indiciated earlier, I prefer to have the option to declare a taskq_ent inline in a structure.  That way we avoid having to go to the kmem allocator, ever.   Its a big performance win on hot code paths.
>>
> If we have cache for them it won't be as bad... but I do sort of like
> the simplicity of having taskq_ent exposed.
>
We could have something like taskq_ent_init() to do something like this:

static void
taskq_ent_init(taskq_ent_t *tqe)
{
     if (tqe == NULL)
           allocate_tqe_from_cache

    tqe->tqent_flag = TQE_USER_MANAGED;
}

Or something like that. I'm fine either way.

- George

- George