Re: GPROF: sleep_state: disable _mcount() across suspend/resume
> ok kettenis@ ok deraadt also
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
> Date: Tue, 11 Jul 2023 15:28:22 -0500 > From: Scott Cheloha > > 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 > > > > > > > > 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 - 1.15 > > > > +++ subr_suspend.c 10 Jul 2023 14:51:01 - > > > > @@ -18,6 +18,7 @@ > > > > > > > > #include > > > > #include > > > > +#include > > > > #include > > > > #include > > > > #include > > > > @@ -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? ok kettenis@ > 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.c2 Jul 2023 19:02:27 - 1.15 > +++ subr_suspend.c11 Jul 2023 20:27:50 - > @@ -26,6 +26,9 @@ > #include > #include > #include > +#ifdef GPROF > +#include > +#endif > #ifdef HIBERNATE > #include > #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); >
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
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 > > > > > > 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.c2 Jul 2023 19:02:27 - 1.15 > > > +++ subr_suspend.c10 Jul 2023 14:51:01 - > > > @@ -18,6 +18,7 @@ > > > > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -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 - 1.15 +++ subr_suspend.c 11 Jul 2023 20:27:50 - @@ -26,6 +26,9 @@ #include #include #include +#ifdef GPROF +#include +#endif #ifdef HIBERNATE #include #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);
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
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 > > > > 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 - 1.15 > > +++ subr_suspend.c 10 Jul 2023 14:51:01 - > > @@ -18,6 +18,7 @@ > > > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -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. -- suspend/resume and hibernate/unhibernate still work with the revised patch below. 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 - 1.15 +++ subr_suspend.c 10 Jul 2023 15:30:22 - @@ -26,6 +26,9 @@ #include #include #include +#ifdef GPROF +#include +#endif #ifdef HIBERNATE #include #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);
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
Mark Kettenis wrote: > 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. It now immediately returns. > > + int gmon_state = gmoninit; > > No variable declarations in the middle of functions please. Yep. Put it at the top. > > + gmoninit = 0; > > + membar_producer(); > > Why are you messing with memory barriers here? I have asked the same question. This has potential to make things worse.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
> Date: Mon, 10 Jul 2023 09:57:39 -0500 > From: Scott Cheloha > > 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.c2 Jul 2023 19:02:27 - 1.15 > +++ subr_suspend.c10 Jul 2023 14:51:01 - > @@ -18,6 +18,7 @@ > > #include > #include > +#include > #include > #include > #include > @@ -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. > + 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? > +#endif > +#ifdef MULTIPROCESSOR > sched_start_secondary_cpus(); > #endif > vfs_stall(curproc, 0); >
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
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). 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 - 1.15 +++ subr_suspend.c 10 Jul 2023 14:51:01 - @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -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; + gmoninit = 0; + membar_producer(); +#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(); +#endif +#ifdef MULTIPROCESSOR sched_start_secondary_cpus(); #endif vfs_stall(curproc, 0);
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
I dare you to write the simplest fix for this, instead of a diff that scrolls by.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
Scott Cheloha wrote: > Secondary CPUs are still running at the top of sleep_state(). To > disable _mcount with gmoninit we would need to wait until after > secondary CPUs have halted to toggle it off, which is way further into > sleep_state(). I suspect you are exaggerating the window of time when _mcount is dangerous to call. Disable it early, re-enable it late, and avoid trying to write a complicated solution.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
On Mon, Jul 10, 2023 at 07:09:19AM -0600, Theo de Raadt wrote: > Mark Kettenis wrote: > > > So isn't the real problem that some of the lower-level code involved > > in the resume path isn't properly marked to not do the > > instrumentation? Traditionally that was assembly code and we'd use > > NENTRY() (in amd64) or ENTRY_NP() (on some other architectures) to > > prevent thise functions from calling _mcount(). But that was only > > ever done for code used during early bootstrap of the kernel. And > > these days there may be C code that needs this as well. > > > > With your diff, functions in the suspend/resume path will still call > > _mcount() which may not be safe. > > I guess you can make critical functions not do _PROF_PROLOGUE > or you can make __mcount or _mcount aware that they should "do nothing", > or "nothing scary". > > Hell, save & toggle the 'gmoninit' variable during the suspend/resume > sequence, and then adjust one comment: > > /* > * Do not profile execution if memory for the current CPU > * descriptor and profiling buffers has not yet been allocated > * or if the CPU we are running on has not yet set its trap > -* handler > +* handler, or disabled during a suspend/resume sequence > */ > if (gmoninit == 0) > return; > > > Does this really need another variable? > > It feels like this can be 4 1-line diffs. Secondary CPUs are still running at the top of sleep_state(). To disable _mcount with gmoninit we would need to wait until after secondary CPUs have halted to toggle it off, which is way further into sleep_state(). Then, on the resume side, you need to keep the secondary CPUs from toggling gmoninit back on until after all other secondary CPUs have finished restarting, which I think means changing how we do sched_start_secondary_cpus(). ... given all of that, I thought adding a second variable was easier and less likely to break something more important than GPROF.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
Mark Kettenis wrote: > So isn't the real problem that some of the lower-level code involved > in the resume path isn't properly marked to not do the > instrumentation? Traditionally that was assembly code and we'd use > NENTRY() (in amd64) or ENTRY_NP() (on some other architectures) to > prevent thise functions from calling _mcount(). But that was only > ever done for code used during early bootstrap of the kernel. And > these days there may be C code that needs this as well. > > With your diff, functions in the suspend/resume path will still call > _mcount() which may not be safe. I guess you can make critical functions not do _PROF_PROLOGUE or you can make __mcount or _mcount aware that they should "do nothing", or "nothing scary". Hell, save & toggle the 'gmoninit' variable during the suspend/resume sequence, and then adjust one comment: /* * Do not profile execution if memory for the current CPU * descriptor and profiling buffers has not yet been allocated * or if the CPU we are running on has not yet set its trap -* handler +* handler, or disabled during a suspend/resume sequence */ if (gmoninit == 0) return; Does this really need another variable? It feels like this can be 4 1-line diffs.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
> Date: Sun, 9 Jul 2023 17:24:41 -0500 > From: Scott Cheloha > > On Sun, Jul 09, 2023 at 08:11:43PM +0200, Claudio Jeker wrote: > > On Sun, Jul 09, 2023 at 12:52:20PM -0500, Scott Cheloha wrote: > > > This patch fixes resume/unhibernate on GPROF kernels where kgmon(8) > > > has activated kernel profiling. > > > > > > I think the problem is that code called from cpu_hatch() does not play > > > nicely with _mcount(), so GPROF kernels crash during resume. I can't > > > point you to which code in particular, but keeping all CPUs out of > > > _mcount() until the primary CPU has completed resume/unhibernate fixes > > > the crash. > > > > > > ok? > > > > To be honest, I'm not sure we need something like this. GPROF is already a > > special case and poeple running a GPROF kernel should probably stop the > > collection of profile data before suspend/hibernate. > > Sorry, I was a little unclear in my original mail. > > When I say "has activated kernel profiling" I mean "has *ever* > activated kernel profiling". > > Regardless of whether or not profiling is active at the moment we > reach sleep_state(), if kernel profiling has *ever* been activated in > the past, the resume crashes. So isn't the real problem that some of the lower-level code involved in the resume path isn't properly marked to not do the instrumentation? Traditionally that was assembly code and we'd use NENTRY() (in amd64) or ENTRY_NP() (on some other architectures) to prevent thise functions from calling _mcount(). But that was only ever done for code used during early bootstrap of the kernel. And these days there may be C code that needs this as well. With your diff, functions in the suspend/resume path will still call _mcount() which may not be safe.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
On Sun, Jul 09, 2023 at 05:24:43PM -0500, Scott Cheloha wrote:
> On Sun, Jul 09, 2023 at 08:11:43PM +0200, Claudio Jeker wrote:
> > On Sun, Jul 09, 2023 at 12:52:20PM -0500, Scott Cheloha wrote:
> > > This patch fixes resume/unhibernate on GPROF kernels where kgmon(8)
> > > has activated kernel profiling.
> > >
> > > I think the problem is that code called from cpu_hatch() does not play
> > > nicely with _mcount(), so GPROF kernels crash during resume. I can't
> > > point you to which code in particular, but keeping all CPUs out of
> > > _mcount() until the primary CPU has completed resume/unhibernate fixes
> > > the crash.
> > >
> > > ok?
> >
> > To be honest, I'm not sure we need something like this. GPROF is already a
> > special case and poeple running a GPROF kernel should probably stop the
> > collection of profile data before suspend/hibernate.
>
> [..]
Also, deraadt@ insisted that GPROF not hang the system across
suspend/resume and hibernate/unhibernate:
1. https://marc.info/?l=openbsd-tech=168721604322193=2
>> Make sure to STOP all kernel profiling before attempting to
>> suspend or hibernate your machine. Otherwise I expect it
>> will hang.
>
> That is not acceptable. People suspend and hibernate machines without
> being aware of what applications are doing.
>
>> GPROF is a kernel compile-time option. If you don't enable it,
>> you have nothing to worry about.
>
> Well that's a great hidden reason why noone would ever turn on this
> subsystem -- so why is it getting done on it??
2. https://marc.info/?l=openbsd-tech=16872161214=2
>> Make sure to STOP all kernel profiling before attempting to
>> suspend or hibernate your machine. Otherwise I expect it
>> will hang.
>
> It is completely acceptable if it produces wrong results, but it must
> not hang the system.
... this patch fits the bill.
Index: sys/lib/libkern/mcount.c
===
RCS file: /cvs/src/sys/lib/libkern/mcount.c,v
retrieving revision 1.14
diff -u -p -r1.14 mcount.c
--- sys/lib/libkern/mcount.c11 Jan 2022 09:21:34 - 1.14
+++ sys/lib/libkern/mcount.c9 Jul 2023 22:03:22 -
@@ -33,6 +33,28 @@
#include
#include
+#ifdef _KERNEL
+#ifdef SUSPEND
+#include
+
+volatile int mcount_disabled;
+
+void
+mcount_disable(void)
+{
+ mcount_disabled = 1;
+ membar_producer();
+}
+
+void
+mcount_enable(void)
+{
+ mcount_disabled = 0;
+ membar_producer();
+}
+#endif /* SUSPEND */
+#endif /* _KERNEL */
+
/*
* mcount is called on entry to each function compiled with the profiling
* switch set. _mcount(), which is declared in a machine-dependent way
@@ -63,7 +85,10 @@ _MCOUNT_DECL(u_long frompc, u_long selfp
*/
if (gmoninit == 0)
return;
-
+#ifdef SUSPEND
+ if (mcount_disabled)
+ return;
+#endif
if ((p = curcpu()->ci_gmon) == NULL)
return;
#else
Index: sys/kern/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
--- sys/kern/subr_suspend.c 2 Jul 2023 19:02:27 - 1.15
+++ sys/kern/subr_suspend.c 9 Jul 2023 22:03:22 -
@@ -26,6 +26,9 @@
#include
#include
#include
+#ifdef GPROF
+#include
+#endif
#ifdef HIBERNATE
#include
#endif
@@ -63,6 +66,9 @@ top:
if (sleep_showstate(v, sleepmode))
return EOPNOTSUPP;
+#ifdef GPROF
+ mcount_disable();
+#endif
#if NWSDISPLAY > 0
wsdisplay_suspend();
#endif
@@ -192,6 +198,9 @@ fail_hiballoc:
start_periodic_resettodr();
#if NWSDISPLAY > 0
wsdisplay_resume();
+#endif
+#ifdef GPROF
+ mcount_enable();
#endif
sys_sync(curproc, NULL, NULL);
if (cpu_setperf != NULL)
Index: sys/sys/gmon.h
===
RCS file: /cvs/src/sys/sys/gmon.h,v
retrieving revision 1.9
diff -u -p -r1.9 gmon.h
--- sys/sys/gmon.h 11 Jan 2022 23:59:55 - 1.9
+++ sys/sys/gmon.h 9 Jul 2023 22:03:22 -
@@ -158,6 +158,10 @@ struct gmonparam {
#ifdef _KERNEL
extern int gmoninit; /* Is the kernel ready for being profiled? */
+#ifdef SUSPEND
+void mcount_disable(void);
+void mcount_enable(void);
+#endif
#else /* !_KERNEL */
#include
Index: lib/libc/gmon/mcount.c
===
RCS file: /cvs/src/lib/libc/gmon/mcount.c,v
retrieving revision 1.16
diff -u -p -r1.16 mcount.c
--- lib/libc/gmon/mcount.c 11 Jan 2022 09:21:34 - 1.16
+++ lib/libc/gmon/mcount.c 9 Jul 2023 22:03:23 -
@@ -31,6 +31,28 @@
#include
#include
+#ifdef _KERNEL
+#ifdef SUSPEND
+#include
+
+volatile int mcount_disabled;
+
+void
+mcount_disable(void)
+{
+ mcount_disabled = 1;
+ membar_producer();
+}
+
+void
+mcount_enable(void)
+{
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
On Sun, Jul 09, 2023 at 08:11:43PM +0200, Claudio Jeker wrote:
> On Sun, Jul 09, 2023 at 12:52:20PM -0500, Scott Cheloha wrote:
> > This patch fixes resume/unhibernate on GPROF kernels where kgmon(8)
> > has activated kernel profiling.
> >
> > I think the problem is that code called from cpu_hatch() does not play
> > nicely with _mcount(), so GPROF kernels crash during resume. I can't
> > point you to which code in particular, but keeping all CPUs out of
> > _mcount() until the primary CPU has completed resume/unhibernate fixes
> > the crash.
> >
> > ok?
>
> To be honest, I'm not sure we need something like this. GPROF is already a
> special case and poeple running a GPROF kernel should probably stop the
> collection of profile data before suspend/hibernate.
Sorry, I was a little unclear in my original mail.
When I say "has activated kernel profiling" I mean "has *ever*
activated kernel profiling".
Regardless of whether or not profiling is active at the moment we
reach sleep_state(), if kernel profiling has *ever* been activated in
the past, the resume crashes.
When sysctl(2) reaches sysctl_doprof(), gmoninit is set. "Assume that
if we're here it is safe to execute profiling":
166 /*
167 * Return kernel profiling information.
168 */
169 int
170 sysctl_doprof(int *name, u_int namelen, void *oldp, size_t *oldlenp,
void *newp,
171 size_t newlen)
172 {
173 CPU_INFO_ITERATOR cii;
174 struct cpu_info *ci;
175 struct gmonparam *gp = NULL;
176 int error, cpuid, op, state;
177
178 /* all sysctl names at this level are name and field */
179 if (namelen != 2)
180 return (ENOTDIR); /* overloaded */
181
182 op = name[0];
183 cpuid = name[1];
184
185 CPU_INFO_FOREACH(cii, ci) {
186 if (cpuid == CPU_INFO_UNIT(ci)) {
187 gp = ci->ci_gmon;
188 break;
189 }
190 }
191
192 if (gp == NULL)
193 return (EOPNOTSUPP);
194
195 /* Assume that if we're here it is safe to execute profiling. */
196 gmoninit = 1;
After that first sysctl(2), all CPUs will stop bouncing out of
_mcount() at the gmoninit check and starting checking per-CPU data
structures to decide whether or not to record the arc. "Do not
profile execution...".
73 _MCOUNT_DECL(u_long frompc, u_long selfpc) __used;
74 /* _mcount; may be static, inline, etc */
75 _MCOUNT_DECL(u_long frompc, u_long selfpc)
76 {
77 u_short *frompcindex;
78 struct tostruct *top, *prevtop;
79 struct gmonparam *p;
80 long toindex;
81 #ifdef _KERNEL
82 int s;
83
84 /*
85 * Do not profile execution if memory for the current CPU
86 * descriptor and profiling buffers has not yet been allocated
87 * or if the CPU we are running on has not yet set its trap
88 * handler.
89 */
90 if (gmoninit == 0)
91 return;
92 #ifdef SUSPEND
93 if (mcount_disabled)
94 return;
95 #endif
96 if ((p = curcpu()->ci_gmon) == NULL)
97 return;
98 #else
99 p = &_gmonparam;
100 #endif
101 /*
102 * check that we are profiling
103 * and that we aren't recursively invoked.
104 */
105 if (p->state != GMON_PROF_ON)
106 return;
107 #ifdef _KERNEL
108 MCOUNT_ENTER;
This patch adds a second check to _mcount(), mcount_disabled, which
has a distinct meaning.
gmoninit means:
The boot initialization is now done and it is safe to touch the
per-CPU GPROF data structures.
mcount_disabled says:
Keep out.
There may be a clever way to merge the two variables, but the simplest
thing I could think of was to just add a second boolean. The current
patch is idiot-proof.
> Unless someone wants to gprof suspend/hibernate but then doing this will
> result in bad data.
That's way out of scope, I'm not advocating for that. If you want to
profile sleep_state() you need to do it some other way.
> Another option is to abort zzz/ZZZ if kernel profiling is running.
I don't think this would be a good user experience. Performing the
suspend without question and possibly returning bad profiling results
is better than failing the suspend.
IMHO, if suspend/resume interferes with kgmon(8) yielding accurate
results we just need to document it in kgmon.8.
I would argue that suspend/resume is an edge case one needs to keep in
mind. You can still profile large parts of the kernel's execution
path.
> In your diff I would remove these asserts:
>
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
On Sun, Jul 09, 2023 at 12:52:20PM -0500, Scott Cheloha wrote:
> This patch fixes resume/unhibernate on GPROF kernels where kgmon(8)
> has activated kernel profiling.
>
> I think the problem is that code called from cpu_hatch() does not play
> nicely with _mcount(), so GPROF kernels crash during resume. I can't
> point you to which code in particular, but keeping all CPUs out of
> _mcount() until the primary CPU has completed resume/unhibernate fixes
> the crash.
>
> ok?
To be honest, I'm not sure we need something like this. GPROF is already a
special case and poeple running a GPROF kernel should probably stop the
collection of profile data before suspend/hibernate.
Unless someone wants to gprof suspend/hibernate but then doing this will
result in bad data.
Another option is to abort zzz/ZZZ if kernel profiling is running.
In your diff I would remove these asserts:
KASSERT(CPU_IS_PRIMARY(curcpu()));
The code does not require the primary cpu there and KASSERT are not for free.
> Index: sys/lib/libkern/mcount.c
> ===
> RCS file: /cvs/src/sys/lib/libkern/mcount.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 mcount.c
> --- sys/lib/libkern/mcount.c 11 Jan 2022 09:21:34 - 1.14
> +++ sys/lib/libkern/mcount.c 9 Jul 2023 17:49:55 -
> @@ -33,6 +33,32 @@
> #include
> #include
>
> +#ifdef _KERNEL
> +#ifdef SUSPEND
> +#include
> +
> +#include /* KASSERT */
> +
> +volatile int mcount_disabled;
> +
> +void
> +mcount_disable(void)
> +{
> + KASSERT(CPU_IS_PRIMARY(curcpu()));
> + mcount_disabled = 1;
> + membar_producer();
> +}
> +
> +void
> +mcount_enable(void)
> +{
> + KASSERT(CPU_IS_PRIMARY(curcpu()));
> + mcount_disabled = 0;
> + membar_producer();
> +}
> +#endif /* SUSPEND */
> +#endif /* _KERNEL */
> +
> /*
> * mcount is called on entry to each function compiled with the profiling
> * switch set. _mcount(), which is declared in a machine-dependent way
> @@ -63,7 +89,10 @@ _MCOUNT_DECL(u_long frompc, u_long selfp
>*/
> if (gmoninit == 0)
> return;
> -
> +#ifdef SUSPEND
> + if (mcount_disabled)
> + return;
> +#endif
> if ((p = curcpu()->ci_gmon) == NULL)
> return;
> #else
> Index: sys/kern/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
> --- sys/kern/subr_suspend.c 2 Jul 2023 19:02:27 - 1.15
> +++ sys/kern/subr_suspend.c 9 Jul 2023 17:49:55 -
> @@ -26,6 +26,9 @@
> #include
> #include
> #include
> +#ifdef GPROF
> +#include
> +#endif
> #ifdef HIBERNATE
> #include
> #endif
> @@ -63,6 +66,9 @@ top:
>
> if (sleep_showstate(v, sleepmode))
> return EOPNOTSUPP;
> +#ifdef GPROF
> + mcount_disable();
> +#endif
> #if NWSDISPLAY > 0
> wsdisplay_suspend();
> #endif
> @@ -192,6 +198,9 @@ fail_hiballoc:
> start_periodic_resettodr();
> #if NWSDISPLAY > 0
> wsdisplay_resume();
> +#endif
> +#ifdef GPROF
> + mcount_enable();
> #endif
> sys_sync(curproc, NULL, NULL);
> if (cpu_setperf != NULL)
> Index: sys/sys/gmon.h
> ===
> RCS file: /cvs/src/sys/sys/gmon.h,v
> retrieving revision 1.9
> diff -u -p -r1.9 gmon.h
> --- sys/sys/gmon.h11 Jan 2022 23:59:55 - 1.9
> +++ sys/sys/gmon.h9 Jul 2023 17:49:55 -
> @@ -158,6 +158,10 @@ struct gmonparam {
> #ifdef _KERNEL
> extern int gmoninit; /* Is the kernel ready for being profiled? */
>
> +#ifdef SUSPEND
> +void mcount_disable(void);
> +void mcount_enable(void);
> +#endif
> #else /* !_KERNEL */
>
> #include
> Index: lib/libc/gmon/mcount.c
> ===
> RCS file: /cvs/src/lib/libc/gmon/mcount.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 mcount.c
> --- lib/libc/gmon/mcount.c11 Jan 2022 09:21:34 - 1.16
> +++ lib/libc/gmon/mcount.c9 Jul 2023 17:49:55 -
> @@ -31,6 +31,32 @@
> #include
> #include
>
> +#ifdef _KERNEL
> +#ifdef SUSPEND
> +#include
> +
> +#include /* KASSERT */
> +
> +volatile int mcount_disabled;
> +
> +void
> +mcount_disable(void)
> +{
> + KASSERT(CPU_IS_PRIMARY(curcpu()));
> + mcount_disabled = 1;
> + membar_producer();
> +}
> +
> +void
> +mcount_enable(void)
> +{
> + KASSERT(CPU_IS_PRIMARY(curcpu()));
> + mcount_disabled = 0;
> + membar_producer();
> +}
> +#endif /* SUSPEND */
> +#endif /* _KERNEL */
> +
> /*
> * mcount is called on entry to each function compiled with the profiling
> * switch set. _mcount(), which is declared in a machine-dependent way
> @@ -61,7 +87,10 @@ _MCOUNT_DECL(u_long frompc, u_long selfp
>*/
> if (gmoninit == 0)
> return;
> -
> +#ifdef SUSPEND
> + if (mcount_disabled)
GPROF: sleep_state: disable _mcount() across suspend/resume
This patch fixes resume/unhibernate on GPROF kernels where kgmon(8)
has activated kernel profiling.
I think the problem is that code called from cpu_hatch() does not play
nicely with _mcount(), so GPROF kernels crash during resume. I can't
point you to which code in particular, but keeping all CPUs out of
_mcount() until the primary CPU has completed resume/unhibernate fixes
the crash.
ok?
Index: sys/lib/libkern/mcount.c
===
RCS file: /cvs/src/sys/lib/libkern/mcount.c,v
retrieving revision 1.14
diff -u -p -r1.14 mcount.c
--- sys/lib/libkern/mcount.c11 Jan 2022 09:21:34 - 1.14
+++ sys/lib/libkern/mcount.c9 Jul 2023 17:49:55 -
@@ -33,6 +33,32 @@
#include
#include
+#ifdef _KERNEL
+#ifdef SUSPEND
+#include
+
+#include/* KASSERT */
+
+volatile int mcount_disabled;
+
+void
+mcount_disable(void)
+{
+ KASSERT(CPU_IS_PRIMARY(curcpu()));
+ mcount_disabled = 1;
+ membar_producer();
+}
+
+void
+mcount_enable(void)
+{
+ KASSERT(CPU_IS_PRIMARY(curcpu()));
+ mcount_disabled = 0;
+ membar_producer();
+}
+#endif /* SUSPEND */
+#endif /* _KERNEL */
+
/*
* mcount is called on entry to each function compiled with the profiling
* switch set. _mcount(), which is declared in a machine-dependent way
@@ -63,7 +89,10 @@ _MCOUNT_DECL(u_long frompc, u_long selfp
*/
if (gmoninit == 0)
return;
-
+#ifdef SUSPEND
+ if (mcount_disabled)
+ return;
+#endif
if ((p = curcpu()->ci_gmon) == NULL)
return;
#else
Index: sys/kern/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
--- sys/kern/subr_suspend.c 2 Jul 2023 19:02:27 - 1.15
+++ sys/kern/subr_suspend.c 9 Jul 2023 17:49:55 -
@@ -26,6 +26,9 @@
#include
#include
#include
+#ifdef GPROF
+#include
+#endif
#ifdef HIBERNATE
#include
#endif
@@ -63,6 +66,9 @@ top:
if (sleep_showstate(v, sleepmode))
return EOPNOTSUPP;
+#ifdef GPROF
+ mcount_disable();
+#endif
#if NWSDISPLAY > 0
wsdisplay_suspend();
#endif
@@ -192,6 +198,9 @@ fail_hiballoc:
start_periodic_resettodr();
#if NWSDISPLAY > 0
wsdisplay_resume();
+#endif
+#ifdef GPROF
+ mcount_enable();
#endif
sys_sync(curproc, NULL, NULL);
if (cpu_setperf != NULL)
Index: sys/sys/gmon.h
===
RCS file: /cvs/src/sys/sys/gmon.h,v
retrieving revision 1.9
diff -u -p -r1.9 gmon.h
--- sys/sys/gmon.h 11 Jan 2022 23:59:55 - 1.9
+++ sys/sys/gmon.h 9 Jul 2023 17:49:55 -
@@ -158,6 +158,10 @@ struct gmonparam {
#ifdef _KERNEL
extern int gmoninit; /* Is the kernel ready for being profiled? */
+#ifdef SUSPEND
+void mcount_disable(void);
+void mcount_enable(void);
+#endif
#else /* !_KERNEL */
#include
Index: lib/libc/gmon/mcount.c
===
RCS file: /cvs/src/lib/libc/gmon/mcount.c,v
retrieving revision 1.16
diff -u -p -r1.16 mcount.c
--- lib/libc/gmon/mcount.c 11 Jan 2022 09:21:34 - 1.16
+++ lib/libc/gmon/mcount.c 9 Jul 2023 17:49:55 -
@@ -31,6 +31,32 @@
#include
#include
+#ifdef _KERNEL
+#ifdef SUSPEND
+#include
+
+#include/* KASSERT */
+
+volatile int mcount_disabled;
+
+void
+mcount_disable(void)
+{
+ KASSERT(CPU_IS_PRIMARY(curcpu()));
+ mcount_disabled = 1;
+ membar_producer();
+}
+
+void
+mcount_enable(void)
+{
+ KASSERT(CPU_IS_PRIMARY(curcpu()));
+ mcount_disabled = 0;
+ membar_producer();
+}
+#endif /* SUSPEND */
+#endif /* _KERNEL */
+
/*
* mcount is called on entry to each function compiled with the profiling
* switch set. _mcount(), which is declared in a machine-dependent way
@@ -61,7 +87,10 @@ _MCOUNT_DECL(u_long frompc, u_long selfp
*/
if (gmoninit == 0)
return;
-
+#ifdef SUSPEND
+ if (mcount_disabled)
+ return;
+#endif
if ((p = curcpu()->ci_gmon) == NULL)
return;
#else
