On 20/03/21(Sat) 13:25, Martin Pieuchot wrote:
> Diff below refactors routines to stop/unstop processes and save the signal
> number which will/can be transmitted it in wait4(2). It does the following:
>
> - Move the "hack" involving P_SINTR to avoid grabbing the SCHED_LOCK()
> recursively inside proc_stop().
>
> - Introduce proc_unstop(), the symmetric routine to proc_stop().
>
> - Manipulate `ps_xsig' only in proc_stop/unstop().
>
> Ok?
Anyone?
> Index: kern/kern_sig.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sig.c,v
> retrieving revision 1.278
> diff -u -p -r1.278 kern_sig.c
> --- kern/kern_sig.c 12 Mar 2021 10:13:28 -0000 1.278
> +++ kern/kern_sig.c 20 Mar 2021 12:16:51 -0000
> @@ -124,7 +124,7 @@ const int sigprop[NSIG + 1] = {
>
> void setsigvec(struct proc *, int, struct sigaction *);
>
> -void proc_stop(struct proc *p, int);
> +int proc_stop(struct proc *p, int, int);
> void proc_stop_sweep(void *);
> void *proc_stop_si;
>
> @@ -1061,8 +1061,7 @@ ptsignal(struct proc *p, int signum, enu
> if (pr->ps_flags & PS_PPWAIT)
> goto out;
> atomic_clearbits_int(siglist, mask);
> - pr->ps_xsig = signum;
> - proc_stop(p, 0);
> + proc_stop(p, signum, 0);
> goto out;
> }
> /*
> @@ -1170,17 +1169,12 @@ out:
> *
> * while (signum = cursig(curproc))
> * postsig(signum);
> - *
> - * Assumes that if the P_SINTR flag is set, we're holding both the
> - * kernel and scheduler locks.
> */
> int
> cursig(struct proc *p)
> {
> struct process *pr = p->p_p;
> int sigpending, signum, mask, prop;
> - int dolock = (p->p_flag & P_SINTR) == 0;
> - int s;
>
> KERNEL_ASSERT_LOCKED();
>
> @@ -1217,31 +1211,22 @@ cursig(struct proc *p)
> */
> if (((pr->ps_flags & (PS_TRACED | PS_PPWAIT)) == PS_TRACED) &&
> signum != SIGKILL) {
> - pr->ps_xsig = signum;
>
> single_thread_set(p, SINGLE_SUSPEND, 0);
> -
> - if (dolock)
> - SCHED_LOCK(s);
> - proc_stop(p, 1);
> - if (dolock)
> - SCHED_UNLOCK(s);
> -
> + signum = proc_stop(p, signum, 1);
> single_thread_clear(p, 0);
>
> /*
> * If we are no longer being traced, or the parent
> * didn't give us a signal, look for more signals.
> */
> - if ((pr->ps_flags & PS_TRACED) == 0 ||
> - pr->ps_xsig == 0)
> + if ((pr->ps_flags & PS_TRACED) == 0 || signum == 0)
> continue;
>
> /*
> * If the new signal is being masked, look for other
> * signals.
> */
> - signum = pr->ps_xsig;
> mask = sigmask(signum);
> if ((p->p_sigmask & mask) != 0)
> continue;
> @@ -1286,12 +1271,7 @@ cursig(struct proc *p)
> (pr->ps_pgrp->pg_jobc == 0 &&
> prop & SA_TTYSTOP))
> break; /* == ignore */
> - pr->ps_xsig = signum;
> - if (dolock)
> - SCHED_LOCK(s);
> - proc_stop(p, 1);
> - if (dolock)
> - SCHED_UNLOCK(s);
> + proc_stop(p, signum, 1);
> break;
> } else if (prop & SA_IGNORE) {
> /*
> @@ -1331,15 +1311,21 @@ keep:
> * Put the argument process into the stopped state and notify the parent
> * via wakeup. Signals are handled elsewhere. The process must not be
> * on the run queue.
> + *
> + * Assumes that if the P_SINTR flag is set, we're holding the scheduler
> + * lock.
> */
> -void
> -proc_stop(struct proc *p, int sw)
> +int
> +proc_stop(struct proc *p, int signum, int sw)
> {
> struct process *pr = p->p_p;
> + int dolock = (p->p_flag & P_SINTR) == 0;
> + int s;
>
> -#ifdef MULTIPROCESSOR
> - SCHED_ASSERT_LOCKED();
> -#endif
> + pr->ps_xsig = signum;
> +
> + if (dolock)
> + SCHED_LOCK(s);
>
> p->p_stat = SSTOP;
> atomic_clearbits_int(&pr->ps_flags, PS_WAITED);
> @@ -1352,6 +1338,13 @@ proc_stop(struct proc *p, int sw)
> softintr_schedule(proc_stop_si);
> if (sw)
> mi_switch();
> +
> + if (dolock)
> + SCHED_UNLOCK(s);
> +
> + signum = pr->ps_xsig;
> +
> + return signum;
> }
>
> /*
> @@ -1376,6 +1369,27 @@ proc_stop_sweep(void *v)
> }
> }
>
> +void
> +proc_unstop(struct proc *p, int signum)
> +{
> + struct process *pr = p->p_p;
> +
> + KASSERT(signum >= 0);
> + KASSERT(p->p_stat == SSTOP);
> +
> + if (signum != 0)
> + pr->ps_xsig = signum;
> +
> + /*
> + * If we're being traced (possibly because someone attached us
> + * while we were stopped), check for a signal from the debugger.
> + */
> + if ((pr->ps_flags & PS_TRACED) != 0 && pr->ps_xsig != 0)
> + atomic_setbits_int(&p->p_siglist, sigmask(pr->ps_xsig));
> +
> + setrunnable(p);
> +}
> +
> /*
> * Take the action for the specified signal
> * from the current set of pending signals.
> @@ -2042,7 +2056,7 @@ single_thread_set(struct proc *p, enum s
> if (q->p_flag & P_WEXIT) {
> if (mode == SINGLE_EXIT) {
> if (q->p_stat == SSTOP) {
> - setrunnable(q);
> + proc_unstop(q, 0);
> atomic_inc_int(&pr->ps_singlecount);
> }
> }
> @@ -2069,7 +2083,7 @@ single_thread_set(struct proc *p, enum s
> break;
> case SSTOP:
> if (mode == SINGLE_EXIT) {
> - setrunnable(q);
> + proc_unstop(q, 0);
> atomic_inc_int(&pr->ps_singlecount);
> }
> break;
> @@ -2138,7 +2152,7 @@ single_thread_clear(struct proc *p, int
> */
> if (q->p_stat == SSTOP && (q->p_flag & flag) == 0) {
> if (q->p_wchan == 0)
> - setrunnable(q);
> + proc_unstop(q, 0);
> else
> q->p_stat = SSLEEP;
> }
> Index: kern/sched_bsd.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/sched_bsd.c,v
> retrieving revision 1.65
> diff -u -p -r1.65 sched_bsd.c
> --- kern/sched_bsd.c 10 Dec 2020 04:26:50 -0000 1.65
> +++ kern/sched_bsd.c 20 Mar 2021 12:16:51 -0000
> @@ -456,12 +456,6 @@ setrunnable(struct proc *p)
> default:
> panic("setrunnable");
> case SSTOP:
> - /*
> - * If we're being traced (possibly because someone attached us
> - * while we were stopped), check for a signal from the debugger.
> - */
> - if ((pr->ps_flags & PS_TRACED) != 0 && pr->ps_xsig != 0)
> - atomic_setbits_int(&p->p_siglist, sigmask(pr->ps_xsig));
> prio = p->p_usrpri;
> unsleep(p);
> break;
> Index: kern/sys_process.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/sys_process.c,v
> retrieving revision 1.86
> diff -u -p -r1.86 sys_process.c
> --- kern/sys_process.c 8 Feb 2021 10:51:02 -0000 1.86
> +++ kern/sys_process.c 20 Mar 2021 12:16:51 -0000
> @@ -484,9 +484,8 @@ ptrace_ctrl(struct proc *p, int req, pid
>
> /* Finally, deliver the requested signal (or none). */
> if (t->p_stat == SSTOP) {
> - tr->ps_xsig = data;
> SCHED_LOCK(s);
> - setrunnable(t);
> + proc_unstop(t, data);
> SCHED_UNLOCK(s);
> } else {
> if (data != 0)
> Index: sys/signalvar.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/signalvar.h,v
> retrieving revision 1.46
> diff -u -p -r1.46 signalvar.h
> --- sys/signalvar.h 4 Mar 2021 09:02:38 -0000 1.46
> +++ sys/signalvar.h 20 Mar 2021 12:16:51 -0000
> @@ -103,6 +103,7 @@ struct sigio_ref;
> /*
> * Machine-independent functions:
> */
> +void proc_unstop(struct proc *, int);
> int coredump(struct proc *p);
> void execsigs(struct proc *p);
> int cursig(struct proc *p);
>
