On Wed, Dec 02, 2020 at 05:27:59PM +1000, Jonathan Matthew wrote:
> On Tue, Dec 01, 2020 at 02:35:18PM -0300, Martin Pieuchot wrote:
> > On 01/12/20(Tue) 15:30, Claudio Jeker wrote:
> > > [...]
> > > 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.
> >
> > I did build GENERIC.MP multiple times on a 4CPU sparc64 with the diff
> > below, without noticeable difference.
> >
> > I'm happy to hear from sceptical performance checkers :o)
>
> On a reasonably fast amd64 box, this increases GENERIC.MP make -j6 build
> time from ~3m06s to ~3m44s, which seems a bit much to me.
The more CPU the worse smr_barrier() behaves since it will require the
callback to be run on all cpus one after the other. This is why
smr_barrier() should not be used in any performance critical path.
> Replacing smr_barrier() with smr_flush() reduces the overhead to a couple of
> seconds, and it seems warranted here.
I still think that using smr_call() here is the best approach. I will dig
out my diff once mpi@ committed his tailq conversion.
> >
> > diff --git lib/libkvm/kvm_proc2.c lib/libkvm/kvm_proc2.c
> > index 96f7dc91b92..1f4f9b914bb 100644
> > --- lib/libkvm/kvm_proc2.c
> > +++ lib/libkvm/kvm_proc2.c
> > @@ -341,8 +341,9 @@ kvm_proclist(kvm_t *kd, int op, int arg, struct process
> > *pr,
> > kp.p_pctcpu = 0;
> > kp.p_stat = (process.ps_flags & PS_ZOMBIE) ? SDEAD :
> > SIDL;
> > - for (p = TAILQ_FIRST(&process.ps_threads); p != NULL;
> > - p = TAILQ_NEXT(&proc, p_thr_link)) {
> > + for (p = SMR_TAILQ_FIRST_LOCKED(&process.ps_threads);
> > + p != NULL;
> > + p = SMR_TAILQ_NEXT_LOCKED(&proc, p_thr_link)) {
> > if (KREAD(kd, (u_long)p, &proc)) {
> > _kvm_err(kd, kd->program,
> > "can't read proc at %lx",
> > @@ -376,8 +377,8 @@ kvm_proclist(kvm_t *kd, int op, int arg, struct process
> > *pr,
> > if (!dothreads)
> > continue;
> >
> > - for (p = TAILQ_FIRST(&process.ps_threads); p != NULL;
> > - p = TAILQ_NEXT(&proc, p_thr_link)) {
> > + for (p = SMR_TAILQ_FIRST_LOCKED(&process.ps_threads); p != NULL;
> > + p = SMR_TAILQ_NEXT_LOCKED(&proc, p_thr_link)) {
> > if (KREAD(kd, (u_long)p, &proc)) {
> > _kvm_err(kd, kd->program,
> > "can't read proc at %lx",
> > diff --git sys/kern/exec_elf.c sys/kern/exec_elf.c
> > index 5e455208663..575273b306c 100644
> > --- sys/kern/exec_elf.c
> > +++ sys/kern/exec_elf.c
> > @@ -85,6 +85,7 @@
> > #include <sys/signalvar.h>
> > #include <sys/stat.h>
> > #include <sys/pledge.h>
> > +#include <sys/smr.h>
> >
> > #include <sys/mman.h>
> >
> > @@ -1360,7 +1361,7 @@ coredump_notes_elf(struct proc *p, void *iocookie,
> > size_t *sizep)
> > * threads in the process have been stopped and the list can't
> > * change.
> > */
> > - TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
> > + SMR_TAILQ_FOREACH_LOCKED(q, &pr->ps_threads, p_thr_link) {
> > if (q == p) /* we've taken care of this thread */
> > continue;
> > error = coredump_note_elf(q, iocookie, ¬esize);
> > diff --git sys/kern/init_main.c sys/kern/init_main.c
> > index fed6be19435..2b657ffe328 100644
> > --- sys/kern/init_main.c
> > +++ sys/kern/init_main.c
> > @@ -519,7 +519,7 @@ main(void *framep)
> > */
> > LIST_FOREACH(pr, &allprocess, ps_list) {
> > nanouptime(&pr->ps_start);
> > - TAILQ_FOREACH(p, &pr->ps_threads, p_thr_link) {
> > + SMR_TAILQ_FOREACH_LOCKED(p, &pr->ps_threads, p_thr_link) {
> > nanouptime(&p->p_cpu->ci_schedstate.spc_runtime);
> > timespecclear(&p->p_rtime);
> > }
> > diff --git sys/kern/kern_exit.c sys/kern/kern_exit.c
> > index a20775419e3..3c526ab83b8 100644
> > --- sys/kern/kern_exit.c
> > +++ sys/kern/kern_exit.c
> > @@ -63,6 +63,7 @@
> > #ifdef SYSVSEM
> > #include <sys/sem.h>
> > #endif
> > +#include <sys/smr.h>
> > #include <sys/witness.h>
> >
> > #include <sys/mount.h>
> > @@ -161,7 +162,8 @@ exit1(struct proc *p, int xexit, int xsig, int flags)
> > }
> >
> > /* unlink ourselves from the active threads */
> > - TAILQ_REMOVE(&pr->ps_threads, p, p_thr_link);
> > + SMR_TAILQ_REMOVE_LOCKED(&pr->ps_threads, p, p_thr_link);
> > + smr_barrier();
> > if ((p->p_flag & P_THREAD) == 0) {
> > /* main thread gotta wait because it has the pid, et al */
> > while (pr->ps_refcnt > 1)
> > @@ -724,7 +726,7 @@ process_zap(struct process *pr)
> > if (pr->ps_ptstat != NULL)
> > free(pr->ps_ptstat, M_SUBPROC, sizeof(*pr->ps_ptstat));
> > pool_put(&rusage_pool, pr->ps_ru);
> > - KASSERT(TAILQ_EMPTY(&pr->ps_threads));
> > + KASSERT(SMR_TAILQ_EMPTY_LOCKED(&pr->ps_threads));
> > lim_free(pr->ps_limit);
> > crfree(pr->ps_ucred);
> > pool_put(&process_pool, pr);
> > diff --git sys/kern/kern_fork.c sys/kern/kern_fork.c
> > index 9fb239bc8b4..e1cb587b2b8 100644
> > --- sys/kern/kern_fork.c
> > +++ sys/kern/kern_fork.c
> > @@ -52,6 +52,7 @@
> > #include <sys/acct.h>
> > #include <sys/ktrace.h>
> > #include <sys/sched.h>
> > +#include <sys/smr.h>
> > #include <sys/sysctl.h>
> > #include <sys/pool.h>
> > #include <sys/mman.h>
> > @@ -179,8 +180,8 @@ process_initialize(struct process *pr, struct proc *p)
> > {
> > /* initialize the thread links */
> > pr->ps_mainproc = p;
> > - TAILQ_INIT(&pr->ps_threads);
> > - TAILQ_INSERT_TAIL(&pr->ps_threads, p, p_thr_link);
> > + SMR_TAILQ_INIT(&pr->ps_threads);
> > + SMR_TAILQ_INSERT_TAIL_LOCKED(&pr->ps_threads, p, p_thr_link);
> > pr->ps_refcnt = 1;
> > p->p_p = pr;
> >
> > @@ -557,7 +558,7 @@ thread_fork(struct proc *curp, void *stack, void *tcb,
> > pid_t *tidptr,
> >
> > LIST_INSERT_HEAD(&allproc, p, p_list);
> > LIST_INSERT_HEAD(TIDHASH(p->p_tid), p, p_hash);
> > - TAILQ_INSERT_TAIL(&pr->ps_threads, p, p_thr_link);
> > + SMR_TAILQ_INSERT_TAIL_LOCKED(&pr->ps_threads, p, p_thr_link);
> >
> > /*
> > * if somebody else wants to take us to single threaded mode,
> > diff --git sys/kern/kern_proc.c sys/kern/kern_proc.c
> > index 70b781e53cb..a2acc741834 100644
> > --- sys/kern/kern_proc.c
> > +++ sys/kern/kern_proc.c
> > @@ -502,7 +502,7 @@ db_kill_cmd(db_expr_t addr, int have_addr, db_expr_t
> > count, char *modif)
> > return;
> > }
> >
> > - p = TAILQ_FIRST(&pr->ps_threads);
> > + p = SMR_TAILQ_FIRST_LOCKED(&pr->ps_threads);
> >
> > /* Send uncatchable SIGABRT for coredump */
> > sigabort(p);
> > @@ -558,7 +558,7 @@ db_show_all_procs(db_expr_t addr, int haddr, db_expr_t
> > count, char *modif)
> > while (pr != NULL) {
> > ppr = pr->ps_pptr;
> >
> > - TAILQ_FOREACH(p, &pr->ps_threads, p_thr_link) {
> > + SMR_TAILQ_FOREACH_LOCKED(p, &pr->ps_threads, p_thr_link) {
> > #ifdef MULTIPROCESSOR
> > if (__mp_lock_held(&kernel_lock, p->p_cpu))
> > has_kernel_lock = 1;
> > diff --git sys/kern/kern_resource.c sys/kern/kern_resource.c
> > index 0111bfa733a..811b8434365 100644
> > --- sys/kern/kern_resource.c
> > +++ sys/kern/kern_resource.c
> > @@ -212,7 +212,7 @@ donice(struct proc *curp, struct process *chgpr, int n)
> > return (EACCES);
> > chgpr->ps_nice = n;
> > SCHED_LOCK(s);
> > - TAILQ_FOREACH(p, &chgpr->ps_threads, p_thr_link) {
> > + SMR_TAILQ_FOREACH_LOCKED(p, &chgpr->ps_threads, p_thr_link) {
> > setpriority(p, p->p_estcpu, n);
> > }
> > SCHED_UNLOCK(s);
> > @@ -488,7 +488,7 @@ dogetrusage(struct proc *p, int who, struct rusage *rup)
> > memset(rup, 0, sizeof(*rup));
> >
> > /* add on all living threads */
> > - TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
> > + SMR_TAILQ_FOREACH_LOCKED(q, &pr->ps_threads, p_thr_link) {
> > ruadd(rup, &q->p_ru);
> > tuagg(pr, q);
> > }
> > diff --git sys/kern/kern_sig.c sys/kern/kern_sig.c
> > index 0cd78b46de5..c378aefd48f 100644
> > --- sys/kern/kern_sig.c
> > +++ sys/kern/kern_sig.c
> > @@ -925,7 +925,7 @@ ptsignal(struct proc *p, int signum, enum signal_type
> > type)
> > * delayed. Otherwise, mark it pending on the
> > * main thread.
> > */
> > - TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
> > + SMR_TAILQ_FOREACH_LOCKED(q, &pr->ps_threads,
> > p_thr_link) {
> > /* ignore exiting threads */
> > if (q->p_flag & P_WEXIT)
> > continue;
> > @@ -1009,7 +1009,7 @@ ptsignal(struct proc *p, int signum, enum signal_type
> > type)
> > * XXX delay processing of SA_STOP signals unless action == SIG_DFL?
> > */
> > if (prop & (SA_CONT | SA_STOP) && type != SPROPAGATED)
> > - TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link)
> > + SMR_TAILQ_FOREACH_LOCKED(q, &pr->ps_threads, p_thr_link)
> > if (q != p)
> > ptsignal(q, signum, SPROPAGATED);
> >
> > @@ -2026,7 +2026,8 @@ single_thread_set(struct proc *p, enum
> > single_thread_mode mode, int deep)
> > pr->ps_singlecount = 0;
> > membar_producer();
> > pr->ps_single = p;
> > - TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
> > + smr_read_enter();
> > + SMR_TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
> > if (q == p)
> > continue;
> > if (q->p_flag & P_WEXIT) {
> > @@ -2072,6 +2073,7 @@ single_thread_set(struct proc *p, enum
> > single_thread_mode mode, int deep)
> > break;
> > }
> > }
> > + smr_read_leave();
> > SCHED_UNLOCK(s);
> >
> > if (mode != SINGLE_PTRACE)
> > @@ -2118,7 +2120,7 @@ single_thread_clear(struct proc *p, int flag)
> > SCHED_LOCK(s);
> > pr->ps_single = NULL;
> > atomic_clearbits_int(&pr->ps_flags, PS_SINGLEUNWIND | PS_SINGLEEXIT);
> > - TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
> > + SMR_TAILQ_FOREACH_LOCKED(q, &pr->ps_threads, p_thr_link) {
> > if (q == p || (q->p_flag & P_SUSPSINGLE) == 0)
> > continue;
> > atomic_clearbits_int(&q->p_flag, P_SUSPSINGLE);
> > diff --git sys/kern/kern_synch.c sys/kern/kern_synch.c
> > index f0338e8f90f..9ef710acf4d 100644
> > --- sys/kern/kern_synch.c
> > +++ sys/kern/kern_synch.c
> > @@ -50,6 +50,7 @@
> > #include <sys/pool.h>
> > #include <sys/refcnt.h>
> > #include <sys/atomic.h>
> > +#include <sys/smr.h>
> > #include <sys/witness.h>
> > #include <sys/tracepoint.h>
> >
> > @@ -633,7 +634,7 @@ sys_sched_yield(struct proc *p, void *v, register_t
> > *retval)
> > * can make some progress.
> > */
> > newprio = p->p_usrpri;
> > - TAILQ_FOREACH(q, &p->p_p->ps_threads, p_thr_link)
> > + SMR_TAILQ_FOREACH_LOCKED(q, &p->p_p->ps_threads, p_thr_link)
> > newprio = max(newprio, q->p_runpri);
> > setrunqueue(p->p_cpu, p, newprio);
> > p->p_ru.ru_nvcsw++;
> > diff --git sys/kern/kern_sysctl.c sys/kern/kern_sysctl.c
> > index a275fa7b6a9..da4fe6a22cf 100644
> > --- sys/kern/kern_sysctl.c
> > +++ sys/kern/kern_sysctl.c
> > @@ -1596,7 +1596,7 @@ again:
> > if (!dothreads)
> > continue;
> >
> > - TAILQ_FOREACH(p, &pr->ps_threads, p_thr_link) {
> > + SMR_TAILQ_FOREACH_LOCKED(p, &pr->ps_threads, p_thr_link) {
> > if (buflen >= elem_size && elem_count > 0) {
> > fill_kproc(pr, kproc, p, show_pointers);
> > error = copyout(kproc, dp, elem_size);
> > @@ -1696,7 +1696,7 @@ fill_kproc(struct process *pr, struct kinfo_proc *ki,
> > struct proc *p,
> > } else {
> > ki->p_pctcpu = 0;
> > ki->p_stat = (pr->ps_flags & PS_ZOMBIE) ? SDEAD : SIDL;
> > - TAILQ_FOREACH(p, &pr->ps_threads, p_thr_link) {
> > + SMR_TAILQ_FOREACH_LOCKED(p, &pr->ps_threads, p_thr_link) {
> > ki->p_pctcpu += p->p_pctcpu;
> > /* find best state: ONPROC > RUN > STOP > SLEEP > .. */
> > if (p->p_stat == SONPROC || ki->p_stat == SONPROC)
> > diff --git sys/kern/subr_witness.c sys/kern/subr_witness.c
> > index 6ee1619da2b..8e44bd930bc 100644
> > --- sys/kern/subr_witness.c
> > +++ sys/kern/subr_witness.c
> > @@ -1888,7 +1888,7 @@ witness_process_has_locks(struct process *pr)
> > {
> > struct proc *p;
> >
> > - TAILQ_FOREACH(p, &pr->ps_threads, p_thr_link) {
> > + SMR_TAILQ_FOREACH_LOCKED(p, &pr->ps_threads, p_thr_link) {
> > if (witness_thread_has_locks(p))
> > return (1);
> > }
> > @@ -2108,7 +2108,7 @@ db_witness_list_all(db_expr_t addr, int have_addr,
> > db_expr_t count, char *modif)
> > LIST_FOREACH(pr, &allprocess, ps_list) {
> > if (!witness_process_has_locks(pr))
> > continue;
> > - TAILQ_FOREACH(p, &pr->ps_threads, p_thr_link) {
> > + SMR_TAILQ_FOREACH_LOCKED(p, &pr->ps_threads, p_thr_link) {
> > if (!witness_thread_has_locks(p))
> > continue;
> > db_printf("Process %d (%s) thread %p (%d)\n",
> > diff --git sys/kern/sys_process.c sys/kern/sys_process.c
> > index 4f6bc30cd76..1ddf711b2a6 100644
> > --- sys/kern/sys_process.c
> > +++ sys/kern/sys_process.c
> > @@ -301,7 +301,7 @@ ptrace_ctrl(struct proc *p, int req, pid_t pid, caddr_t
> > addr, int data)
> > error = ESRCH;
> > goto fail;
> > }
> > - t = TAILQ_FIRST(&tr->ps_threads);
> > + t = SMR_TAILQ_FIRST_LOCKED(&tr->ps_threads);
> > break;
> >
> > /* calls that accept a PID or a thread ID */
> > @@ -561,9 +561,9 @@ ptrace_kstate(struct proc *p, int req, pid_t pid, void
> > *addr)
> > return ESRCH;
> > if (t->p_p != tr)
> > return EINVAL;
> > - t = TAILQ_NEXT(t, p_thr_link);
> > + t = SMR_TAILQ_NEXT_LOCKED(t, p_thr_link);
> > } else {
> > - t = TAILQ_FIRST(&tr->ps_threads);
> > + t = SMR_TAILQ_FIRST_LOCKED(&tr->ps_threads);
> > }
> >
> > if (t == NULL)
> > @@ -754,7 +754,7 @@ process_tprfind(pid_t tpid, struct proc **tp)
> >
> > if (tr == NULL)
> > return NULL;
> > - *tp = TAILQ_FIRST(&tr->ps_threads);
> > + *tp = SMR_TAILQ_FIRST_LOCKED(&tr->ps_threads);
> > return tr;
> > }
> > }
> > diff --git sys/kern/tty.c sys/kern/tty.c
> > index efd8bbf5cb9..cdf5b1feca1 100644
> > --- sys/kern/tty.c
> > +++ sys/kern/tty.c
> > @@ -2118,7 +2118,7 @@ process_sum(struct process *pr, fixpt_t *estcpup)
> >
> > ret = 0;
> > estcpu = 0;
> > - TAILQ_FOREACH(p, &pr->ps_threads, p_thr_link) {
> > + SMR_TAILQ_FOREACH_LOCKED(p, &pr->ps_threads, p_thr_link) {
> > if (p->p_stat == SRUN || p->p_stat == SONPROC)
> > ret = 1;
> > estcpu += p->p_pctcpu;
> > @@ -2220,12 +2220,12 @@ update_pickpr:
> > * - prefer living
> > * Otherwise take the newest thread
> > */
> > - pick = p = TAILQ_FIRST(&pickpr->ps_threads);
> > + pick = p = SMR_TAILQ_FIRST_LOCKED(&pickpr->ps_threads);
> > if (p == NULL)
> > goto empty;
> > run = p->p_stat == SRUN || p->p_stat == SONPROC;
> > pctcpu = p->p_pctcpu;
> > - while ((p = TAILQ_NEXT(p, p_thr_link)) != NULL) {
> > + while ((p = SMR_TAILQ_NEXT_LOCKED(p, p_thr_link)) != NULL) {
> > run2 = p->p_stat == SRUN || p->p_stat == SONPROC;
> > pctcpu2 = p->p_pctcpu;
> > if (run) {
> > diff --git sys/sys/proc.h sys/sys/proc.h
> > index d4ec6450a7a..196e48ce9a6 100644
> > --- sys/sys/proc.h
> > +++ sys/sys/proc.h
> > @@ -50,6 +50,7 @@
> > #include <sys/resource.h> /* For struct rusage */
> > #include <sys/rwlock.h> /* For struct rwlock */
> > #include <sys/sigio.h> /* For struct sigio */
> > +#include <sys/smr.h>
> >
> > #ifdef _KERNEL
> > #include <sys/atomic.h>
> > @@ -156,6 +157,9 @@ struct unveil;
> > * R rlimit_lock
> > * S scheduler lock
> > * T itimer_mtx
> > + *
> > + * For SMR related structures that allow lock-free reads, the write lock
> > + * is indicated below.
> > */
> > struct process {
> > /*
> > @@ -167,7 +171,7 @@ struct process {
> > struct ucred *ps_ucred; /* Process owner's identity. */
> >
> > LIST_ENTRY(process) ps_list; /* List of all processes. */
> > - TAILQ_HEAD(,proc) ps_threads; /* Threads in this process. */
> > + SMR_TAILQ_HEAD(,proc) ps_threads; /* [K] Threads in this process. */
> >
> > LIST_ENTRY(process) ps_pglist; /* List of processes in pgrp. */
> > struct process *ps_pptr; /* Pointer to parent process. */
> > @@ -330,16 +334,20 @@ struct p_inentry {
> > /*
> > * Locks used to protect struct members in this file:
> > * I immutable after creation
> > - * S scheduler lock
> > + * K kernel lock
> > * l read only reference, see lim_read_enter()
> > * o owned (read/modified only) by this thread
> > + * S scheduler lock
> > + *
> > + * For SMR related structures that allow lock-free reads, the write lock
> > + * is indicated below.
> > */
> > struct proc {
> > TAILQ_ENTRY(proc) p_runq; /* [S] current run/sleep queue */
> > LIST_ENTRY(proc) p_list; /* List of all threads. */
> >
> > struct process *p_p; /* [I] The process of this thread. */
> > - TAILQ_ENTRY(proc) p_thr_link; /* Threads in a process linkage. */
> > + SMR_TAILQ_ENTRY(proc) p_thr_link; /* [K] Threads in a process linkage */
> >
> > TAILQ_ENTRY(proc) p_fut_link; /* Threads in a futex linkage. */
> > struct futex *p_futex; /* Current sleeping futex. */
> > @@ -425,8 +433,9 @@ struct proc {
> > #define SONPROC 7 /* Thread is currently on a CPU. */
> >
> > #define P_ZOMBIE(p) ((p)->p_stat == SDEAD)
> > -#define P_HASSIBLING(p) (TAILQ_FIRST(&(p)->p_p->ps_threads) != (p) || \
> > - TAILQ_NEXT((p), p_thr_link) != NULL)
> > +#define P_HASSIBLING(p)
> > \
> > + (SMR_TAILQ_FIRST_LOCKED(&(p)->p_p->ps_threads) != (p) || \
> > + SMR_TAILQ_NEXT_LOCKED((p), p_thr_link) != NULL)
> >
> > /*
> > * These flags are per-thread and kept in p_flag
> > 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:
> >
>
--
:wq Claudio
