[illumos-Developer] initial review: bombproof taskq dispatch

[Garrett D'Amore]

On Fri, 2011-07-08 at 07:57 -0700, George Wilson wrote:
> 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?

Growing the taskq_ent structure may have a negative impact on memory, or
on kmem related performance.  I was trying to avoid changing the size,
as it isn't strictly necessary for this round of changes.

Actually, as I went back through the code, PREPOPULATED is used, instead
of PREALLOCATED... so there "shouldn't" be confusion.

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

It would be evil for someone to pass in such a structure.  I presume
that these things have to be zeroed out by the caller before passing in.

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

Its absolutely true.  The thing is that function that is being called by
the taskq might actually *free* the enclosing structure.  That's the
best way to deal with it, at any rate.  So calling the task function
hands ownership of the taskq_ent back to the caller.

If we had explicit alloc/free routines that were used, then your
approach would work, but I'm loathe to require that as a big part of the
win here is avoiding extra calls into kmem.

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

That could be done, yes.  I'm ok with such a function, though I see no
need for it now.  I like that the caller can just pass in a zero'ed
structure without a need to initialize it otherwise.  This works nicely
since most state structures are kmem_zalloc'd anyway.

I'm going to send out an updated webrev shortly, stay tuned.

	- Garrett