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 -0000 1.14
> +++ sys/lib/libkern/mcount.c 9 Jul 2023 17:49:55 -0000
> @@ -33,6 +33,32 @@
> #include <sys/param.h>
> #include <sys/gmon.h>
>
> +#ifdef _KERNEL
> +#ifdef SUSPEND
> +#include <sys/atomic.h>
> +
> +#include <lib/libkern/libkern.h> /* 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 -0000 1.15
> +++ sys/kern/subr_suspend.c 9 Jul 2023 17:49:55 -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
> @@ -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 -0000 1.9
> +++ sys/sys/gmon.h 9 Jul 2023 17:49:55 -0000
> @@ -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 <sys/cdefs.h>
> 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 -0000 1.16
> +++ lib/libc/gmon/mcount.c 9 Jul 2023 17:49:55 -0000
> @@ -31,6 +31,32 @@
> #include <sys/types.h>
> #include <sys/gmon.h>
>
> +#ifdef _KERNEL
> +#ifdef SUSPEND
> +#include <sys/atomic.h>
> +
> +#include <lib/libkern/libkern.h> /* 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
>
--
:wq Claudio
