lkml.org 
[lkml]   [2008]   [Sep]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 1/2] utrace core
    Date
    > 1. UTRACE_ATTACH_MATCH_DATA is not used.

    I don't understand this comment. The patch adds a new API, not users of
    the API. That flag is implemented and documented.

    > 2. s/utrace_attached_engine/utrace_engine/g
    >
    > Engine which is not attached is either just allocated or scheduled
    > for freeing

    In the way I've talked about things since the beginning, and probably in
    some email and maybe some of the documentation text, I've used "a tracing
    engine" to mean the whole coordinated body of code. (There is no data
    structure in the utrace API that corresponds to this, it's just a way of
    talking about the "user of utrace" code.) So you might talk about having
    your "engine" attach to many tasks. This is what I had in mind when I
    first made it 'struct utrace_attached_engine' for the API object that's
    about one attachment.

    But I won't quibble about names. I am happy to change any of the names to
    whatever the consensus prefers.

    > 3. get_utrace_lock() is funny.
    >
    > From name one should get valid struct utrace or -E depending only on
    > task, why engine matters is a mystery.

    This "mystery" is addressed in the comments and the documentation.
    (Comments above the function says exactly what it does.) Details about
    why we do that are in manual section called "Engine and task pointers"
    (in the file added by the patch, Documentation/DocBook/utrace.tmpl).
    If you don't like to read the XML or wait for 'make htmldocs', I've put
    a copy of the formatted tree at:
    http://people.redhat.com/roland/utrace/DocBook/

    > 4. subsys_initcall
    >
    > Just use module_init() or comment why it's needed.

    Since I never found much documentation about these macros, and it's not
    a module, that looked like the one to use based on the other uses in the
    source. If module_init is the generic one to use in nonmodule code,
    that's fine by me.

    > 5. Dummy in examiner -- not used (of course!)

    (Of course, that's why it's called that!) I found I had to add that or
    the kerneldoc magic would freak out. The type name is used by users of
    the API, so I wanted to have it documented with description text, but
    its contents are all private to the utrace implementation. When I had
    no magic /* public: */ comment or no fields declared after it, the 'make
    htmldocs' et al process would barf.
    > Engine grew a refcount.
    >
    > Was it added to fix some bug?
    >
    > Old utrace git doesn't have this, new utrace git has only two
    > posted patches, so asking.

    I said this is a substantially different API from the first utrace
    prototype, and I was not lying. The implementation didn't follow from
    the same tree's history, because it took an entirely different merging
    approach. In the new effort I've been doing since 2.6.25, I have only
    been preparing new versions of patches for posting. (My current GIT
    trees are updated using git-rebase.)

    In the first utrace prototype, every caller of the utrace API had to
    understand some things about RCU and/or some special rules about narrow
    guarantees. It was too easy to write code that wasn't robust when tasks
    died or engines detached. This was a failure at one of the main goals
    of utrace: to make it easier not to write bugs.

    The "Tear-down Races" manual section talks about everything you have to
    do to handle asynchronous changes robustly. It takes some care, but the
    rules to follow are pretty straightforward and don't constrain the
    calling code much (e.g. it can block). It's easy to write sloppy code
    that leaks engine refs. But it's not especially easy to have sloppy
    bugs really louse things up.


    Thanks,
    Roland


    \
     
     \ /
      Last update: 2008-09-03 14:13    [from the cache]
    ©2003-2014 Jasper Spaans. Advertise on this site