This looks great. Some minor comments below. I highly encourage other people to look at this patch set, since it's an example of how to break a complex change up into understandable, logical changes (a.k.a., a commit).
Thanks, Barret On 2015-11-11 at 04:19 Kevin Klues <[email protected]> wrote: > The following changes since commit > a759dfde3f64ecc4e7f84d074ed69900445ede85: > > Migrated position dependent initialization, to label based > (2015-11-10 11:43:08 -0500) > > are available in the git repository at: > > [email protected]:klueska/akaros sigpipe-support > From 2846f15b685bfa11d705cbad7309163fd042c8d7 Mon Sep 17 00:00:00 2001 > From: Kevin Klues <[email protected]> > Date: Mon, 9 Nov 2015 18:45:25 -0800 > Subject: Weasel apart parlib/libc symbols for signals (XCC) > --- a/user/parlib/signal.c > +++ b/user/parlib/signal.c > @@ -234,36 +129,27 @@ void init_posix_signals(void) > posix_sig_ev_q->ev_flags = EVENT_IPI | EVENT_INDIR | EVENT_SPAM_INDIR | > EVENT_WAKEUP; > register_kevent_q(posix_sig_ev_q, EV_POSIX_SIGNAL); > + signal_ops = &default_signal_ops; Minor race: probably need to set signal_ops before registering the kevent_q. > wfl_init(&sigdata_list); > } > From d38bf552fecffadbc5336db29e3d101a0401be2c Mon Sep 17 00:00:00 2001 > From: Kevin Klues <[email protected]> > Date: Tue, 10 Nov 2015 13:50:52 -0800 > Subject: Add arch independent accessor for user ctx stack > --- a/user/parlib/include/x86/vcore32.h > +++ b/user/parlib/include/x86/vcore32.h > @@ -315,6 +315,11 @@ static inline void init_user_ctx(struct user_context > *ctx, uint32_t entry_pt, > sw_tf->tf_fpucw = 0x037f; /* x86 default FP CW */ > } > > +static inline uintptr_t get_user_ctx_stack(struct user_context *ctx) > +{ > + return ctx->tf.sw_tf.tf_esp; > +} > + You'll need to switch on the context type for all of these. It could be a HW TF, for example. > From f080545ec5b79bbb4563b5b3123b90e5932e2f8e Mon Sep 17 00:00:00 2001 > From: Kevin Klues <[email protected]> > Date: Tue, 10 Nov 2015 18:10:27 -0800 > Subject: Add uthread_paused() API call > +/* Function indicating an external event has temporarily paused a uthread, > but > + * it is ok to resume it if possible. */ > +void uthread_paused(struct uthread *uthread) > +{ > + uthread->state = UT_NOT_RUNNING; I think this should be an assert(uthread->state == UT_NOT_RUNNING). This is called from a uthread_yield() callback, right? In that case, NOT_RUNNING should already be set. And if not, we'll catch buggy callers. > + /* Call out to the 2LS to package up its uthread */ > + assert(sched_ops->thread_paused); > + sched_ops->thread_paused(uthread); > +} > + > From b133811ff974d66a61fe007034ee981deb3abd94 Mon Sep 17 00:00:00 2001 > From: Kevin Klues <[email protected]> > Date: Tue, 10 Nov 2015 19:00:04 -0800 > Subject: Migrate signal code from pthread.c to signal.c > For 2LSs that don't require posix signals, the only waste is a few words in > the uthread struct, and a few unnecessary instructions to clear these fields > when a uthread is first initialized. There is no actual performance hit > unless the APIs from parlib/signal.h are actually called (as they are in > pthread.c now). Sounds good. IIUC, for another 2LS to use signal handling, it'll need to do something like this: > @@ -250,7 +114,7 @@ static void __attribute__((noreturn)) > pth_sched_entry(void) > } while (1); > /* Prep the pthread to run any pending posix signal handlers registered > * via pthread_kill once it is restored. */ > - __pthread_prep_for_pending_posix_signals(new_thread); > + uthread_prep_pending_signals((struct uthread*)new_thread); > /* Run the thread itself */ > run_uthread((struct uthread*)new_thread); and: > +static void __signal_and_restart(struct uthread *uthread, > + int signo, int code, void *addr) > +{ > + uthread_prep_signal_from_fault(uthread, signo, code, addr); > + pth_thread_runnable(uthread); > +} Which sounds good. > @@ -664,8 +531,7 @@ void __attribute__((constructor)) pthread_lib_init(void) > sysc_mgmt[i].ev_q->ev_mbox = sysc_mbox; > } > #endif > - /* Publish our signal_ops. Sched ops is set by 2ls_init */ > - signal_ops = &pthread_signal_ops; > + /* Sched ops is set by 2ls_init */ > uthread_2ls_init((struct uthread*)t, &pthread_sched_ops); How does pthread (or any 2LS) go about mucking with the signal ops? I imagine it's just "overwrite signal_ops->whatever = my_whatever_func_ptr"? If so, that sounds good. -- 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.
