From: Peter W. <pwi...@bi...> - 2006-06-22 01:11:15
|
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 pwi...@bi... "Learning, n. The kind of ignorance distinguishing the studious." -- Ambrose Bierce |