On 04/03/21(Thu) 10:36, Claudio Jeker wrote:
> On Thu, Mar 04, 2021 at 10:28:50AM +0100, Martin Pieuchot wrote:
> > SINGLE_PTRACE has almost the same semantic as SINGLE_SUSPEND. The
> > difference is that there's no need to wait for other threads to be
> > parked.
> >
> > Diff below changes single_thread_set() to be explicit when waiting is
> > required. This allows us to get rid of SINGLE_PTRACE now and soon to
> > use SINGLE_SUSPEND around proc_stop(), even when the thread is not being
> > traced.
> >
> > ok?
> >
>
>
> > @@ -2000,14 +2000,12 @@ single_thread_check(struct proc *p, int
> > * where the other threads should stop:
> > * - SINGLE_SUSPEND: stop wherever they are, will later either be told to
> > exit
> > * (by setting to SINGLE_EXIT) or be released (via
> > single_thread_clear())
> > - * - SINGLE_PTRACE: stop wherever they are, will wait for them to stop
> > - * later (via single_thread_wait()) and released as with SINGLE_SUSPEND
> > * - SINGLE_UNWIND: just unwind to kernel boundary, will be told to exit
> > * or released as with SINGLE_SUSPEND
> > * - SINGLE_EXIT: unwind to kernel boundary and exit
> > */
> > int
> > -single_thread_set(struct proc *p, enum single_thread_mode mode, int deep)
> > +single_thread_set(struct proc *p, enum single_thread_mode mode, int wait)
> > {
> > struct process *pr = p->p_p;
> > struct proc *q;
> > @@ -2016,7 +2014,7 @@ single_thread_set(struct proc *p, enum s
> > KASSERT(curproc == p);
> >
> > SCHED_LOCK(s);
> > - error = single_thread_check_locked(p, deep, s);
> > + error = single_thread_check_locked(p, (mode == SINGLE_UNWIND), s);
>
> Either the comment above or the code itself are not correct.
> SINGLE_EXIT is also supposed to unwind according to comment.
The comment documents what sibling threads are supposed to do once the
current one has called single_thread_set() with a given SINGLE_* option.
Sibling threads will continue to execute until the next parking point
where single_thread_check() are. Parking points are divided in two
categories. In the "deep" ones unwinding is preferred for UNWIND and
EXIT, in the others only context switching occurs.
Every single_thread_set() call is in itself a parking point to prevent
races. The only "deap" parking point is the one in sys_execve() for
obvious reasons.
So maybe we should rename SINGLE_UNWIND into SINGLE_EXEC, would that be
clearer? If we go this road we might want to rename SINGLE_SUSPEND to
SINGLE_STOP to better describe the reason for parking sibling threads.
