lkml.org 
[lkml]   [2006]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 00/11] Task watchers: Introduction
Matt Helsley wrote:
> 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.

Can't help thinking why the easier option of adding setuid and setgid
hooks to PAGG and then including PAGG wasn't adopted.

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

Yes, this won't be an issue until there's a client in a loadable module.

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

"Learning, n. The kind of ignorance distinguishing the studious."
-- Ambrose Bierce
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-06-22 06:29    [W:0.088 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site