On Mon, Jul 10, 2023 at 07:09:19AM -0600, Theo de Raadt wrote: > Mark Kettenis <mark.kette...@xs4all.nl> 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.
