On Fri, 4 Oct 2013, Philip Guenther wrote:
> 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?
Nope.
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.
P.S. The diff below misses a chunk from kern_time.c.
> (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
--
"Action without study is fatal. Study without action is futile."
-- Mary Ritter Beard
