On Tue, Dec 01, 2020 at 10:46:00AM -0300, Martin Pieuchot wrote:
> 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?
Did you run a make build with that smr_barrier() in it and checked that it
does not cause a slow down? I am sceptical, smr_barrier() is a very slow
construct which introduces large delays and should be avoided whenever
possible.
--
:wq Claudio
