Code Review - Sigpipe support

[Kevin Klues]

The following changes since commit 59d25eaabfc8838e52452dd21fe6f50d4e89dc89:

Adjust checkpatch.pl for Akaros's style (2015-10-15 12:18:37 -0400)

are available in the git repository at:

g...@github.com:klueska/akaros.git sigpipe-support

for you to fetch changes up to 8d27a5de79f071eb602c0581144e0d5bf0a24dbf:

Add support for EPIPE and SIGPIPE (2015-10-27 11:20:09 -0700)

----------------------------------------------------------------
Kevin Klues (4):
Fix bug in pthread_sigmask() semantics.
Add support for sigprocmask in SCPs with no 2LS
Allow sys_self_notify() to accept (vcoreid == -1)
Add support for EPIPE and SIGPIPE

kern/drivers/dev/pipe.c |
27 +++++----------------------
kern/src/syscall.c |
10 ++++++++--
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/write.c |
25 +++++++++++++++++++++----
user/parlib/signal.c |
47 ++++++++++++++++++++++++++++++++++++++++-------
user/pthread/pthread.c |
30 +++++++++++++++++-------------
5 files changed, 91 insertions(+), 48 deletions(-)

[ron minnich]

How hard would it be to make the link for these things something I can click on.

--
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 akaros+un...@googlegroups.com.
To post to this group, send email to aka...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Kevin Klues]

Here is a link to look at it:
https://github.com/brho/akaros/compare/59d25ea...8d27a5d?ts=4

Unfortunately you will have to come back to the email thread to
comment though. If you try and comment inline it will probably
generate an email, but it won't be associated with this email thread
and it might not appear to come from you. I feel your pain though.
I'd prefer not to be doing the reviews over email at all...

--
~Kevin

[Davide Libenzi]

Do you really need a global sigmask?

I am not sure that having a global vs. per-thread sigmask fits the pthread signal semantics.

The SIGPIPE is delivered to the thread doing the write(), and it's such thread sigmask which should be used.

[Kevin Klues]

