[PATCH] utrace: utrace_attach_task() forgets to return when ->utrace == NULL

[Oleg Nesterov]

On 11/16, Roland McGrath wrote:
>
> > 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.)

Yes, sure. I meant, we shouldn't worry that this barrier adds too much
overhead to task_utrace_struct().

> > 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?

Yes,

> This only needs smp_rmb(), right?

I think yes.

> 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);

We don't have the explicit barrier, but I think it is not needed.
In this case utrace_attach_task() does unlock+lock at least once
before changing ->utrace_flags, this implies mb().

>  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;
>  }

Yes, I think this patch should close this (pure theoretical) problem.

But this smp_rmb() in task_utrace_struct() is only needed when the
caller does something like

	if (task_utrace_flags(tsk))
		do_something(task_utrace_struct());

it would be more logical to move this rmb() info task_utrace_flags().
However task_utrace_flags() must be fast.


Perhaps, to avoid this smp_rmb(), we can change

	bool utrace_interrupt_pending(void)
	{
		struct utrace *utrace = task_utrace_struct(current);
		return utrace && utrace->resume == UTRACE_INTERRUPT;
	}

I dunno.

Oleg.