On Thu, Mar 04, 2021 at 11:06:21AM +0100, Martin Pieuchot wrote:
> 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 "deep" parking point is the one in sys_execve() for
> obvious reasons.
Actually this is where I got confused. This is the place where "deep" is
the wrong word. The fact that SINGLE_UNWIND will abort the
single_thread_set() if another thread is in the process to run single
threaded should probably be added as a comment here. In the end this is
here to prevent a race between two threads calling execve() at the same
time.
> 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.
No, the current names are OK. I prefer to know what the end result is
(UNWIND vs EXIT vs SUSPEND) than from where it was called. Especially
since sys_execve() uses both UNWIND and EXIT.
--
:wq Claudio
