Date: Fri, 10 May 2002 13:15:20 -0400 (EDT) From: Robert Watson <rwatson@freebsd.org> To: Jake Burkholder <jake@locore.ca> Cc: Jonathan Mini <mini@freebsd.org>, Perforce Change Reviews <perforce@freebsd.org> Subject: Re: PERFORCE change 11120 for review Message-ID: <Pine.NEB.3.96L.1020510131101.69160H-100000@fledge.watson.org> In-Reply-To: <20020510123716.D2566@locore.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
I actually also was curious about the locking environment UMA provides when initializing and destroying memory slabs... A lot of "common" cleanup will involve locking -- removing things from chains, freeing allocated references to other types of objects, etc. If UMA locks are held at that point, that may cause problems. It could be that for destructors, UMA needs a worker thread iterating on a work list that doesn't require held locks during destruction to avoid locking incidentals. Or at the very least, someone needs to document what locks are held :-). Robert N M Watson FreeBSD Core Team, TrustedBSD Project robert@fledge.watson.org NAI Labs, Safeport Network Services On Fri, 10 May 2002, Jake Burkholder wrote: > Apparently, On Fri, May 10, 2002 at 08:30:49AM -0700, > Jonathan Mini said words to the effect of; > > > http://people.freebsd.org/~peter/p4db/chv.cgi?CH=11120 > > > > Change 11120 by mini@mini_stylus on 2002/05/10 08:30:36 > > > > Give UMA control over thread allocation and caching: > > > > - Use uma init/fini/ctor/dtor functions. > > - Instead of keeping a PCPU list of zombie threads to be > > reaped at earliest convenvience, keep a single "spare" > > thread in the KSE. > > - When abandoning a thread (in thread_exit()), push the > > thread into its KSE's spare thread slot, and free the > > thread that is already there (if any). > > - When performing an upcall, pull the spare thread (if > > available) before allocating a new thread from uma. This > > is especially useful in msleep(), where not blocking again > > is highly preferable. > > - When pulling the KSE spare thread, allocate a new spare > > thread for the KSE before returning to userland for the > > upcall. > > I'm not sure that using the uma callouts for all of this is safe. > > > #endif > > > > - active_threads--; > > - if (cached_threads < thread_cache_size) { > > - TAILQ_INSERT_HEAD(&free_threads, td, td_plist); > > - cached_threads++; /* XXXSMP */ > > - } else { > > - /* free tha pages from the stack object etc. */ > > - allocated_threads--; > > - pmap_dispose_thread(td); > > + mtx_lock_spin(&sched_lock); > > + > > + mtx_unlock_spin(&sched_lock); > > ??? > > > + > > + /* Update counters. */ > > + active_threads--; /* XXXSMP */ > > + cached_threads++; /* XXXSMP */ > > +} > > + > > +/* > > + * Initialize type-stable parts of a thread (when newly created). > > + */ > > +static void > > +thread_init(void *mem, int size) > > +{ > > + struct thread *td; > > + > > + KASSERT((size == sizeof(struct thread)), > > + ("size mismatch: %d != %d\n", size, sizeof(struct thread))); > > + > > + td = (struct thread *)mem; > > + pmap_new_thread(td); > > + cpu_thread_setup(td); > > + cached_threads++; /* XXXSMP */ > > + allocated_threads++; /* XXXSMP */ > > +} > > + > > +/* > > + * Tear down type-stable parts of a thread (just before being discarded). > > + */ > > +static void > > +thread_fini(void *mem, int size) > > +{ > > + struct thread *td; > > + > > + KASSERT((size == sizeof(struct thread)), > > + ("size mismatch: %d != %d\n", size, sizeof(struct thread))); > > + > > + td = (struct thread *)mem; > > + pmap_dispose_thread(td); > > + vm_object_deallocate(td->td_kstack_obj); > > + cached_threads--; /* XXXSMP */ > > + allocated_threads--; /* XXXSMP */ > > +} > > These pmap and vm_object calls might sleep, and are called with the uma > zone locked. > > > + > > +/* > > + * Unlink thread from its process, and reassign its KSE to another thread. > > + */ > > +static void > > +thread_unlink(struct thread *td) > > +{ > > + struct proc *p; > > + struct ksegrp *kg; > > + struct kse *ke; > > + > > + p = td->td_proc; > > + kg = td->td_ksegrp; > > + ke = td->td_kse; > > > > -#if 0 > > - /* Free the object itslef */ > > - /* As zones are type stable this can be skipped for now */ > > - vm_object_deallocate(td->td_kstack_obj); > > - td->td_kstack_obj = NULL; > > -#endif > > + /* Reassign this thread's KSE. */ > > + if (ke != NULL) { > > + ke->ke_thread = NULL; > > + td->td_kse = NULL; > > + ke->ke_state = KES_UNQUEUED; > > + kse_reassign(ke); > > + } > > Almost all of the new code you added looks like its indented 4 spaces, > which should be tabs :) > > > > > - /* put the thread back in the zone */ > > - uma_zfree(thread_zone, td); > > + /* Unlink this thread from its proc. */ > > + if (p != NULL) { > > + TAILQ_REMOVE(&p->p_threads, td, td_plist); > > + if (kg != NULL) > > + TAILQ_REMOVE(&kg->kg_threads, td, td_kglist); > > + p->p_numthreads--; > > + if (P_SHOULDSTOP(p) == P_STOPPED_SNGL) { > > + if (p->p_numthreads == > > + ((p->p_flag & P_SINGLE_EXIT) ? 1 : (p->p_suspcount + 1))) { > > + setrunqueue(p->p_singlethread); > > + p->p_singlethread = NULL; > > + } > > + } > > } > > + if (kg != NULL) > > + kg->kg_numthreads--; > > + td->td_state = TDS_SURPLUS; > > + td->td_proc = NULL; > > + td->td_ksegrp = NULL; > > + td->td_last_kse = NULL; > > +} > > + > > +/* > > + * Initialize global thread allocation resources. > > + */ > > +void > > +threadinit(void) > > +{ > > + > > + thread_zone = uma_zcreate("THREAD", sizeof (struct thread), > > + thread_ctor, thread_dtor, thread_init, thread_fini, > > + UMA_ALIGN_CACHE, 0); > > } > > > > -#define RANGEOF(type, start, end) (offsetof(type, end) - offsetof(type, start)) > > -/* > > - * Try get a new thread from the cache, but if that fails, > > - * create one from the zone as per normal > > +/* > > + * Allocate a thread. > > */ > > struct thread * > > thread_alloc(void) > > { > > - struct thread *td; > > + return (uma_zalloc(thread_zone, M_WAITOK)); > > +} > > > > - thread_reap(); /* recover any partly deallocated threads */ > > - if (cached_threads) { > > - td = TAILQ_FIRST(&free_threads); > > - TAILQ_REMOVE(&free_threads, td, td_plist); > > - cached_threads--; /* XXXSMP */ > > - /* Probably should clean up stuff here */ > > - } else { > > - /* allocate the thread structure itself */ > > - td = uma_zalloc(thread_zone, M_WAITOK); > > - > > - allocated_threads++; > > - pmap_new_thread(td); > > - cpu_thread_setup(td); > > - } > > - /* may need to set some stuff here.. re state? */ > > - /* Make sure the zero'd section is in fact zero'd */ > > - bzero(&td->td_startzero, > > - (unsigned) RANGEOF(struct thread, td_startzero, td_endzero)); > > - td->td_state = TDS_NEW; > > - td->td_flags |= TDF_UNBOUND; > > - active_threads++; > > - return (td); > > +/* > > + * Deallocate a thread. > > + */ > > +void > > +thread_free(struct thread *td) > > +{ > > + uma_zfree(thread_zone, td); > > } > > > > int > > @@ -190,22 +275,36 @@ > > } > > > > > > -/* > > - * Put the half dead thread on a per CPU list of threads that need > > - * to be reaped. > > +/* > > + * Discard the current thread and exit from its context. > > + * > > + * Because we can't free a thread while we're operating under its context, > > + * push the current thread into our KSE's ke_tdspare slot, freeing the > > + * thread that might be there currently. Because we know that only this > > + * processor will run our KSE, we needn't worry about someone else grabbing > > + * our context before we do a cpu_throw. > > */ > > void > > thread_exit(void) > > { > > - struct thread *td = curthread; > > + struct thread *td; > > + struct kse *ke; > > > > + td = curthread; > > + ke = td->td_kse; > > CTR1(KTR_PROC, "thread_exit: thread %p", td); > > KASSERT(!mtx_owned(&Giant), ("dying thread owns giant")); > > - cpu_thread_exit(td); > > - td->td_state = TDS_SURPLUS; > > - td->td_nextzombie = PCPU_GET(freethread); > > - PCPU_SET(freethread, td); > > - thread_unlink(td); /* reassignes kse detach it from it's process etc. */ > > + > > + if (ke->ke_tdspare != NULL) { > > + mtx_unlock_spin(&sched_lock); > > + mtx_lock(&Giant); > > + thread_free(ke->ke_tdspare); > > + mtx_unlock(&Giant); > > + mtx_lock_spin(&sched_lock); > > + } > > + cpu_thread_exit(td); /* XXXSMP */ > > + thread_unlink(td); > > + ke->ke_tdspare = td; > > cpu_throw(); > > } > > > > @@ -232,71 +331,6 @@ > > td->td_kse = NULL; > > } > > > > - > > -void > > -thread_unlink(struct thread *td) > > -{ > > - struct proc *p = td->td_proc; > > - struct ksegrp *kg = td->td_ksegrp; > > - struct kse *ke = td->td_kse; /* may be NULL */ > > - > > - if (ke) { > > - ke->ke_thread = NULL; > > - td->td_kse = NULL; > > - ke->ke_state = KES_UNQUEUED; > > - kse_reassign(ke); > > - } > > - > > - switch(td->td_state) { > > - case TDS_SLP: /* we must never get to unlink a thread */ > > - case TDS_MTX: /* that is in one of these states as to */ > > - case TDS_RUNQ: /* do so might leak all sorts of stuff */ > > - panic ("bad state for thread unlinking"); > > - break; > > - case TDS_UNQUEUED: > > - case TDS_NEW: > > - break; > > - case TDS_RUNNING: > > - break; > > - case TDS_SURPLUS: > > - break; > > - default: > > - panic("bad thread state"); > > - } > > - TAILQ_REMOVE(&p->p_threads, td, td_plist); > > - TAILQ_REMOVE(&kg->kg_threads, td, td_kglist); > > - p->p_numthreads--; > > - if (P_SHOULDSTOP(p) == P_STOPPED_SNGL) { > > - if (p->p_numthreads == > > - ((p->p_flag & P_SINGLE_EXIT) ? 1 : (p->p_suspcount + 1))){ > > - setrunqueue(p->p_singlethread); > > - p->p_singlethread = NULL; > > - } > > - } > > - kg->kg_numthreads--; > > - td->td_state = TDS_SURPLUS; > > - td->td_proc = NULL; > > - td->td_ksegrp = NULL; > > - td->td_last_kse = NULL; > > -} > > -/* > > - * reap any zombie threads for this Processor. > > - */ > > -void > > -thread_reap(void) > > -{ > > - struct thread *td_reaped, *td_next; > > - > > - if ((td_reaped = PCPU_GET(freethread))) { > > - PCPU_SET(freethread, NULL); > > - while (td_reaped) { > > - td_next = td_reaped->td_nextzombie; > > - thread_free(td_reaped); > > - td_reaped = td_next; > > - } > > - } > > -} > > - > > /* > > * set up the upcall pcb in either a given thread or a new one > > * if none given. Use the upcall for the given KSE > > @@ -307,18 +341,24 @@ > > { > > struct thread *td2; > > > > - td2 = thread_alloc(); > > - if (td2) { > > - CTR3(KTR_PROC, "thread_schedule_upcall: thread %p (pid %d, %s)", > > - td, td->td_proc->p_pid, td->td_proc->p_comm); > > - thread_link(td2, ke->ke_ksegrp); > > - cpu_set_upcall(td2, ke->ke_pcb); > > - td2->td_ucred = crhold(td->td_ucred); > > - td2->td_kse = NULL; /* back as it was */ > > - td2->td_flags = TDF_UNBOUND|TDF_UPCALLING; > > - td2->td_priority = td->td_priority; > > - setrunqueue(td2); > > + mtx_assert(&sched_lock, MA_OWNED); > > + if (ke->ke_tdspare != NULL) { > > + td2 = ke->ke_tdspare; > > + ke->ke_tdspare = NULL; > > + } else { > > + mtx_unlock_spin(&sched_lock); > > + td2 = thread_alloc(); > > + mtx_lock_spin(&sched_lock); > > } > > + CTR3(KTR_PROC, "thread_schedule_upcall: thread %p (pid %d, %s)", > > + td, td->td_proc->p_pid, td->td_proc->p_comm); > > + thread_link(td2, ke->ke_ksegrp); > > + cpu_set_upcall(td2, ke->ke_pcb); > > + td2->td_ucred = crhold(td->td_ucred); > > + td2->td_kse = NULL; /* back as it was */ > > + td2->td_flags = TDF_UNBOUND|TDF_UPCALLING; > > + td2->td_priority = td->td_priority; > > + setrunqueue(td2); > > return (td2); > > } > > > > > > ==== //depot/projects/kse/sys/kern/subr_pcpu.c#7 (text+ko) ==== > > > > @@ -119,12 +119,6 @@ > > td->td_proc->p_comm); > > else > > db_printf("none\n"); > > - db_printf("freethread = "); > > - td = pc->pc_freethread; > > - if (td != NULL) > > - db_printf("%p: next: %p\n", td, td->td_nextzombie); > > - else > > - db_printf("none\n"); > > db_printf("curpcb = %p\n", pc->pc_curpcb); > > db_printf("fpcurthread = "); > > td = pc->pc_fpcurthread; > > > > ==== //depot/projects/kse/sys/kern/subr_trap.c#53 (text+ko) ==== > > > > @@ -116,9 +116,16 @@ > > * We also need to check to see if we have to exit or wait > > * due to a single threading requirement. > > */ > > - PROC_LOCK(p); > > - thread_suspend_check(0); /* Can suspend or kill */ > > - PROC_UNLOCK(p); > > + if (p->p_flag & P_KSES) { > > + PROC_LOCK(p); > > + thread_suspend_check(0); /* Can suspend or kill */ > > + PROC_UNLOCK(p); > > + if (ke->ke_tdspare == NULL) { > > + mtx_lock(&Giant); > > + ke->ke_tdspare = thread_alloc(); > > + mtx_unlock(&Giant); > > + } > > + } > > if (td->td_flags & TDF_UNBOUND) { > > /* maybe this should be in a separate function */ > > /* > > > > ==== //depot/projects/kse/sys/sys/pcpu.h#11 (text+ko) ==== > > > > @@ -53,7 +53,6 @@ > > struct thread *pc_curthread; /* Current thread */ > > struct thread *pc_idlethread; /* Idle thread */ > > struct thread *pc_fpcurthread; /* Fp state owner */ > > - struct thread *pc_freethread; /* thread we are freeing */ > > struct pcb *pc_curpcb; /* Current pcb */ > > struct bintime pc_switchtime; > > int pc_switchticks; > > > > ==== //depot/projects/kse/sys/sys/proc.h#98 (text+ko) ==== > > > > @@ -270,7 +270,6 @@ > > int td_flags; /* (j) TDF_* flags. */ > > struct kse *td_last_kse; /* Where it wants to be if possible. */ > > struct kse *td_kse; /* Current KSE if running. */ > > - struct thread *td_nextzombie; /* PCPU chain of zombie threads */ > > int td_dupfd; /* (k) Ret value from fdopen. XXX */ > > void *td_wchan; /* (j) Sleep address. */ > > const char *td_wmesg; /* (j) Reason for sleep. */ > > @@ -360,6 +359,7 @@ > > KES_RUNNING > > } ke_state; /* (j) S* process status. */ > > void *ke_mailbox; /* the userland mailbox address */ > > + struct thread *ke_tdspare; /* spare thread for upcalls */ > > #define ke_endzero ke_dummy > > > > #define ke_startcopy ke_endzero > > @@ -797,8 +797,8 @@ > > int cpu_coredump(struct thread *, struct vnode *, struct ucred *); > > > > /* new in KSE */ > > -struct thread *thread_alloc(void); > > -void thread_free(struct thread *td); > > +struct thread *thread_alloc(void); > > +void thread_free(struct thread *td); > > int cpu_export_context(struct thread *td); > > void cpu_free_kse_mdstorage(struct kse *kse); > > void cpu_save_upcall(struct thread *td, struct kse *newkse); > > @@ -814,14 +814,12 @@ > > void thread_exit(void) __dead2; > > int thread_export_context(struct thread *td); > > void thread_link(struct thread *td, struct ksegrp *kg); > > -void thread_reap(void); > > struct thread *thread_schedule_upcall(struct thread *td, struct kse *ke); > > int thread_single(int how); > > #define SNGLE_WAIT 0 /* values for 'how' */ > > #define SNGLE_EXIT 1 > > void thread_single_end(void); > > int thread_suspend_check(int how); > > -void thread_unlink(struct thread *td); > > void thread_unsuspend(struct proc *p); > > > > #endif /* _KERNEL */ > To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe p4-projects" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.NEB.3.96L.1020510131101.69160H-100000>