LinuxLists.cc - [RFC,PATCH 14/14] utrace core

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

Oleg Nesterov <[email protected]> writes:

> From: Roland McGrath <[email protected]>
>
> This adds the utrace facility, a new modular interface in the kernel
> for implementing user thread tracing and debugging. This fits on top
> of the tracehook_* layer, so the new code is well-isolated.

Could we just drop the tracehook layer if this finally merged
and call the low level functions directly?

It might have been reasonably early on when it was still out of tree,
but longer term when it's integrated having strange opaque hooks
like that just makes the coder harder to read and maintain.

-Andi

--
[email protected] -- Speaking for myself only.

2009-11-24 20:47:15

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On 11/24, Andi Kleen wrote:
>
> Oleg Nesterov <[email protected]> writes:
>
> > From: Roland McGrath <[email protected]>
> >
> > This adds the utrace facility, a new modular interface in the kernel
> > for implementing user thread tracing and debugging. This fits on top
> > of the tracehook_* layer, so the new code is well-isolated.
>
> Could we just drop the tracehook layer if this finally merged
> and call the low level functions directly?

Not sure I understand. Tracehooks are trivial inline wrappers on
top utrace calls,

> It might have been reasonably early on when it was still out of tree,
> but longer term when it's integrated having strange opaque hooks
> like that just makes the coder harder to read and maintain.

Well, I don't think the code will be better if we remove tracehooks.

For example. tracehook_report_syscall_entry() has a lot of callers
in arch/, each callsite should be changed to do

if ((task_utrace_flags(current) & UTRACE_EVENT(SYSCALL_ENTRY)) &&
utrace_report_syscall_entry(regs))
ret = -1; // this depends on machine

instead of simply calling tracehook_report_syscall_entry().

What is the point?

But again, perhaps I misunderstood you.

Oleg.

2009-11-24 21:26:15

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On Tue, Nov 24, 2009 at 09:41:52PM +0100, Oleg Nesterov wrote:
> On 11/24, Andi Kleen wrote:
> >
> > Oleg Nesterov <[email protected]> writes:
> >
> > > From: Roland McGrath <[email protected]>
> > >
> > > This adds the utrace facility, a new modular interface in the kernel
> > > for implementing user thread tracing and debugging. This fits on top
> > > of the tracehook_* layer, so the new code is well-isolated.
> >
> > Could we just drop the tracehook layer if this finally merged
> > and call the low level functions directly?
>
> Not sure I understand. Tracehooks are trivial inline wrappers on
> top utrace calls,

Yes that's the problem -- they are unnecessary obfuscation
when you can just call directly.

>
> > It might have been reasonably early on when it was still out of tree,
> > but longer term when it's integrated having strange opaque hooks
> > like that just makes the coder harder to read and maintain.
>
> Well, I don't think the code will be better if we remove tracehooks.
>
> For example. tracehook_report_syscall_entry() has a lot of callers
> in arch/, each callsite should be changed to do
>
> if ((task_utrace_flags(current) & UTRACE_EVENT(SYSCALL_ENTRY)) &&
> utrace_report_syscall_entry(regs))
> ret = -1; // this depends on machine
>
> instead of simply calling tracehook_report_syscall_entry().

That should be in the utrace code?

I don't have a problem with having common code somewhere,
just not a whole layer whose only purpose seems to be obfuscation.

> What is the point?

Less code obfuscation.

When it's a utrace call, call it a utrace call, not something else.

-Andi
--
[email protected] -- Speaking for myself only.

2009-11-24 21:31:13

by Frank Ch. Eigler

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

Hi -

On Tue, Nov 24, 2009 at 10:26:19PM +0100, Andi Kleen wrote:
> [...]
> > For example. tracehook_report_syscall_entry() has a lot of callers
> > in arch/, each callsite should be changed to do
> >
> > if ((task_utrace_flags(current) & UTRACE_EVENT(SYSCALL_ENTRY)) &&
> > utrace_report_syscall_entry(regs))
> > ret = -1; // this depends on machine
> >
> > instead of simply calling tracehook_report_syscall_entry().
>
> That should be in the utrace code?
>
> I don't have a problem with having common code somewhere,
> just not a whole layer whose only purpose seems to be obfuscation.

One man's obfuscation is another man's abstraction.
Would you be satisfied if "tracehook_" was renamed "utracehook_"?

- FChE

2009-11-24 21:34:20

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

> One man's obfuscation is another man's abstraction.
> Would you be satisfied if "tracehook_" was renamed "utracehook_"?

I would prefer if there were no thin inlines, and the functions should
have a name clearly indicating for which subsystem they are for.

-Andi

--
[email protected] -- Speaking for myself only.

2009-11-24 21:50:43

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

(restore cc's)

On 11/24, Andi Kleen wrote:
>
> On Tue, Nov 24, 2009 at 09:41:52PM +0100, Oleg Nesterov wrote:
> > On 11/24, Andi Kleen wrote:
> > >
> > > Oleg Nesterov <[email protected]> writes:
> > >
> > > > From: Roland McGrath <[email protected]>
> > > >
> > > > This adds the utrace facility, a new modular interface in the kernel
> > > > for implementing user thread tracing and debugging. This fits on top
> > > > of the tracehook_* layer, so the new code is well-isolated.
> > >
> > > Could we just drop the tracehook layer if this finally merged
> > > and call the low level functions directly?
> >
> > Not sure I understand. Tracehooks are trivial inline wrappers on
> > top utrace calls,
>
> Yes that's the problem -- they are unnecessary obfuscation
> when you can just call directly.

This is subjective, but personally I disagree. Contrary, imho it
is good that tracehook hides the (simple) details. I do not understand
why the reader of, say, do_fork() should see the contents of
tracehook_report_clone_complete(). This will complicate the understanding.
Those people who want to understand/change fork() do not care about
utrace/ptrace usually.

And please note that it is much, much easier to change this code
when it lives in tracehooks.h instead of sched.c/signal.c/etc.

> > What is the point?
>
> Less code obfuscation.
>
> When it's a utrace call, call it a utrace call, not something else.

Why do you think this is obfuscation? Well, we can rename these
helpers, s/tracehook_/utrace_/, but I don't see how this can make
the code more readable.

Oleg.

2009-11-25 08:46:15

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

> This is subjective, but personally I disagree. Contrary, imho it
> is good that tracehook hides the (simple) details. I do not understand
> why the reader of, say, do_fork() should see the contents of
> tracehook_report_clone_complete(). This will complicate the understanding.

Someone who has to debug or review fork needs to know what's going on.

Yes they can find out by going through inlines, but that
just costs more time and distracts from the actual problem.

> Those people who want to understand/change fork() do not care about
> utrace/ptrace usually.
>
> And please note that it is much, much easier to change this code
> when it lives in tracehooks.h instead of sched.c/signal.c/etc.

The problem is that when you have to trace this code when something
goes wrong the extra layer just holds you up. For debugging usually
abstraction is a bad idea.

My experience is also that in general such extra "abstraction layers"
are frowned upon in Linux kernel code style. For example when new
vendor drivers are submitted for hardware like NICs etc,
they frequently tend to have all kinds of "abstraction layers".
Typically the first step to linuxify them is to get rid of those.

This makes the code more readable, shorter, better to debug and read.

Another classic example is: lock_foo() is frowned upon (Linus
tends to always complain about that), rather prefer spin_lock(&foo_lock)
or mutex_lock(&foo_lock) that makes it clear what's going on.

I don't see why this should be any different for utrace.

> > Less code obfuscation.
> >
> > When it's a utrace call, call it a utrace call, not something else.
>
> Why do you think this is obfuscation? Well, we can rename these
> helpers, s/tracehook_/utrace_/, but I don't see how this can make
> the code more readable.

Because the inlines do not add anything to functionality and actually
hide what the code does, that is obfuscation. For you it might be obvious
because you've been hacking that code for quite some time, but for
someone who is not in your position that's different.

-Andi

--
[email protected] -- Speaking for myself only.

2009-11-25 15:01:33

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On 11/25, Andi Kleen wrote:
>
> > This is subjective, but personally I disagree. Contrary, imho it
> > is good that tracehook hides the (simple) details. I do not understand
> > why the reader of, say, do_fork() should see the contents of
> > tracehook_report_clone_complete(). This will complicate the understanding.
>
> Someone who has to debug or review fork needs to know what's going on.
>
> Yes they can find out by going through inlines, but that
> just costs more time and distracts from the actual problem.
>
> > Those people who want to understand/change fork() do not care about
> > utrace/ptrace usually.
> >
> > And please note that it is much, much easier to change this code
> > when it lives in tracehooks.h instead of sched.c/signal.c/etc.
>
> The problem is that when you have to trace this code when something
> goes wrong the extra layer just holds you up. For debugging usually
> abstraction is a bad idea.
>
> My experience is also that in general such extra "abstraction layers"
> are frowned upon in Linux kernel code style. For example when new
> vendor drivers are submitted for hardware like NICs etc,
> they frequently tend to have all kinds of "abstraction layers".
> Typically the first step to linuxify them is to get rid of those.
>
> This makes the code more readable, shorter, better to debug and read.

OK, let's try to remove these helpers. Let's take a random one,
tracehook_report_exec().

The current code in search_binary_handler:

if (retval >= 0) {
if (depth == 0)
tracehook_report_exec(fmt, bprm, regs);
put_binfmt(fmt);
allow_write_access(bprm->file);
if (bprm->file)
fput(bprm->file);
bprm->file = NULL;
current->did_exec = 1;
proc_exec_connector(current);
return retval;
}

becomes:

if (retval >= 0) {
if (depth == 0) {
if (unlikely(task_utrace_flags(current) & UTRACE_EVENT(EXEC)))
utrace_report_exec(fmt, bprm, regs);
if (!ptrace_event(PT_TRACE_EXEC, PTRACE_EVENT_EXEC, 0) &&
unlikely(task_ptrace(current) & PT_PTRACED))
send_sig(SIGTRAP, current, 0);
}
put_binfmt(fmt);
allow_write_access(bprm->file);
if (bprm->file)
fput(bprm->file);
bprm->file = NULL;
current->did_exec = 1;
proc_exec_connector(current);
return retval;
}

Cleanup? I don't think so.

OK, when CONFIG_UTRACE goes away, we can kill a lot of old code, and
in tracehooks too. So the code above becomes

if (retval >= 0) {
if (depth == 0) {
if (unlikely(task_utrace_flags(current) & UTRACE_EVENT(EXEC)))
utrace_report_exec(fmt, bprm, regs);
}
put_binfmt(fmt);
allow_write_access(bprm->file);
if (bprm->file)
fput(bprm->file);
bprm->file = NULL;
current->did_exec = 1;
proc_exec_connector(current);
return retval;
}

Much better. But in this case please note that most of tracehooks
just do:

if (unlikely(task_utrace_flags(current) & SOME_EVENT))
utrace_report_some_event();

I really don't understand why we shouldn't have (trivial!) helpers
for this. (As for naming - personally I do not care at all ;)

You can argue that some tracehooks (say, exit_notify() path) can be
simplified. Yes, we are going to do this. And again, when CONFIG_UTRACE
goes away, we can just kill some tracehooks. Say, most of them in
do_fork() path.

> Because the inlines do not add anything to functionality and actually
> hide what the code does, that is obfuscation.

This applies to any function. As for tracehooks, they mostly hide
"if (task_utrace_flags(current))" check and nothing more.

> For you it might be obvious
> because you've been hacking that code for quite some time, but for
> someone who is not in your position that's different.

Yes, this is true. Let me repeat, I know that this is subjective and
I am biased.

Oleg.

2009-11-25 16:00:48

by Ingo Molnar

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

* Oleg Nesterov <[email protected]> wrote:

> Much better. But in this case please note that most of tracehooks just
> do:
>
> if (unlikely(task_utrace_flags(current) & SOME_EVENT))
> utrace_report_some_event();
>
> I really don't understand why we shouldn't have (trivial!) helpers for
> this. (As for naming - personally I do not care at all ;)

We prefer helpers in most such cases - especially when in the normal
case the helper has no side effects - as here. Then we want to compress
all such reporting/callback as much as possible.

Using helpers to abstract away functionality is one of the basic
elements of writing clean kernel code.

Ingo

2009-11-25 21:50:29

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On Tue, Nov 24, 2009 at 09:32:27PM +0100, Andi Kleen wrote:
> Oleg Nesterov <[email protected]> writes:
>
> > From: Roland McGrath <[email protected]>
> >
> > This adds the utrace facility, a new modular interface in the kernel
> > for implementing user thread tracing and debugging. This fits on top
> > of the tracehook_* layer, so the new code is well-isolated.
>
> Could we just drop the tracehook layer if this finally merged
> and call the low level functions directly?
>
> It might have been reasonably early on when it was still out of tree,
> but longer term when it's integrated having strange opaque hooks
> like that just makes the coder harder to read and maintain.

I agree that linux/tracehook.h as a separate layer is pretty annoying,
but a lot of the helper actually are quite useful. I'd suggest to throw
a patch ontop of the series to rename all the useful macros to utrace_
and move them into utrace.h and just remove those that are really
trivial wrappers.

2009-12-01 20:14:38

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On Tue, 2009-11-24 at 21:02 +0100, Oleg Nesterov wrote:

> + <sect2 id="reap"><title>Final callbacks</title>
> + <para>
> + The <function>report_reap</function> callback is always the final event
> + in the life cycle of a traced thread. Tracing engines can use this as
> + the trigger to clean up their own data structures. The
> + <function>report_death</function> callback is always the penultimate
> + event a tracing engine might see; it's seen unless the thread was
> + already in the midst of dying when the engine attached. Many tracing
> + engines will have no interest in when a parent reaps a dead process,
> + and nothing they want to do with a zombie thread once it dies; for
> + them, the <function>report_death</function> callback is the natural
> + place to clean up data structures and detach. To facilitate writing
> + such engines robustly, given the asynchrony of
> + <constant>SIGKILL</constant>, and without error-prone manual
> + implementation of synchronization schemes, the
> + <application>utrace</application> infrastructure provides some special
> + guarantees about the <function>report_death</function> and
> + <function>report_reap</function> callbacks. It still takes some care
> + to be sure your tracing engine is robust to tear-down races, but these
> + rules make it reasonably straightforward and concise to handle a lot of
> + corner cases correctly.
> + </para>
> + </sect2>

This above text seems inconsistent. First you say report_reap() is the
final callback and should be used for cleanup, then you say
report_death() is the penultimate callback but might not always happen
and people would normally use that as cleanup.

If we cannot rely on report_death() then I would suggest to simply
recommend report_reap() as cleanup callback and leave it at that.

> + <para>
> + There is nothing a kernel module can do to keep a <structname>struct
> + task_struct</structname> alive outside of
> + <function>rcu_read_lock</function>.

Sure there is, get_task_struct() comes to mind.

> When the task dies and is reaped
> + by its parent (or itself), that structure can be freed so that any
> + dangling pointers you have stored become invalid.
> + <application>utrace</application> will not prevent this, but it can
> + help you detect it safely. By definition, a task that has been reaped
> + has had all its engines detached. All
> + <application>utrace</application> calls can be safely called on a
> + detached engine if the caller holds a reference on that engine pointer,
> + even if the task pointer passed in the call is invalid. All calls
> + return <constant>-ESRCH</constant> for a detached engine, which tells
> + you that the task pointer you passed could be invalid now. Since
> + <function>utrace_control</function> and
> + <function>utrace_set_events</function> do not block, you can call those
> + inside a <function>rcu_read_lock</function> section and be sure after
> + they don't return <constant>-ESRCH</constant> that the task pointer is
> + still valid until <function>rcu_read_unlock</function>. The
> + infrastructure never holds task references of its own.

And here you imply its existence.

> Though neither
> + <function>rcu_read_lock</function> nor any other lock is held while
> + making a callback, it's always guaranteed that the <structname>struct
> + task_struct</structname> and the <structname>struct
> + utrace_engine</structname> passed as arguments remain valid
> + until the callback function returns.
> + </para>
> +
> + <para>
> + The common means for safely holding task pointers that is available to
> + kernel modules is to use <structname>struct pid</structname>, which
> + permits <function>put_pid</function> from kernel modules. When using
> + that, the calls <function>utrace_attach_pid</function>,
> + <function>utrace_control_pid</function>,
> + <function>utrace_set_events_pid</function>, and
> + <function>utrace_barrier_pid</function> are available.
> + </para>

The above seems weird to me at best... why hold a pid around when you
can keep a ref on the task struct? A pid might get reused leaving you
with a completely different task than you thought you had.

> + </sect2>
> +
> + <sect2 id="reap-after-death">
> + <title>
> + Serialization of <constant>DEATH</constant> and <constant>REAP</constant>
> + </title>
> + <para>
> + The second guarantee is the serialization of
> + <constant>DEATH</constant> and <constant>REAP</constant> event
> + callbacks for a given thread. The actual reaping by the parent
> + (<function>release_task</function> call) can occur simultaneously
> + while the thread is still doing the final steps of dying, including
> + the <function>report_death</function> callback. If a tracing engine
> + has requested both <constant>DEATH</constant> and
> + <constant>REAP</constant> event reports, it's guaranteed that the
> + <function>report_reap</function> callback will not be made until
> + after the <function>report_death</function> callback has returned.
> + If the <function>report_death</function> callback itself detaches
> + from the thread, then the <function>report_reap</function> callback
> + will never be made. Thus it is safe for a
> + <function>report_death</function> callback to clean up data
> + structures and detach.
> + </para>
> + </sect2>

Right, except you cannot generally rely on this report_death() thing, so
a trace engine needs to deal with the report_reap() thing anyway.

> + <sect2 id="interlock"><title>Interlock with final callbacks</title>
> + <para>
> + The final sort of guarantee is that a tracing engine will know for sure
> + whether or not the <function>report_death</function> and/or
> + <function>report_reap</function> callbacks will be made for a certain
> + thread. These tear-down races are disambiguated by the error return
> + values of <function>utrace_set_events</function> and
> + <function>utrace_control</function>. Normally
> + <function>utrace_control</function> called with
> + <constant>UTRACE_DETACH</constant> returns zero, and this means that no
> + more callbacks will be made. If the thread is in the midst of dying,
> + it returns <constant>-EALREADY</constant> to indicate that the
> + <constant>report_death</constant> callback may already be in progress;
> + when you get this error, you know that any cleanup your
> + <function>report_death</function> callback does is about to happen or
> + has just happened--note that if the <function>report_death</function>
> + callback does not detach, the engine remains attached until the thread
> + gets reaped. If the thread is in the midst of being reaped,
> + <function>utrace_control</function> returns <constant>-ESRCH</constant>
> + to indicate that the <function>report_reap</function> callback may
> + already be in progress; this means the engine is implicitly detached
> + when the callback completes. This makes it possible for a tracing
> + engine that has decided asynchronously to detach from a thread to
> + safely clean up its data structures, knowing that no
> + <function>report_death</function> or <function>report_reap</function>
> + callback will try to do the same. <constant>utrace_detach</constant>
> + returns <constant>-ESRCH</constant> when the <structname>struct
> + utrace_engine</structname> has already been detached, but is
> + still a valid pointer because of its reference count. A tracing engine
> + can use this to safely synchronize its own independent multiple threads
> + of control with each other and with its event callbacks that detach.
> + </para>
> +
> + <para>
> + In the same vein, <function>utrace_set_events</function> normally
> + returns zero; if the target thread was stopped before the call, then
> + after a successful call, no event callbacks not requested in the new
> + flags will be made. It fails with <constant>-EALREADY</constant> if
> + you try to clear <constant>UTRACE_EVENT(DEATH)</constant> when the
> + <function>report_death</function> callback may already have begun, if
> + you try to clear <constant>UTRACE_EVENT(REAP)</constant> when the
> + <function>report_reap</function> callback may already have begun, or if
> + you try to newly set <constant>UTRACE_EVENT(DEATH)</constant> or
> + <constant>UTRACE_EVENT(QUIESCE)</constant> when the target is already
> + dead or dying. Like <function>utrace_control</function>, it returns
> + <constant>-ESRCH</constant> when the thread has already been detached
> + (including forcible detach on reaping). This lets the tracing engine
> + know for sure which event callbacks it will or won't see after
> + <function>utrace_set_events</function> has returned. By checking for
> + errors, it can know whether to clean up its data structures immediately
> + or to let its callbacks do the work.
> + </para>
> + </sect2>

Hrmm, better mention this earlier, or at least refer to this when
describing the above cleanup bits.

> +<para>
> + One task can examine another only after a callback in the target task that
> + returns <constant>UTRACE_STOP</constant> so that task will not return to user
> + mode after the safe point. This guarantees that the task will not resume
> + until the same engine uses <function>utrace_control</function>, unless the
> + task dies suddenly. To examine safely, one must use a pair of calls to
> + <function>utrace_prepare_examine</function> and
> + <function>utrace_finish_examine</function> surrounding the calls to
> + <structname>struct user_regset</structname> functions or direct examination
> + of task data structures. <function>utrace_prepare_examine</function> returns
> + an error if the task is not properly stopped and not dead.

'or' dead?

> After a
> + successful examination, the paired <function>utrace_finish_examine</function>
> + call returns an error if the task ever woke up during the examination. If
> + so, any data gathered may be scrambled and should be discarded. This means
> + there was a spurious wake-up (which should not happen), or a sudden death.
> +</para>

> @@ -351,6 +394,10 @@ static inline void tracehook_report_vfor
> */
> static inline void tracehook_prepare_release_task(struct task_struct *task)
> {
> + /* see utrace_add_engine() about this barrier */
> + smp_mb();
> + if (task_utrace_flags(task))
> + utrace_release_task(task);
> }

OK, that seems to properly order ->exit_state vs ->utrace_flags,

This site does:

assign ->state
mb
observe ->utrace_flags

and the other site does:

assign ->utrace_flags
mb
observe ->exit_state

> @@ -560,6 +625,20 @@ static inline void tracehook_report_deat
> int signal, void *death_cookie,
> int group_dead)
> {
> + /*
> + * This barrier ensures that our caller's setting of
> + * @task->exit_state precedes checking @task->utrace_flags here.
> + * If utrace_set_events() was just called to enable
> + * UTRACE_EVENT(DEATH), then we are obliged to call
> + * utrace_report_death() and not miss it. utrace_set_events()
> + * uses tasklist_lock to synchronize enabling the bit with the
> + * actual change to @task->exit_state, but we need this barrier
> + * to be sure we see a flags change made just before our caller
> + * took the tasklist_lock.
> + */
> + smp_mb();
> + if (task_utrace_flags(task) & _UTRACE_DEATH_EVENTS)
> + utrace_report_death(task, death_cookie, group_dead, signal);
> }

I don't think its allowed to pair a mb with a lock-barrier, since the
lock barriers are semi-permeable.

> @@ -589,10 +668,20 @@ static inline void set_notify_resume(str
> * asynchronously, this will be called again before we return to
> * user mode.
> *
> - * Called without locks.
> + * Called without locks. However, on some machines this may be
> + * called with interrupts disabled.
> */
> static inline void tracehook_notify_resume(struct pt_regs *regs)
> {
> + struct task_struct *task = current;
> + /*
> + * This pairs with the barrier implicit in set_notify_resume().
> + * It ensures that we read the nonzero utrace_flags set before
> + * set_notify_resume() was called by utrace setup.
> + */
> + smp_rmb();
> + if (task_utrace_flags(task))
> + utrace_resume(task, regs);
> }

Sending an IPI implies the mb? I seem to remember a discussion on that
subject a while back, maybe add that do
Documentation/memory-barriers.txt

> +/*
> + * Hooks in <linux/tracehook.h> call these entry points to the
> + * utrace dispatch. They are weak references here only so
> + * tracehook.h doesn't need to #ifndef CONFIG_UTRACE them to
> + * avoid external references in case of unoptimized compilation.
> + */
> +void utrace_free_task(struct task_struct *)
> + __attribute__((weak));
> +bool utrace_interrupt_pending(void)
> + __attribute__((weak));
> +void utrace_resume(struct task_struct *, struct pt_regs *)
> + __attribute__((weak));
> +void utrace_finish_stop(void)
> + __attribute__((weak));
> +int utrace_get_signal(struct task_struct *, struct pt_regs *,
> + siginfo_t *, struct k_sigaction *)
> + __attribute__((weak));
> +void utrace_report_clone(unsigned long, struct task_struct *)
> + __attribute__((weak));
> +void utrace_finish_vfork(struct task_struct *)
> + __attribute__((weak));
> +void utrace_report_exit(long *exit_code)
> + __attribute__((weak));
> +void utrace_report_death(struct task_struct *, struct utrace *, bool, int)
> + __attribute__((weak));
> +void utrace_report_jctl(int notify, int type)
> + __attribute__((weak));
> +void utrace_report_exec(struct linux_binfmt *, struct linux_binprm *,
> + struct pt_regs *regs)
> + __attribute__((weak));
> +bool utrace_report_syscall_entry(struct pt_regs *)
> + __attribute__((weak));
> +void utrace_report_syscall_exit(struct pt_regs *)
> + __attribute__((weak));
> +void utrace_signal_handler(struct task_struct *, int)
> + __attribute__((weak));

Can the compiler optimize away the callsite using weak functions like
this? If not I think the normal static inline stubs make more sense.

Also, what unoptimized compilation?

> +static inline struct utrace *task_utrace_struct(struct task_struct *task)
> +{
> + struct utrace *utrace;
> +
> + /*
> + * This barrier ensures that any prior load of task->utrace_flags
> + * is ordered before this load of task->utrace. We use those
> + * utrace_flags checks in the hot path to decide to call into
> + * the utrace code. The first attach installs task->utrace before
> + * setting task->utrace_flags nonzero, with a barrier between.
> + * See utrace_task_alloc().
> + */
> + smp_rmb();
> + utrace = task->utrace;
> +
> + smp_read_barrier_depends(); /* See utrace_task_alloc(). */
> + return utrace;
> +}

I spot two barriers here, but only 1 over in utrace_task_alloc(), hmm?

> +/*
> + * Version number of the API defined in this file. This will change
> + * whenever a tracing engine's code would need some updates to keep
> + * working. We maintain this here for the benefit of tracing engine code
> + * that is developed concurrently with utrace API improvements before they
> + * are merged into the kernel, making LINUX_VERSION_CODE checks unwieldy.
> + */
> +#define UTRACE_API_VERSION 20090421

I don't think this is desired. Out-of-tree stuff just doesn't exist,
plain pure and simple.

> +enum utrace_resume_action {
> + UTRACE_STOP,
> + UTRACE_INTERRUPT,
> + UTRACE_REPORT,
> + UTRACE_SINGLESTEP,
> + UTRACE_BLOCKSTEP,
> + UTRACE_RESUME,
> + UTRACE_DETACH
> +};
> +#define UTRACE_RESUME_MASK 0x07

> +enum utrace_signal_action {
> + UTRACE_SIGNAL_DELIVER = 0x00,
> + UTRACE_SIGNAL_IGN = 0x10,
> + UTRACE_SIGNAL_TERM = 0x20,
> + UTRACE_SIGNAL_CORE = 0x30,
> + UTRACE_SIGNAL_STOP = 0x40,
> + UTRACE_SIGNAL_TSTP = 0x50,
> + UTRACE_SIGNAL_REPORT = 0x60,
> + UTRACE_SIGNAL_HANDLER = 0x70
> +};
> +#define UTRACE_SIGNAL_MASK 0xf0
> +#define UTRACE_SIGNAL_HOLD 0x100 /* Flag, push signal back on queue. */

> +enum utrace_syscall_action {
> + UTRACE_SYSCALL_RUN = 0x00,
> + UTRACE_SYSCALL_ABORT = 0x10
> +};
> +#define UTRACE_SYSCALL_MASK 0xf0
> +#define UTRACE_SYSCALL_RESUMED 0x100 /* Flag, report_syscall_entry() repeats */

Right, so this lot doesn't seem very consistent.

At the very least I would put signal action and syscall action into
different ranges.

> +/*
> + * Flags for utrace_attach_task() and utrace_attach_pid().
> + */
> +#define UTRACE_ATTACH_CREATE 0x0010 /* Attach a new engine. */
> +#define UTRACE_ATTACH_EXCLUSIVE 0x0020 /* Refuse if existing match. */
> +#define UTRACE_ATTACH_MATCH_OPS 0x0001 /* Match engines on ops. */
> +#define UTRACE_ATTACH_MATCH_DATA 0x0002 /* Match engines on data. */
> +#define UTRACE_ATTACH_MATCH_MASK 0x000f

My OCD tells me these are ordered wrong :-)

> +/**
> + * struct utrace_engine - per-engine structure
> + * @ops: &struct utrace_engine_ops pointer passed to utrace_attach_task()
> + * @data: engine-private &void * passed to utrace_attach_task()
> + * @flags: event mask set by utrace_set_events() plus internal flag bits
> + *
> + * The task itself never has to worry about engines detaching while
> + * it's doing event callbacks. These structures are removed from the
> + * task's active list only when it's stopped, or by the task itself.
> + *
> + * utrace_engine_get() and utrace_engine_put() maintain a reference count.
> + * When it drops to zero, the structure is freed. One reference is held
> + * implicitly while the engine is attached to its task.
> + */
> +struct utrace_engine {
> +/* private: */
> + struct kref kref;
> + void (*release)(void *);
> + struct list_head entry;
> +
> +/* public: */
> + const struct utrace_engine_ops *ops;
> + void *data;
> +
> + unsigned long flags;
> +};

Sorry, the kernel is written in C, not C++.

> +/**
> + * struct utrace_engine_ops - tracing engine callbacks
> + *
> + * Each @report_*() callback corresponds to an %UTRACE_EVENT(*) bit.
> + * utrace_set_events() calls on @engine choose which callbacks will be made
> + * to @engine from @task.
> + *
> + * Most callbacks take an @action argument, giving the resume action
> + * chosen by other tracing engines. All callbacks take an @engine
> + * argument, and a @task argument, which is always equal to @current.

Given that some functions have a lot of arguments (depleting regparam),
isn't it more expensive to push current on the stack than it is to
simply read it again?

> + * For some calls, @action also includes bits specific to that event
> + * and utrace_resume_action() is used to extract the resume action.
> + * This shows what would happen if @engine wasn't there, or will if
> + * the callback's return value uses %UTRACE_RESUME. This always
> + * starts as %UTRACE_RESUME when no other tracing is being done on
> + * this task.
> + *
> + * All return values contain &enum utrace_resume_action bits. For
> + * some calls, other bits specific to that kind of event are added to
> + * the resume action bits with OR. These are the same bits used in
> + * the @action argument.

> + */
> +struct utrace_engine_ops {

> + u32 (*report_signal)(u32 action,
> + struct utrace_engine *engine,
> + struct task_struct *task,
> + struct pt_regs *regs,
> + siginfo_t *info,
> + const struct k_sigaction *orig_ka,
> + struct k_sigaction *return_ka);

> + u32 (*report_clone)(enum utrace_resume_action action,
> + struct utrace_engine *engine,
> + struct task_struct *parent,
> + unsigned long clone_flags,
> + struct task_struct *child);

> +};

Seems inconsistent on u32 vs enum utrace_resume_action.

Why force enum utrace_resume_action into a u32?

> +/**
> + * struct utrace_examiner - private state for using utrace_prepare_examine()
> + *
> + * The members of &struct utrace_examiner are private to the implementation.
> + * This data type holds the state from a call to utrace_prepare_examine()
> + * to be used by a call to utrace_finish_examine().
> + */
> +struct utrace_examiner {
> +/* private: */
> + long state;
> + unsigned long ncsw;
> +};

Again, its not C++, if you want a private state like that, use an opaque
type, like:

struct utrace_examiner;

and only define the thing in utrace.c or something.

> +static inline __must_check int utrace_control_pid(
> + struct pid *pid, struct utrace_engine *engine,
> + enum utrace_resume_action action)
> +{
> + /*
> + * We don't bother with rcu_read_lock() here to protect the
> + * task_struct pointer, because utrace_control will return
> + * -ESRCH without looking at that pointer if the engine is
> + * already detached. A task_struct pointer can't die before
> + * all the engines are detached in release_task() first.
> + */
> + struct task_struct *task = pid_task(pid, PIDTYPE_PID);
> + return unlikely(!task) ? -ESRCH : utrace_control(task, engine, action);
> +}

Is that comment correct? Without rcu_read_lock() the pidhash can change
under our feet and maybe cause funny things?

Same for utrace_*_pid() it seems.

Does pid_task() in generaly rely on havin rcu_read_lock() called? If so,
a change in the implementation could break the assumptions here.

> +/*
> + * Per-thread structure private to utrace implementation.

See, you do know how to do private in C ;-)

> + */
> +struct utrace {
> + spinlock_t lock;
> + struct list_head attached, attaching;
> +
> + struct task_struct *cloning;
> +
> + struct utrace_engine *reporting;
> +
> + enum utrace_resume_action resume:3;
> + unsigned int signal_handler:1;
> + unsigned int vfork_stop:1; /* need utrace_stop() before vfork wait */
> + unsigned int death:1; /* in utrace_report_death() now */
> + unsigned int reap:1; /* release_task() has run */
> + unsigned int pending_attach:1; /* need splice_attaching() */
> +};

Seems inconsistent in the bitfield type, also it feels like that 3 the 7
and the enum should be more tightly integrated, maybe add:

UTRACE_RESUME_MAX

#define UTRACE_RESUME_BITS (ilog2(UTRACE_RESUME_MAX))
#define UTRACE_RESUME_MASK ((1 << UTRACE_RESUME_BITS) - 1)

And pick something consistent: u32, unsigned int or enum
utrace_resume_action.

> +/*
> + * Set up @task.utrace for the first time. We can have races
> + * between two utrace_attach_task() calls here. The task_lock()
> + * governs installing the new pointer. If another one got in first,
> + * we just punt the new one we allocated.
> + *
> + * This returns false only in case of a memory allocation failure.
> + */
> +static bool utrace_task_alloc(struct task_struct *task)
> +{
> + struct utrace *utrace = kmem_cache_zalloc(utrace_cachep, GFP_KERNEL);
> + if (unlikely(!utrace))
> + return false;
> + spin_lock_init(&utrace->lock);
> + INIT_LIST_HEAD(&utrace->attached);
> + INIT_LIST_HEAD(&utrace->attaching);
> + utrace->resume = UTRACE_RESUME;
> + task_lock(task);
> + if (likely(!task->utrace)) {
> + /*
> + * This barrier makes sure the initialization of the struct
> + * precedes the installation of the pointer. This pairs
> + * with smp_read_barrier_depends() in task_utrace_struct().
> + */
> + smp_wmb();
> + task->utrace = utrace;
> + }
> + task_unlock(task);
> + /*
> + * That unlock after storing task->utrace acts as a memory barrier
> + * ordering any subsequent task->utrace_flags store afterwards.
> + * This pairs with smp_rmb() in task_utrace_struct().
> + */
> + if (unlikely(task->utrace != utrace))
> + kmem_cache_free(utrace_cachep, utrace);
> + return true;
> +}

Again, not sure we can pair an UNLOCK-barrier with a RMB. In fact, both
are NOPs on x86.

> +static struct utrace_engine *matching_engine(
> + struct utrace *utrace, int flags,
> + const struct utrace_engine_ops *ops, void *data)
> +{
> + struct utrace_engine *engine;
> + list_for_each_entry(engine, &utrace->attached, entry)
> + if (engine_matches(engine, flags, ops, data))
> + return engine;
> + list_for_each_entry(engine, &utrace->attaching, entry)
> + if (engine_matches(engine, flags, ops, data))
> + return engine;
> + return NULL;
> +}

The function does a search, suggesting the function name ought to have
something like find or search in it.

> +/*
> + * Called without locks, when we might be the first utrace engine to attach.
> + * If this is a newborn thread and we are not the creator, we have to wait
> + * for it. The creator gets the first chance to attach. The PF_STARTING
> + * flag is cleared after its report_clone hook has had a chance to run.
> + */
> +static inline int utrace_attach_delay(struct task_struct *target)
> +{
> + if ((target->flags & PF_STARTING) &&
> + task_utrace_struct(current) &&
> + task_utrace_struct(current)->cloning != target)
> + do {
> + schedule_timeout_interruptible(1);
> + if (signal_pending(current))
> + return -ERESTARTNOINTR;
> + } while (target->flags & PF_STARTING);
> +
> + return 0;
> +}

Quite gross this.. can't we key off the
tracehoook_report_clone_complete() and use a wakeup there?

Does this really need the inline?

Also, that if stmt really wants brackets.

> +/*
> + * Called with utrace->lock held and utrace->reap set.
> + * Notify and clean up all engines, then free utrace.
> + */
> +static void utrace_reap(struct task_struct *target, struct utrace *utrace)
> + __releases(utrace->lock)
> +{
> + struct utrace_engine *engine, *next;
> +
> + /* utrace_add_engine() checks ->utrace_flags != 0 */
> + target->utrace_flags = 0;
> + splice_attaching(utrace);
> +
> + /*
> + * Since we were called with @utrace->reap set, nobody can
> + * set/clear UTRACE_EVENT(REAP) in @engine->flags or change
> + * @engine->ops, and nobody can change @utrace->attached.
> + */
> + spin_unlock(&utrace->lock);
> +
> + list_for_each_entry_safe(engine, next, &utrace->attached, entry) {
> + if (engine->flags & UTRACE_EVENT(REAP))
> + engine->ops->report_reap(engine, target);
> +
> + engine->ops = NULL;
> + engine->flags = 0;
> + list_del_init(&engine->entry);
> +
> + utrace_engine_put(engine);
> + }
> +}

Asymmetric locking like this is really better not done, and looking at
the callsites its really no bother to clean that up, arguably even makes
them saner.

> +/*
> + * Asynchronously mark an engine as being detached.
> + *
> + * This must work while the target thread races with us doing
> + * start_callback(), defined below. It uses smp_rmb() between checking
> + * @engine->flags and using @engine->ops. Here we change @engine->ops
> + * first, then use smp_wmb() before changing @engine->flags. This ensures
> + * it can check the old flags before using the old ops, or check the old
> + * flags before using the new ops, or check the new flags before using the
> + * new ops, but can never check the new flags before using the old ops.
> + * Hence, utrace_detached_ops might be used with any old flags in place.
> + * It has report_quiesce() and report_reap() callbacks to handle all cases.
> + */
> +static void mark_engine_detached(struct utrace_engine *engine)
> +{
> + engine->ops = &utrace_detached_ops;
> + smp_wmb();
> + engine->flags = UTRACE_EVENT(QUIESCE);
> +}

It would be more readable to split that comment into two, one part going
near the wmb, and leaving the top part describing the function, not hte
implementation.

The comments in general tend to be too long and verbose, split them up
in to shorter 3-4 line paragraphs with clear subjects/points.

Furthermore I'd add a function like:

static struct utrace_engine_ops *
get_utrace_ops(struct utrace_engine *engine, unsigned long *flags)
{
*flags = engine->flags;
/*
* This pairs with the barrier in mark_engine_detached().
* It makes sure that we never see the old ops vector with
* the new flags, in case the original vector had no
* report_quiesce.
*/
smp_rmb();
return engine->ops;
}

to take out and explicitly comment that common bit.

Also, I'm not quite sure on why we play so many barrier games, looking
at start_callback() we have 2 barriers in the callback loop, why not a
per engine lock?

