lkml.org 
[lkml]   [2006]   [Jun]   [21]   [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 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
    -
    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 03:13    [W:0.036 / U:0.172 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site