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

[Peter Williams <pwil3058@bigpond.net.au>]

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.

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

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

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

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce