> Just in case fyi, I cooked the almost identical patch yesterday, but
> it didn't help: make xcheck crashes. Not that I really expected it can
> help on x86.
Right.
> Not sure I understand exactly... Yes, sometimes we know we don't need
> read_barrier_depends(). For example,
>
> @@ -302,7 +309,7 @@ struct utrace_engine *utrace_attach_task(
> if (!utrace) {
> if (unlikely(!utrace_task_alloc(target)))
> return ERR_PTR(-ENOMEM);
> - utrace = target->utrace;
> + utrace = task_utrace_struct(target);
> }
>
> I think this change is not necessary (but imho good anyway), this path
> must take task_lock() and thus it must always see the correct *utrace.
Ok. If we were to change it we should add a comment about that.
> But read_barrier_depends() does nothig except on alpha, why should we
> care?
I thought this was the barrier you were talking about. Anyway, we should
be pedantically correct about these. (People do even still build Linux/Alpha.)
> This reminds me, utrace_interrupt_pending() and similar code needs rmb()
> or task->utrace != NULL check.
I take it you mean that:
if (task->utrace_flags)
... use task->utrace ...
paths need a barrier to ensure task->utrace_flags read nonzero implies
task->utrace will be read non-null when racing with the first attach.
Right?
This only needs smp_rmb(), right? Is there an existing explicit barrier
this pairs with? The corresponding smp_wmb() needs to be somewhere between
utrace_task_alloc()'s "task->utrace = utrace;" and utrace_add_engine():
if (!target->utrace_flags) {
target->utrace_flags = UTRACE_EVENT(REAP);
Right? A lot goes on in between there that probably has some implied
barriers (task_unlock, kmem_cache_alloc, spin_lock). But do we really
have the exact barrier we need? Or maybe we need:
/*
* This barrier makes sure the initialization of the struct
* precedes the installation of the pointer. This pairs
* with read_barrier_depends() in task_utrace_struct().
*/
wmb();
task->utrace = utrace;
/*
* This barrier pairs with task_utrace_struct()'s smp_rmb().
* It ensures this task->utrace store is always seen before
* task->utrace_flags is first set in utrace_add_engine().
*/
smp_wmb();
This is basically all the tracehook.h paths calling into utrace.
I think the easiest thing to do is to assume that use pattern and
also roll this barrier into task_utrace_struct().
diff --git a/include/linux/utrace.h b/include/linux/utrace.h
index 8924783..0000000 100644
--- a/include/linux/utrace.h
+++ b/include/linux/utrace.h
@@ -157,7 +157,18 @@ static inline unsigned long task_utrace_
static inline struct utrace *task_utrace_struct(struct task_struct *task)
{
- struct utrace *utrace = task->utrace;
+ 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.
+ */
+ smp_rmb();
+ utrace = task->utrace;
+
read_barrier_depends(); /* See utrace_task_alloc(). */
return utrace;
}
Is that what you have in mind?
Thanks,
Roland
