On Mon, Jul 10, 2023 at 10:41:15AM -0500, Scott Cheloha wrote: > On Mon, Jul 10, 2023 at 05:19:35PM +0200, Mark Kettenis wrote: > > > Date: Mon, 10 Jul 2023 09:57:39 -0500 > > > From: Scott Cheloha <scottchel...@gmail.com> > > > > > > On Mon, Jul 10, 2023 at 07:42:55AM -0600, Theo de Raadt wrote: > > > > I dare you to write the simplest fix for this, instead of a diff that > > > > scrolls by. > > > > > > This patch seems to work. Will need to bang on it for a few more days. > > > > > > 1. Disable gmoninit after sched_stop_scondary_cpus(). The secondary > > > CPUs have halted, so we aren't racing sysctl(2) on a secondary CPU. > > > > > > 2. Restore gmoninit between resume_mp() and sched_start_secondary_cpus(). > > > The secondary CPUs are out of cpu_hatch(), which is probably where we > > > are crashing during resume. The secondary CPUs haven't started > > > scheduling yet, so we aren't racing sysctl(2). > > > > It is still a bit scary to have cpu_hatch() call _mcount() but I guess > > adding __attribute__((no_profile)) to all of the functions called by > > cpu_hatch() isn't really workable either. > > > > That said... > > > > > Index: subr_suspend.c > > > =================================================================== > > > RCS file: /cvs/src/sys/kern/subr_suspend.c,v > > > retrieving revision 1.15 > > > diff -u -p -r1.15 subr_suspend.c > > > --- subr_suspend.c 2 Jul 2023 19:02:27 -0000 1.15 > > > +++ subr_suspend.c 10 Jul 2023 14:51:01 -0000 > > > @@ -18,6 +18,7 @@ > > > > > > #include <sys/param.h> > > > #include <sys/systm.h> > > > +#include <sys/atomic.h> > > > #include <sys/buf.h> > > > #include <sys/clockintr.h> > > > #include <sys/reboot.h> > > > @@ -100,6 +101,14 @@ top: > > > #ifdef MULTIPROCESSOR > > > sched_stop_secondary_cpus(); > > > KASSERT(CPU_IS_PRIMARY(curcpu())); > > > +#endif > > > +#ifdef GPROF > > > + extern int gmoninit; > > > + int gmon_state = gmoninit; > > > > No variable declarations in the middle of functions please. > > Yep, moved up. > > > > + gmoninit = 0; > > > + membar_producer(); > > > > Why are you messing with memory barriers here? > > > > > +#endif > > > +#ifdef MULTIPROCESSOR > > > sleep_mp(); > > > #endif > > > > > > @@ -172,6 +181,12 @@ fail_suspend: > > > resume_randomness(rndbuf, rndbuflen); > > > #ifdef MULTIPROCESSOR > > > resume_mp(); > > > +#endif > > > +#ifdef GPROF > > > + gmoninit = gmon_state; > > > + membar_producer(); > > > > And here? > > gmoninit is not volatile. I had this idea that I wanted the store to > be globally visible before proceeding. But you and Theo are telling > me I don't need the barrier. I'll take your word for it, dropped.
... is this ok or is something still amiss here? Index: subr_suspend.c =================================================================== RCS file: /cvs/src/sys/kern/subr_suspend.c,v retrieving revision 1.15 diff -u -p -r1.15 subr_suspend.c --- subr_suspend.c 2 Jul 2023 19:02:27 -0000 1.15 +++ subr_suspend.c 11 Jul 2023 20:27:50 -0000 @@ -26,6 +26,9 @@ #include <sys/mount.h> #include <sys/syscallargs.h> #include <dev/wscons/wsdisplayvar.h> +#ifdef GPROF +#include <sys/gmon.h> +#endif #ifdef HIBERNATE #include <sys/hibernate.h> #endif @@ -49,6 +52,9 @@ sleep_state(void *v, int sleepmode) extern int perflevel; size_t rndbuflen; char *rndbuf; +#ifdef GPROF + int gmon_state; +#endif #if NSOFTRAID > 0 extern void sr_quiesce(void); #endif @@ -100,6 +106,12 @@ top: #ifdef MULTIPROCESSOR sched_stop_secondary_cpus(); KASSERT(CPU_IS_PRIMARY(curcpu())); +#endif +#ifdef GPROF + gmon_state = gmoninit; + gmoninit = 0; +#endif +#ifdef MULTIPROCESSOR sleep_mp(); #endif @@ -172,6 +184,11 @@ fail_suspend: resume_randomness(rndbuf, rndbuflen); #ifdef MULTIPROCESSOR resume_mp(); +#endif +#ifdef GPROF + gmoninit = gmon_state; +#endif +#ifdef MULTIPROCESSOR sched_start_secondary_cpus(); #endif vfs_stall(curproc, 0);