The global sigmask is only used if a thread specific scheduler has not registered a different sigmask function (I.e it's only in effect for single threaded processes)

[Davide Libenzi]

How does that work during the switch from single thread, to multi thread?

Is there a "switch" at all?

Does the global sigmask get copied into the first "thread world", when it starts to materialize?

[Kevin Klues]

It happens in the pthread constructor function.

By default, the global signal_ops struct is set to default_signal_ops,
but when the pthread constructor function runs, it sets signal_ops =
pthread_signal_ops. The default_signal_ops rely on the global sigmask,
the pthread_signal_ops rely on a pthread specific implementation of
these ops.

In handle_event (from signal.c), we have the following check:

if ((signal_ops != default_sig_ops) || !__sigismember(&sigmask, sig_nr)) {
struct siginfo info = {0};

info.si_code = SI_USER;
trigger_posix_signal(sig_nr, &info, 0);
}

The intent here was to check the global sigmask only if (signal_ops ==
default_sig_ops) and then trigger the signal if it is unmasked. If a
scheduler specific signal_ops is installed (e.g. pthread), we should
defer triggering to that scheduler.

Looking at the code more closely now, this is not what is happening
though. We are properly handling the case of the default_sig_ops, but
then blindly triggering the signal in all other cases. What we should
do instead is add a new function to the signal_ops struct that handles
things properly for whatever scheduler is hooked in, i.e.

static void handle_event(struct event_msg *ev_msg, unsigned int ev_type,
void *data)
{
assert(ev_msg);
signal_ops->incoming_signal(ev_msg->ev_arg1)
}

default_sig_ops = {
.incoming_signal = __incoming_signal;
...
}

static void __incoming_signal(int sig_nr)
{
if (!__sigismember(&sigmask, sig_nr)) {
struct siginfo info = {0};

info.si_code = SI_USER;
trigger_posix_signal(sig_nr, &info, 0);
}
}

pthread_sig_ops = {
.incoming_signal = pth_incoming_signal;
...
}

static void pth_incoming_signal(int sig_nr)
{
pthread_kill((pthread_t)current_uthread, sig_nr)
}

On Wed, Oct 28, 2015 at 10:44 AM, 'Davide Libenzi' via Akaros

[Kevin Klues]

I've updated the branch to integrate the fixes mentioned above.

The following changes since commit 59d25eaabfc8838e52452dd21fe6f50d4e89dc89:

Adjust checkpatch.pl for Akaros's style (2015-10-15 12:18:37 -0400)

are available in the git repository at:

g...@github.com:klueska/akaros.git sigpipe-support

for you to fetch changes up to 9aaf3a6347dfa824e3334e942c252fe2c8a7a175:

Add support for EPIPE and SIGPIPE (2015-10-28 13:33:06 -0700)

----------------------------------------------------------------
Kevin Klues (4):
Fix bug in pthread_sigmask() semantics.
Add support for sigprocmask in SCPs with no 2LS
Allow sys_self_notify() to accept (vcoreid == -1)
Add support for EPIPE and SIGPIPE

kern/drivers/dev/pipe.c |
27 +++++----------------------
kern/src/syscall.c |
10 ++++++++--
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/write.c |
25 +++++++++++++++++++++----

user/parlib/include/parlib.h | 1 +
user/parlib/signal.c |
55 +++++++++++++++++++++++++++++++++++++++++++++----------
user/pthread/pthread.c |
37 ++++++++++++++++++++++++-------------
6 files changed, 104 insertions(+), 51 deletions(-)

--
~Kevin

[Barret Rhoden]

Hi -

On 2015-10-28 at 13:41 Kevin Klues <klu...@gmail.com> wrote:
> I've updated the branch to integrate the fixes mentioned above.
>
> The following changes since commit
> 59d25eaabfc8838e52452dd21fe6f50d4e89dc89:
>
> Adjust checkpatch.pl for Akaros's style (2015-10-15 12:18:37 -0400)
>
> are available in the git repository at:
>
> g...@github.com:klueska/akaros.git sigpipe-support
>
> for you to fetch changes up to
> 9aaf3a6347dfa824e3334e942c252fe2c8a7a175:
>
> Add support for EPIPE and SIGPIPE (2015-10-28 13:33:06 -0700)

Comments below.

> From 8fea4fabb85d5193bd6ae351d1ee2ea6df5256df Mon Sep 17 00:00:00 2001
> From: Kevin Klues <klu...@cs.berkeley.edu>
> Date: Tue, 27 Oct 2015 10:19:54 -0700
> Subject: Add support for sigprocmask in SCPs with no 2LS
>
> Previously, we were only able to support signal masks in pthread
> programs. Now we can mask them in SCPs without a 2LS as well.
>

In:

> +static int __sigprocmask(int how, __const sigset_t *__restrict set,
> + sigset_t *__restrict oset)
> {

> + }
> + /* Ensures signals we just unmasked get processed if they are pending */
> + sched_yield();

Does this actually work? In the pthread side, the signals are in the pending
set. On the default 2LS side, I think if they aren't fireably immediately, they
get ignored.

The main thing that caught my eye here was sched_yield(), which although legal,
is a bit dangerous, since it's a full yield to the kernel instead of from a
uthread to a 2LS. I might get rid of it or fix it eventually, due to occasional
bugs from porting other code.


> From 9050d4322914cf99eebe765e526d824c2f83e8c3 Mon Sep 17 00:00:00 2001
> From: Kevin Klues <klu...@cs.berkeley.edu>
> Date: Tue, 27 Oct 2015 10:22:11 -0700
> Subject: Allow sys_self_notify() to accept (vcoreid == -1)
>
> When vcoreid == -1, treat this as a request to be notified on the
> current physical core. This supercedes the need to set the DONT_MIGRATE
^virtual
> flag for cases when we know we want to notify ourselves on the current
> core. The motivation for this is user-space support of SGIPIPE, as will
> appear in a subsequent commit. It should prove generally useful though
> anytime we want to force ourselves into vcore context to handle an event
> on the current core without setting the DONT_MIGRATE flag (when would we
> unset this if we never return, for example).

If this is so that a notification can be tied to the uthread on a VC,
then it won't work.

Here's the potential race:
- uthread decides to notify its vcore (via sigpipe), say VC 6 on PC 5.
- uthread traps into the kernel, asks for -1 here. kernel determines
it's VC 6
- __preempt occurs. for now, this is a message sitting in PC 5's RKM list.
- the event from sys_self_notify() gets put into VC 6's mbox. the
__notify occurs. for now, this is a message sitting in PC 5's RKM
list.
- the syscall completes and goes to return to the user, handling the
RKMs in order.
- the __preempt saves the uthread's ctx into VCPD for VC 6 (this ctx is
the one that trapped in), and clears owning_proc/owning vcoreid.
- the __notify has no effect, since the owning_proc is now wrong.
- the preemption handler will notice that VC 6 was not in VC context and
could yank the uthread from VCPD and start it on another vcore. A
DONT_MIGRATE would prevent this.

