On 01/12/20(Tue) 21:47, Jonathan Matthew wrote:
> On Tue, Dec 01, 2020 at 10:31:43AM +0100, Claudio Jeker wrote:
> > On Mon, Nov 30, 2020 at 07:10:47PM -0300, Martin Pieuchot wrote:
> > > Every multi-threaded process keeps a list of threads in `ps_threads'.
> > > This list is iterated in interrupt and process context which makes it
> > > complicated to protect it with a rwlock.
> > >
> > > One of the places where such iteration is done is inside the tsleep(9)
> > > routines, directly in single_thread_check() or via CURSIG(). In order
> > > to take this code path out of the KERNEL_LOCK(), claudio@ proposed to
> > > use SMR_TAILQ. This has the advantage of not introducing lock
> > > dependencies and allow us to address every iteration one-by-one.
> > >
> > > Diff below is a first step into this direction, it replaces the existing
> > > TAILQ_* macros by the locked version of SMR_TAILQ*. This is mostly lifted
> > > from claudio@'s diff and should not introduce any side effect.
> > >
> > > ok?
> > >
> > > diff --git sys/uvm/uvm_glue.c sys/uvm/uvm_glue.c
> > > index 390307c4c81..40a10e4c1c5 100644
> > > --- sys/uvm/uvm_glue.c
> > > +++ sys/uvm/uvm_glue.c
> > > @@ -369,7 +369,7 @@ uvm_swapout_threads(void)
> > > * the smallest p_slptime
> > > */
> > > slpp = NULL;
> > > - TAILQ_FOREACH(p, &pr->ps_threads, p_thr_link) {
> > > + SMR_TAILQ_FOREACH_LOCKED(p, &pr->ps_threads, p_thr_link) {
> > > switch (p->p_stat) {
> > > case SRUN:
> > > case SONPROC:
> > >
> >
> > Why did you not include the smr_call() to safely free struct proc in this
> > diff?
So we can discuss it separately, I'm happy to do it now :)
> I was wondering about this too. Freeing the struct proc is already delayed
> by some amount since it happens in the reaper or in the parent process, does
> it make sense to combine that with the SMR wait?
exit1() for a multi-threaded process does the following:
atomic_setbits_int(&p->p_flag, P_WEXIT);
single_thread_check(p, 0); // directly or via single_thread_set()
SMR_TAILQ_REMOVE_LOCKED(&pr->ps_threads, p, p_thr_link);
Note that exit1() can't be called in parallel.
If exit1() is called with EXIT_NORMAL, the current thread will start by
setting `ps_single' then iterate over `ps_thread'. This is done before
updating `ps_thread' so there's no race in that case.
If exit1() is called with EXIT_THREAD, the current thread might end up
removing itself from `ps_thread' while another one is iterating over it.
Since we're currently concerned about the iteration in single_thread_set()
notice that `P_WEXIT' is checked first. So if my understanding is correct
is should be enough to do the following:
SMR_TAILQ_REMOVE_LOCKED(&pr->ps_threads, p, p_thr_link);
smr_barrier();
That would delays exit1() a bit but doesn't involve callback which is
IMHO simpler.
Did I miss anything?
