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

[Roland McGrath]

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

Ah!  Sorry, I misread you.  Yes, good point.

> 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().

Ok, this is exactly the sort of thing that merits explicit comments.

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

If you look at where task_utrace_struct() is used, it's basically always
like this.  All the tracehook.h callers into utrace.c do that.

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

Right.  The hot path is that you test task->utrace_flags and it's zero.
It seems better not to add anything else to that path, not even lfence.

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

But that's not the only place, is it?  Every utrace_report_* and most every
other utrace.c entry point is called from tracehook.h code using this pattern.
Those can have the same issue.  So you'd have to make all of those do an:
	if (unlikely(!utrace))
		return;
sort of check.  Is that what you mean?

I guess we'd need some chip-knowledgeable microoptimizers to tell us
whether:
	smp_rmb(); /* guarantees we can't be here seeing utrace==NULL */
	utrace = task->utrace;
is better or worse than:
	utrace = task->utrace;
	if (unlikely(!utrace))
		return;
on today's chips.

The latter is more hairy for the utrace.c code to always be sure to check.
But it's not prohibitively so.


Thanks,
Roland