> +/*
> + * Get @target to stop and return true if it is already stopped now.
> + * If we return false, it will make some event callback soonish.
> + * Called with @utrace locked.
> + */
> +static bool utrace_do_stop(struct task_struct *target, struct utrace *utrace)
> +{
> + if (task_is_stopped(target)) {
> + /*
> + * Stopped is considered quiescent; when it wakes up, it will
> + * go through utrace_finish_stop() before doing anything else.
> + */
> + spin_lock_irq(&target->sighand->siglock);
> + if (likely(task_is_stopped(target)))
> + __set_task_state(target, TASK_TRACED);
> + spin_unlock_irq(&target->sighand->siglock);
> + } else if (utrace->resume > UTRACE_REPORT) {
> + utrace->resume = UTRACE_REPORT;
> + set_notify_resume(target);
> + }
> +
> + return task_is_traced(target);

You could codify locking assumptions like here using:

lockdep_assert_held(&utrace->lock);

Saves a comment and actually validates the assumption.

> +/*
> + * This is called when there might be some detached engines on the list or
> + * some stale bits in @task->utrace_flags. Clean them up and recompute the
> + * flags. Returns true if we're now fully detached.
> + *
> + * Called with @utrace->lock held, returns with it released.
> + * After this returns, @utrace might be freed if everything detached.
> + */
> +static bool utrace_reset(struct task_struct *task, struct utrace *utrace)
> + __releases(utrace->lock)
> +{
> + struct utrace_engine *engine, *next;
> + unsigned long flags = 0;
> + LIST_HEAD(detached);
> +
> + splice_attaching(utrace);
> +
> + /*
> + * Update the set of events of interest from the union
> + * of the interests of the remaining tracing engines.
> + * For any engine marked detached, remove it from the list.
> + * We'll collect them on the detached list.
> + */
> + list_for_each_entry_safe(engine, next, &utrace->attached, entry) {
> + if (engine->ops == &utrace_detached_ops) {
> + engine->ops = NULL;
> + list_move(&engine->entry, &detached);
> + } else {
> + flags |= engine->flags | UTRACE_EVENT(REAP);
> + }
> + }
> +
> + if (task->exit_state) {
> + /*
> + * Once it's already dead, we never install any flags
> + * except REAP. When ->exit_state is set and events
> + * like DEATH are not set, then they never can be set.
> + * This ensures that utrace_release_task() knows
> + * positively that utrace_report_death() can never run.
> + */
> + BUG_ON(utrace->death);
> + flags &= UTRACE_EVENT(REAP);
> + } else if (!(flags & UTRACE_EVENT_SYSCALL) &&
> + test_tsk_thread_flag(task, TIF_SYSCALL_TRACE)) {
> + clear_tsk_thread_flag(task, TIF_SYSCALL_TRACE);
> + }
> +
> + if (!flags) {
> + /*
> + * No more engines, cleared out the utrace.
> + */
> + utrace->resume = UTRACE_RESUME;
> + utrace->signal_handler = 0;
> + }
> +
> + if (task_is_traced(task) && !(flags & ENGINE_STOP))
> + /*
> + * No more engines want it stopped. Wake it up.
> + */
> + utrace_wakeup(task, utrace);

wants braces.

> + /*
> + * In theory spin_lock() doesn't imply rcu_read_lock().
> + * Once we clear ->utrace_flags this task_struct can go away
> + * because tracehook_prepare_release_task() path does not take
> + * utrace->lock when ->utrace_flags == 0.
> + */
> + rcu_read_lock();
> + task->utrace_flags = flags;
> + spin_unlock(&utrace->lock);
> + rcu_read_unlock();

yuck!

why not simply keep a task reference over the utrace_reset call?

Also, that unlock really wants to be in the caller, things like:

if (reset)
utrace_reset()
else
spin_unlock(&utrace->lock)

are just vile.

> + put_detached_list(&detached);
> +
> + return !flags;
> +}

> +/*
> + * Perform %UTRACE_STOP, i.e. block in TASK_TRACED until woken up.
> + * @task == current, @utrace == current->utrace, which is not locked.
> + * Return true if we were woken up by SIGKILL even though some utrace
> + * engine may still want us to stay stopped.
> + */
> +static void utrace_stop(struct task_struct *task, struct utrace *utrace,
> + enum utrace_resume_action action)
> +{
> +relock:
> + spin_lock(&utrace->lock);
> +
> + if (action < utrace->resume) {
> + /*
> + * Ensure a reporting pass when we're resumed.
> + */
> + utrace->resume = action;
> + if (action == UTRACE_INTERRUPT)
> + set_thread_flag(TIF_SIGPENDING);
> + else
> + set_thread_flag(TIF_NOTIFY_RESUME);
> + }
> +
> + /*
> + * If the ENGINE_STOP bit is clear in utrace_flags, that means
> + * utrace_reset() ran after we processed some UTRACE_STOP return
> + * values from callbacks to get here. If all engines have detached
> + * or resumed us, we don't stop. This check doesn't require
> + * siglock, but it should follow the interrupt/report bookkeeping
> + * steps (this can matter for UTRACE_RESUME but not UTRACE_DETACH).
> + */
> + if (unlikely(!(task->utrace_flags & ENGINE_STOP))) {
> + utrace_reset(task, utrace);
> + if (task->utrace_flags & ENGINE_STOP)
> + goto relock;
> + return;
> + }
> +
> + /*
> + * The siglock protects us against signals. As well as SIGKILL
> + * waking us up, we must synchronize with the signal bookkeeping
> + * for stop signals and SIGCONT.
> + */
> + spin_lock_irq(&task->sighand->siglock);
> +
> + if (unlikely(__fatal_signal_pending(task))) {
> + spin_unlock_irq(&task->sighand->siglock);
> + spin_unlock(&utrace->lock);
> + return;
> + }
> +
> + __set_current_state(TASK_TRACED);
> +
> + /*
> + * If there is a group stop in progress,
> + * we must participate in the bookkeeping.
> + */
> + if (unlikely(task->signal->group_stop_count) &&
> + !--task->signal->group_stop_count)
> + task->signal->flags = SIGNAL_STOP_STOPPED;
> +
> + spin_unlock_irq(&task->sighand->siglock);
> + spin_unlock(&utrace->lock);
> +
> + /*
> + * If ptrace is among the reasons for this stop, do its
> + * notification now. This could not just be done in
> + * ptrace's own event report callbacks because it has to
> + * be done after we are in TASK_TRACED. This makes the
> + * synchronization with ptrace_do_wait() work right.
> + */
> + ptrace_notify_stop(task);

Well, this is a bit disappointing isn't it? So we cannot implement
ptrace on utrace without special purpose hooks?

> + schedule();
> +
> + utrace_finish_stop();
> +
> + /*
> + * While in TASK_TRACED, we were considered "frozen enough".
> + * Now that we woke up, it's crucial if we're supposed to be
> + * frozen that we freeze now before running anything substantial.
> + */
> + try_to_freeze();
> +
> + /*
> + * While we were in TASK_TRACED, complete_signal() considered
> + * us "uninterested" in signal wakeups. Now make sure our
> + * TIF_SIGPENDING state is correct for normal running.
> + */
> + spin_lock_irq(&task->sighand->siglock);
> + recalc_sigpending();
> + spin_unlock_irq(&task->sighand->siglock);
> +}

> +/**

> + */
> +int utrace_control(struct task_struct *target,
> + struct utrace_engine *engine,
> + enum utrace_resume_action action)
> +{
> + struct utrace *utrace;
> + bool reset;
> + int ret;
> +
> + if (unlikely(action > UTRACE_DETACH))
> + return -EINVAL;

If you'd have added UTRACE_RESUME_MAX, you could have avoided the
assumption DETACH is the last one.

> + /*
> + * This is a sanity check for a programming error in the caller.
> + * Their request can only work properly in all cases by relying on
> + * a follow-up callback, but they didn't set one up! This check
> + * doesn't do locking, but it shouldn't matter. The caller has to
> + * be synchronously sure the callback is set up to be operating the
> + * interface properly.
> + */
> + if (action >= UTRACE_REPORT && action < UTRACE_RESUME &&
> + unlikely(!(engine->flags & UTRACE_EVENT(QUIESCE))))
> + return -EINVAL;

If its a programming error, WARN_ON might be appropriate, no point in
being nice about it.

> + utrace = get_utrace_lock(target, engine, true);
> + if (unlikely(IS_ERR(utrace)))
> + return PTR_ERR(utrace);
> +
> + reset = task_is_traced(target);
> + ret = 0;
> +
> + /*
> + * ->exit_state can change under us, this doesn't matter.
> + * We do not care about ->exit_state in fact, but we do
> + * care about ->reap and ->death. If either flag is set,
> + * we must also see ->exit_state != 0.
> + */
> + if (unlikely(target->exit_state)) {
> + ret = utrace_control_dead(target, utrace, action);
> + if (ret) {
> + spin_unlock(&utrace->lock);
> + return ret;
> + }
> + reset = true;
> + }
> +
> + switch (action) {

> + }
> +
> + /*
> + * Let the thread resume running. If it's not stopped now,
> + * there is nothing more we need to do.
> + */
> + if (reset)
> + utrace_reset(target, utrace);
> + else
> + spin_unlock(&utrace->lock);

yuckness..

> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(utrace_control);
> +
> +/**
> + * utrace_barrier - synchronize with simultaneous tracing callbacks
> + * @target: thread to affect
> + * @engine: engine to affect (can be detached)
> + *
> + * This blocks while @target might be in the midst of making a callback to
> + * @engine. It can be interrupted by signals and will return -%ERESTARTSYS.
> + * A return value of zero means no callback from @target to @engine was
> + * in progress. Any effect of its return value (such as %UTRACE_STOP) has
> + * already been applied to @engine.
> + *
> + * It's not necessary to keep the @target pointer alive for this call.
> + * It's only necessary to hold a ref on @engine. This will return
> + * safely even if @target has been reaped and has no task refs.
> + *
> + * A successful return from utrace_barrier() guarantees its ordering
> + * with respect to utrace_set_events() and utrace_control() calls. If
> + * @target was not properly stopped, event callbacks just disabled might
> + * still be in progress; utrace_barrier() waits until there is no chance
> + * an unwanted callback can be in progress.
> + */
> +int utrace_barrier(struct task_struct *target, struct utrace_engine *engine)
> +{
> + struct utrace *utrace;
> + int ret = -ERESTARTSYS;
> +
> + if (unlikely(target == current))
> + return 0;
> +
> + do {
> + utrace = get_utrace_lock(target, engine, false);
> + if (unlikely(IS_ERR(utrace))) {
> + ret = PTR_ERR(utrace);
> + if (ret != -ERESTARTSYS)
> + break;
> + } else {
> + /*
> + * All engine state changes are done while
> + * holding the lock, i.e. before we get here.
> + * Since we have the lock, we only need to
> + * worry about @target making a callback.
> + * When it has entered start_callback() but
> + * not yet gotten to finish_callback(), we
> + * will see utrace->reporting == @engine.
> + * When @target doesn't take the lock, it uses
> + * barriers to order setting utrace->reporting
> + * before it examines the engine state.
> + */
> + if (utrace->reporting != engine)
> + ret = 0;
> + spin_unlock(&utrace->lock);
> + if (!ret)
> + break;
> + }
> + schedule_timeout_interruptible(1);
> + } while (!signal_pending(current));

Seriously ugly, again. Use a wakeup where appropriate.

> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(utrace_barrier);

> +/*
> + * We are now making the report, so clear the flag saying we need one.
> + * When there is a new attach, ->pending_attach is set just so we will
> + * know to do splice_attaching() here before the callback loop.
> + */
> +static enum utrace_resume_action start_report(struct utrace *utrace)
> +{
> + enum utrace_resume_action resume = utrace->resume;
> + if (utrace->pending_attach ||
> + (resume > UTRACE_INTERRUPT && resume < UTRACE_RESUME)) {
> + spin_lock(&utrace->lock);
> + splice_attaching(utrace);
> + resume = utrace->resume;
> + if (resume > UTRACE_INTERRUPT)
> + utrace->resume = UTRACE_RESUME;
> + spin_unlock(&utrace->lock);
> + }
> + return resume;
> +}

Its not entirely clear why we can check pending_attach outside of the
utrace->lock and not be racy.

> +static inline void finish_report_reset(struct task_struct *task,
> + struct utrace *utrace,
> + struct utrace_report *report)
> +{
> + if (unlikely(report->spurious || report->detaches)) {
> + spin_lock(&utrace->lock);
> + if (utrace_reset(task, utrace))
> + report->action = UTRACE_RESUME;
> + }
> +}

More asymmetric locking...

> +static inline void finish_callback_report(struct task_struct *task,
> + struct utrace *utrace,
> + struct utrace_report *report,
> + struct utrace_engine *engine,
> + enum utrace_resume_action action)
> +{
> + /*
> + * If utrace_control() was used, treat that like UTRACE_DETACH here.
> + */
> + if (action == UTRACE_DETACH || engine->ops == &utrace_detached_ops) {
> + engine->ops = &utrace_detached_ops;
> + report->detaches = true;
> + return;
> + }
> +
> + if (action < report->action)
> + report->action = action;
> +
> + if (action != UTRACE_STOP) {
> + if (action < report->resume_action)
> + report->resume_action = action;
> +
> + if (engine_wants_stop(engine)) {
> + spin_lock(&utrace->lock);
> + clear_engine_wants_stop(engine);
> + spin_unlock(&utrace->lock);
> + }

Reads funny, but I guess it can only race the right way round?

> + return;
> + }
> +
> + if (!engine_wants_stop(engine)) {
> + spin_lock(&utrace->lock);
> + /*
> + * If utrace_control() came in and detached us
> + * before we got the lock, we must not stop now.
> + */
> + if (unlikely(engine->ops == &utrace_detached_ops))
> + report->detaches = true;
> + else
> + mark_engine_wants_stop(task, engine);
> + spin_unlock(&utrace->lock);
> + }
> +}

I'm pretty sure that inline isn't really needed.

> +/*
> + * Apply the return value of one engine callback to @report.
> + * Returns true if @engine detached and should not get any more callbacks.
> + */
> +static bool finish_callback(struct task_struct *task, struct utrace *utrace,
> + struct utrace_report *report,
> + struct utrace_engine *engine,
> + u32 ret)
> +{
> + report->result = ret & ~UTRACE_RESUME_MASK;
> + finish_callback_report(task, utrace, report, engine,
> + utrace_resume_action(ret));
> +
> + /*
> + * Now that we have applied the effect of the return value,
> + * clear this so that utrace_barrier() can stop waiting.
> + * A subsequent utrace_control() can stop or resume @engine
> + * and know this was ordered after its callback's action.
> + *
> + * We don't need any barriers here because utrace_barrier()
> + * takes utrace->lock. If we touched engine->flags above,
> + * the lock guaranteed this change was before utrace_barrier()
> + * examined utrace->reporting.
> + */
> + utrace->reporting = NULL;
> +
> + /*
> + * This is a good place to make sure tracing engines don't
> + * introduce too much latency under voluntary preemption.
> + */
> + if (need_resched())
> + cond_resched();

Simply cond_resched() is sufficient, but that comment sucks, as it
doesn't mention _why_ it is a good place.

It seems to turn out that finish_callback() is always the last thing we
do in the engine iterations in REPORT_CALLBACKS() and
utrace_get_signal().

> +
> + return engine->ops == &utrace_detached_ops;
> +}
> +
> +/*
> + * Start the callbacks for @engine to consider @event (a bit mask).
> + * This makes the report_quiesce() callback first. If @engine wants
> + * a specific callback for @event, we return the ops vector to use.
> + * If not, we return NULL. The return value from the ops->callback
> + * function called should be passed to finish_callback().
> + */
> +static const struct utrace_engine_ops *start_callback(
> + struct utrace *utrace, struct utrace_report *report,
> + struct utrace_engine *engine, struct task_struct *task,
> + unsigned long event)
> +{
> + const struct utrace_engine_ops *ops;
> + unsigned long want;
> +
> + /*
> + * This barrier ensures that we've set utrace->reporting before
> + * we examine engine->flags or engine->ops. utrace_barrier()
> + * relies on this ordering to indicate that the effect of any
> + * utrace_control() and utrace_set_events() calls is in place
> + * by the time utrace->reporting can be seen to be NULL.
> + */
> + utrace->reporting = engine;
> + smp_mb();
> +
> + /*
> + * This pairs with the barrier in mark_engine_detached().
> + * It makes sure that we never see the old ops vector with
> + * the new flags, in case the original vector had no report_quiesce.
> + */
> + want = engine->flags;
> + smp_rmb();
> + ops = engine->ops;
> +
> + if (want & UTRACE_EVENT(QUIESCE)) {
> + if (finish_callback(task, utrace, report, engine,
> + (*ops->report_quiesce)(report->action,
> + engine, task,
> + event)))
> + return NULL;
> +
> + /*
> + * finish_callback() reset utrace->reporting after the
> + * quiesce callback. Now we set it again (as above)
> + * before re-examining engine->flags, which could have
> + * been changed synchronously by ->report_quiesce or
> + * asynchronously by utrace_control() or utrace_set_events().
> + */
> + utrace->reporting = engine;
> + smp_mb();
> + want = engine->flags;
> + }
> +
> + if (want & ENGINE_STOP)
> + report->action = UTRACE_STOP;
> +
> + if (want & event) {
> + report->spurious = false;
> + return ops;
> + }
> +
> + utrace->reporting = NULL;
> + return NULL;
> +}

This function makes my head hurt... it would be very nice to see an
obvious version and a set of patches obfuscating it, with numbers
defending the obfuscation.

> +/*
> + * Do a normal reporting pass for engines interested in @event.
> + * @callback is the name of the member in the ops vector, and remaining
> + * args are the extras it takes after the standard three args.
> + */
> +#define REPORT(task, utrace, report, event, callback, ...) \
> + do { \
> + start_report(utrace); \
> + REPORT_CALLBACKS(, task, utrace, report, event, callback, \
> + (report)->action, engine, current, \
> + ## __VA_ARGS__); \
> + finish_report(task, utrace, report, true); \
> + } while (0)
> +#define REPORT_CALLBACKS(rev, task, utrace, report, event, callback, ...) \
> + do { \
> + struct utrace_engine *engine; \
> + const struct utrace_engine_ops *ops; \
> + list_for_each_entry##rev(engine, &utrace->attached, entry) { \
> + ops = start_callback(utrace, report, engine, task, \
> + event); \
> + if (!ops) \
> + continue; \
> + finish_callback(task, utrace, report, engine, \
> + (*ops->callback)(__VA_ARGS__)); \
> + } \
> + } while (0)

My OCD makes me tell you that these macros are defined in the wrong
order.

> +void utrace_report_clone(unsigned long clone_flags, struct task_struct *child)
> +{
> + struct task_struct *task = current;
> + struct utrace *utrace = task_utrace_struct(task);
> + INIT_REPORT(report);
> +
> + /*
> + * We don't use the REPORT() macro here, because we need
> + * to clear utrace->cloning before finish_report().
> + * After finish_report(), utrace can be a stale pointer
> + * in cases when report.action is still UTRACE_RESUME.
> + */
> + start_report(utrace);
> + utrace->cloning = child;
> +
> + REPORT_CALLBACKS(, task, utrace, &report,
> + UTRACE_EVENT(CLONE), report_clone,
> + report.action, engine, task, clone_flags, child);
> +
> + utrace->cloning = NULL;
> + finish_report(task, utrace, &report, !(clone_flags & CLONE_VFORK));
> +
> + /*
> + * For a vfork, we will go into an uninterruptible block waiting
> + * for the child. We need UTRACE_STOP to happen before this, not
> + * after. For CLONE_VFORK, utrace_finish_vfork() will be called.
> + */
> + if (report.action == UTRACE_STOP && (clone_flags & CLONE_VFORK)) {
> + spin_lock(&utrace->lock);
> + utrace->vfork_stop = 1;
> + spin_unlock(&utrace->lock);
> + }

So much optimization, weird locking, barriers and here you didn't use
atomic bit ops?

> +}
> +
> +/*
> + * We're called after utrace_report_clone() for a CLONE_VFORK.
> + * If UTRACE_STOP was left from the clone report, we stop here.
> + * After this, we'll enter the uninterruptible wait_for_completion()
> + * waiting for the child.
> + */
> +void utrace_finish_vfork(struct task_struct *task)
> +{
> + struct utrace *utrace = task_utrace_struct(task);
> +
> + if (utrace->vfork_stop) {
> + spin_lock(&utrace->lock);
> + utrace->vfork_stop = 0;
> + spin_unlock(&utrace->lock);
> + utrace_stop(task, utrace, UTRACE_RESUME); /* XXX */
> + }
> +}

I'm sure that XXX means something,... again that vfork_stop stuff can
only race the right way about, right?

> +
> +/*
> + * Called iff UTRACE_EVENT(JCTL) flag is set.
> + *
> + * Called with siglock held.
> + */
> +void utrace_report_jctl(int notify, int what)
> +{
> + struct task_struct *task = current;
> + struct utrace *utrace = task_utrace_struct(task);
> + INIT_REPORT(report);
> +
> + spin_unlock_irq(&task->sighand->siglock);
> +
> + REPORT(task, utrace, &report, UTRACE_EVENT(JCTL),
> + report_jctl, what, notify);
> +
> + spin_lock_irq(&task->sighand->siglock);
> +}

So much documentation, and non of it says that the JCTL (funny name btw)
callback is done holding siglock... tskk.

> +/*
> + * Finish the last reporting pass before returning to user mode.
> + */
> +static void finish_resume_report(struct task_struct *task,
> + struct utrace *utrace,
> + struct utrace_report *report)
> +{
> + finish_report_reset(task, utrace, report);
> +
> + switch (report->action) {
> + case UTRACE_STOP:
> + utrace_stop(task, utrace, report->resume_action);
> + break;
> +
> + case UTRACE_INTERRUPT:
> + if (!signal_pending(task))
> + set_tsk_thread_flag(task, TIF_SIGPENDING);
> + break;
> +
> + case UTRACE_BLOCKSTEP:
> + if (likely(arch_has_block_step())) {
> + user_enable_block_step(task);
> + break;
> + }
> +
> + /*
> + * This means some callback is to blame for failing
> + * to check arch_has_block_step() itself. Warn and
> + * then fall through to treat it as SINGLESTEP.
> + */
> + WARN_ON(1);
> +
> + case UTRACE_SINGLESTEP:
> + if (likely(arch_has_single_step()))
> + user_enable_single_step(task);
> + else
> + /*
> + * This means some callback is to blame for failing
> + * to check arch_has_single_step() itself. Spew
> + * about it so the loser will fix his module.
> + */
> + WARN_ON(1);

wants braces

> + break;
> +
> + case UTRACE_REPORT:
> + case UTRACE_RESUME:
> + default:
> + user_disable_single_step(task);
> + break;
> + }
> +}
> +
> +/*
> + * This is called when TIF_NOTIFY_RESUME had been set (and is now clear).
> + * We are close to user mode, and this is the place to report or stop.
> + * When we return, we're going to user mode or into the signals code.
> + */
> +void utrace_resume(struct task_struct *task, struct pt_regs *regs)
> +{
> + struct utrace *utrace = task_utrace_struct(task);
> + INIT_REPORT(report);
> + struct utrace_engine *engine;
> +
> + /*
> + * Some machines get here with interrupts disabled. The same arch
> + * code path leads to calling into get_signal_to_deliver(), which
> + * implicitly reenables them by virtue of spin_unlock_irq.
> + */
> + local_irq_enable();

Hrmm, I would much prefer to fix up the calling conventions of
tracehook_notify_resume() than to bury something like this in the guts
of a tracehook user.

> + /*
> + * If this flag is still set it's because there was a signal
> + * handler setup done but no report_signal following it. Clear
> + * the flag before we get to user so it doesn't confuse us later.
> + */
> + if (unlikely(utrace->signal_handler)) {
> + spin_lock(&utrace->lock);
> + utrace->signal_handler = 0;
> + spin_unlock(&utrace->lock);
> + }

OK, so maybe you get to explain why this works..

> +
> + /*
> + * Update our bookkeeping even if there are no callbacks made here.
> + */
> + report.action = start_report(utrace);
> +
> + switch (report.action) {
> + case UTRACE_RESUME:
> + /*
> + * Anything we might have done was already handled by
> + * utrace_get_signal(), or this is an entirely spurious
> + * call. (The arch might use TIF_NOTIFY_RESUME for other
> + * purposes as well as calling us.)
> + */
> + return;
> + case UTRACE_REPORT:
> + if (unlikely(!(task->utrace_flags & UTRACE_EVENT(QUIESCE))))
> + break;
> + /*
> + * Do a simple reporting pass, with no specific
> + * callback after report_quiesce.
> + */
> + report.action = UTRACE_RESUME;
> + list_for_each_entry(engine, &utrace->attached, entry)
> + start_callback(utrace, &report, engine, task, 0);
> + break;
> + default:
> + /*
> + * Even if this report was truly spurious, there is no need
> + * for utrace_reset() now. TIF_NOTIFY_RESUME was already
> + * cleared--it doesn't stay spuriously set.
> + */
> + report.spurious = false;
> + break;
> + }
> +
> + /*
> + * Finish the report and either stop or get ready to resume.
> + * If utrace->resume was not UTRACE_REPORT, this applies its
> + * effect now (i.e. step or interrupt).
> + */
> + finish_resume_report(task, utrace, &report);
> +}

> +/*
> + * Take the siglock and push @info back on our queue.
> + * Returns with @task->sighand->siglock held.
> + */
> +static void push_back_signal(struct task_struct *task, siginfo_t *info)
> + __acquires(task->sighand->siglock)
> +{
> + struct sigqueue *q;
> +
> + if (unlikely(!info->si_signo)) { /* Oh, a wise guy! */
> + spin_lock_irq(&task->sighand->siglock);
> + return;
> + }
> +
> + q = sigqueue_alloc();
> + if (likely(q)) {
> + q->flags = 0;
> + copy_siginfo(&q->info, info);
> + }
> +
> + spin_lock_irq(&task->sighand->siglock);
> +
> + sigaddset(&task->pending.signal, info->si_signo);
> + if (likely(q))
> + list_add(&q->list, &task->pending.list);
> +
> + set_tsk_thread_flag(task, TIF_SIGPENDING);
> +}

Yes, exactly what we need, more funky locking..

/me gives up..

OK, so what this code needs is some serious restructuring and
de-obfuscation, after that we can maybe get creative and add some of it
back if the performance numbers agree.

But there's too much ugly locking and barriers to be fully sane.

Also the documentation needs more whitespace, its very hard to digest in
its current form.

I'll try and look at the actual state machine later, when my brain has
recovered from this.

2009-12-01 22:14:38

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On 12/01, Peter Zijlstra wrote:
>
> On Tue, 2009-11-24 at 21:02 +0100, Oleg Nesterov wrote:
>
> > + <para>
> > + There is nothing a kernel module can do to keep a <structname>struct
> > + task_struct</structname> alive outside of
> > + <function>rcu_read_lock</function>.
>
> Sure there is, get_task_struct() comes to mind.

it is not exported ;)

Peter, I skipped other comments about the documentation, I never read
it myself. Update: I skipped a lot more for today ;)

> > @@ -351,6 +394,10 @@ static inline void tracehook_report_vfor
> > */
> > static inline void tracehook_prepare_release_task(struct task_struct *task)
> > {
> > + /* see utrace_add_engine() about this barrier */
> > + smp_mb();
> > + if (task_utrace_flags(task))
> > + utrace_release_task(task);
> > }
>
> OK, that seems to properly order ->exit_state vs ->utrace_flags,
>
> This site does:
>
> assign ->state
> mb
> observe ->utrace_flags
>
> and the other site does:
>
> assign ->utrace_flags
> mb
> observe ->exit_state

Yes, we hope.

> > @@ -560,6 +625,20 @@ static inline void tracehook_report_deat
> > int signal, void *death_cookie,
> > int group_dead)
> > {
> > + /*
> > + * This barrier ensures that our caller's setting of
> > + * @task->exit_state precedes checking @task->utrace_flags here.
> > + * If utrace_set_events() was just called to enable
> > + * UTRACE_EVENT(DEATH), then we are obliged to call
> > + * utrace_report_death() and not miss it. utrace_set_events()
> > + * uses tasklist_lock to synchronize enabling the bit with the
> > + * actual change to @task->exit_state, but we need this barrier
> > + * to be sure we see a flags change made just before our caller
> > + * took the tasklist_lock.
> > + */
> > + smp_mb();
> > + if (task_utrace_flags(task) & _UTRACE_DEATH_EVENTS)
> > + utrace_report_death(task, death_cookie, group_dead, signal);
> > }
>
> I don't think its allowed to pair a mb with a lock-barrier, since the
> lock barriers are semi-permeable.

