Hi - Comments on the first patch:
> From 43dad97271022f4f36988cf0fda27e2b5f0bb9dd Mon Sep 17 00:00:00 2001 > From: Christopher Koch <[email protected]> > Date: Thu, 4 Aug 2016 18:31:03 -0700 > Subject: Moved IDs and added list of threads in uthreads. > > IDs are moved from pthreads to uthreads and uthreads now maintain a list of > current threads. I have a couple issues with this. The first is race conditions involved with threads exiting. Say on vcore 1, we create the thread. Another vcore (2) gets the uthread pointer by ID. Then the thread is destroyed on vcore 1. Now VC 2 has a pointer to freed memory. I think in conversation you said this would only ever be called from stop-the-world, but if we're going to have the API, we should be able to handle more circumstances. For instance, I can imagine wanting to interrogate a 2LS without stopping the world. Unless we can have a more GDB-specific solution that isn't an arbitrary lookup, we'll eventually need something like krefs. But for now, I'd be okay with keeping this mostly as-is, but extend the API a bit to deal with a future in which we have refcounted uthreads. So while we add this: struct uthread *uthread_get_thread_by_id(uint64_t id); We should also add this, which drops the reference: void uthread_put_thread(struct uthread *uth); which can do nothing for now, and we can have a TODO to properly do reference counting. Another issue is that this now requires all thread creation and destruction to globally synchronize. Previously, the only global synchronization required was in the specific 2LS or memory allocator, both of which could be parallelized. To be fair, that latter limitation is due to the implementation. Perhaps we could make this per-vcore in the future, at least on creation (though destruction is hard, since you might have migrated), or use a scalable hash table or something else cool. In anticipation of this, a couple helper functions might help in the implementation (see below). On a related note, I'm not sure that this shouldn't be somehow integrated into specific 2LSs more, but I don't have a good answer on that, so I guess we should leave it in Parlib for now. Most of my apprehension is due to the performance/parallelism of it all. A couple minor things below: > diff --git a/user/parlib/include/parlib/uthread.h > b/user/parlib/include/parlib/uthread.h > index f0b2b0773213..04d8521f1135 100644 > --- a/user/parlib/include/parlib/uthread.h > +++ b/user/parlib/include/parlib/uthread.h > @@ -3,6 +3,7 @@ > #include <parlib/vcore.h> > #include <parlib/signal.h> > #include <ros/syscall.h> > +#include <sys/queue.h> > > __BEGIN_DECLS > > @@ -24,6 +25,8 @@ __BEGIN_DECLS > * cast their threads to uthreads when talking with vcore code. > Vcore/default > * 2LS code won't touch udata or beyond. */ > struct uthread { > + LIST_ENTRY(uthread) entry; > + uint64_t id; > struct user_context u_ctx; > struct ancillary_state as; > void *tls_desc; > @@ -38,6 +41,8 @@ struct uthread { > int err_no; > char err_str[MAX_ERRSTR_LEN]; > }; > +LIST_HEAD(uthread_list, uthread); > +static struct uthread_list all_uthreads = > LIST_HEAD_INITIALIZER(all_uthreads); I think you can move this into uthread.c. We don't need to expose all_uthreads, or the fact that it is a list, outside uthread.c itself. Incidentally, I think this might be making an all_uthreads list in every C file, all of which are unused except for the one in uthread.c. > diff --git a/user/parlib/uthread.c b/user/parlib/uthread.c > index 520c94b4d2a8..b04f6268dbaa 100644 > --- a/user/parlib/uthread.c > +++ b/user/parlib/uthread.c > @@ -2,15 +2,15 @@ > * Barret Rhoden <[email protected]> > * See LICENSE for details. */ > > -#include <ros/arch/membar.h> > #include <parlib/arch/atomic.h> > +#include <parlib/arch/trap.h> > +#include <parlib/assert.h> > +#include <parlib/event.h> > +#include <parlib/mcs.h> > #include <parlib/parlib.h> > -#include <parlib/vcore.h> > #include <parlib/uthread.h> > -#include <parlib/event.h> > +#include <ros/arch/membar.h> > #include <stdlib.h> > -#include <parlib/assert.h> > -#include <parlib/arch/trap.h> > > /* SCPs have a default 2LS that only manages thread 0. Any other 2LS, such > as > * pthreads, should override sched_ops in its init code. */ > @@ -22,8 +22,12 @@ __thread struct uthread *current_uthread = 0; > * extensively about the details. Will call out when necessary. */ > static struct event_queue *preempt_ev_q; > > +/* Lock for creatting a new thread id. */ > +struct mcs_pdr_lock thread_list_lock; > + This can be a static struct. Also, let's make it a spin_pdr_lock. I think most threads are made from a single core, and the spin lock will have less overhead than the MCS lock. Unfortunately, this is a heuristic, which might be wrong for various applications or 2LSs. > /* Helpers: */ > #define UTH_TLSDESC_NOTLS (void*)(-1) > +static uint64_t __get_next_tid(void); > static inline bool __uthread_has_tls(struct uthread *uthread); > static int __uthread_allocate_tls(struct uthread *uthread); > static int __uthread_reinit_tls(struct uthread *uthread); > @@ -55,8 +59,13 @@ static void uthread_init_thread0(struct uthread *uthread) > /* need to track thread0 for TLS deallocation */ > uthread->flags |= UTHREAD_IS_THREAD0; > uthread->notif_disabled_depth = 0; > - /* setting the uthread's TLS var. this is idempotent for SCPs (us) */ > + /* setting the uthread's TLS var. this is idempotent for SCPs (us) */ > __vcoreid = 0; > + /* Assign a thread ID and add to thread list. */ > + mcs_pdr_lock(&thread_list_lock); > + uthread->id = __get_next_tid(); > + LIST_INSERT_HEAD(&all_uthreads, uthread, entry); > + mcs_pdr_unlock(&thread_list_lock); This is basically the same code used in thread_init. I'd just use a helper for all of these cases (new uth and cleaning up a uth), which also hides the spinlock and list implementation details from the rest of uthread.c. > } > > /* Helper, makes VC ctx tracks uthread as its current_uthread in its TLS. > @@ -212,6 +221,7 @@ void __attribute__((constructor)) uthread_lib_init(void) > /* Only run once, but make sure that vcore_lib_init() has run already. > */ > init_once_racy(return); > vcore_lib_init(); > + mcs_pdr_init(&thread_list_lock); If you use a spinlock, you can also use a static initializer, instead of dealing with it at runtime (grep SPINLOCK_INITIALIZER). > > ret = posix_memalign((void**)&thread0_uth, __alignof__(struct uthread), > sizeof(struct uthread)); > @@ -273,6 +283,16 @@ void __attribute__((noreturn)) uthread_vcore_entry(void) > assert(0); /* 2LS sched_entry should never return */ > } > > +/* __get_next_tid returns an unused thread id. > + * > + * Warning: this will reuse numbers eventually. */ > +static uint64_t __get_next_tid(void) > +{ > + static atomic_t next_tid; > + > + return atomic_fetch_and_add(&next_tid, 1); Since this is only called while the lock is held, you don't need an atomic. (It doesn't help, and it slightly hurts). Barret > +} > + > /* Does the uthread initialization of a uthread that the caller created. > Call > * this whenever you are "starting over" with a thread. */ > void uthread_init(struct uthread *new_thread, struct uth_thread_attr *attr) > @@ -291,6 +311,13 @@ void uthread_init(struct uthread *new_thread, struct > uth_thread_attr *attr) > * were interrupted off a core. */ > new_thread->flags |= UTHREAD_SAVED; > new_thread->notif_disabled_depth = 0; > + > + /* New thread ID and addition to thread list. */ > + mcs_pdr_lock(&thread_list_lock); > + new_thread->id = __get_next_tid(); > + LIST_INSERT_HEAD(&all_uthreads, new_thread, entry); > + mcs_pdr_unlock(&thread_list_lock); > + > if (attr && attr->want_tls) { > /* Get a TLS. If we already have one, reallocate/refresh it */ > if (new_thread->tls_desc) > @@ -489,6 +516,11 @@ void uthread_sleep_forever(void) > void uthread_cleanup(struct uthread *uthread) > { > printd("[U] thread %08p on vcore %d is DYING!\n", uthread, vcore_id()); > + > + mcs_pdr_lock(&thread_list_lock); > + LIST_REMOVE(uthread, entry); > + mcs_pdr_unlock(&thread_list_lock); > + > /* we alloc and manage the TLS, so lets get rid of it, except for > thread0. > * glibc owns it. might need to keep it around for a full exit() */ > if (__uthread_has_tls(uthread) && !(uthread->flags & > UTHREAD_IS_THREAD0)) > @@ -1149,3 +1181,20 @@ static void __uthread_free_tls(struct uthread *uthread) > free_tls(uthread->tls_desc); > uthread->tls_desc = NULL; > } > + > +/* TODO(chrisko): hash table instead of list. */ > +struct uthread *uthread_get_thread_by_id(uint64_t id) > +{ > + struct uthread *t = NULL, *ret = NULL; > + > + mcs_pdr_lock(&thread_list_lock); > + LIST_FOREACH(t, &all_uthreads, entry) { > + if (t->id == id) { > + ret = t; > + break; > + } > + } > + > + mcs_pdr_unlock(&thread_list_lock); > + return ret; > +} -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
