On Sun, 6 Oct 2013, Joel Sing wrote:
...
> I like the concept of this diff, however it is not sufficient to solve
> the multithreaded profiling problem. The reason for this is that you're
> still sending SPROCESS signals, which are delivered to the main thread
> unless another thread has diverted the signal. This means that all of
> the profiling signals end up on the main thread, not the thread that was
> consuming CPU time.
>
> This can be fixed by either using ptsignal(p, SIG{PROF,VTALARM},
> STHREAD) instead of psignal(), or by including your other diff that
> changes the signal delivery within ptsignal - regardless, I think the
> previous diff is worth pursuing since it potentially reduces the number
> of signal related context switches.
Okay, here's a complete diff that, at least on my box with a regress test
that jsing@ should be committing soon, solves the problem, reduces the
total complexity of the itimer signal delivery, and reduces the context
switches caused by signal by having a thread sending a signal to its own
process take delivery of the signal if it can.
(While checking up on something else, I found that FreeBSD already uses
this take the cannoli^Wsignal logic.)
ok?
Philip
Index: bin/ps/ps.1
===================================================================
RCS file: /cvs/src/bin/ps/ps.1,v
retrieving revision 1.84
diff -u -p -r1.84 ps.1
--- bin/ps/ps.1 22 Sep 2013 17:28:34 -0000 1.84
+++ bin/ps/ps.1 7 Oct 2013 02:08:32 -0000
@@ -222,6 +222,8 @@ The thread flags (in hexadecimal), as de
.Aq Pa sys/proc.h :
.Bd -literal
P_INKTR 0x1 writing ktrace(2) record
+P_PROFPEND 0x2 this thread needs SIGPROF
+P_ALRMPEND 0x4 this thread needs SIGVTALRM
P_SIGSUSPEND 0x8 need to restore before-suspend mask
P_SELECT 0x40 selecting; wakeup/waiting danger
P_SINTR 0x80 sleep is interruptible
Index: sys/sys/proc.h
===================================================================
RCS file: /cvs/src/sys/sys/proc.h,v
retrieving revision 1.170
diff -u -p -r1.170 proc.h
--- sys/sys/proc.h 22 Sep 2013 17:28:33 -0000 1.170
+++ sys/sys/proc.h 7 Oct 2013 02:08:35 -0000
@@ -209,8 +209,6 @@ struct process {
struct timespec ps_start; /* starting time. */
struct timeout ps_realit_to; /* real-time itimer trampoline. */
- struct timeout ps_virt_to; /* virtual itimer trampoline. */
- struct timeout ps_prof_to; /* prof itimer trampoline. */
int ps_refcnt; /* Number of references. */
};
@@ -362,6 +360,8 @@ struct proc {
* These flags are per-thread and kept in p_flag
*/
#define P_INKTR 0x000001 /* In a ktrace op, don't
recurse */
+#define P_PROFPEND 0x000002 /* SIGPROF needs to be posted */
+#define P_ALRMPEND 0x000004 /* SIGVTALRM needs to be posted
*/
#define P_SIGSUSPEND 0x000008 /* Need to restore
before-suspend mask*/
#define P_SELECT 0x000040 /* Selecting; wakeup/waiting
danger. */
#define P_SINTR 0x000080 /* Sleep is interruptible. */
@@ -380,7 +380,8 @@ struct proc {
#define P_CPUPEG 0x40000000 /* Do not move to another cpu. */
#define P_BITS \
- ("\20\01INKTR\04SIGSUSPEND\07SELECT\010SINTR\012SYSTEM" \
+ ("\20\01INKTR\02PROFPEND\03ALRMPEND\04SIGSUSPEND\07SELECT" \
+ "\010SINTR\012SYSTEM" \
"\013TIMEOUT\016WEXIT\020OWEUPC\024SUSPSINGLE" \
"\025NOZOMBIE\027SYSTRACE\030CONTINUED\033THREAD" \
"\034SUSPSIG\035SOFTDEP\036STOPPED\037CPUPEG")
Index: sys/sys/resourcevar.h
===================================================================
RCS file: /cvs/src/sys/sys/resourcevar.h,v
retrieving revision 1.16
diff -u -p -r1.16 resourcevar.h
--- sys/sys/resourcevar.h 3 Jun 2013 16:55:22 -0000 1.16
+++ sys/sys/resourcevar.h 7 Oct 2013 02:08:35 -0000
@@ -68,8 +68,5 @@ struct plimit *limcopy(struct plimit *);
void limfree(struct plimit *);
void ruadd(struct rusage *, struct rusage *);
-
-void virttimer_trampoline(void *);
-void proftimer_trampoline(void *);
#endif
#endif /* !_SYS_RESOURCEVAR_H_ */
Index: sys/kern/kern_clock.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_clock.c,v
retrieving revision 1.82
diff -u -p -r1.82 kern_clock.c
--- sys/kern/kern_clock.c 13 Aug 2013 05:52:23 -0000 1.82
+++ sys/kern/kern_clock.c 7 Oct 2013 02:08:37 -0000
@@ -144,40 +144,11 @@ initclocks(void)
/*
* hardclock does the accounting needed for ITIMER_PROF and ITIMER_VIRTUAL.
* We don't want to send signals with psignal from hardclock because it makes
- * MULTIPROCESSOR locking very complicated. Instead we use a small trick
- * to send the signals safely and without blocking too many interrupts
- * while doing that (signal handling can be heavy).
- *
- * hardclock detects that the itimer has expired, and schedules a timeout
- * to deliver the signal. This works because of the following reasons:
- * - The timeout can be scheduled with a 1 tick time because we're
- * doing it before the timeout processing in hardclock. So it will
- * be scheduled to run as soon as possible.
- * - The timeout will be run in softclock which will run before we
- * return to userland and process pending signals.
- * - If the system is so busy that several VIRTUAL/PROF ticks are
- * sent before softclock processing, we'll send only one signal.
- * But if we'd send the signal from hardclock only one signal would
- * be delivered to the user process. So userland will only see one
- * signal anyway.
+ * MULTIPROCESSOR locking very complicated. Instead, to use an idea from
+ * FreeBSD, we set a flag on the thread and when it goes to return to
+ * userspace it signals itself.
*/
-void
-virttimer_trampoline(void *v)
-{
- struct process *pr = v;
-
- psignal(pr->ps_mainproc, SIGVTALRM);
-}
-
-void
-proftimer_trampoline(void *v)
-{
- struct process *pr = v;
-
- psignal(pr->ps_mainproc, SIGPROF);
-}
-
/*
* The real-time timer, interrupting hz times per second.
*/
@@ -196,11 +167,15 @@ hardclock(struct clockframe *frame)
*/
if (CLKF_USERMODE(frame) &&
timerisset(&pr->ps_timer[ITIMER_VIRTUAL].it_value) &&
- itimerdecr(&pr->ps_timer[ITIMER_VIRTUAL], tick) == 0)
- timeout_add(&pr->ps_virt_to, 1);
+ itimerdecr(&pr->ps_timer[ITIMER_VIRTUAL], tick) == 0) {
+ atomic_setbits_int(&p->p_flag, P_ALRMPEND);
+ need_proftick(p);
+ }
if (timerisset(&pr->ps_timer[ITIMER_PROF].it_value) &&
- itimerdecr(&pr->ps_timer[ITIMER_PROF], tick) == 0)
- timeout_add(&pr->ps_prof_to, 1);
+ itimerdecr(&pr->ps_timer[ITIMER_PROF], tick) == 0) {
+ atomic_setbits_int(&p->p_flag, P_PROFPEND);
+ need_proftick(p);
+ }
}
/*
Index: sys/kern/kern_exit.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_exit.c,v
retrieving revision 1.127
diff -u -p -r1.127 kern_exit.c
--- sys/kern/kern_exit.c 14 Sep 2013 01:35:00 -0000 1.127
+++ sys/kern/kern_exit.c 7 Oct 2013 02:08:37 -0000
@@ -185,8 +185,6 @@ exit1(struct proc *p, int rv, int flags)
if ((p->p_flag & P_THREAD) == 0) {
timeout_del(&pr->ps_realit_to);
- timeout_del(&pr->ps_virt_to);
- timeout_del(&pr->ps_prof_to);
#ifdef SYSVSEM
semexit(pr);
#endif
Index: sys/kern/kern_fork.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_fork.c,v
retrieving revision 1.153
diff -u -p -r1.153 kern_fork.c
--- sys/kern/kern_fork.c 14 Aug 2013 05:26:14 -0000 1.153
+++ sys/kern/kern_fork.c 7 Oct 2013 02:08:38 -0000
@@ -183,8 +183,6 @@ process_new(struct proc *p, struct proce
pr->ps_limit->p_refcnt++;
timeout_set(&pr->ps_realit_to, realitexpire, pr);
- timeout_set(&pr->ps_virt_to, virttimer_trampoline, pr);
- timeout_set(&pr->ps_prof_to, proftimer_trampoline, pr);
pr->ps_flags = parent->ps_flags & (PS_SUGID | PS_SUGIDEXEC);
if (parent->ps_session->s_ttyvp != NULL &&
Index: sys/kern/kern_sig.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.153
diff -u -p -r1.153 kern_sig.c
--- sys/kern/kern_sig.c 6 Oct 2013 19:44:42 -0000 1.153
+++ sys/kern/kern_sig.c 7 Oct 2013 02:08:39 -0000
@@ -811,27 +811,39 @@ ptsignal(struct proc *p, int signum, enu
}
/*
- * A process-wide signal can be diverted to a different
- * thread that's in sigwait() for this signal. If there
- * isn't such a thread, then pick a thread that doesn't
- * have it blocked so that the stop/kill consideration
- * isn't delayed. Otherwise, mark it pending on the
- * main thread.
- */
- TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
- /* ignore exiting threads */
- if (q->p_flag & P_WEXIT)
- continue;
-
- /* sigwait: definitely go to this thread */
- if (q->p_sigdivert & mask) {
- p = q;
- break;
+ * If the current thread can process the signal
+ * immediately, either because it's sigwait()ing
+ * on it or has it unblocked, then have it take it.
+ */
+ q = curproc;
+ if (q != NULL && q->p_p == pr && (q->p_flag & P_WEXIT) == 0 &&
+ ((q->p_sigdivert & mask) || (q->p_sigmask & mask) == 0))
+ p = q;
+ else {
+ /*
+ * A process-wide signal can be diverted to a
+ * different thread that's in sigwait() for this
+ * signal. If there isn't such a thread, then
+ * pick a thread that doesn't have it blocked so
+ * that the stop/kill consideration isn't
+ * delayed. Otherwise, mark it pending on the
+ * main thread.
+ */
+ TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
+ /* ignore exiting threads */
+ if (q->p_flag & P_WEXIT)
+ continue;
+
+ /* sigwait: definitely go to this thread */
+ if (q->p_sigdivert & mask) {
+ p = q;
+ break;
+ }
+
+ /* unblocked: possibly go to this thread */
+ if ((q->p_sigmask & mask) == 0)
+ p = q;
}
-
- /* unblocked: possibly go to this thread */
- if ((q->p_sigmask & mask) == 0)
- p = q;
}
}
@@ -1735,6 +1747,20 @@ void
userret(struct proc *p)
{
int sig;
+
+ /* send SIGPROF or SIGVTALRM if their timers interrupted this thread */
+ if (p->p_flag & P_PROFPEND) {
+ atomic_clearbits_int(&p->p_flag, P_PROFPEND);
+ KERNEL_LOCK();
+ psignal(p, SIGPROF);
+ KERNEL_UNLOCK();
+ }
+ if (p->p_flag & P_ALRMPEND) {
+ atomic_clearbits_int(&p->p_flag, P_ALRMPEND);
+ KERNEL_LOCK();
+ psignal(p, SIGVTALRM);
+ KERNEL_UNLOCK();
+ }
while ((sig = CURSIG(p)) != 0)
postsig(sig);
Index: sys/kern/kern_time.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_time.c,v
retrieving revision 1.83
diff -u -p -r1.83 kern_time.c
--- sys/kern/kern_time.c 6 Oct 2013 01:27:50 -0000 1.83
+++ sys/kern/kern_time.c 7 Oct 2013 02:08:39 -0000
@@ -571,10 +571,6 @@ sys_setitimer(struct proc *p, void *v, r
itimerround(&aitv.it_interval);
s = splclock();
pr->ps_timer[which] = aitv;
- if (which == ITIMER_VIRTUAL)
- timeout_del(&pr->ps_virt_to);
- if (which == ITIMER_PROF)
- timeout_del(&pr->ps_prof_to);
splx(s);
}