Could you clarify?

> > @@ -589,10 +668,20 @@ static inline void set_notify_resume(str
> > * asynchronously, this will be called again before we return to
> > * user mode.
> > *
> > - * Called without locks.
> > + * Called without locks. However, on some machines this may be
> > + * called with interrupts disabled.
> > */
> > static inline void tracehook_notify_resume(struct pt_regs *regs)
> > {
> > + struct task_struct *task = current;
> > + /*
> > + * This pairs with the barrier implicit in set_notify_resume().
> > + * It ensures that we read the nonzero utrace_flags set before
> > + * set_notify_resume() was called by utrace setup.
> > + */
> > + smp_rmb();
> > + if (task_utrace_flags(task))
> > + utrace_resume(task, regs);
> > }
>
> Sending an IPI implies the mb?

Yes, but this has nothing to do with IPI. The caller, do_notify_resume(),
does:
clear_thread_flag(TIF_NOTIFY_RESUME);
tracehook_notify_resume:
if (task_utrace_flags(task))
utrace_resume();

We should not read task_utrace_flags() before we clear TIF_NOTIFY_RESUME.

> > +static inline struct utrace *task_utrace_struct(struct task_struct *task)
> > +{
> > + struct utrace *utrace;
> > +
> > + /*
> > + * This barrier ensures that any prior load of task->utrace_flags
> > + * is ordered before this load of task->utrace. We use those
> > + * utrace_flags checks in the hot path to decide to call into
> > + * the utrace code. The first attach installs task->utrace before
> > + * setting task->utrace_flags nonzero, with a barrier between.
> > + * See utrace_task_alloc().
> > + */
> > + smp_rmb();
> > + utrace = task->utrace;
> > +
> > + smp_read_barrier_depends(); /* See utrace_task_alloc(). */
> > + return utrace;
> > +}
>
> I spot two barriers here, but only 1 over in utrace_task_alloc(), hmm?

smp_read_barrier_depends() pairs with utrace_task_alloc()->wmb().

smp_rmb() is needed for another reason. Suppose the code does:

if (task_utrace_flags() & SOMETHING)
do_something_with(task->utrace);

if we race with utrace_attach_task(), we can see ->utrace_flags != 0
but task->utrace == NULL without rmb().

> > +struct utrace_engine {
> > +/* private: */
> > + struct kref kref;
> > + void (*release)(void *);
> > + struct list_head entry;
> > +
> > +/* public: */
> > + const struct utrace_engine_ops *ops;
> > + void *data;
> > +
> > + unsigned long flags;
> > +};
>
> Sorry, the kernel is written in C, not C++.

Hmm. I almost never read the comments, but these 2 look very clear
to me ;)

> > + * Most callbacks take an @action argument, giving the resume action
> > + * chosen by other tracing engines. All callbacks take an @engine
> > + * argument, and a @task argument, which is always equal to @current.
>
> Given that some functions have a lot of arguments (depleting regparam),
> isn't it more expensive to push current on the stack than it is to
> simply read it again?

Yes, perhaps. Only ->report_reap() really needs @task, it may be
!current.

> > +struct utrace_engine_ops {
>
> > + u32 (*report_signal)(u32 action,
> > + struct utrace_engine *engine,
> > + struct task_struct *task,
> > + struct pt_regs *regs,
> > + siginfo_t *info,
> > + const struct k_sigaction *orig_ka,
> > + struct k_sigaction *return_ka);
>
> > + u32 (*report_clone)(enum utrace_resume_action action,
> > + struct utrace_engine *engine,
> > + struct task_struct *parent,
> > + unsigned long clone_flags,
> > + struct task_struct *child);
>
> > +};
>
> Seems inconsistent on u32 vs enum utrace_resume_action.

Well, this u32 can hold utrace_resume_action | utrace_signal_action,
for example.

> > +struct utrace_examiner {
> > +/* private: */
> > + long state;
> > + unsigned long ncsw;
> > +};
>
> Again, its not C++, if you want a private state like that, use an opaque
> type, like:
>
> struct utrace_examiner;
>
> and only define the thing in utrace.c or something.

Then the caller of utrace_prepare_examine() has to alloc utrace_examiner
somehow. I disagree here. But of course we can remove this comment.

> > +static inline __must_check int utrace_control_pid(
> > + struct pid *pid, struct utrace_engine *engine,
> > + enum utrace_resume_action action)
> > +{
> > + /*
> > + * We don't bother with rcu_read_lock() here to protect the
> > + * task_struct pointer, because utrace_control will return
> > + * -ESRCH without looking at that pointer if the engine is
> > + * already detached. A task_struct pointer can't die before
> > + * all the engines are detached in release_task() first.
> > + */
> > + struct task_struct *task = pid_task(pid, PIDTYPE_PID);
> > + return unlikely(!task) ? -ESRCH : utrace_control(task, engine, action);
> > +}
>
> Is that comment correct? Without rcu_read_lock() the pidhash can change
> under our feet and maybe cause funny things?

If pid itself can't go away, it is always safe to use pid_task(). Yes,
we can't trust the returned value, that is why utrace_control() verifies
this task_struct* is still valid.

> Does pid_task() in generaly rely on havin rcu_read_lock() called?

See above. pid_task() itself doesn't need rcu_read_lock(), but without
rcu lock around you can't simply use the result.

> > +static bool utrace_task_alloc(struct task_struct *task)
> > +{
> > + struct utrace *utrace = kmem_cache_zalloc(utrace_cachep, GFP_KERNEL);
> > + if (unlikely(!utrace))
> > + return false;
> > + spin_lock_init(&utrace->lock);
> > + INIT_LIST_HEAD(&utrace->attached);
> > + INIT_LIST_HEAD(&utrace->attaching);
> > + utrace->resume = UTRACE_RESUME;
> > + task_lock(task);
> > + if (likely(!task->utrace)) {
> > + /*
> > + * This barrier makes sure the initialization of the struct
> > + * precedes the installation of the pointer. This pairs
> > + * with smp_read_barrier_depends() in task_utrace_struct().
> > + */
> > + smp_wmb();
> > + task->utrace = utrace;
> > + }
> > + task_unlock(task);
> > + /*
> > + * That unlock after storing task->utrace acts as a memory barrier
> > + * ordering any subsequent task->utrace_flags store afterwards.
> > + * This pairs with smp_rmb() in task_utrace_struct().
> > + */
> > + if (unlikely(task->utrace != utrace))
> > + kmem_cache_free(utrace_cachep, utrace);
> > + return true;
> > +}
>
> Again, not sure we can pair an UNLOCK-barrier with a RMB. In fact, both
> are NOPs on x86.

We can't. I think the comment is confusing. We need the barrier
between setting "task->utrace = utrace" and changing ->utrace_flags.
We have unlock+lock in between, this implies mb().

> > +static inline int utrace_attach_delay(struct task_struct *target)
> > +{
> > + if ((target->flags & PF_STARTING) &&
> > + task_utrace_struct(current) &&
> > + task_utrace_struct(current)->cloning != target)
> > + do {
> > + schedule_timeout_interruptible(1);
> > + if (signal_pending(current))
> > + return -ERESTARTNOINTR;
> > + } while (target->flags & PF_STARTING);
> > +
> > + return 0;
> > +}
>
> Quite gross this.. can't we key off the
> tracehoook_report_clone_complete() and use a wakeup there?

Yes, it would be very nice to avoid this schedule_timeout_interruptible().
But currently we don't see a simple solution, on the TODO list. But, to
clarify, this case is very unlikely.

> Furthermore I'd add a function like:
>
> static struct utrace_engine_ops *
> get_utrace_ops(struct utrace_engine *engine, unsigned long *flags)
> {
> *flags = engine->flags;
> /*
> * This pairs with the barrier in mark_engine_detached().
> * It makes sure that we never see the old ops vector with
> * the new flags, in case the original vector had no
> * report_quiesce.
> */
> smp_rmb();
> return engine->ops;
> }
>
> to take out and explicitly comment that common bit.
>
> Also, I'm not quite sure on why we play so many barrier games, looking
> at start_callback() we have 2 barriers in the callback loop, why not a
> per engine lock?

Exactly to avoid the lock, I guess ;)

> > + /*
> > + * In theory spin_lock() doesn't imply rcu_read_lock().
> > + * Once we clear ->utrace_flags this task_struct can go away
> > + * because tracehook_prepare_release_task() path does not take
> > + * utrace->lock when ->utrace_flags == 0.
> > + */
> > + rcu_read_lock();
> > + task->utrace_flags = flags;
> > + spin_unlock(&utrace->lock);
> > + rcu_read_unlock();
>
> yuck!
>
> why not simply keep a task reference over the utrace_reset call?

Yes, we could use get_task_struct() instead. Not sure this would
be more clean, though.

> > +static void utrace_stop(struct task_struct *task, struct utrace *utrace,
> > + enum utrace_resume_action action)
> > ...
> > + /*
> > + * If ptrace is among the reasons for this stop, do its
> > + * notification now. This could not just be done in
> > + * ptrace's own event report callbacks because it has to
> > + * be done after we are in TASK_TRACED. This makes the
> > + * synchronization with ptrace_do_wait() work right.
> > + */
> > + ptrace_notify_stop(task);
>
> Well, this is a bit disappointing isn't it? So we cannot implement
> ptrace on utrace without special purpose hooks?

Yes, currently we need the special hook for ptrace. Because ptrace
is really special, no other engine should cooperate with do_wait/etc.

That said, I agree. We need something more general which could be
used by other engines too.

> > +static enum utrace_resume_action start_report(struct utrace *utrace)
> > +{
> > + enum utrace_resume_action resume = utrace->resume;
> > + if (utrace->pending_attach ||
> > + (resume > UTRACE_INTERRUPT && resume < UTRACE_RESUME)) {
> > + spin_lock(&utrace->lock);
> > + splice_attaching(utrace);
> > + resume = utrace->resume;
> > + if (resume > UTRACE_INTERRUPT)
> > + utrace->resume = UTRACE_RESUME;
> > + spin_unlock(&utrace->lock);
> > + }
> > + return resume;
> > +}
>
> Its not entirely clear why we can check pending_attach outside of the
> utrace->lock and not be racy.

We can safely miss utrace->pending_attach here, or even read the "stale"
utrace->resume. Both can be changed after start_report().

