[PATCH 5/6] finish_utrace_stop: check ->stopped lockless

[Roland McGrath]

> Of course, we can have the false positive. Even simpler: by the time
> finish_utrace_stop() takes utrace->lock the tracer can clear ->stopped
> even if it was really set after schedule().

Right.  It didn't even occur to me that the only scenario I thought of was
a false positive.  I was just thinking through the statement you made about
why lockless was OK ("we will have set it ourselves").

> But I think we do not care. What we do care is: ->stopped must be F
> after finish_utrace_stop(), whoever checks it under utrace->lock.

We do care about a false positive if it makes finish_utrace_stop() hit its
WARN_ON and return true when there was no SIGKILL.

> when utrace_do_stop() sets ->stopped, we hold ->siglock, this means
> we can't race with SIGCONT/SIGKILL. If the signal somes after that,
> we can rely on rq->lock, both try_to_wake_up() and schedule() take
> this lock, it should act as a barrier. I think.

Ok.  If we are in fact relying on any implicit barrier semantics like
that, we should have a clear comment about it.

> Perhaps it makes sense to move utrace_stop()->recalc_sigpending()
> code up, before finish_utrace_stop(). This way it would be a bit
> clearer why we can't race with signals.

Ok.  I'm not quite seeing just now why this is clearer.  But if you
have a patch with comments in it, I will probably understand why.

> BTW, I think that finish_utrace_stop() doesn't need ->siglock to
> check the pending SIGKILL.

Shouldn't one always hold siglock to use task->pending at all?

> Anyway, this change is very minor. The only reason I sent this patch
> is that I spent some time trying to understand which races this
> unconditional spin_lock(&utrace->lock) tries to close.

Good!  Indeed I don't want to waste too much time on minor things, and
we can always clean up more later.  But it is crucial that you are
thinking thoroughly about all the locking and race questions in the
code, so that is always time well spent.

> I think you are right, and ->stopped _can_ go away. But, as a devil's
> advocate, I'd like to give a couple of weak arguments against this
> change.

Sure!  We want to go whichever way makes the code overall easiest to
follow, review, and be convinced it's right.

> 	- Currently, if we see ->stopped == T under utrace->lock we
> 	  know that the tracee can do nothing interesting from utrace
> 	  pov. It can't be waked up by utrace_wakeup(). If the tracee
> 	  is killed it must take ->lock to clear ->stopped before it
> 	  can do anything else.

Right.  My original thinking was that it must always get into
utrace_get_signal() right then, and that would be responsible for any
synchronization that mattered.

> 	  If we remove ->stopped, we can't rely on TASK_TRACED in the
> 	  same manner. For example, a killed tracee can can call
> 	  utrace_report_jctl()->REPORT() while utrace thinks it is
> 	  stopped.

You mean the tracehook_notify_jctl(why, CLD_CONTINUED) case, right?
That is literally the only thing that happens after do_signal_stop()
wakes up and before utrace_get_signal() is called.

I wonder if it might not make sense anyway to suppress this for the
SIGKILL case.  Not just the tracing, but the entire CLD_CONTINUED
notification.  A parent doesn't really need a SIGCHLD,CLD_CONTINUED
immediately followed by a SIGCHLD,CLD_KILLED.  

OTOH, there is still the general point.  utrace_get_signal() doesn't
take the utrace lock unconditionally, and I don't think we want it to.
Even if the only thing that happens is dequeuing SIGKILL, then that
could still get to utrace_report_exit().

> 	- The exiting task with _UTRACE_DEATH_EVENTS can be considered
	      ^dead/dying       ^out
> 	  as quiescent. But, without ->stopped, looking at this task we
> 	  can't know if some engine wants this task to be stopped. IOW,
> 	  if we see such a task we can't figure out was utrace_do_stop()
> 	  called or not.

(I try to reserve "exiting" for PF_EXITING and "dying" for "running
with ->exit_state set", because the difference is important.)

Who cares?  There is no meaning to UTRACE_STOP for a dying or dead
task.  It counts as quiescent for purposes of utrace_control knowing
what it can do, that's all.  If it's not quiescent yet, it won't ever
"stop", it will just finish dying (i.e. report_death callbacks will
finish).

> 	- I _think_ that ->stopped makes the code a bit more readable and
> 	  understandable. More "explicit". But this is really subjective.

Ok.  I'll leave it to you.  My first reaction was just that fewer
state bits means less bookkeeping to convince oneself is correct,
so easier to read in that way.  But it's not a strong opinion.

> On the other hand I agree with your arguments. And, as you pointed out,
> we can kill utrace_finish_jctl().
> 
> In short - I do not know.

I don't either.  Your first point above is fairly compelling.
OTOH we can just keep utrace_finish_jctl() and have it and
finish_utrace_stop() just always take the utrace lock purely
to synchronize (and then drop it without changing anything).
Then we'd have the invariant that both the task itself going
into or out of TASK_TRACED and the waker always hold the utrace
lock, without ->stopped.  Thus (leaving aside the dying task issue)
TASK_TRACED gives all the guarantees you cited for ->stopped.


Thanks,
Roland