Ultimately, without DONT_MIGRATE, a uthread is not bound to a particular
vcore.

> static int sys_self_notify(struct proc *p, uint32_t vcoreid,
> unsigned int ev_type, struct event_msg *u_msg,
> bool priv)
> {
> struct event_msg local_msg = {0};
> +
> + /* If vcoreid = -1, notify on the current vcore. */
> + if (vcoreid == -1) {
> + vcoreid = p->procinfo->pcoremap[core_id()].vcoreid;
> + }

Use pcpui->owning_vcoreid here. The pcoremap could be changed by a
concurrent preemption (I think).

Also, checkpatch didn't complain about this for some reason, but we
don't need/want the extra {} for one liners.


> From 9aaf3a6347dfa824e3334e942c252fe2c8a7a175 Mon Sep 17 00:00:00 2001
> From: Kevin Klues <klu...@cs.berkeley.edu>
> Date: Tue, 27 Oct 2015 11:05:23 -0700
> Subject: Add support for EPIPE and SIGPIPE

Don't forget the (XCC) and something like "rebuild glibc".

> With this change, we now support proper handling of SIGPIPE. This
> allows us to now run things such as:

> static long pipewrite(struct chan *c, void *va, long n, int64_t ignored)
> {
> ERRSTACK(2);
> Pipe *p;
> - //Prog *r;
>
> if (waserror()) {
> - /* avoid exceptions when pipe is a mounted queue */
> - if ((c->flag & CMSG) == 0) {
> -/*
> - r = up->iprog;
> - if(r != NULL && r->kill == NULL)
> - r->kill = "write on closed pipe";
> -*/
> - }

Even though that's commented out, we might want to keep it around. It could
also be related to the bug you mentioned in your commit message.

> - nexterror();
> + poperror();
> + error(EPIPE, NULL);

This style is a little weird. A lot of times in Plan 9, they just set_errstr
and call error. Either is okay. The problem here is that errstr is now gone.
If you want to keep errstr, you could set_errno to EPIPE, then call nexterror.

> diff --git a/tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/write.c b/tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/write.c

> +ssize_t __libc_write(int fd, const void *buf, size_t nbytes)
> {
> - return ros_syscall(SYS_write, fd, buf, nbytes, 0, 0, 0);
> + int ret = ros_syscall(SYS_write, fd, buf, nbytes, 0, 0, 0);
> +
> + if (unlikely((ret != 0) && (errno == EPIPE))) {

This probably won't compile on recent versions of Akaros, since unlikely() isn't
in the kernel headers anymore.

> + sigset_t mask;

My general take on declaring variables halfway through a function instead of at
the top is that you'd be better off with a helper function. But I didn't write
that in the Contributing guide and neither did Linus.

> +
> + sigprocmask(0, NULL, &mask);
> + if (!__sigismember(&mask, SIGPIPE)) {
> + struct event_msg msg = {0};
> +
> + local_msg.ev_type = EV_POSIX_SIGNAL;
> + local_msg.ev_arg1 = SIGPIPE;
> + ros_syscall(SYS_self_notify, -1, EV_POSIX_SIGNAL, &msg, 0, 0, 0);

I'll think about this over the weekend a bit. Maybe there's another nice way to
get this to work. Hopefully this won't pop up for anything else.

The whole "generate a signal for a syscall" seems nasty, since syscalls are
synchronous vs async, instead of people just checking their return codes and
exiting. I'm sure there was a reason for it.

Barret

[Kevin Klues]

I was thinking about this as I wrote it, and decided to just put it in
because it was symmetrical with the pthread implementation. It's
probably OK to take it out, as it probably does nothing. It does
actually work though if you're asking about it continuing to run after
this point.

>
> The main thing that caught my eye here was sched_yield(), which although legal,
> is a bit dangerous, since it's a full yield to the kernel instead of from a
> uthread to a 2LS. I might get rid of it or fix it eventually, due to occasional
> bugs from porting other code.
>
>
>> From 9050d4322914cf99eebe765e526d824c2f83e8c3 Mon Sep 17 00:00:00 2001
>> From: Kevin Klues <klu...@cs.berkeley.edu>
>> Date: Tue, 27 Oct 2015 10:22:11 -0700
>> Subject: Allow sys_self_notify() to accept (vcoreid == -1)
>>
>> When vcoreid == -1, treat this as a request to be notified on the
>> current physical core. This supercedes the need to set the DONT_MIGRATE
> ^virtual

I actually meant on the current physical core, regardless of what the
vcoreid was.

>> flag for cases when we know we want to notify ourselves on the current
>> core. The motivation for this is user-space support of SGIPIPE, as will
>> appear in a subsequent commit. It should prove generally useful though
>> anytime we want to force ourselves into vcore context to handle an event
>> on the current core without setting the DONT_MIGRATE flag (when would we
>> unset this if we never return, for example).
>
> If this is so that a notification can be tied to the uthread on a VC,
> then it won't work.

That was the idea. We need a way of stalling the current uthread and
generating a signal in its place before allowing the uthread to
continue (if it ever continues at all). In the pthread world, this
would have simply been a pthread_kill(pthread_self(), SIGPIPE)
followed by a pthread_yield(). However, I needed a way to make this
work for non-pthread code. Anyway, we can't call any parlib/pthread
code from within the write() call because of link errors when build
rtld.

If we could get around the rtld problem, we could probably move some
of the signal logic to thread0_sched.c and make uthread lib calls for
uthread_sigmask() etc., which the pthread_code just wrapped.

>
> Here's the potential race:
> - uthread decides to notify its vcore (via sigpipe), say VC 6 on PC 5.
> - uthread traps into the kernel, asks for -1 here. kernel determines
> it's VC 6
> - __preempt occurs. for now, this is a message sitting in PC 5's RKM list.
> - the event from sys_self_notify() gets put into VC 6's mbox. the
> __notify occurs. for now, this is a message sitting in PC 5's RKM
> list.
> - the syscall completes and goes to return to the user, handling the
> RKMs in order.
> - the __preempt saves the uthread's ctx into VCPD for VC 6 (this ctx is
> the one that trapped in), and clears owning_proc/owning vcoreid.
> - the __notify has no effect, since the owning_proc is now wrong.
> - the preemption handler will notice that VC 6 was not in VC context and
> could yank the uthread from VCPD and start it on another vcore. A
> DONT_MIGRATE would prevent this.
>
> Ultimately, without DONT_MIGRATE, a uthread is not bound to a particular
> vcore.

Given this reace, using sys_self_notify() at all, seems like a bad
approach. We should have userspace take care of everything per some
scheme like the one I propose above. Just not sure how to make it
work when building glibc yet.

>
>> static int sys_self_notify(struct proc *p, uint32_t vcoreid,
>> unsigned int ev_type, struct event_msg *u_msg,
>> bool priv)
>> {
>> struct event_msg local_msg = {0};
>> +
>> + /* If vcoreid = -1, notify on the current vcore. */
>> + if (vcoreid == -1) {
>> + vcoreid = p->procinfo->pcoremap[core_id()].vcoreid;
>> + }
>
> Use pcpui->owning_vcoreid here. The pcoremap could be changed by a
> concurrent preemption (I think).
>
> Also, checkpatch didn't complain about this for some reason, but we
> don't need/want the extra {} for one liners.

Fair enough. I think I had a printf in there at some point while
testing, which is why I had the braces.

>
>
>> From 9aaf3a6347dfa824e3334e942c252fe2c8a7a175 Mon Sep 17 00:00:00 2001
>> From: Kevin Klues <klu...@cs.berkeley.edu>
>> Date: Tue, 27 Oct 2015 11:05:23 -0700
>> Subject: Add support for EPIPE and SIGPIPE
>
> Don't forget the (XCC) and something like "rebuild glibc".

Doh.

>
>> With this change, we now support proper handling of SIGPIPE. This
>> allows us to now run things such as:
>
>> static long pipewrite(struct chan *c, void *va, long n, int64_t ignored)
>> {
>> ERRSTACK(2);
>> Pipe *p;
>> - //Prog *r;
>>
>> if (waserror()) {
>> - /* avoid exceptions when pipe is a mounted queue */
>> - if ((c->flag & CMSG) == 0) {
>> -/*
>> - r = up->iprog;
>> - if(r != NULL && r->kill == NULL)
>> - r->kill = "write on closed pipe";
>> -*/
>> - }
>
> Even though that's commented out, we might want to keep it around. It could
> also be related to the bug you mentioned in your commit message.

Yeah, I was thinking about that. I'll put it back in.

>
>> - nexterror();
>> + poperror();
>> + error(EPIPE, NULL);
>
> This style is a little weird. A lot of times in Plan 9, they just set_errstr
> and call error. Either is okay. The problem here is that errstr is now gone.
> If you want to keep errstr, you could set_errno to EPIPE, then call nexterror.

Yeah, I've never gotten comfortable with the waserror() stuff in
general. If that's the preferred way, then I'll do it that way.

>
>> diff --git a/tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/write.c b/tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/write.c
>
>> +ssize_t __libc_write(int fd, const void *buf, size_t nbytes)
>> {
>> - return ros_syscall(SYS_write, fd, buf, nbytes, 0, 0, 0);
>> + int ret = ros_syscall(SYS_write, fd, buf, nbytes, 0, 0, 0);
>> +
>> + if (unlikely((ret != 0) && (errno == EPIPE))) {
>
> This probably won't compile on recent versions of Akaros, since unlikely() isn't
> in the kernel headers anymore.

There is no longer a ros/compile.h? From what I read, I thought that
remained, but it just got removed from automatic inclusion in
ros/common.h

>
>> + sigset_t mask;
>
> My general take on declaring variables halfway through a function instead of at
> the top is that you'd be better off with a helper function. But I didn't write
> that in the Contributing guide and neither did Linus.

I typically declare vars in the block they are going to be used (in
this case, within the if statement). It is only scoped for use with
the if statement then, and if accessed outside, it will throw a
compilation error.

>
>> +
>> + sigprocmask(0, NULL, &mask);
>> + if (!__sigismember(&mask, SIGPIPE)) {
>> + struct event_msg msg = {0};
>> +
>> + local_msg.ev_type = EV_POSIX_SIGNAL;
>> + local_msg.ev_arg1 = SIGPIPE;
>> + ros_syscall(SYS_self_notify, -1, EV_POSIX_SIGNAL, &msg, 0, 0, 0);
>
> I'll think about this over the weekend a bit. Maybe there's another nice way to
> get this to work. Hopefully this won't pop up for anything else.
>
> The whole "generate a signal for a syscall" seems nasty, since syscalls are
> synchronous vs async, instead of people just checking their return codes and
> exiting. I'm sure there was a reason for it.

I agree. I think we can find a way to make this work completely from
userspace, but it will take some magic with getting glibc to compile
if we end up calling any parlib/pthread functions from __libc_write().

>
> Barret

[Barret Rhoden]

On 2015-10-30 at 17:32 Kevin Klues <klu...@gmail.com> wrote:
> >> + /* Ensures signals we just unmasked get processed if they
> >> are pending */
> >> + sched_yield();
> >
> > Does this actually work? In the pthread side, the signals are in
> > the pending set. On the default 2LS side, I think if they aren't
> > fireably immediately, they get ignored.
>
> I was thinking about this as I wrote it, and decided to just put it in
> because it was symmetrical with the pthread implementation. It's
> probably OK to take it out, as it probably does nothing. It does
> actually work though if you're asking about it continuing to run after
> this point.

As far as "actually work" goes, I meant would it force the reception of
blocked signals, which I think won't happen.

> That was the idea. We need a way of stalling the current uthread and
> generating a signal in its place before allowing the uthread to
> continue (if it ever continues at all). In the pthread world, this
> would have simply been a pthread_kill(pthread_self(), SIGPIPE)
> followed by a pthread_yield(). However, I needed a way to make this
> work for non-pthread code. Anyway, we can't call any parlib/pthread
> code from within the write() call because of link errors when build
> rtld.

Yeah, it's a mess.

> >> - nexterror();
> >> + poperror();
> >> + error(EPIPE, NULL);
> >
> > This style is a little weird. A lot of times in Plan 9, they just
> > set_errstr and call error. Either is okay. The problem here is
> > that errstr is now gone. If you want to keep errstr, you could
> > set_errno to EPIPE, then call nexterror.
>
> Yeah, I've never gotten comfortable with the waserror() stuff in
> general. If that's the preferred way, then I'll do it that way.

It's not a huge deal either way - more of a "what do you want to do
here". As written, it'll do what you want, but it'll clobber errstr.
I was mostly trying to let you know that it's totally fine to just
change errno or errstr on its own and then call nexterror().

> > This probably won't compile on recent versions of Akaros, since
> > unlikely() isn't in the kernel headers anymore.
>
> There is no longer a ros/compile.h? From what I read, I thought that
> remained, but it just got removed from automatic inclusion in
> ros/common.h

Nope, it was totally yanked. My rationale was that it's not even part
of the kernel interface, so we don't need it in there. Given the
likelihood of collisions (e.g. libunwind), I was hesitant about putting
it anywhere.

We have a similar issue with ARRAY_SIZE. It used to be part of the
kernel headers, thinking that common kernel and user code could use it,
but then it turns out that it's so useful that many projects have it
too, and then we conflict.

It'd be nice if we could have a way to 'weak alias' these sorts of
things, but I don't know of any preprocessor tricks to do that.

> I agree. I think we can find a way to make this work completely from
> userspace, but it will take some magic with getting glibc to compile
> if we end up calling any parlib/pthread functions from __libc_write().

Yeah, let's sort this out in the flesh!

Barret

[Kevin Klues]

I've finally gotten around to updating this branch. As part of this, I
refactored the code for generating signals so that it is reusable
across different 2LSs (previously it was written specifically for the
pthread scheduler). Now, both the pthread scheduler and our simple
thread0 scheduler have built-in support for standard posix signals,
and it is straightforward (it only takes two strategically placed
commands) to add support to any future schedulers.

As part of this, I also updated the code to allow a signal handler to
continue running on the stack of whatever thread it is interrupting. I
also push the saved user context (and any other ancillary state it
needs to save) onto this stack before running the signal handler.
Previously, I had been allocating space for this on the side, so it
had to be cleaned up once the signal handler completed. With these
changes, Davide should now be able to do his longjmp calls from within
a signal handler without repercussions.

Finally, I used this new setup to implement proper SIGPIPE support in
user-space. The kernel always returns EPIPE in cases where there is a
write after all readers have closed, and user-space takes care of
generating the SIGPIPE signal on the current thread if necessary. With
the new way the code has been refactored, adding this new
functionality was trivial.

Rebuild your cross compiler!

The changes in this request can be viewed online at:
https://github.com/brho/akaros/compare/a759dfd...adf81cf

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:

g...@github.com:klueska/akaros sigpipe-support

for you to fetch changes up to adf81cfb9ae150a0e1b0202aeb6c76f38ab1f0f8:

Add support for EPIPE and SIGPIPE (XCC) (2015-11-11 00:55:00 -0800)

----------------------------------------------------------------
Kevin Klues (11):

Fix bug in pthread_sigmask() semantics.

Add parlib-compat.c for weak symbols with parlib
Weasel apart parlib/libc symbols for signals (XCC)
Add arch independent accessor for user ctx stack
Remove need for externally alloced sigdata struct
Encapsulate pthread sigstate into a single struct
Add uthread_paused() API call
Migrate signal code from pthread.c to signal.c
Add signal support to our basic thread0 scheduler
Add a sigself() signal_op
Add support for EPIPE and SIGPIPE (XCC)

kern/drivers/dev/pipe.c | 5 +-
.../glibc-2.19-akaros/sysdeps/akaros/Makefile | 5 +
.../glibc-2.19-akaros/sysdeps/akaros/Versions | 6 +
.../sysdeps/akaros/parlib-compat.c | 28 ++
.../glibc-2.19-akaros/sysdeps/akaros/sigaction.c | 166 ++++++++
.../glibc-2.19-akaros/sysdeps/akaros/sigaltstack.c | 27 ++
.../glibc-2.19-akaros/sysdeps/akaros/sigintr.c | 28 ++
.../glibc-2.19-akaros/sysdeps/akaros/sigpending.c | 28 ++
.../glibc-2.19-akaros/sysdeps/akaros/sigprocmask.c | 30 ++
.../glibc-2.19-akaros/sysdeps/akaros/sigqueue.c | 28 ++
.../glibc-2.19-akaros/sysdeps/akaros/sigreturn.c | 26 ++
.../glibc-2.19-akaros/sysdeps/akaros/sigstack.c | 27 ++
.../glibc-2.19-akaros/sysdeps/akaros/sigsuspend.c | 32 ++
.../sysdeps/akaros/sigtimedwait.c | 29 ++
.../glibc-2.19-akaros/sysdeps/akaros/sigwait.c | 28 ++
.../glibc-2.19-akaros/sysdeps/akaros/sigwaitinfo.c | 28 ++
.../glibc-2.19-akaros/sysdeps/akaros/write.c | 22 +-
user/parlib/include/parlib.h | 18 -
user/parlib/include/riscv/vcore.h | 5 +
user/parlib/include/signal.h | 61 +++
user/parlib/include/uthread.h | 3 +
user/parlib/include/x86/vcore32.h | 5 +
user/parlib/include/x86/vcore64.h | 5 +
user/parlib/signal.c | 458 ++++++++++-----------
user/parlib/thread0_sched.c | 3 +
user/parlib/uthread.c | 18 +
user/pthread/pthread.c | 193 +--------
user/pthread/pthread.h | 4 +-
28 files changed, 872 insertions(+), 444 deletions(-)
create mode 100644
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/parlib-compat.c
create mode 100644
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigaction.c
create mode 100644
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigaltstack.c
create mode 100644
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigintr.c
create mode 100644
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigpending.c
create mode 100644
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigprocmask.c
create mode 100644
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigqueue.c
create mode 100644
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigreturn.c
create mode 100644
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigstack.c
create mode 100644
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigsuspend.c
create mode 100644
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigtimedwait.c
create mode 100644
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigwait.c
create mode 100644
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigwaitinfo.c
create mode 100644 user/parlib/include/signal.h

[Barret Rhoden]

On 2015-11-11 at 04:19 Kevin Klues <klu...@gmail.com> wrote:
> I've finally gotten around to updating this branch.

looking at this now. btw, checkpatch mentioned a couple typos and such
(e.g. WARNING: 'fucntion' may be misspelled - perhaps 'function'?). I
can just touch that up, but I wanted to make sure your checkpatch /
git commit stuff was working or not.

barret

[Kevin Klues]

Hmm, it should be. I had to change lots of things due to checkpatch
finding stuff. Which commit is that in specifically?

[Kevin Klues]

Looking more closely, this spelling error is in a commit message, not
the code base. My workflow won't check for spelling errors in the
commit message because it applies checkpatch as a pre-commit hook.

--
~Kevin

[Barret Rhoden]

On 2015-11-13 at 13:08 Kevin Klues <klu...@gmail.com> wrote:
> Looking more closely, this spelling error is in a commit message, not
> the code base. My workflow won't check for spelling errors in the
> commit message because it applies checkpatch as a pre-commit hook.

commits 5 and 9.

we talked about setting up a post-commit hook of some sort too. maybe
that would have caught it?

no big deal either way.

[Kevin Klues]

With a post commit hook, the commit still occurs, but you get the
warnings after the fact. With a pre-commit hook, it blocks the commit.
There is also a commit-msg hook (which also blocks the commit), which
I could run checkpatch over as well. However, if you ever do a git
commit -n, then the commit-msg hook never runs. Seems like we need it
in all thre places: pre-commit, commit-msg, post-commit.

[Barret Rhoden]

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 <klu...@gmail.com> 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:
>
> g...@github.com:klueska/akaros sigpipe-support

> From 2846f15b685bfa11d705cbad7309163fd042c8d7 Mon Sep 17 00:00:00 2001
> From: Kevin Klues <klu...@cs.berkeley.edu>
> 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 <klu...@cs.berkeley.edu>
> 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 <klu...@cs.berkeley.edu>
> 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 <klu...@cs.berkeley.edu>
> 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.

[Kevin Klues]

Comments below based on your comments. I also patched up a bug
related to sigprocmask() that I noticed independent of your comments.
This change is reflected in:

e11dfb4 Fix bug in pthread_sigmask() semantics.
5c2be9f Migrate signal code from pthread.c to signal.c

>> --- 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);
>> }

Fixed

>> From d38bf552fecffadbc5336db29e3d101a0401be2c Mon Sep 17 00:00:00 2001
>> From: Kevin Klues <klu...@cs.berkeley.edu>
>> 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.

Fixed

>> From f080545ec5b79bbb4563b5b3123b90e5932e2f8e Mon Sep 17 00:00:00 2001
>> From: Kevin Klues <klu...@cs.berkeley.edu>
>> 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.

Agreed. Fixed.

That's right. Take a look at user/parlib/thread0_sched.c It does this
exact thing to add signal support for the dumb thread0 scheduler.

>> - /* 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.

Yes that's right. It just so happens that the pthread scheduler
doesn't need to do anything different than the default anymore, so it
doesn't overwrite any of the function pointers. If your scheduler
needs to do something custom, just overwrite the calls you care about.

[Kevin Klues]

On Fri, Nov 13, 2015 at 3:25 PM, Kevin Klues <klu...@gmail.com> wrote:
> Comments below based on your comments. I also patched up a bug
> related to sigprocmask() that I noticed independent of your comments.
> This change is reflected in:
>
> e11dfb4 Fix bug in pthread_sigmask() semantics.
> 5c2be9f Migrate signal code from pthread.c to signal.c

I lied, I reintroduced an old bug with this "fix". I'll revert and
repush in a sec.

--
~Kevin

[Kevin Klues]

OK, fixed. And rebased onto master.

--
~Kevin

[Kevin Klues]

Newest patchset, for reference:

https://github.com/brho/akaros/compare/bc87a54...ab5b2c5

git fetch g...@github.com:klueska/akaros sigpipe-support

--
~Kevin

[Barret Rhoden]

On 2015-11-13 at 15:45 Kevin Klues <klu...@gmail.com> wrote:
> Newest patchset, for reference:
>
> https://github.com/brho/akaros/compare/bc87a54...ab5b2c5
>
> git fetch g...@github.com:klueska/akaros sigpipe-support

Thanks, merged to master at bef4b38..1185c0a (from, to]

You can see the entire diff with 'git diff' or at
https://github.com/brho/akaros/compare/bef4b38...1185c0a

p.s. - rebuild busybox too.

Barret