> > +static inline void finish_callback_report(struct task_struct *task,
> > + struct utrace *utrace,
> > + struct utrace_report *report,
> > + struct utrace_engine *engine,
> > + enum utrace_resume_action action)
> > +{
> > + /*
> > + * If utrace_control() was used, treat that like UTRACE_DETACH here.
> > + */
> > + if (action == UTRACE_DETACH || engine->ops == &utrace_detached_ops) {
> > + engine->ops = &utrace_detached_ops;
> > + report->detaches = true;
> > + return;
> > + }
> > +
> > + if (action < report->action)
> > + report->action = action;
> > +
> > + if (action != UTRACE_STOP) {
> > + if (action < report->resume_action)
> > + report->resume_action = action;
> > +
> > + if (engine_wants_stop(engine)) {
> > + spin_lock(&utrace->lock);
> > + clear_engine_wants_stop(engine);
> > + spin_unlock(&utrace->lock);
> > + }
>
> Reads funny, but I guess it can only race the right way round?

Not sure I understand... could you explain?

> > + /*
> > + * This is a good place to make sure tracing engines don't
> > + * introduce too much latency under voluntary preemption.
> > + */
> > + if (need_resched())
> > + cond_resched();
>
> Simply cond_resched() is sufficient, but that comment sucks, as it
> doesn't mention _why_ it is a good place.

Hmm, I agree.

> > + /*
> > + * For a vfork, we will go into an uninterruptible block waiting
> > + * for the child. We need UTRACE_STOP to happen before this, not
> > + * after. For CLONE_VFORK, utrace_finish_vfork() will be called.
> > + */
> > + if (report.action == UTRACE_STOP && (clone_flags & CLONE_VFORK)) {
> > + spin_lock(&utrace->lock);
> > + utrace->vfork_stop = 1;
> > + spin_unlock(&utrace->lock);
> > + }
>
> So much optimization, weird locking, barriers and here you didn't use
> atomic bit ops?

The point is, the state of the tracee must be "stable" under utrace->lock.
As for ->vfork_stop in particular, it should die (imho) but we need further
cleanups outside of utrace.c.

> > +void utrace_finish_vfork(struct task_struct *task)
> > +{
> > + struct utrace *utrace = task_utrace_struct(task);
> > +
> > + if (utrace->vfork_stop) {
> > + spin_lock(&utrace->lock);
> > + utrace->vfork_stop = 0;
> > + spin_unlock(&utrace->lock);
> > + utrace_stop(task, utrace, UTRACE_RESUME); /* XXX */
> > + }
> > +}
>
> I'm sure that XXX means something,... again that vfork_stop stuff can
> only race the right way about, right?

UTRACE_RESUME is not exactly right, we have the pending patches but
need more discussion.

> > +void utrace_report_jctl(int notify, int what)
> > +{
> > + struct task_struct *task = current;
> > + struct utrace *utrace = task_utrace_struct(task);
> > + INIT_REPORT(report);
> > +
> > + spin_unlock_irq(&task->sighand->siglock);
> > +
> > + REPORT(task, utrace, &report, UTRACE_EVENT(JCTL),
> > + report_jctl, what, notify);
> > +
> > + spin_lock_irq(&task->sighand->siglock);
> > +}
>
> So much documentation, and non of it says that the JCTL (funny name btw)
> callback is done holding siglock... tskk.

Not sure I understand, but we unlock ->siglock before REPORT(). If you mean
that utrace_report_jctl() is called under ->siglock, then yes.

Oleg.

2009-12-01 23:47:33

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

> Could we just drop the tracehook layer if this finally merged
> and call the low level functions directly?

We can certainly do appropriate streamlining cleanups later, by all
means. The original purpose of the tracehook layer is simply this:

A person hacking on core kernel code or arch code should not have to
think about all the innards of tracing (ptrace, utrace, or anything
else). If he comes across a tracehook_* call, he can just read its
kernel-doc for explanation of its parameters, return value, what it
expects about its context (locking et al), and what the semantic
significance of making that particular call is. If changes to the
core/arch code in question do not require changing any of those
aspects, then said person need not consider tracing issues further.
If a change to a function's calling interface or contextual
assumptions looks warranted, then he knows he should discuss the
details with some tracing-related folks (i.e. find tracehook.h in
MAINTAINERS and thus get to me and Oleg).

Likewise, a person hacking on tracing code should not have to think
about every corner of interaction with the core code or arch code.
Each tracehook_* call's kernel-doc comments say everything that
matters about how and where it's called. If some of those details
are problematic for what we want to do inside the tracing code, then
we know we have to hash out the details with the maintainers of the
core or arch code that makes those calls. Otherwise we can keep our
focus on tracing infrastructure without spending time getting lost
in arcane details of the arch or core kernel code.

These two things seem permanently worthwhile to me: that the core
and arch source code use simple function calls without open-coding
any innards of the tracing infrastructure; and that these functions
have clear and complete documentation about their calling interfaces
and context (locking et al). I find it natural and convenient that
such calls have a common prefix that makes it obvious to any reader
of the core code what subsystem the call relates to.

Beyond those ideas, I certainly don't care at all what the names of
these functions are, what common prefix is used, or in which source
files those declarations and kernel-doc comments appear.

Thanks,
Roland

2009-12-02 05:45:30

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

> This above text seems inconsistent. First you say report_reap() is the
> final callback and should be used for cleanup, then you say
> report_death() is the penultimate callback but might not always happen
> and people would normally use that as cleanup.
>
> If we cannot rely on report_death() then I would suggest to simply
> recommend report_reap() as cleanup callback and leave it at that.

I'm sorry the explanation was not clear to you. I'll try to make it clear
now and then I'd appreciate your suggestions on how the documentation could
be worded better. (I do appreciate your suggestion here but it's one based
on an idea that's not factual, so I don't think we should follow it.)

There is no "unreliable" aspect to it. If you call utrace_set_events() to
ask for report_death() callbacks then you will get that callback for that
task--guaranteed--unless utrace_set_events() returned an error code that
tells you unambiguously that you could not get that callback.

What's true is that report_reap() is the last callback you can ever
get for a given task. If you had asked for report_death() callbacks,
then you always get report_death() and if you've asked for both these
callbacks then report_death() is always before report_reap().
(This is a special ordering guarantee, because in actuality the
release_task() call that constitutes "reap" can happen in the parent
simultaneously with the task's own exit path.)

The one situation in which you cannot get report_death() is when the task
was already dead when you called utrace_set_events(). In that case, it
gives an error code as I mentioned above. Even in that situation, you can
still ask to enable just the report_reap() callback. With either of these,
if you enabled it successfully, then you are guaranteed to get it.

When you get report_death() and are not interested in getting report_reap()
afterwards, then you can return UTRACE_DETACH from report_death(). If you
don't detach, then you will get report_reap() later too. The documentation
mentions using report_death() to detach and clean up because many kinds of
uses will have no interest in report_reap() at all. The only reason to get
a report_reap() callback is if you are interested in tracking when a task's
(real) parent reaps it with wait* calls, but usually people are only really
interested in a task until it dies.

> > + <para>
> > + There is nothing a kernel module can do to keep a <structname>struct
> > + task_struct</structname> alive outside of
> > + <function>rcu_read_lock</function>.
>
> Sure there is, get_task_struct() comes to mind.

__put_task_struct() is not exported to modules and so put_task_struct()
cannot be used by modules.

> > + still valid until <function>rcu_read_unlock</function>. The
> > + infrastructure never holds task references of its own.
>
> And here you imply its existence.

[I take it this refers to the next quoted bit:]

> > Though neither
> > + <function>rcu_read_lock</function> nor any other lock is held while
> > + making a callback, it's always guaranteed that the <structname>struct
> > + task_struct</structname> and the <structname>struct
> > + utrace_engine</structname> passed as arguments remain valid
> > + until the callback function returns.

No, we do not imply that the utrace infrastructure holds any task
reference. The current task_struct is always live while it's running
and until it's passed to release_task(). The task_struct passed to
callbacks is current. That's all it means.

The true facts are that utrace callbacks are all made in the current task
except for report_reap(), which is made at the start of release_task().
So the kernel's core logic is holding a task reference at all times that
utrace callbacks are made. If you really think it is clearer to explain
that set of facts as "utrace holds a reference", then please suggest the
exact wording you have in mind.

> The above seems weird to me at best... why hold a pid around when you
> can keep a ref on the task struct? A pid might get reused leaving you
> with a completely different task than you thought you had.

The *_pid interfaces are only there because put_pid() can be used by
modules while put_task_struct() cannot. If we can make __put_task_struct()
an exported symbol again, I would see no reason for these *_pid calls.

> Right, except you cannot generally rely on this report_death() thing, so
> a trace engine needs to deal with the report_reap() thing anyway.

This is a false antecedent. I hope the explanation above made this
subject clear to you.

> > + <sect2 id="interlock"><title>Interlock with final callbacks</title>
[...]
> Hrmm, better mention this earlier, or at least refer to this when
> describing the above cleanup bits.

This explanation is somewhat long and has its own whole section so as to
be thoroughly explicit and clear. Do you think there should just be a
cross reference here in the earlier mention of report_reap() and
report_death()? Or do you think that the "Final callbacks" text should
be merged together with the "Interlock" section?

> > + an error if the task is not properly stopped and not dead.
>
> 'or' dead?

Yes, fixed. Thanks!

I've relied on Oleg to review and fix the barrier-related logic.
So I'll leave it to him to hash out those parts of the review.

> Can the compiler optimize away the callsite using weak functions like
> this? If not I think the normal static inline stubs make more sense.

I'm not sure I understand the question. If the !CONFIG_UTRACE case, this
all boils down to "if (0) utrace_foo(...);" calls after constant folding.
I think the compiler will always compile those out, but I'm not entirely
sure about all old compilers or about -O0.

> Also, what unoptimized compilation?

If there really is none then I guess there isn't a problem. We can just
drop these weak attributes and the dead calls will be optimized away
before link time anyway. If we are not positive that is going to fly
everywhere, then I have no objection to adding static inline stubs in the
#else branch if you think that is preferable.

> Right, so this lot doesn't seem very consistent.

Please explain what kind of consistency you have in mind.
I don't follow.

> At the very least I would put signal action and syscall action into
> different ranges.

Those two can never be used in the same place. So I don't really
understand that. It doesn't matter what their values are, so I don't
have a problem with changing them to be disjoint. But I don't understand
why it would be better in any way at all.

> Sorry, the kernel is written in C, not C++.

We know that perfectly well, and I haven't been able ascertain what
benefit you gain by being snide. Our code follows the instructions in
Documentation/kernel-doc-nano-HOWTO.txt, so if you have a complaint with
that established practice recommened for the kernel please take it up
with the appropriate maintainers. We're following what the kernel's own
documentation tells us to do.

If you use /** for a struct and omit @field for some fields, then you get
warnings from make *docs. If you omit all the fields (because none are
part of the documented API, such as in struct utrace_examiner), then you
get a build error. So unless kernel-doc changes, the options are to
follow its specified /* private: */ convention as we do now, to add a
bunch of "@field: this is an internal field, do not touch it" comments,
or to punt on kernel-doc for these structs. Which do you want?

> Again, its not C++, if you want a private state like that, use an opaque
> type, like:
>
> struct utrace_examiner;
>
> and only define the thing in utrace.c or something.

Again, we are following the kernel-doc instructions for this situation.
If you look at the uses of this type in the interface, you'll see that
its purpose is to have the caller allocate the space. The only way to
make it opaque would be to use a pointer type and do dynamic allocation
inside some utrace.c function. That would be inane for holding two words
needed momentarily during one caller's frame.

> Given that some functions have a lot of arguments (depleting regparam),
> isn't it more expensive to push current on the stack than it is to
> simply read it again?

Perhaps so. The only callback where the task argument is not current is
report_reap(). That one already differs from the calling pattern of all
the others because it has no return value. So I guess we could drop the
task argument to all the "normal" report_* calls.

I'll pick up with responding to the rest of your remarks as soon as I
can. (Right now I have to prepare for an airplane early in the morning.)

Thanks,
Roland

2009-12-02 18:40:20

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On 12/01, Peter Zijlstra wrote:
>
> > +static inline __must_check int utrace_control_pid(
> > + struct pid *pid, struct utrace_engine *engine,
> > + enum utrace_resume_action action)
> > +{
> > + /*
> > + * We don't bother with rcu_read_lock() here to protect the
> > + * task_struct pointer, because utrace_control will return
> > + * -ESRCH without looking at that pointer if the engine is
> > + * already detached. A task_struct pointer can't die before
> > + * all the engines are detached in release_task() first.
> > + */
> > + struct task_struct *task = pid_task(pid, PIDTYPE_PID);
> > + return unlikely(!task) ? -ESRCH : utrace_control(task, engine, action);
> > +}
>
> Is that comment correct? Without rcu_read_lock() the pidhash can change
> under our feet and maybe cause funny things?

I already tried to answer, but I guess my email was not very clear. Let me
try again.

pid_task() by itself is safe, but yes, it is possible that utrace_control()
is called with target == NULL, or this task_task was already freed/reused.

utrace_control(target) path does not use target until it verifies it is
safe to dereference it.

get_utrace_lock() calls rcu_read_lock() and checks that engine->ops
is not cleared (NULL or utrace_detached_ops). If we see the valid ->ops
under rcu_read_lock() it is safe to dereference target, even if we race
with release_task() we know that it has not passed utrace_release_task()
yet, and thus we know call_rcu(delayed_put_task_struct) was not yet
called _before_ we took rcu_read_lock().

If it is safe to dereference target, we can take utrace->lock. Once
we take this lock (and re-check engine->ops) the task can't go away
until we drop it, get_utrace_lock() drops rcu lock and returns with
utrace->lock held.

utrace_control() can safely play with target under utrace->lock.

> > + /*
> > + * If this flag is still set it's because there was a signal
> > + * handler setup done but no report_signal following it. Clear
> > + * the flag before we get to user so it doesn't confuse us later.
> > + */
> > + if (unlikely(utrace->signal_handler)) {
> > + spin_lock(&utrace->lock);
> > + utrace->signal_handler = 0;
> > + spin_unlock(&utrace->lock);
> > + }
>
> OK, so maybe you get to explain why this works..

Missed this part yesterday.

Well. ->signal_handler is set by handle_signal() when the signal was
delivered to the tracee. This flag is checked by utrace_get_signal()
to detect the stepping. But we should not return to user-mode with
this flag set, that is why utrace_resume() clears it.

However. This reminds me we were going to try to simplify this logic,
I'll try to think about this.

Oleg.

2009-12-02 18:55:25

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On 12/01, Peter Zijlstra wrote:
>
> > +void utrace_resume(struct task_struct *task, struct pt_regs *regs)
> > +{
> > + struct utrace *utrace = task_utrace_struct(task);
> > + INIT_REPORT(report);
> > + struct utrace_engine *engine;
> > +
> > + /*
> > + * Some machines get here with interrupts disabled. The same arch
> > + * code path leads to calling into get_signal_to_deliver(), which
> > + * implicitly reenables them by virtue of spin_unlock_irq.
> > + */
> > + local_irq_enable();
>
> Hrmm, I would much prefer to fix up the calling conventions of
> tracehook_notify_resume() than to bury something like this in the guts
> of a tracehook user.

Missed this part too.

May be, I dunno...

But in any case, imho it would be better to do this after we merge utrace,
otherwise we need more subtle arch-dependent changes before.

Oleg.

2009-12-05 19:22:34

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

> > > + * Some machines get here with interrupts disabled. The same arch
> > > + * code path leads to calling into get_signal_to_deliver(), which
> > > + * implicitly reenables them by virtue of spin_unlock_irq.
> > > + */
> > > + local_irq_enable();
> >
> > Hrmm, I would much prefer to fix up the calling conventions of
> > tracehook_notify_resume() than to bury something like this in the guts
> > of a tracehook user.

The reason I did it this way was mainly just not to make the requirement
for arch maintainers' too subtle. As it is, we just say that you call
tracehook_notify_resume() after clearing TIF_NOTIFY_RESUME, when it was
set. That keeps the specification quite simple. Of course, that is not
really much of a reason. If arch folks don't mind the requirement to
replace e.g.:

if (thread_info_flags & _TIF_NOTIFY_RESUME) {
clear_thread_flag(TIF_NOTIFY_RESUME);
tracehook_notify_resume(regs);
}

with:

if (thread_info_flags & _TIF_NOTIFY_RESUME) {
local_irq_enable();
clear_thread_flag(TIF_NOTIFY_RESUME);
tracehook_notify_resume(regs);
}

then that is certainly fine by me. But we do now have almost all the
arch's calling tracehook_notify_resume() and I don't know how many of
them do it in irqs-disabled context so they would need this change.

> But in any case, imho it would be better to do this after we merge utrace,
> otherwise we need more subtle arch-dependent changes before.

I tend to agree.

Thanks,
Roland

2009-12-07 18:34:45

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On Tue, 2009-12-01 at 23:08 +0100, Oleg Nesterov wrote:

> > > @@ -560,6 +625,20 @@ static inline void tracehook_report_deat
> > > int signal, void *death_cookie,
> > > int group_dead)
> > > {
> > > + /*
> > > + * This barrier ensures that our caller's setting of
> > > + * @task->exit_state precedes checking @task->utrace_flags here.
> > > + * If utrace_set_events() was just called to enable
> > > + * UTRACE_EVENT(DEATH), then we are obliged to call
> > > + * utrace_report_death() and not miss it. utrace_set_events()
> > > + * uses tasklist_lock to synchronize enabling the bit with the
> > > + * actual change to @task->exit_state, but we need this barrier
> > > + * to be sure we see a flags change made just before our caller
> > > + * took the tasklist_lock.
> > > + */
> > > + smp_mb();
> > > + if (task_utrace_flags(task) & _UTRACE_DEATH_EVENTS)
> > > + utrace_report_death(task, death_cookie, group_dead, signal);
> > > }
> >
> > I don't think its allowed to pair a mb with a lock-barrier, since the
> > lock barriers are semi-permeable.
>
> Could you clarify?

According to memory-barriers.txt a mb can be paired with mb,wmb,rmb
depending on the situation.

For LOCK it states:

(1) LOCK operation implication:

Memory operations issued after the LOCK will be completed after the LOCK
operation has completed.

Memory operations issued before the LOCK may be completed after the LOCK
operation has completed.

Which is not something I can see making sense to pair with an mb.

So either the comment is confusing and its again referring to an UNLOCK
+LOCK pair, or there's something fishy

> > > @@ -589,10 +668,20 @@ static inline void set_notify_resume(str
> > > * asynchronously, this will be called again before we return to
> > > * user mode.
> > > *
> > > - * Called without locks.
> > > + * Called without locks. However, on some machines this may be
> > > + * called with interrupts disabled.
> > > */
> > > static inline void tracehook_notify_resume(struct pt_regs *regs)
> > > {
> > > + struct task_struct *task = current;
> > > + /*
> > > + * This pairs with the barrier implicit in set_notify_resume().
> > > + * It ensures that we read the nonzero utrace_flags set before
> > > + * set_notify_resume() was called by utrace setup.
> > > + */
> > > + smp_rmb();
> > > + if (task_utrace_flags(task))
> > > + utrace_resume(task, regs);
> > > }
> >
> > Sending an IPI implies the mb?
>
> Yes, but this has nothing to do with IPI. The caller, do_notify_resume(),
> does:
> clear_thread_flag(TIF_NOTIFY_RESUME);
> tracehook_notify_resume:
> if (task_utrace_flags(task))
> utrace_resume();
>
> We should not read task_utrace_flags() before we clear TIF_NOTIFY_RESUME.

Then this comment needs an update.. that wasn't at all clear to me.

> > > +static inline struct utrace *task_utrace_struct(struct task_struct *task)
> > > +{
> > > + struct utrace *utrace;
> > > +
> > > + /*
> > > + * This barrier ensures that any prior load of task->utrace_flags
> > > + * is ordered before this load of task->utrace. We use those
> > > + * utrace_flags checks in the hot path to decide to call into
> > > + * the utrace code. The first attach installs task->utrace before
> > > + * setting task->utrace_flags nonzero, with a barrier between.
> > > + * See utrace_task_alloc().
> > > + */
> > > + smp_rmb();
> > > + utrace = task->utrace;
> > > +
> > > + smp_read_barrier_depends(); /* See utrace_task_alloc(). */
> > > + return utrace;
> > > +}
> >
> > I spot two barriers here, but only 1 over in utrace_task_alloc(), hmm?
>
> smp_read_barrier_depends() pairs with utrace_task_alloc()->wmb().
>
> smp_rmb() is needed for another reason. Suppose the code does:
>
> if (task_utrace_flags() & SOMETHING)
> do_something_with(task->utrace);
>
> if we race with utrace_attach_task(), we can see ->utrace_flags != 0
> but task->utrace == NULL without rmb().

Still not clear what we pair with.. There is no obvious barrier in
utrace_attach_task() nor a comment referring to this site.

> > > +struct utrace_engine {
> > > +/* private: */
> > > + struct kref kref;
> > > + void (*release)(void *);
> > > + struct list_head entry;
> > > +
> > > +/* public: */
> > > + const struct utrace_engine_ops *ops;
> > > + void *data;
> > > +
> > > + unsigned long flags;
> > > +};
> >
> > Sorry, the kernel is written in C, not C++.
>
> Hmm. I almost never read the comments, but these 2 look very clear
> to me ;)

I see no point in adding comments like that at all.

> > > + * Most callbacks take an @action argument, giving the resume action
> > > + * chosen by other tracing engines. All callbacks take an @engine
> > > + * argument, and a @task argument, which is always equal to @current.
> >
> > Given that some functions have a lot of arguments (depleting regparam),
> > isn't it more expensive to push current on the stack than it is to
> > simply read it again?
>
> Yes, perhaps. Only ->report_reap() really needs @task, it may be
> !current.
>
> > > +struct utrace_engine_ops {
> >
> > > + u32 (*report_signal)(u32 action,
> > > + struct utrace_engine *engine,
> > > + struct task_struct *task,
> > > + struct pt_regs *regs,
> > > + siginfo_t *info,
> > > + const struct k_sigaction *orig_ka,
> > > + struct k_sigaction *return_ka);
> >
> > > + u32 (*report_clone)(enum utrace_resume_action action,
> > > + struct utrace_engine *engine,
> > > + struct task_struct *parent,
> > > + unsigned long clone_flags,
> > > + struct task_struct *child);
> >
> > > +};
> >
> > Seems inconsistent on u32 vs enum utrace_resume_action.
>
> Well, this u32 can hold utrace_resume_action | utrace_signal_action,
> for example.

Sure, but then make all of them u32, or unsigned int, as that gets mixed
in below.

> > > +struct utrace_examiner {
> > > +/* private: */
> > > + long state;
> > > + unsigned long ncsw;
> > > +};
> >
> > Again, its not C++, if you want a private state like that, use an opaque
> > type, like:
> >
> > struct utrace_examiner;
> >
> > and only define the thing in utrace.c or something.
>
> Then the caller of utrace_prepare_examine() has to alloc utrace_examiner
> somehow. I disagree here. But of course we can remove this comment.

Ah, right then simply drop the comment.

> > > +static bool utrace_task_alloc(struct task_struct *task)
> > > +{
> > > + struct utrace *utrace = kmem_cache_zalloc(utrace_cachep, GFP_KERNEL);
> > > + if (unlikely(!utrace))
> > > + return false;
> > > + spin_lock_init(&utrace->lock);
> > > + INIT_LIST_HEAD(&utrace->attached);
> > > + INIT_LIST_HEAD(&utrace->attaching);
> > > + utrace->resume = UTRACE_RESUME;
> > > + task_lock(task);
> > > + if (likely(!task->utrace)) {
> > > + /*
> > > + * This barrier makes sure the initialization of the struct
> > > + * precedes the installation of the pointer. This pairs
> > > + * with smp_read_barrier_depends() in task_utrace_struct().
> > > + */
> > > + smp_wmb();
> > > + task->utrace = utrace;
> > > + }
> > > + task_unlock(task);
> > > + /*
> > > + * That unlock after storing task->utrace acts as a memory barrier
> > > + * ordering any subsequent task->utrace_flags store afterwards.
> > > + * This pairs with smp_rmb() in task_utrace_struct().
> > > + */
> > > + if (unlikely(task->utrace != utrace))
> > > + kmem_cache_free(utrace_cachep, utrace);
> > > + return true;
> > > +}
> >
> > Again, not sure we can pair an UNLOCK-barrier with a RMB. In fact, both
> > are NOPs on x86.
>
> We can't. I think the comment is confusing. We need the barrier
> between setting "task->utrace = utrace" and changing ->utrace_flags.
> We have unlock+lock in between, this implies mb().

So this is the task_unlock() in utrace_task_alloc() and the
spin_lock(&utrace->lock) in utrace_add_engine() ?

Talk about non-obvious.

> > > +static inline int utrace_attach_delay(struct task_struct *target)
> > > +{
> > > + if ((target->flags & PF_STARTING) &&
> > > + task_utrace_struct(current) &&
> > > + task_utrace_struct(current)->cloning != target)
> > > + do {
> > > + schedule_timeout_interruptible(1);
> > > + if (signal_pending(current))
> > > + return -ERESTARTNOINTR;
> > > + } while (target->flags & PF_STARTING);
> > > +
> > > + return 0;
> > > +}
> >
> > Quite gross this.. can't we key off the
> > tracehoook_report_clone_complete() and use a wakeup there?
>
> Yes, it would be very nice to avoid this schedule_timeout_interruptible().
> But currently we don't see a simple solution, on the TODO list. But, to
> clarify, this case is very unlikely.

Use a global wait-queue and put a wakeup_all in
tracehook_report_clone_complete() or wherever that ends up in utrace?

> > Furthermore I'd add a function like:
> >
> > static struct utrace_engine_ops *
> > get_utrace_ops(struct utrace_engine *engine, unsigned long *flags)
> > {
> > *flags = engine->flags;
> > /*
> > * This pairs with the barrier in mark_engine_detached().
> > * It makes sure that we never see the old ops vector with
> > * the new flags, in case the original vector had no
> > * report_quiesce.
> > */
> > smp_rmb();
> > return engine->ops;
> > }
> >
> > to take out and explicitly comment that common bit.
> >
> > Also, I'm not quite sure on why we play so many barrier games, looking
> > at start_callback() we have 2 barriers in the callback loop, why not a
> > per engine lock?
>
> Exactly to avoid the lock, I guess ;)

I'm really thinking this code ought to get simplified a lot, there's too
much non-obvious barriers.

Then, and only if performance numbers show it, add back some of these
optimizations, in simple small steps, that way its much easier to review
as well.

> > > + /*
> > > + * In theory spin_lock() doesn't imply rcu_read_lock().
> > > + * Once we clear ->utrace_flags this task_struct can go away
> > > + * because tracehook_prepare_release_task() path does not take
> > > + * utrace->lock when ->utrace_flags == 0.
> > > + */
> > > + rcu_read_lock();
> > > + task->utrace_flags = flags;
> > > + spin_unlock(&utrace->lock);
> > > + rcu_read_unlock();
> >
> > yuck!
> >
> > why not simply keep a task reference over the utrace_reset call?
>
> Yes, we could use get_task_struct() instead. Not sure this would
> be more clean, though.

For one it would allow getting rid of that insane assymetric locking.

> > > +static inline void finish_callback_report(struct task_struct *task,
> > > + struct utrace *utrace,
> > > + struct utrace_report *report,
> > > + struct utrace_engine *engine,
> > > + enum utrace_resume_action action)
> > > +{
> > > + /*
> > > + * If utrace_control() was used, treat that like UTRACE_DETACH here.
> > > + */
> > > + if (action == UTRACE_DETACH || engine->ops == &utrace_detached_ops) {
> > > + engine->ops = &utrace_detached_ops;
> > > + report->detaches = true;
> > > + return;
> > > + }
> > > +
> > > + if (action < report->action)
> > > + report->action = action;
> > > +
> > > + if (action != UTRACE_STOP) {
> > > + if (action < report->resume_action)
> > > + report->resume_action = action;
> > > +
> > > + if (engine_wants_stop(engine)) {
> > > + spin_lock(&utrace->lock);
> > > + clear_engine_wants_stop(engine);
> > > + spin_unlock(&utrace->lock);
> > > + }
> >
> > Reads funny, but I guess it can only race the right way round?
>
> Not sure I understand... could you explain?

It can only race with another thread also clearing the bit, not with one
setting the bit? Otherwise there's no guarantee its not set after that
if stmt.

> > > +void utrace_report_jctl(int notify, int what)
> > > +{
> > > + struct task_struct *task = current;
> > > + struct utrace *utrace = task_utrace_struct(task);
> > > + INIT_REPORT(report);
> > > +
> > > + spin_unlock_irq(&task->sighand->siglock);
> > > +
> > > + REPORT(task, utrace, &report, UTRACE_EVENT(JCTL),
> > > + report_jctl, what, notify);
> > > +
> > > + spin_lock_irq(&task->sighand->siglock);
> > > +}
> >
> > So much documentation, and non of it says that the JCTL (funny name btw)
> > callback is done holding siglock... tskk.
>
> Not sure I understand, but we unlock ->siglock before REPORT(). If you mean
> that utrace_report_jctl() is called under ->siglock, then yes.

Apparently means I'm cross-eyed as I read it to lock before and unlock
after :-/

2009-12-08 15:10:07

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On 12/07, Peter Zijlstra wrote:
>
> On Tue, 2009-12-01 at 23:08 +0100, Oleg Nesterov wrote:
>
> > > > @@ -560,6 +625,20 @@ static inline void tracehook_report_deat
> > > > int signal, void *death_cookie,
> > > > int group_dead)
> > > > {
> > > > + /*
> > > > + * This barrier ensures that our caller's setting of
> > > > + * @task->exit_state precedes checking @task->utrace_flags here.
> > > > + * If utrace_set_events() was just called to enable
> > > > + * UTRACE_EVENT(DEATH), then we are obliged to call
> > > > + * utrace_report_death() and not miss it. utrace_set_events()
> > > > + * uses tasklist_lock to synchronize enabling the bit with the
> > > > + * actual change to @task->exit_state, but we need this barrier
> > > > + * to be sure we see a flags change made just before our caller
> > > > + * took the tasklist_lock.
> > > > + */
> > > > + smp_mb();
> > > > + if (task_utrace_flags(task) & _UTRACE_DEATH_EVENTS)
> > > > + utrace_report_death(task, death_cookie, group_dead, signal);
> > > > }
> > >
> > > I don't think its allowed to pair a mb with a lock-barrier, since the
> > > lock barriers are semi-permeable.
> >
> > Could you clarify?
>
> According to memory-barriers.txt a mb can be paired with mb,wmb,rmb
> depending on the situation.
>
> For LOCK it states:
>
> (1) LOCK operation implication:
>
> Memory operations issued after the LOCK will be completed after the LOCK
> operation has completed.
>
> Memory operations issued before the LOCK may be completed after the LOCK
> operation has completed.
>
> Which is not something I can see making sense to pair with an mb.
>
> So either the comment is confusing and its again referring to an UNLOCK
> +LOCK pair, or there's something fishy

Ah. I missed your point because I didn't read the comment.

Hmm. And since I didn't read the comment I misunderstood this code. Now
I think the comment is wrong, and this mb() is not needed.

If utrace utrace_set_events() adds _UTRACE_DEATH_EVENTS bits, it does
this under read_lock(tasklist). This means tracehook_report_death()
must always see the flags change, the caller took write_lock(tasklist).

IOW, in _UTRACE_DEATH_EVENTS case

utrace_set_events:

read_lock(tasklist);
if (!target->exit_state)
target->utrace_flags |= UTRACE_EVENT(DEATH);
read_unlock(tasklist);

exit_notify:

write_lock(tasklist);
task->exit_state = EXIT_XXX;
write_unlock(tasklist);

if (target->utrace_flags & _UTRACE_DEATH_EVENTS)
utrace_report_death();

It doesn't matter if the reading of ->utrace_flags can be reordered with
the setting of ->exit_state, in no way this LOAD can happen _before_ we
take tasklist for writing. And, if ->utrace_flags was already changed
before we take tasklist, since utrace_set_events() does this under the
same lock, we must see the change after write_unlock(tasklist).

Roland?

> > > > static inline void tracehook_notify_resume(struct pt_regs *regs)
> > > > {
> > > > + struct task_struct *task = current;
> > > > + /*
> > > > + * This pairs with the barrier implicit in set_notify_resume().
> > > > + * It ensures that we read the nonzero utrace_flags set before
> > > > + * set_notify_resume() was called by utrace setup.
> > > > + */
> > > > + smp_rmb();
> > > > + if (task_utrace_flags(task))
> > > > + utrace_resume(task, regs);
> > > > }
> > >
> > > Sending an IPI implies the mb?
> >
> > Yes, but this has nothing to do with IPI. The caller, do_notify_resume(),
> > does:
> > clear_thread_flag(TIF_NOTIFY_RESUME);
> > tracehook_notify_resume:
> > if (task_utrace_flags(task))
> > utrace_resume();
> >
> > We should not read task_utrace_flags() before we clear TIF_NOTIFY_RESUME.
>
> Then this comment needs an update.. that wasn't at all clear to me.

OK.

> > > > +static inline struct utrace *task_utrace_struct(struct task_struct *task)
> > > > +{
> > > > + struct utrace *utrace;
> > > > +
> > > > + /*
> > > > + * This barrier ensures that any prior load of task->utrace_flags
> > > > + * is ordered before this load of task->utrace. We use those
> > > > + * utrace_flags checks in the hot path to decide to call into
> > > > + * the utrace code. The first attach installs task->utrace before
> > > > + * setting task->utrace_flags nonzero, with a barrier between.
> > > > + * See utrace_task_alloc().
> > > > + */
> > > > + smp_rmb();
> > > > + utrace = task->utrace;
> > > > +
> > > > + smp_read_barrier_depends(); /* See utrace_task_alloc(). */
> > > > + return utrace;
> > > > +}
> > >
> > > I spot two barriers here, but only 1 over in utrace_task_alloc(), hmm?
> >
> > smp_read_barrier_depends() pairs with utrace_task_alloc()->wmb().
> >
> > smp_rmb() is needed for another reason. Suppose the code does:
> >
> > if (task_utrace_flags() & SOMETHING)
> > do_something_with(task->utrace);
> >
> > if we race with utrace_attach_task(), we can see ->utrace_flags != 0
> > but task->utrace == NULL without rmb().
>
> Still not clear what we pair with.. There is no obvious barrier in
> utrace_attach_task() nor a comment referring to this site.

Yes, agreed, we should move (and improve) the comment from
utrace_task_alloc() to utrace_attach_task() or utrace_add_engine()
which changed ->utrace_flags, see below.

> > > > +static bool utrace_task_alloc(struct task_struct *task)
> > > > +{
> > > > + struct utrace *utrace = kmem_cache_zalloc(utrace_cachep, GFP_KERNEL);
> > > > + if (unlikely(!utrace))
> > > > + return false;
> > > > + spin_lock_init(&utrace->lock);
> > > > + INIT_LIST_HEAD(&utrace->attached);
> > > > + INIT_LIST_HEAD(&utrace->attaching);
> > > > + utrace->resume = UTRACE_RESUME;
> > > > + task_lock(task);
> > > > + if (likely(!task->utrace)) {
> > > > + /*
> > > > + * This barrier makes sure the initialization of the struct
> > > > + * precedes the installation of the pointer. This pairs
> > > > + * with smp_read_barrier_depends() in task_utrace_struct().
> > > > + */
> > > > + smp_wmb();
> > > > + task->utrace = utrace;
> > > > + }
> > > > + task_unlock(task);
> > > > + /*
> > > > + * That unlock after storing task->utrace acts as a memory barrier
> > > > + * ordering any subsequent task->utrace_flags store afterwards.
> > > > + * This pairs with smp_rmb() in task_utrace_struct().
> > > > + */
> > > > + if (unlikely(task->utrace != utrace))
> > > > + kmem_cache_free(utrace_cachep, utrace);
> > > > + return true;
> > > > +}
> > >
> > > Again, not sure we can pair an UNLOCK-barrier with a RMB. In fact, both
> > > are NOPs on x86.
> >
> > We can't. I think the comment is confusing. We need the barrier
> > between setting "task->utrace = utrace" and changing ->utrace_flags.
> > We have unlock+lock in between, this implies mb().
>
> So this is the task_unlock() in utrace_task_alloc() and the
> spin_lock(&utrace->lock) in utrace_add_engine() ?

Yes, and see above. This UNLOCK+LOCK provides the necessary barrier.

> Talk about non-obvious.

Well, agreed. We should fix the comment, or just add the explicit mb().
I personally think the "good" comment is enough.

> > > > +struct utrace_engine_ops {
> > >
> > > > + u32 (*report_signal)(u32 action,
> > > > + struct utrace_engine *engine,
> > > > + struct task_struct *task,
> > > > + struct pt_regs *regs,
> > > > + siginfo_t *info,
> > > > + const struct k_sigaction *orig_ka,
> > > > + struct k_sigaction *return_ka);
> > >
> > > > + u32 (*report_clone)(enum utrace_resume_action action,
> > > > + struct utrace_engine *engine,
> > > > + struct task_struct *parent,
> > > > + unsigned long clone_flags,
> > > > + struct task_struct *child);
> > >
> > > > +};
> > >
> > > Seems inconsistent on u32 vs enum utrace_resume_action.
> >
> > Well, this u32 can hold utrace_resume_action | utrace_signal_action,
> > for example.
>
> Sure, but then make all of them u32, or unsigned int, as that gets mixed
> in below.

Not sure I understand, but if you meant all these callbacks should
return int/long then I'd personally agree.

> > > > +static inline int utrace_attach_delay(struct task_struct *target)
> > > > +{
> > > > + if ((target->flags & PF_STARTING) &&
> > > > + task_utrace_struct(current) &&
> > > > + task_utrace_struct(current)->cloning != target)
> > > > + do {
> > > > + schedule_timeout_interruptible(1);
> > > > + if (signal_pending(current))
> > > > + return -ERESTARTNOINTR;
> > > > + } while (target->flags & PF_STARTING);
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > Quite gross this.. can't we key off the
> > > tracehoook_report_clone_complete() and use a wakeup there?
> >
> > Yes, it would be very nice to avoid this schedule_timeout_interruptible().
> > But currently we don't see a simple solution, on the TODO list. But, to
> > clarify, this case is very unlikely.
>
> Use a global wait-queue and put a wakeup_all in
> tracehook_report_clone_complete() or wherever that ends up in utrace?

- First of all, can't resist, I think a global wait-queue for
this is just "ugly by definition". OK, please ignore, this
is non-technical and subjective.

- If we change tracehook_report_clone_complete() to do the wakeup,
this wakeup must be unconditional, it should be done even if the
forking thread is not traced.

The current code is extremly simple and works. Yes, yes, yes, any
schedule_timeout(1) in a loop asks for the better solution. I don't
know what Roland thinks, but as for me this problem is low priority,
currently I didn't really try to think how to make this code "better".
This case is very unlikely, and it is not a hot-path. This happens
when the unrelated process tries to attach to the new thread in the
middle of do_fork().

But there is another reason why personally I think we should improve
this code later (and the similar code in utrace_barrier). A few lines
below you wrote:

> > > Also, I'm not quite sure on why we play so many barrier games, looking
> > > at start_callback() we have 2 barriers in the callback loop, why not a
> > > per engine lock?
> >
> > Exactly to avoid the lock, I guess ;)
>
> I'm really thinking this code ought to get simplified a lot, there's too
> much non-obvious barriers.
>
> Then, and only if performance numbers show it, add back some of these
> optimizations, in simple small steps, that way its much easier to review
> as well.

and above you suggest to complicate utrace_attach_delay() before merging.

As for "in simple small steps", I agree, it would be much better. The
problem is, this code was developed out-of-tree. That is why we would
like to merge it asap, then do other changes which could be easily
reviewed.

Now, do you really mean we should throw out the working code, rewrite
it avoiding these barriers, and resubmit? Sure, everything is possible.
But this means another round of out-of-tree development with unclear
results.

Some barriers are easy, we can simply take utrace->lock. Some are not.
In no way (imho) we should use spinlocks in tracehooks. OK, we can
embed "struct utrace" back into task_struct, then it would be easy
to eliminate more barriers, not sure this would be a good change though.

> > > > + /*
> > > > + * In theory spin_lock() doesn't imply rcu_read_lock().
> > > > + * Once we clear ->utrace_flags this task_struct can go away
> > > > + * because tracehook_prepare_release_task() path does not take
> > > > + * utrace->lock when ->utrace_flags == 0.
> > > > + */
> > > > + rcu_read_lock();
> > > > + task->utrace_flags = flags;
> > > > + spin_unlock(&utrace->lock);
> > > > + rcu_read_unlock();
> > >
> > > yuck!
> > >
> > > why not simply keep a task reference over the utrace_reset call?
> >
> > Yes, we could use get_task_struct() instead. Not sure this would
> > be more clean, though.
>
> For one it would allow getting rid of that insane assymetric locking.

Well, this is subjective, but I don't agree that

get_task_struct(task);
task->utrace_flags = flags;
spin_unlock(&utrace->lock);
put_task_struct(task);

looks better. We can move get_task_struct (or rcu_read_lock) up, but
then the code becomes less obvious. This get/lock is only needed for
spin_unlock(&utrace->lock).

I'd wish we had rcu_read_lock_in_atomic (or whatever) which could be
used under spinlocks...

That said, if you think get_task_struct() can help to merge utrace
I personally do not care much.

> > > > +static inline void finish_callback_report(struct task_struct *task,
> > > > + struct utrace *utrace,
> > > > + struct utrace_report *report,
> > > > + struct utrace_engine *engine,
> > > > + enum utrace_resume_action action)
> > > > +{
> > > > + /*
> > > > + * If utrace_control() was used, treat that like UTRACE_DETACH here.
> > > > + */
> > > > + if (action == UTRACE_DETACH || engine->ops == &utrace_detached_ops) {
> > > > + engine->ops = &utrace_detached_ops;
> > > > + report->detaches = true;
> > > > + return;
> > > > + }
> > > > +
> > > > + if (action < report->action)
> > > > + report->action = action;
> > > > +
> > > > + if (action != UTRACE_STOP) {
> > > > + if (action < report->resume_action)
> > > > + report->resume_action = action;
> > > > +
> > > > + if (engine_wants_stop(engine)) {
> > > > + spin_lock(&utrace->lock);
> > > > + clear_engine_wants_stop(engine);
> > > > + spin_unlock(&utrace->lock);
> > > > + }
> > >
> > > Reads funny, but I guess it can only race the right way round?
> >
> > Not sure I understand... could you explain?
>
> It can only race with another thread also clearing the bit, not with one
> setting the bit? Otherwise there's no guarantee its not set after that
> if stmt.

We do not care if another thread (the owner of this engine) sets or clears
ENGINE_STOP in engine->flags. This can happen if the tracer is "insane" and
races with itself. Nothing bad can happen, the last change wins.

Oleg.

2009-12-08 15:29:29

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On Tue, 2009-12-08 at 16:04 +0100, Oleg Nesterov wrote:
> > > > > + /*
> > > > > + * In theory spin_lock() doesn't imply rcu_read_lock().
> > > > > + * Once we clear ->utrace_flags this task_struct can go away
> > > > > + * because tracehook_prepare_release_task() path does not take
> > > > > + * utrace->lock when ->utrace_flags == 0.
> > > > > + */
> > > > > + rcu_read_lock();
> > > > > + task->utrace_flags = flags;
> > > > > + spin_unlock(&utrace->lock);
> > > > > + rcu_read_unlock();
> > > >
> > > > yuck!
> > > >
> > > > why not simply keep a task reference over the utrace_reset call?
> > >
> > > Yes, we could use get_task_struct() instead. Not sure this would
> > > be more clean, though.
> >
> > For one it would allow getting rid of that insane assymetric locking.
>
> Well, this is subjective, but I don't agree that
>
> get_task_struct(task);
> task->utrace_flags = flags;
> spin_unlock(&utrace->lock);
> put_task_struct(task);
>
> looks better.

No, what I mean by assymetric locking is that utrace_reset() and
utrace_reap() drop the utrace->lock where their caller acquired it,
resulting in non-obvious like:

utrace_control()
{

...
spin_lock(&utrace->lock);

...

if (reset)
utrace_reset(utrace);
else
spin_unlock(&utrace->lock);
}

If you take a task ref you can write the much saner:

utrace_control()
{
...
spin_lock(&utrace->lock);
...
if (reset)
utrace_reset(utrace);

spin_unlock(&utrace->lock);
}

2009-12-08 15:36:03

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On Tue, 2009-12-08 at 16:04 +0100, Oleg Nesterov wrote:
> The
> problem is, this code was developed out-of-tree. That is why we would
> like to merge it asap, then do other changes which could be easily
> reviewed.
>
> Now, do you really mean we should throw out the working code, rewrite
> it avoiding these barriers, and resubmit? Sure, everything is possible.
> But this means another round of out-of-tree development with unclear
> results.

Out-of-tree development is bad, it having taken lot of effort is no
excuse for merging ugly.

Now, I'm not against barriers at all, but code that is as barrier heavy
as this, with such bad comments and no clear indication it was actually
worth using so many barriers make me wonder.

Barriers aren't free either, and having multiple such things in quick
succession isn't nessecarily faster than a lock, but much less obvious.

2009-12-08 16:37:13

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On 12/08, Peter Zijlstra wrote:
>
> On Tue, 2009-12-08 at 16:04 +0100, Oleg Nesterov wrote:
> >
> > Well, this is subjective, but I don't agree that
> >
> > get_task_struct(task);
> > task->utrace_flags = flags;
> > spin_unlock(&utrace->lock);
> > put_task_struct(task);
> >
> > looks better.
>
> No, what I mean by assymetric locking is that utrace_reset() and
> utrace_reap() drop the utrace->lock where their caller acquired it,
> resulting in non-obvious like:
>
> utrace_control()
> {
>
> ...
> spin_lock(&utrace->lock);
>
> ...
>
> if (reset)
> utrace_reset(utrace);
> else
> spin_unlock(&utrace->lock);
> }

Agreed, the code like this never looks good.

> If you take a task ref you can write the much saner:
>
> utrace_control()
> {
> ...
> spin_lock(&utrace->lock);
> ...
> if (reset)
> utrace_reset(utrace);
>
> spin_unlock(&utrace->lock);
> }

No, get_task_struct() in utrace_reset() can't help, we should move
it into utrace_control() then. And in this case it becomes even more
subtle: it is needed because ->utrace_flags may be cleared inside
utrace_reset() and after that utrace_control()->spin_unlock() becomes
unsafe.

Also. utrace_reset() drops utrace->lock to call put_detached_list()
lockless. If we want to avoid the assymetric locking, every caller
should pass "struct list_head *detached" to utrace_reset(), drop
utrace->lock, and call put_detached_list().

Oleg.

2009-12-08 17:57:10

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On 12/08, Peter Zijlstra wrote:
>
> On Tue, 2009-12-08 at 16:04 +0100, Oleg Nesterov wrote:
> > The
> > problem is, this code was developed out-of-tree. That is why we would
> > like to merge it asap, then do other changes which could be easily
> > reviewed.
> >
> > Now, do you really mean we should throw out the working code, rewrite
> > it avoiding these barriers, and resubmit? Sure, everything is possible.
> > But this means another round of out-of-tree development with unclear
> > results.
>
> Out-of-tree development is bad, it having taken lot of effort is no
> excuse for merging ugly.
>
> Now, I'm not against barriers at all, but code that is as barrier heavy
> as this, with such bad comments and no clear indication it was actually
> worth using so many barriers make me wonder.

Well. First of all, I agree at least partly. If you ask me, I feel
that in any case utrace needs more cleanups (in fact, like almost
any code in kernel) even if we forget about the barriers. In no
way utrace is finished or perfect. I think that Roland won't argue ;)

But. It would be much easier to do the futher development step by
step, patch by patch, which the changelogs, with the possibilty to
have the review. And it is much easier to change the code which is
already used by people. And, cleanups/simplifications are the most
hard part of the development.

However, of course I can't "prove" that the current code is "good
enough" for merging.

> Barriers aren't free either, and having multiple such things in quick
> succession isn't nessecarily faster than a lock, but much less obvious.

It is hardly possible to argue.

Oleg.

2009-12-08 18:19:46

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On Tue, 2009-12-08 at 17:31 +0100, Oleg Nesterov wrote:

> > If you take a task ref you can write the much saner:
> >
> > utrace_control()
> > {
> > ...
> > spin_lock(&utrace->lock);
> > ...
> > if (reset)
> > utrace_reset(utrace);
> >
> > spin_unlock(&utrace->lock);
> > }
>
> No, get_task_struct() in utrace_reset() can't help, we should move
> it into utrace_control() then. And in this case it becomes even more
> subtle: it is needed because ->utrace_flags may be cleared inside
> utrace_reset() and after that utrace_control()->spin_unlock() becomes
> unsafe.

The task->utrace pointer is cleaned up on
free_task()->tracehook_free_task()->utrace_free_task(), so by holding a
ref on the task, we ensure ->utrace stays around, and we can do
spin_unlock(), right?

> Also. utrace_reset() drops utrace->lock to call put_detached_list()
> lockless. If we want to avoid the assymetric locking, every caller
> should pass "struct list_head *detached" to utrace_reset(), drop
> utrace->lock, and call put_detached_list().

All that seems to do is call ->release() and kmem_cache_free()s the
utrace_engine thing, why can't that be done with utrace->lock held?

But yeah, passing that list along does seem like a better solution.

2009-12-08 18:43:40

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On 12/08, Peter Zijlstra wrote:
>
> On Tue, 2009-12-08 at 17:31 +0100, Oleg Nesterov wrote:
>
> > > If you take a task ref you can write the much saner:
> > >
> > > utrace_control()
> > > {
> > > ...
> > > spin_lock(&utrace->lock);
> > > ...
> > > if (reset)
> > > utrace_reset(utrace);
> > >
> > > spin_unlock(&utrace->lock);
> > > }
> >
> > No, get_task_struct() in utrace_reset() can't help, we should move
> > it into utrace_control() then. And in this case it becomes even more
> > subtle: it is needed because ->utrace_flags may be cleared inside
> > utrace_reset() and after that utrace_control()->spin_unlock() becomes
> > unsafe.
>
> The task->utrace pointer is cleaned up on
> free_task()->tracehook_free_task()->utrace_free_task(), so by holding a
> ref on the task, we ensure ->utrace stays around, and we can do
> spin_unlock(), right?

Yes. That is why utrace_control() (which does unlock) should take the ref,
not utrace_reset().

> > Also. utrace_reset() drops utrace->lock to call put_detached_list()
> > lockless. If we want to avoid the assymetric locking, every caller
> > should pass "struct list_head *detached" to utrace_reset(), drop
> > utrace->lock, and call put_detached_list().
>
> All that seems to do is call ->release() and kmem_cache_free()s the
> utrace_engine thing, why can't that be done with utrace->lock held?

We can, but then ->release() will be called in atomic context. Utrace
tries hard to not "restrict" the module writers.

> But yeah, passing that list along does seem like a better solution.

Well, it has multiple callers, everyone will be complicated.

Oleg.

2009-12-13 20:49:08

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

> All that seems to do is call ->release() and kmem_cache_free()s the
> utrace_engine thing, why can't that be done with utrace->lock held?

Calling ->release with a lock held is clearly insane, sorry. It is true
that any engine-writer who does anything like utrace_* calls inside their
release callback is doing things the wrong way. But guaranteeing that
simple mistakes result in spin-lock deadlocks just seems clearly wrong to
me. A main point of the utrace API is to make it easier to work in this
space and help you avoid writing the pathological bugs. Adding picayune
gotchas like this just does not help anyone. No other API callback is made
holding some internal implementation lock, and making this one the sole
exception seems just obviously ill-advised on its face. I can't really
imagine what a justification for such an obfuscation would be.

> But yeah, passing that list along does seem like a better solution.

So you find it cleaner to have each caller of utrace_reset take another
output parameter and be followed with the same exact source code duplicated
in each call site, than to have utrace_reset() do the unlock and then the
common code itself. I guess there is no accounting for taste.

We try not to get excited about trivia, so on matters like this one we will
do whatever the consensus of gate-keeping reviewers wants. My patch to
implement your suggestion adds 13 lines of source and 134 bytes of compiled
text (x86-64). Is that what you prefer?

I'll note that the code as it stands uses the __releases annotation for
sparse, as well as thoroughly documenting the locking details in comments.
I gather that clear explanation of the code is in your eyes no excuse for
ever writing code that requires one to actually read the comments. I'm not
sure that attitude can ever be satisfied by any code that is nontrivial or
makes any attempts at optimization.

Thanks,
Roland

2009-12-14 00:26:17

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

I'm sorry for the delay. I'm picking up with responding to the parts of
your review that I did not include in my first reply. I appreciate very
much the discussion you've had with Oleg about the issues that I did not
address myself. I look forward to your replies to my comments in that
first reply, which we have yet to see.

> Seems inconsistent on u32 vs enum utrace_resume_action.
>
> Why force enum utrace_resume_action into a u32?

The first argument to almost all callbacks (all the ones made when the
task is alive) is called "action" and it's consistent that its low bits
contain an enum utrace_resume_action. The argument is "u32 action" in
the report_signal and report_syscall_entry callbacks, where it combines
an enum utrace_resume_action with an enum utrace_{signal,syscall}_action
(respectively). It would indeed be more consistent to use "u32 action"
throughout, but it seemed nicer not to have engine writers always
writing "utrace_resume_action(action)" for all the cases where there are
no other bits in there.

The return value is a mirror of the "u32 action" argument. In many
calls, it has only an enum utrace_resume_action in it. But in some it
combines that with another enum utrace_*_action. There I went for
consistency (and less typing) in the return type, since it doesn't make
any difference to how you have to write the code in your callback
function. The main reason I used "u32" at all instead of "unsigned int"
is just its shortness for less typing.

I don't really mind changing these details to whatever people think is
best. The other people writing code to use the utrace API may have more
opinions than I do. I guess it could even be OK to make the return
value and "action" argument always a plain enum utrace_resume_action,
and use a second in/out "enum utrace_{signal,syscall}_action *"
parameter in those particular calls. But that does put some more
register pressure and loads/stores into this API.

> Seems inconsistent in the bitfield type, also it feels like that 3 the 7
> and the enum should be more tightly integrated, maybe add:
>
> UTRACE_RESUME_MAX
>
> #define UTRACE_RESUME_BITS (ilog2(UTRACE_RESUME_MAX))
> #define UTRACE_RESUME_MASK ((1 << UTRACE_RESUME_BITS) - 1)

Yes, that's a good cleanup. Thanks.
(ilog2(7) is 2, so ilog2() + 1 is what you meant.)

> > +static struct utrace_engine *matching_engine(
[...]
> The function does a search, suggesting the function name ought to have
> something like find or search in it.

Ok, I'll make it find_matching_engine.

> Quite gross this.. can't we key off the
> tracehoook_report_clone_complete() and use a wakeup there?

Yes, we intended to clean this up at some point later. But doing that
is just a distraction and complication right now so we put it off. For
this case, however, I suppose we could just punt for the initial version.

This code exists to support the special semantics of calling
utrace_attach_task() from inside the parent's report_clone() callback.
That guarantees the parent that it wins any race with any third thread
calling utrace_attach_task(). This guarantees it will be first attacher
in the callback order, but the general subject of callback order is
already something we think we will want to revisit in the future after
we have more experience with lots of different engines trying to work
together. It's most meaningful when using the UTRACE_ATTACH_EXCLUSIVE
flag--then you can use UTRACE_ATTACH_EXCLUSIVE|UTRACE_ATTACH_MATCH_OPS
to synchronize with other threads trying to attach the same kind of
engine to a task, and give special priority in that to the engine that
attaches in report_clone() from tracing the parent.

Really I came up with this special semantics originally just with ptrace
in mind. ptrace is an engine that wants to exclude other tracer threads
attaching asynchronously with the same kind of engine, and that wants to
give special priority on a child to the parent's tracer (to implement
PTRACE_O_TRACECLONE et al). However, Oleg's current ptrace code still
relies on the old hard-coded locking kludges to exclude the separate
attachers and to magically privilege the auto-attach from the parent's
tracer. So we are not relying on this special semantics yet.

We could just punt utrace_attach_delay() and remove the associated
documentation about the special rights of report_clone() calling
utrace_attach_task(). If we decide it helps clean things up later when
we finish more cleanup of the ptrace world, then we can add the fancy
semantics back in later.

> Does this really need the inline?

It has one caller and that call is unconditional.

> Asymmetric locking like this is really better not done, and looking at
> the callsites its really no bother to clean that up, arguably even makes
> them saner.

By "assymetric" you mean that utrace_reap releases a lock (as the
__releases annotation indicates). As should be obvious from the code, the
unlock is done before the loop that does ->report_reap callbacks and
utrace_engine_put() (which can make ->release callbacks). Surely you are
not suggesting that all these callbacks should be made with a spin lock
held, because that would obviously be quite insane.

I tried splitting utrace_reap() into two functions, so the callers make the
first call, then unlock, and then make the second call. Both callers do
this identically, so this just replaces
utrace_reap(task, utrace);
with:
prepare_utrace_reap(task, utrace);
spin_unlock(&utrace->lock);
finish_utrace_reap(task, utrace);
in two places. That change adds 13 source lines and 71 bytes of compiled
text (x86-64). Is that what you had in mind?

> You could codify locking assumptions like here using:
>
> lockdep_assert_held(&utrace->lock);
>
> Saves a comment and actually validates the assumption.

Ok, that is fine with me.

> > + /*
> > + * If ptrace is among the reasons for this stop, do its
> > + * notification now. This could not just be done in
> > + * ptrace's own event report callbacks because it has to
> > + * be done after we are in TASK_TRACED. This makes the
> > + * synchronization with ptrace_do_wait() work right.
> > + */
> > + ptrace_notify_stop(task);
>
> Well, this is a bit disappointing isn't it? So we cannot implement
> ptrace on utrace without special purpose hooks?

It's more of an issue that ptrace is built around special-purpose hooks
in wait. We do intend to clean this up later. But it is as much about
cleaning up the remaining deep insanity of the old ptrace implementation
as about giving utrace better facilities for this wrinkle of
synchronization. I don't doubt that the utrace API will get some
changes and improvements along the way as we move incrementally to all
of the ptrace internals being done in as clean and sane fashion as the
legacy ptrace userland ABI makes possible. But Oleg has not yet gotten
to that part of the the ptrace cleanup, and the actual problem that
necessitates this kludge for ptrace is not an issue at all for many
other uses of utrace that don't have to tie into a broken old model of
things. So being perfectly clean here is not something we should even
think we know how best to do yet, since there has been so little real
call for it. It would be counterproductive to make this perfection an
obstacle to incremental merging of the current utrace pieces that
already enable other kinds of new functionality.

> If its a programming error, WARN_ON might be appropriate, no point in
> being nice about it.

Sure, except that any WARN_ON shows up as "oops in utrace.c" and then
people think the bug is in utrace rather than in the caller.

> Seriously ugly, again. Use a wakeup where appropriate.

> Its not entirely clear why we can check pending_attach outside of the
> utrace->lock and not be racy.

I think it can be racy sometimes but that does not matter.
Maybe Oleg can verify my logic here. If it's right, he can
add some comments to make it more clear.

There is only a very limited sort of "timeliness" guarantee about
getting your callbacks after utrace_attach_task()+utrace_set_events().
If you know somehow that the task was definitely still in TASK_STOPPED
or TASK_TRACED after utrace_attach_task() returned, then your engine
gets all possible callbacks starting from when it resumes. Otherwise,
you can use utrace_control() with UTRACE_REPORT to ask to get some
callback "pretty soon". The only callback events you are ever 100%
guaranteed about (after success return from utrace_set_events()) are for
DEATH and REAP, which have an unconditional lock-and-check before making
engine callbacks.

In the stopped cases, there are lots of locks and barriers and things
after resuming. (Oleg?)

In the "pretty soon" case, that means set_notify_resume:
if (!test_and_set_tsk_thread_flag(task, TIF_NOTIFY_RESUME))
kick_process(task);
i.e. IPI after test_and_set. The test_and_set implies an smp_mb().
So it should be the case that the set of utrace->pending_attach was seen
before the set of TIF_NOTIFY_RESUME. In a race where the tracee just
saw utrace->pending_attach==0, then it has TIF_NOTIFY_RESUME set still
(or again), and so will go around again before getting back to user mode.

> > + if (!engine_wants_stop(engine)) {
> > + spin_lock(&utrace->lock);
> > + /*
> > + * If utrace_control() came in and detached us
> > + * before we got the lock, we must not stop now.
> > + */
> > + if (unlikely(engine->ops == &utrace_detached_ops))
> > + report->detaches = true;
> > + else
> > + mark_engine_wants_stop(task, engine);
> > + spin_unlock(&utrace->lock);
> > + }
> > +}
>
> I'm pretty sure that inline isn't really needed.

You mean mark_engine_wants_stop? You prefer repeating the two lines of
code to using a helper to encapsulate the magic? Really?

> Simply cond_resched() is sufficient, but that comment sucks, as it
> doesn't mention _why_ it is a good place.
>
> It seems to turn out that finish_callback() is always the last thing we
> do in the engine iterations in REPORT_CALLBACKS() and
> utrace_get_signal().

Oh, sorry. I didn't realize it wasn't obvious that finish_callback() is
called after every engine callback, given its name. I've changed it to:

/*
* We've just done an engine callback. These are allowed to sleep,
* though all well-behaved ones restrict that to blocking kalloc()
* or quickly-acquired mutex_lock() and the like. This is a good
* place to make sure tracing engines don't introduce too much
* latency under voluntary preemption.
*/
might_sleep();

> Also the documentation needs more whitespace, its very hard to digest in
> its current form.

Please make specific suggestions of exactly where you want what changes. I
tend to use paragraph breaks between groups of sentences that work together
interdependently to communicate a single idea, like they taught me to write
English in elementary school. In a few cases, I think the kernel-doc
formatting might force you to avoid extra blank lines or else you wind up
with the wrong kinds of grouping in HTML output or something (it's been a
while since I wrote some of the longer kernel-doc comments, and at the time
I looked carefully at how the make htmldocs and make mandocs results came
out to make them readable that way).

Of course the purpose of comments and kernel-doc is to communicate clearly.
So I am happy to amend the comments in whatever fashions make them more
effective. But you will have to cite exactly what you want.

Thanks,
Roland

2009-12-14 13:52:10

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On Sun, 2009-12-13 at 16:25 -0800, Roland McGrath wrote:
> I'm sorry for the delay. I'm picking up with responding to the parts of
> your review that I did not include in my first reply. I appreciate very
> much the discussion you've had with Oleg about the issues that I did not
> address myself. I look forward to your replies to my comments in that
> first reply, which we have yet to see.

Yeah, no worries, I'm not too quick these days myself..

> > Seems inconsistent on u32 vs enum utrace_resume_action.
> >
> > Why force enum utrace_resume_action into a u32?
>
> The first argument to almost all callbacks (all the ones made when the
> task is alive) is called "action" and it's consistent that its low bits
> contain an enum utrace_resume_action. The argument is "u32 action" in
> the report_signal and report_syscall_entry callbacks, where it combines
> an enum utrace_resume_action with an enum utrace_{signal,syscall}_action
> (respectively). It would indeed be more consistent to use "u32 action"
> throughout, but it seemed nicer not to have engine writers always
> writing "utrace_resume_action(action)" for all the cases where there are
> no other bits in there.

C does implicit casts from enum to integer types, right? So always using
u32 here would not impose any extra typing on the user, or am I missing
something subtle here?

> The return value is a mirror of the "u32 action" argument. In many
> calls, it has only an enum utrace_resume_action in it. But in some it
> combines that with another enum utrace_*_action. There I went for
> consistency (and less typing) in the return type, since it doesn't make
> any difference to how you have to write the code in your callback
> function. The main reason I used "u32" at all instead of "unsigned int"
> is just its shortness for less typing.
>
> I don't really mind changing these details to whatever people think is
> best. The other people writing code to use the utrace API may have more
> opinions than I do. I guess it could even be OK to make the return
> value and "action" argument always a plain enum utrace_resume_action,
> and use a second in/out "enum utrace_{signal,syscall}_action *"
> parameter in those particular calls. But that does put some more
> register pressure and loads/stores into this API.

I don't mind the sharing of the argument, it just looked odd to have the
u32/unsigned int/enum thing intermixed, since you care about typing
length (as good a criteria as any) I'd just be lazy and make everything
u32 ;-)

> > Quite gross this.. can't we key off the
> > tracehoook_report_clone_complete() and use a wakeup there?
>
> Yes, we intended to clean this up at some point later. But doing that
> is just a distraction and complication right now so we put it off. For
> this case, however, I suppose we could just punt for the initial version.
>
> This code exists to support the special semantics of calling
> utrace_attach_task() from inside the parent's report_clone() callback.
> That guarantees the parent that it wins any race with any third thread
> calling utrace_attach_task().

> This guarantees it will be first attacher
> in the callback order, but the general subject of callback order is
> already something we think we will want to revisit in the future after
> we have more experience with lots of different engines trying to work
> together.

> It's most meaningful when using the UTRACE_ATTACH_EXCLUSIVE
> flag -- then you can use UTRACE_ATTACH_EXCLUSIVE|UTRACE_ATTACH_MATCH_OPS
> to synchronize with other threads trying to attach the same kind of
> engine to a task, and give special priority in that to the engine that
> attaches in report_clone() from tracing the parent.

I broke the text so it reads easier for me, it might be me, it might not
be proper English -- I'm not a native speaker -- but this is an example
of what you asked for below.

The thing is, your sentences are rather long, with lots of sub-parts and
similar. I find I need a break after digesting a few such things.

As to the content, can't you accomplish the same thing by processing
such exclusive parent registration before exposing the child in the
pid-hash? Right before cgroup_fork_callback() in
kernel/fork.c:copy_process() seems like the ideal site.

> Really I came up with this special semantics originally just with ptrace
> in mind. ptrace is an engine that wants to exclude other tracer threads
> attaching asynchronously with the same kind of engine, and that wants to
> give special priority on a child to the parent's tracer (to implement
> PTRACE_O_TRACECLONE et al). However, Oleg's current ptrace code still
> relies on the old hard-coded locking kludges to exclude the separate
> attachers and to magically privilege the auto-attach from the parent's
> tracer. So we are not relying on this special semantics yet.
>
> We could just punt utrace_attach_delay() and remove the associated
> documentation about the special rights of report_clone() calling
> utrace_attach_task(). If we decide it helps clean things up later when
> we finish more cleanup of the ptrace world, then we can add the fancy
> semantics back in later.

Best would be to fix up the utrace-ptrace implementation and get rid of
those other kludges I think, not sure.. its all a bit involved and I'm
not at all sure I'm fully aware of all the ptrace bits.

> > Does this really need the inline?
>
> It has one caller and that call is unconditional.

Won't gcc inline it anyway in that case?

> > Asymmetric locking like this is really better not done, and looking at
> > the callsites its really no bother to clean that up, arguably even makes
> > them saner.
>
> By "assymetric" you mean that utrace_reap releases a lock (as the
> __releases annotation indicates). As should be obvious from the code, the
> unlock is done before the loop that does ->report_reap callbacks and
> utrace_engine_put() (which can make ->release callbacks). Surely you are
> not suggesting that all these callbacks should be made with a spin lock
> held, because that would obviously be quite insane.

Because there can be many engines attached?

> I tried splitting utrace_reap() into two functions, so the callers make the
> first call, then unlock, and then make the second call. Both callers do
> this identically, so this just replaces
> utrace_reap(task, utrace);
> with:
> prepare_utrace_reap(task, utrace);
> spin_unlock(&utrace->lock);
> finish_utrace_reap(task, utrace);
> in two places. That change adds 13 source lines and 71 bytes of compiled
> text (x86-64). Is that what you had in mind?

Or in case of utrace_reap() maybe push the spin_lock() into it?

> > > + /*
> > > + * If ptrace is among the reasons for this stop, do its
> > > + * notification now. This could not just be done in
> > > + * ptrace's own event report callbacks because it has to
> > > + * be done after we are in TASK_TRACED. This makes the
> > > + * synchronization with ptrace_do_wait() work right.
> > > + */
> > > + ptrace_notify_stop(task);
> >
> > Well, this is a bit disappointing isn't it? So we cannot implement
> > ptrace on utrace without special purpose hooks?
>
> It's more of an issue that ptrace is built around special-purpose hooks
> in wait. We do intend to clean this up later. But it is as much about
> cleaning up the remaining deep insanity of the old ptrace implementation
> as about giving utrace better facilities for this wrinkle of
> synchronization.

> I don't doubt that the utrace API will get some
> changes and improvements along the way as we move incrementally to all
> of the ptrace internals being done in as clean and sane fashion as the
> legacy ptrace userland ABI makes possible. But Oleg has not yet gotten
> to that part of the the ptrace cleanup, and the actual problem that
> necessitates this kludge for ptrace is not an issue at all for many
> other uses of utrace that don't have to tie into a broken old model of
> things.

> So being perfectly clean here is not something we should even
> think we know how best to do yet, since there has been so little real
> call for it. It would be counterproductive to make this perfection an
> obstacle to incremental merging of the current utrace pieces that
> already enable other kinds of new functionality.

Well, yes and no. Also I'm in no way a gate-keeper for acceptance of
this code, I just went through it and pointed out things I found 'odd'
about it.

The major improvement this utrace stuff brings over the old ptrace is
that it fully multiplexes the task tracing bits, however if the new bit
isn't powerful enough to express all of the old bits with that then that
seems to take the shine of the new bits.

I'm well aware that ptrace had some quirky bits in, and this might well
be one of the crazier parts of it, but to the un-initiated reviewer (me)
this could have done with a comment explaining exactly why this
particular site is not worth properly abstracting etc..

That is, you're presenting a patch for merger and have the wish that
people read it, yet half the comments on the rather brittle barrier
logic were incomplete or even flat out wrong and things like this are
not explained.

That does not paint a convincing picture.

> > If its a programming error, WARN_ON might be appropriate, no point in
> > being nice about it.
>
> Sure, except that any WARN_ON shows up as "oops in utrace.c" and then
> people think the bug is in utrace rather than in the caller.

Not if the comment right above the WARN_ON() says that its a clueless
caller.. but if you're really worried about it, use something like:

WARN(cond, "Dumb-ass caller\n");

> > Its not entirely clear why we can check pending_attach outside of the
> > utrace->lock and not be racy.
>
> I think it can be racy sometimes but that does not matter.
> Maybe Oleg can verify my logic here. If it's right, he can
> add some comments to make it more clear.
>
> There is only a very limited sort of "timeliness" guarantee about
> getting your callbacks after utrace_attach_task()+utrace_set_events().
> If you know somehow that the task was definitely still in TASK_STOPPED
> or TASK_TRACED after utrace_attach_task() returned, then your engine
> gets all possible callbacks starting from when it resumes.

> Otherwise,
> you can use utrace_control() with UTRACE_REPORT to ask to get some
> callback "pretty soon". The only callback events you are ever 100%
> guaranteed about (after success return from utrace_set_events()) are for
> DEATH and REAP, which have an unconditional lock-and-check before making
> engine callbacks.

OK, so in this case its a best effort thing, and any races are
non-destructive.

> In the stopped cases, there are lots of locks and barriers and things
> after resuming. (Oleg?)
>
> In the "pretty soon" case, that means set_notify_resume:
> if (!test_and_set_tsk_thread_flag(task, TIF_NOTIFY_RESUME))
> kick_process(task);
> i.e. IPI after test_and_set. The test_and_set implies an smp_mb().
> So it should be the case that the set of utrace->pending_attach was seen
> before the set of TIF_NOTIFY_RESUME.

Not exactly sure where the matching rmb() would come from in
start_report() and friends -- task_utrace_struct() ?

> In a race where the tracee just
> saw utrace->pending_attach==0, then it has TIF_NOTIFY_RESUME set still
> (or again), and so will go around again before getting back to user mode.
>
> > > + if (!engine_wants_stop(engine)) {
> > > + spin_lock(&utrace->lock);
> > > + /*
> > > + * If utrace_control() came in and detached us
> > > + * before we got the lock, we must not stop now.
> > > + */
> > > + if (unlikely(engine->ops == &utrace_detached_ops))
> > > + report->detaches = true;
> > > + else
> > > + mark_engine_wants_stop(task, engine);
> > > + spin_unlock(&utrace->lock);
> > > + }
> > > +}
> >
> > I'm pretty sure that inline isn't really needed.
>
> You mean mark_engine_wants_stop? You prefer repeating the two lines of
> code to using a helper to encapsulate the magic? Really?

No,

+static inline void finish_callback_report(struct task_struct *task,

That function looks rather large for an inline, and if its a single
callsite gcc will inline it anyway due to that static thing.

> > Simply cond_resched() is sufficient, but that comment sucks, as it
> > doesn't mention _why_ it is a good place.
> >
> > It seems to turn out that finish_callback() is always the last thing we
> > do in the engine iterations in REPORT_CALLBACKS() and
> > utrace_get_signal().
>
> Oh, sorry. I didn't realize it wasn't obvious that finish_callback() is
> called after every engine callback, given its name. I've changed it to:

Yeah, the name does seem to suggest some 'finish' of sorts, but it
wasn't clear until I traced its callchain back a little that it was the
tail of the for-each-engine loop.

> /*
> * We've just done an engine callback. These are allowed to sleep,
> * though all well-behaved ones restrict that to blocking kalloc()
> * or quickly-acquired mutex_lock() and the like. This is a good
> * place to make sure tracing engines don't introduce too much
> * latency under voluntary preemption.
> */
> might_sleep();

Right, might_sleep() captures more cases.

> > Also the documentation needs more whitespace, its very hard to digest in
> > its current form.
>
> Please make specific suggestions of exactly where you want what changes. I
> tend to use paragraph breaks between groups of sentences that work together
> interdependently to communicate a single idea, like they taught me to write
> English in elementary school.

> In a few cases, I think the kernel-doc
> formatting might force you to avoid extra blank lines or else you wind up
> with the wrong kinds of grouping in HTML output or something (it's been a
> while since I wrote some of the longer kernel-doc comments, and at the time
> I looked carefully at how the make htmldocs and make mandocs results came
> out to make them readable that way).
>
> Of course the purpose of comments and kernel-doc is to communicate clearly.
> So I am happy to amend the comments in whatever fashions make them more
> effective. But you will have to cite exactly what you want.

Right, so I've broken up some of the longer paragraphs, including the
penultimate quoted one, in order to illustrate what I meant. Like said
before, it might just be me.

2009-12-14 17:09:34

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On 12/13, Roland McGrath wrote:
>
> > Its not entirely clear why we can check pending_attach outside of the
> > utrace->lock and not be racy.
>
> I think it can be racy sometimes but that does not matter.
> Maybe Oleg can verify my logic here. If it's right, he can
> add some comments to make it more clear.
>
> There is only a very limited sort of "timeliness" guarantee about
> getting your callbacks after utrace_attach_task()+utrace_set_events().
> If you know somehow that the task was definitely still in TASK_STOPPED
> or TASK_TRACED after utrace_attach_task() returned, then your engine
> gets all possible callbacks starting from when it resumes. Otherwise,
> you can use utrace_control() with UTRACE_REPORT to ask to get some
> callback "pretty soon". The only callback events you are ever 100%
> guaranteed about (after success return from utrace_set_events()) are for
> DEATH and REAP, which have an unconditional lock-and-check before making
> engine callbacks.

Yes, I think this is correct. It is fine to miss ->pending_attach == T,
and in any case the new attacher can come right after the check, even
if it was checked under utrace->lock.

It is important that the tracee can't miss, say, UTRACE_REPORT request
(as you already explained), and every time the tracee clears ->resume
it calls splice_attaching().

> In the stopped cases, there are lots of locks and barriers and things
> after resuming. (Oleg?)

Every time the tracee resumes after TASK_TRACED it uses utrace->lock
to synchronize with utrace_control/etc, it must see any changes.

Oleg.

2009-12-14 17:47:39

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On 12/14, Peter Zijlstra wrote:
>
> > > Quite gross this.. can't we key off the
> > > tracehoook_report_clone_complete() and use a wakeup there?
> >
> > Yes, we intended to clean this up at some point later. But doing that
> > is just a distraction and complication right now so we put it off. For
> > this case, however, I suppose we could just punt for the initial version.
> >
> > This code exists to support the special semantics of calling
> > utrace_attach_task() from inside the parent's report_clone() callback.
> > That guarantees the parent that it wins any race with any third thread
> > calling utrace_attach_task().
>
> > This guarantees it will be first attacher
> > in the callback order, but the general subject of callback order is
> > already something we think we will want to revisit in the future after
> > we have more experience with lots of different engines trying to work
> > together.
>
> > It's most meaningful when using the UTRACE_ATTACH_EXCLUSIVE
> > flag -- then you can use UTRACE_ATTACH_EXCLUSIVE|UTRACE_ATTACH_MATCH_OPS
> > to synchronize with other threads trying to attach the same kind of
> > engine to a task, and give special priority in that to the engine that
> > attaches in report_clone() from tracing the parent.
>
> As to the content, can't you accomplish the same thing by processing
> such exclusive parent registration before exposing the child in the
> pid-hash? Right before cgroup_fork_callback() in
> kernel/fork.c:copy_process() seems like the ideal site.

I thought about this too, but there are some complications. Just for
example, what if copy_process() fails after we already reported the
new child was attached. And, the new child is not properly initialized
yet, it doesn't even have the valid pid/real_parent/etc. Just imagine
a callback wants to record task_pid_vnr(). Or it decides to send a
fatal signal (even private) to the new tracee.

> Best would be to fix up the utrace-ptrace implementation and get rid of
> those other kludges I think, not sure.. its all a bit involved and I'm
> not at all sure I'm fully aware of all the ptrace bits.

It is not that utrace-ptrace is buggy. We try to preserve the current
semantics. Yes, we can move this kludge to ptrace layer, but I am not
sure about other UTRACE_ATTACH_EXCLUSIVE engines.

If we move this to ptrace, then we can probably mark the new child
as "ptrace_attach() should fail, because we are going to auto-attach".
But in this case we need multiple hooks in do_fork() path again, like
the old ptrace does, while utrace needs only one.

> The major improvement this utrace stuff brings over the old ptrace is
> that it fully multiplexes the task tracing bits, however if the new bit
> isn't powerful enough to express all of the old bits with that then that
> seems to take the shine of the new bits.

Note that it would be easy to add another callback, and hide this
special case. But we should think twice before doing this.

> I'm well aware that ptrace had some quirky bits in, and this might well
> be one of the crazier parts of it, but to the un-initiated reviewer (me)
> this could have done with a comment explaining exactly why this
> particular site is not worth properly abstracting etc..

Well, agreed.

> > In the "pretty soon" case, that means set_notify_resume:
> > if (!test_and_set_tsk_thread_flag(task, TIF_NOTIFY_RESUME))
> > kick_process(task);
> > i.e. IPI after test_and_set. The test_and_set implies an smp_mb().
> > So it should be the case that the set of utrace->pending_attach was seen
> > before the set of TIF_NOTIFY_RESUME.
>
> Not exactly sure where the matching rmb() would come from in
> start_report() and friends -- task_utrace_struct() ?

I am a bit confused...

As Roland said, we don't have any "timing" guarantees, and we can't have.
Whatever we do, the tracee can miss, say, ->report_syscall_entry() event
even if it does a system call "after" utrace_set_events(UTRACE_EVENT_SYSCALL),
this is fine.

But it shouldn't miss TIF_NOTIFY_RESUME/signal_pending/etc. I mean,
"sooner or later" it must hanlde the signal or TIF_NOTIFY_RESUME. And in
this case we can't miss ->pending_attach.

IOW, we must ensure that if ever clear TIF_NOTIFY_RESUME we must not
miss ->pending_attach, correct? and for this case we have mb() after
clear_thread_flag(). Perhaps instead we should add mb__after_clear_bit()
into arch/ hooks, but this needs a lot of arch-dependent changes.

And, btw, the usage of ->replacement_session_keyring looks racy,
exactly because (without utracee) we done have the necessary barriers
on both sides.

Oleg.

2009-12-14 19:37:38

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On 12/14, Oleg Nesterov wrote:
>
> IOW, we must ensure that if ever clear TIF_NOTIFY_RESUME we must not
> miss ->pending_attach, correct? and for this case we have mb() after
> clear_thread_flag(). Perhaps instead we should add mb__after_clear_bit()
> into arch/ hooks, but this needs a lot of arch-dependent changes.

Cough. And I always read this "rmb" as "mb". Even when I changed
the comment to explain that we need a barrier between clear bit
and read flags, I didn't notice it is in fact rmb...

I guess we need the trivial fix, Roland?

Oleg.

2009-12-14 19:43:11

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

> On 12/14, Oleg Nesterov wrote:
> >
> > IOW, we must ensure that if ever clear TIF_NOTIFY_RESUME we must not
> > miss ->pending_attach, correct? and for this case we have mb() after
> > clear_thread_flag(). Perhaps instead we should add mb__after_clear_bit()
> > into arch/ hooks, but this needs a lot of arch-dependent changes.

Since it's utrace/tracehook code that relies on the barrier I think it
makes sense to have it in tracehook_notify_resume() or utrace_resume().
The arch requirement is having done clear_thread_flag() beforehand, so the
arch-independent code can reasonably assume whatever semantics that is
guaranteed to have.

> Cough. And I always read this "rmb" as "mb". Even when I changed
> the comment to explain that we need a barrier between clear bit
> and read flags, I didn't notice it is in fact rmb...
>
> I guess we need the trivial fix, Roland?

You're the barrier man, send me what changes it should get.

Thanks,
Roland

2009-12-14 19:45:12

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

> Yes, I think this is correct. It is fine to miss ->pending_attach == T,
> and in any case the new attacher can come right after the check, even
> if it was checked under utrace->lock.

Right.

> It is important that the tracee can't miss, say, UTRACE_REPORT request
> (as you already explained), and every time the tracee clears ->resume
> it calls splice_attaching().

Right.

> > In the stopped cases, there are lots of locks and barriers and things
> > after resuming. (Oleg?)
>
> Every time the tracee resumes after TASK_TRACED it uses utrace->lock
> to synchronize with utrace_control/etc, it must see any changes.

And TASK_STOPPED?

Please send me patches to add whatever comments would make all this clear
enough to Peter when reading the code.

Thanks,
Roland

2009-12-14 20:30:51

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

On 12/14, Roland McGrath wrote:
>
> > > In the stopped cases, there are lots of locks and barriers and things
> > > after resuming. (Oleg?)
> >
> > Every time the tracee resumes after TASK_TRACED it uses utrace->lock
> > to synchronize with utrace_control/etc, it must see any changes.
>
> And TASK_STOPPED?

SIGCONT can wake up the TASK_STOPPED tracee. I don't think the tracer
should ever rely on TASK_STOPPED (utrace never does). If the tracer
needs the "really stopped" tracee we have UTRACE_STOP, and this means
TASK_TRACED.

But I am not sure I understand this part of discussion... In any case
the tracee should see any changes which were done before the wakeup.
But TASK_STOPPED can't guarantee the tracee won't be resumed until
the tracer wakes it up. Of course, TASK_TRACED can't prevent SIGKILL,
but in this case we should only care about "the tracee can't resume
until we drop utrace->lock" case.

Oleg.

2009-12-15 02:59:56

[permalink] [raw]

Subject: Re: [RFC,PATCH 14/14] utrace core

> SIGCONT can wake up the TASK_STOPPED tracee. I don't think the tracer
> should ever rely on TASK_STOPPED (utrace never does). If the tracer
> needs the "really stopped" tracee we have UTRACE_STOP, and this means
> TASK_TRACED.

Ah, yes. utrace_control(task,,UTRACE_STOP) while task is in TASK_STOPPED
means turning it into TASK_TRACED.

Thanks,
Roland

2009-12-16 11:18:48