Re: [PATCH 00/11] Task watchers: Introduction

[Matt Helsley]

On Thu, 2006-06-22 at 11:11 +1000, Peter Williams wrote:
> Matt Helsley wrote:
> > On Thu, 2006-06-22 at 09:04 +1000, Peter Williams wrote:
> >> Matt Helsley wrote:
> >>> On Wed, 2006-06-21 at 21:41 +1000, Peter Williams wrote:
> >>>> Peter Williams wrote:
> >>>>> Matt Helsley wrote:
> >>>>>> On Wed, 2006-06-21 at 15:41 +1000, Peter Williams wrote:
> >>>>>>> On a related note, I can't see where the new task's notify field gets 
> >>>>>>> initialized during fork.
> >>>>>> It's initialized in kernel/sys.c:notify_per_task_watchers(), which calls
> >>>>>> RAW_INIT_NOTIFIER_HEAD(&task->notify) in response to WATCH_TASK_INIT.
> >>>>> I think that's too late.  It needs to be done at the start of 
> >>>>> notify_watchers() before any other watchers are called for the new task.
> >>> 	I don't see why you think it's too late. It needs to be initialized
> >>> before it's used. Waiting until notify_per_task_watchers() is called
> >>> with WATCH_TASK_INIT does this.
> >> I probably didn't understand the code well enough.  I'm still learning 
> >> how it all hangs together :-).
> >>
> >>>> On second thoughts, it would simpler just before the WATCH_TASK_INIT 
> >>>> call in copy_process() in fork.c.  It can be done unconditionally there.
> >>>>
> >>>> Peter
> >>> 	That would work. It would not simplify the control flow of the code.
> >>> The branch for WATCH_TASK_INIT in notify_per_task_watchers() is
> >>> unavoidable; we need to call the parent task's chain in that case since
> >>> we know the child task's is empty.
> >>>
> >>> 	It is also counter to one goal of the patches -- reducing the "clutter"
> >>> in these paths. Arguably task watchers is the same kind of clutter that
> >>> existed before. However, it is a means of factoring such clutter into
> >>> fewer instances (ideally one) of the pattern.
> >> Maybe a few comments in the code to help reviewers such as me learn how 
> >> it works more quickly would be worthwhile.
> > 
> > Good point. I'll keep this in mind as I consider the multi-chain
> > approach suggested by Andrew -- I suspect improvments in my commenting
> > will be even more critical there.
> > 
> >> BTW as a former user of PAGG, I think there are ideas in the PAGG 
> >> implementation that you should look at.  In particular:
> >>
> >> 1. The use of an array of function pointers (one for each hook) can cut 
> >> down on the overhead.  The notifier_block only needs to contain a 
> >> pointer to the array so there's no increase in the size of that 
> >> structure.  Within the array a null pointer would mean "don't bother 
> >> calling".  Only one real array needs to exist even for per task as 
> >> they're all using the same functions (just separate data).  It removes 
> >> the need for a switch statement in the client's function as well as 
> >> saving on unnecessary function calls.
> > 
> > 	I don't think having an explicit array of function pointers is likely
> > to be as fast as a switch statement (or a simple branch) generated by
> > the compiler.
> 
> With the array there's no need for any switch or branching.  You know 
> exactly which function in the array to use in each hook.

	I don't forsee enough of a difference to make this worth arguing about.
You're welcome to benchmark and compare arrays vs. switches/branches on
a variety of archs, SMP boxen, NUMA boxen, etc. and post the results.
I'm going to focus on other issues for now.

> > 
> > 	It doesn't save unecessary function calls unless I modify the core
> > notifier block structure. Otherwise I still need to stuff a generic
> > function into .notifier_call and from there get the pointer to the array
> > to make the next call. So it adds more pointer indirection but does not
> > reduce the number of intermediate function calls.
> 
> There comes a point when trying to reuse existing code is less cost 
> effective than starting over.

Write my own notifier chains just to avoid a function call? I don't
think that's sufficient justification for implementing my own.

> > 
> > 	As far as the multi-chain approach is concerned, I'm still leaning
> > towards registering a single function with a mask describing what it
> > wants to be notified of.
> 
> I think that will be less efficient than the function array.

Well if I don't register with the mask there are other approaches that
wouldn't require the switch or the array.

> > 
> >> 2. A helper mechanism to allow a client that's being loaded as a module 
> >> to visit all existing tasks and do whatever initialization it needs to 
> >> do.  Without this every client would have to implement such a mechanism 
> >> themselves (and it's not pretty).
> > 
> > 	Interesting idea. It should resemble existing macros. Something like:
> > 	register_task_watcher(&my_nb, &unnoticed);
> > 	for_each_unnoticed_task(unnoticed)
> > 		...
> 
> Something like that.  It involved some tricky locking issues and was
> reasonably complex (which made providing it a good option when compared 
> to each client implementing its own version).  Rather than trying to do 
> this from scratch I'd advise getting a copy of the most recent PAGG
> patches and using that as a model as a fair bit of effort was spent 
> ironing out all the problems involved.  It's not as easy as it sounds.

	Yes, it does sound quite hairy.

	My feeling is that such code should be largely independent of task
watchers and I'd like to stay focused. So I have no plans to work on
something like "for_each_unnoticed_task()" for now.

Cheers,
	-Matt Helsley

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/