[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 09/15] Hurd signals: implement global signal dispositions
From: |
Samuel Thibault |
Subject: |
Re: [PATCH 09/15] Hurd signals: implement global signal dispositions |
Date: |
Sun, 3 Jul 2011 02:16:34 +0200 |
User-agent: |
Mutt/1.5.21+34 (58baf7c9f32f) (2010-12-30) |
Jeremie Koenig, le Wed 29 Jun 2011 18:30:21 +0200, a écrit :
> Currently each thread has a full "sigstate" structure which keeps track
> of the signal dispositions, blocking mask and pending signals for this
> thread. Process-wide signals are delivered to the main thread.
>
> However, the semantics for POSIX threads is that all of them share the
> same signal dispositions, although their blocking masks may differ.
> Signals sent to the process as a whole can be delivered to any thread
> which does not block them.
>
> This is implemented here in addition to the current behavior: libpthread
> will call _hurd_sigstate_set_global_rcv to mark newly created threads as
> global signal receivers, while cthreads-based programs can continue to
> rely on the Hurd semantics.
Agreed, just a few details following.
Jeremie Koenig, le Wed 29 Jun 2011 18:30:21 +0200, a écrit :
> diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
> index 74a01a6..eaf5ac1 100644
> --- a/hurd/hurdsig.c
> +++ b/hurd/hurdsig.c
> +sigstate_is_global_rcv (const struct hurd_sigstate *ss)
> +_hurd_sigstate_lock (struct hurd_sigstate *ss)
> +_hurd_sigstate_unlock (struct hurd_sigstate *ss)
> +_hurd_sigstate_pending (const struct hurd_sigstate *ss)
> +sigstate_clear_pending (struct hurd_sigstate *ss, int signo)
> +_hurd_sigstate_actions (struct hurd_sigstate *ss)
These should probably be made extern inline.
> diff --git a/hurd/hurdmsg.c b/hurd/hurdmsg.c
> index ffcce61..85b87aa 100644
> --- a/hurd/hurdmsg.c
> +++ b/hurd/hurdmsg.c
> @@ -124,7 +125,7 @@ get_int (int which, int *value)
> return 0;
> case INIT_SIGMASK:
> {
> - struct hurd_sigstate *ss = _hurd_thread_sigstate (_hurd_sigthread);
> + struct hurd_sigstate *ss = _hurd_global_sigstate;
> __spin_lock (&ss->lock);
> *value = ss->blocked;
> __spin_unlock (&ss->lock);
Mmm, this should be left as _hurd_thread_sigstate: the global sigstate's
blocked field does not make much sense, right?
> @@ -210,7 +211,7 @@ set_int (int which, int value)
> /* These are pretty odd things to do. But you asked for it. */
> case INIT_SIGMASK:
> {
> - struct hurd_sigstate *ss = _hurd_thread_sigstate (_hurd_sigthread);
> + struct hurd_sigstate *ss = _hurd_global_sigstate;
> __spin_lock (&ss->lock);
> ss->blocked = value;
> __spin_unlock (&ss->lock);
Likewise.
> diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
> index 74a01a6..eaf5ac1 100644
> --- a/hurd/hurdsig.c
> +++ b/hurd/hurdsig.c
> @@ -83,7 +83,7 @@ _hurd_thread_sigstate (thread_t thread)
> {
> ss = malloc (sizeof (*ss));
> if (ss == NULL)
> - __libc_fatal ("hurd: Can't allocate thread sigstate\n");
> + __libc_fatal ("hurd: Can't allocate sigstate\n");
> ss->thread = thread;
> __spin_lock_init (&ss->lock);
>
This seems spurious?
> + /* The global sigstate is not added to the _hurd_sigstates list,
> + and while it is created using _hurd_sigthread (MACH_PORT_NULL),
_hurd_thread_sigstate (MACH_PORT_NULL)
> /* Check for a preempted signal. Preempted signals can arrive during
> critical sections. */
> + /* XXX how does this mix with _hurd_global_sigstate? Should its PREEMPTORS
> + * field replace _hurdsig_preemptors? */
It could be an idea, indeed.
> - /* Post the signal to the designated signal-receiving thread. This will
> + /* Post the signal to XXX[the designated signal-receiving thread.] This
> will
Are you looking for a way to explain that either the signal-receiving
thread, or any thread, will receiving, depending on pthread vs cthreads?
> @@ -1258,8 +1343,8 @@ extern void __mig_init (void *);
>
> #include <mach/task_special_ports.h>
>
> -/* Initialize the message port and _hurd_sigthread and start the signal
> - thread. */
> +/* Initialize the message port, _hurd_sigthread and _hurd_global_sigstate
> + and start the signal thread. */
_hurd_sigthread does not exist any more with the patch.
> diff --git a/sysdeps/mach/hurd/fork.c b/sysdeps/mach/hurd/fork.c
> index 3288f18..a4f3055 100644
> --- a/sysdeps/mach/hurd/fork.c
> +++ b/sysdeps/mach/hurd/fork.c
> @@ -459,6 +459,7 @@ __fork (void)
> function, accounted for by mach_port_names (and which will thus be
> accounted for in the child below). This extra right gets consumed
> in the child by the store into _hurd_sigthread in the child fork. */
> + /* XXX consumed? (_hurd_sigthread is no more) */
> if (thread_refs > 1 &&
> (err = __mach_port_mod_refs (newtask, ss->thread,
> MACH_PORT_RIGHT_SEND,
I don't understand either. I don't see port references around
the existing _hurd_sigthread management.
> diff --git a/sysdeps/mach/hurd/i386/trampoline.c
> b/sysdeps/mach/hurd/i386/trampoline.c
> index 99d9308..ec52847 100644
> --- a/sysdeps/mach/hurd/i386/trampoline.c
> +++ b/sysdeps/mach/hurd/i386/trampoline.c
> @@ -77,7 +77,11 @@ _hurd_setup_sighandler (struct hurd_sigstate *ss,
> __sighandler_t handler,
> interrupted RPC frame. */
> state->basic.esp = state->basic.uesp;
>
> - if ((ss->actions[signo].sa_flags & SA_ONSTACK) &&
> + /* XXX what if handler != action->handler (for instance, if a signal
> + * preemptor took over) ? */
> + action = & _hurd_sigstate_actions (ss) [signo];
> +
> + if ((action->sa_flags & SA_ONSTACK) &&
The SA_ONSTACK logic remains the same. I believe this is right. In
the Hurd semantic case your patch does not change the behavior. In the
POSIX semantic all threads share the SA_ONSTACK logic. This is what
POSIX says for signals. I don't think it is a problem for preemptors.
> diff --git a/sysdeps/mach/hurd/spawni.c b/sysdeps/mach/hurd/spawni.c
> index 244ca2d..373da8d 100644
> --- a/sysdeps/mach/hurd/spawni.c
> +++ b/sysdeps/mach/hurd/spawni.c
> @@ -239,26 +239,29 @@ __spawni (pid_t *pid, const char *file,
> assert (! __spin_lock_locked (&ss->critical_section_lock));
> __spin_lock (&ss->critical_section_lock);
>
> - __spin_lock (&ss->lock);
> + _hurd_sigstate_lock (ss);
> ints[INIT_SIGMASK] = ss->blocked;
> - ints[INIT_SIGPENDING] = ss->pending;
> + ints[INIT_SIGPENDING] = _hurd_sigstate_pending (ss); /* XXX really? */
Mmm. According to POSIX, fork() is supposed to clear pending signals,
but GNU/Hurd currently does not clear them. According to POSIX,
exec() is supposed to not clear pending signals. So at least, spawn()
inheriting pending signals is coherent in GNU/Hurd. Making fork() and
spwan() clear pending signals would be a separate fix.
Samuel
- Re: [PATCH 09/15] Hurd signals: implement global signal dispositions,
Samuel Thibault <=