On Fri, 16 Aug 2013, Ted Unangst wrote:
> As per http://research.swtch.com/macpprof
>
> We deliver all prof signals to the main thread, which is unlikely to
> result in accurate profiling info. I think the diff below fixes things.
How about we take an idea from FreeBSD and have hardclock() just set a
flag on the thread and then have the thread send SIGPROF (and SIGVTALRM)
to itself from userret(). The signal gets sent by the thread itself right
before it checks for pending signals when returning to userspace, so
that's absolutely as soon as possible, and it's done from process context
instead of from softclock by a timeout, so no "which CPU are we on?"
issues that might delay the delivery.
Ted, Joel, does this solve your profiling problems?
(No, this had nothing at all to do with my splitting the process and
thread flags, what could possibly make you think that...)
Philip
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 4 Oct 2013 01:00:18 -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 +361,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 +381,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 4 Oct 2013 01:00:18 -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 4 Oct 2013 01:00:19 -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 4 Oct 2013 01:00:20 -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 4 Oct 2013 01:00:20 -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.152
diff -u -p -r1.152 kern_sig.c
--- sys/kern/kern_sig.c 1 Jun 2013 04:05:26 -0000 1.152
+++ sys/kern/kern_sig.c 4 Oct 2013 01:00:23 -0000
@@ -1735,6 +1735,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: 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 4 Oct 2013 01:00:23 -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
