Re: [PATCH 1/2] utrace core

[Roland McGrath]

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/