On Mon, Apr 15, 2019 at 04:35:24PM +0300, Ville Syrjälä wrote:
> On Mon, Nov 26, 2018 at 02:01:22PM -0800, Paul E. McKenney wrote:
> > On Wed, Nov 14, 2018 at 12:20:13PM -0800, Paul E. McKenney wrote:
> > > On Tue, Nov 13, 2018 at 07:10:37AM -0800, Paul E. McKenney wrote:
> > > > On Tue, Nov 13, 2018 at 03:54:53PM +0200, Ville Syrjälä wrote:
> > > > > Hi Paul,
> > > > >
> > > > > After 4.20-rc1 some of my 32bit UP machines no longer reboot/shutdown.
> > > > > I bisected this down to commit 45975c7d21a1 ("rcu: Define RCU-sched
> > > > > API in terms of RCU for Tree RCU PREEMPT builds").
> > > > >
> > > > > I traced the hang into
> > > > > -> cpufreq_suspend()
> > > > > -> cpufreq_stop_governor()
> > > > > -> cpufreq_dbs_governor_stop()
> > > > > -> gov_clear_update_util()
> > > > > -> synchronize_sched()
> > > > > -> synchronize_rcu()
> > > > >
> > > > > Only PREEMPT=y is affected for obvious reasons, but that couldn't
> > > > > explain why the same UP kernel booted on an SMP machine worked fine.
> > > > > Eventually I realized that the difference between working and
> > > > > non-working machine was IOAPIC vs. PIC. With initcall_debug I saw
> > > > > that we mask everything in the PIC before cpufreq is shut down,
> > > > > and came up with the following fix:
> > > > >
> > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > > > index 7aa3dcad2175..f88bf3c77fc0 100644
> > > > > --- a/drivers/cpufreq/cpufreq.c
> > > > > +++ b/drivers/cpufreq/cpufreq.c
> > > > > @@ -2605,4 +2605,4 @@ static int __init cpufreq_core_init(void)
> > > > > return 0;
> > > > > }
> > > > > module_param(off, int, 0444);
> > > > > -core_initcall(cpufreq_core_init);
> > > > > +late_initcall(cpufreq_core_init);
> > > >
> > > > Thank you for testing this and tracking it down!
> > > >
> > > > I am glad that you have a fix, but I hope that we can arrive at a less
> > > > constraining one.
> > > >
> > > > > Here's the resulting change in inutcall_debug:
> > > > > pci 0000:00:00.1: shutdown
> > > > > hub 4-0:1.0: hub_ext_port_status failed (err = -110)
> > > > > agpgart-intel 0000:00:00.0: shutdown
> > > > > + PM: Calling cpufreq_suspend+0x0/0x100
> > > > > PM: Calling mce_syscore_shutdown+0x0/0x10
> > > > > PM: Calling i8259A_shutdown+0x0/0x10
> > > > > - PM: Calling cpufreq_suspend+0x0/0x100
> > > > > + reboot: Restarting system
> > > > > + reboot: machine restart
> > > > >
> > > > > I didn't really look into what other ramifications the cpufreq
> > > > > initcall change might have. cpufreq_global_kobject worries
> > > > > me a bit. Maybe that one has to remain in core_initcall() and
> > > > > we could just move the suspend to late_initcall()? Anyways,
> > > > > I figured I'd leave this for someone more familiar with the
> > > > > code to figure out ;)
> > > >
> > > > Let me guess...
> > > >
> > > > When the system suspends or shuts down, there comes a point after which
> > > > there is only a single CPU that is running with preemption and
> > > > interrupts
> > > > are disabled. At this point, RCU must change the way that it works, and
> > > > the commit you bisected to would make the change more necessary. But if
> > > > I am guessing correctly, we have just been getting lucky in the past.
> > > >
> > > > It looks like RCU needs to create a struct syscore_ops with a shutdown
> > > > function and pass this to register_syscore_ops(). Maybe a suspend
> > > > function as well. And RCU needs to invoke register_syscore_ops() at
> > > > a time that causes RCU's shutdown function to be invoked in the right
> > > > order with respect to the other work in flight. The hope would be that
> > > > RCU's suspend function gets called just as the system transitions into
> > > > a mode where the scheduler is no longer active, give or take.
> > > >
> > > > Does this make sense, or am I confused?
> > >
> > > Well, it certainly does not make sense in that blocking is still legal
> > > at .shutdown() invocation time, which means that RCU cannot revert to
> > > its boot-time approach at that point. Looks like I need hooks in a
> > > bunch of arch-dependent functions. Which is certainly doable, but will
> > > take a bit more digging.
> >
> > A bit more detail, after some additional discussion at Linux Plumbers
> > conference...
> >
> > The preferred approach is to hook into syscore_suspend(),
> > syscore_resume(), and syscore_shutdown(). This can be done easily by
> > creating an appropriately initialized struct syscore_ops and passing a
> > pointer to it to register_syscore_ops() during boot. Taking these three
> > functions in turn:
> >
> > syscore_suspend():
> >
> > o arch/x86/kernel/apm_32.c suspend(), standby()
> >
> > These calls to syscore_suspend() has interrupts disabled, which
> > is very good, but they are immediately re-enabled, and only then
> > is the call to set_system_power_state(). Unless both interrupts
> > and preemption are prevented somehow, it is not safe for
> > CONFIG_PREEMPT=y RCU implementations to revert back to boot-time
> > behavior at this point.
> >
> > o drivers/xen/manage.c xen_suspend()
> >
> > This looks to have interrupts disabled throughout. It is also
> > invoked within stop_machine(), which means that the other CPUs,
> > though online, are quiescent. This allows RCU to safely switch
> > back to early boot operating mode. That is, this is safe only
> > if there is no interaction with RCU-preempt read-side critical
> > sections that might well be underway in the other CPUs. This
> > assumption is likely violated in CONFIG_PREEMPT=y kernels. One
> > alternative that would work with RCU in CONFIG_PREEMPT=y kernels
> > is CPU-hotplug removing all but one CPU, but that might have
> > some other disadvantages.
> >
> > o kernel/kexec_core.c kernel_kexec()
> >
> > Before we get here, disable_nonboot_cpus() has been invoked, which
> > in turn invokes freeze_secondary_cpus(), which offlines all but
> > the boot CPU. Prior to that, all user-space tasks are frozen.
> > So in this case, it would be safe for RCU to revert back to its
> > boot-time behavior. Aside from the possibility of unfreezable
> > kthreads being preempted within RCU-preempt read-side critical
> > sections, anyway... :-/
> >
> > However, one can argue that as long as the kthreads preempted
> > within an RCU-preempt read-side critical section are guaranteed
> > to never ever run again, we might be OK. And this guarantee
> > seems consistent with the kernel_kexec() operation. At least
> > when there are no errors that cause the kernel_kexec() to return
> > control to the initial kernel image...
> >
> > Of course, this line of reasoning does not apply when the
> > kernel is to resume on the same hardware, as in some of the
> > cases above.
> >
> > o kernel/power/hibernate.c create_image()
> >
> > Same as for kernel_kexec(), except that freeze_kernel_threads()
> > is invoked, which hopefully gets all tasks out of RCU read-side
> > critical sections. So this one might actually permit RCU to
> > revert back to boot-time behavior. Except for the possibility of
> > an error condition forcing an abort back into the original kernel
> > image, which again could have trouble with kthreads that were
> > preempted within an RCU read-side critical section throughout.
> >
> > o kernel/power/hibernate.c resume_target_kernel()
> > kernel/power/hibernate.c hibernation_platform_enter()
> > kernel/power/suspend.c suspend_enter()
> >
> > Same as for kernel_kexec(), but no obvious pretense of freezing
> > any tasks.
> >
> >
> > syscore_resume():
> >
> > o arch/x86/kernel/apm_32.c suspend(), standby()
> >
> > Resume-time counterparts to the calls to syscore_suspend() called
> > out above, with the same interrupt-enabling problem, as well as
> > issues with tasks being preempted throughout within RCU-preempt
> > read-side critical sections.
> >
> > o drivers/xen/manage.c xen_suspend()
> >
> > Resume-time counterpart to the calls to xen_suspend() called out
> > above, with the same issues with tasks being preempted throughout
> > within RCU-preempt read-side critical sections.
> >
> > o kernel/kexec_core.c kernel_kexec()
> >
> > Resume-time counterpart to the calls to kernel_kexec() called out
> > above. This is the error case that causes trouble due to the
> > possibility of preempted RCU read-side critical sections.
> >
> > o kernel/power/hibernate.c create_image()
> > kernel/power/hibernate.c resume_target_kernel()
> > kernel/power/hibernate.c hibernation_platform_enter()
> > kernel/power/hibernate.c suspend_enter()
> >
> > Resume-time counterparts to calls within kernel/power/hibernate.c
> > and kernel/power/suspend.c called out above. This is the error
> > case that causes trouble due to the possibility of preempted
> > RCU read-side critical sections.
> >
> >
> > syscore_shutdown():
> >
> > o kernel/reboot.c kernel_restart()
> > kernel/reboot.c kernel_halt()
> > kernel/reboot.c kernel_power_off()
> >
> > These appears to leave all CPUs online, which prevents RCU from
> > safely reverting back to boot-time mode.
> >
> >
> > So what is to be done?
> >
> > Here are the options I can see:
> >
> > 1. Status quo, which means that synchronize_rcu() and friends
> > cannot be used in syscore_suspend(), syscore_resume(), and
> > syscore_shutdown() callbacks. At the moment, this appears to
> > be the only workable approach, though ideas and suggestions are
> > quite welcome.
> >
> > 2. Make each code path to syscore_suspend(), syscore_resume(), and
> > syscore_shutdown() offline all but the boot CPU, ensure that
> > all tasks exit any RCU read-side critical sections that they
> > might be in, then run the remainder of the code path on the
> > boot CPU with interrupts disabled.
> >
> > Making all tasks exit any RCU read-side critical sections is
> > easy when CONFIG_PREEMPT=n via things like stop-machine, but
> > it is difficult and potentially time-consuming for
> > CONFIG_PREEMPT=y kernels.
> >
> > 3. Do error checking so that there cannot possibly be failures
> > beyond the time that syscore_suspend(), syscore_resume(),
> > and syscore_shutdown() are invoked. This is fine for
> > syscore_shutdown() and syscore_resume(), but syscore_suspend()'s
> > callbacks are permitted to return errors that force suspend
> > failures.
> >
> > And there are syscore_suspend() callbacks that actually do
> > return errors, for example, fsl_lbc_syscore_suspend()
> > in arch/powerpc/sysdev/fsl_lbc.c can return -ENOMEM.
> > As can save_ioapic_entries() in arch/x86/kernel/apic/io_apic.c
> > and arch/x86/include/asm/io_apic.h. And mvebu_mbus_suspend()
> > in drivers/bus/mvebu-mbus.c. And iommu_suspend() in
> > drivers/iommu/intel-iommu.c.
> >
> > And its_save_disable() in drivers/irqchip/irq-gic-v3-its.c
> > can return -EBUSY.
> >
> > Perhaps these can be remedied somehow, but unless that can
> > be done, this approach cannot work.
> >
> > 4. Your idea here!!!
>
> Paul, are we any closer to fixing this regression? It's been around
> for far too long, and I'd like to stop carrying my original hack
> around.
Actually, no. If I got any response before now, I fat-fingered it.
Seeing no responses, I assumed that nobody cared, and that option #1
(status quo) was preferred.
Any feedback from anyone on the various options? Or, better yet, some
better options?
Thanx, Paul
