| Message ID | CF154B5C3C8E7E64+20250224074357.673094-1-wangyuli@uniontech.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] x86 / hibernate: Eliminate the redundant smp_ops.play_dead assignment | expand |
On Mon, Feb 24, 2025 at 8:45 AM WangYuli <wangyuli@uniontech.com> wrote: > > It's unnecessary to re-initialize smp_ops.play_dead to play_dead as > it naturally goes back to play_dead in the freshly booted kernel upon > device resume. Is it really? And what if freeze_secondary_cpus() returns an error, for one example? > Suggested-by: Huacai Chen <chenhuacai@loongson.cn> > Co-developed-by: Wentao Guan <guanwentao@uniontech.com> > Signed-off-by: Wentao Guan <guanwentao@uniontech.com> > Signed-off-by: WangYuli <wangyuli@uniontech.com> > --- > arch/x86/power/cpu.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c > index 63230ff8cf4f..023cf9421467 100644 > --- a/arch/x86/power/cpu.c > +++ b/arch/x86/power/cpu.c > @@ -297,9 +297,6 @@ static void __noreturn resume_play_dead(void) > > int hibernate_resume_nonboot_cpu_disable(void) > { > - void (*play_dead)(void) = smp_ops.play_dead; > - int ret; > - > /* > * Ensure that MONITOR/MWAIT will not be used in the "play dead" loop > * during hibernate image restoration, because it is likely that the > @@ -316,13 +313,11 @@ int hibernate_resume_nonboot_cpu_disable(void) > * resume) sleep afterwards, and the resumed kernel will decide itself > * what to do with them. > */ > - ret = cpuhp_smt_enable(); > + int ret = cpuhp_smt_enable(); > if (ret) > return ret; > smp_ops.play_dead = resume_play_dead; > - ret = freeze_secondary_cpus(0); > - smp_ops.play_dead = play_dead; > - return ret; > + return freeze_secondary_cpus(0); > } > #endif > > -- > 2.47.2 >
Hello, Thanks for your reply. In my opinion, the only logic different before the patch is delete smp_ops.play_dead save and restore, as the comment "the resumed kernel will decide itself" and same logic as which in arch/arm64/kernel/hibernate.c, the ok path will work as expect. When discussing the error path and ret value that we not restore play_dead, I will try to analyze the difference between native_play_dead and resume_play_dead, and sev_es_play_dead [the all possiable three value], and I see some mwait and hlt way difference.[maybe it happens as disable the cpu failed and goes to Enable_cpus path in func:resume_target_kernel in hibernate.c? ] Is that it desgin to do and we can move it to a common place in hibernate.c and left some comments ? BRs Wentao Guan
On Wed, Feb 26, 2025 at 7:08 PM Wentao Guan <guanwentao@uniontech.com> wrote: > > Hello, > > Thanks for your reply. > > In my opinion, the only logic different before the patch is delete smp_ops.play_dead > save and restore, as the comment "the resumed kernel will decide itself" and same > logic as which in arch/arm64/kernel/hibernate.c, the ok path will work as expect. Yes, the OK path will work as expected so long as smp_ops.play_dead is switched over to resume_play_dead before calling freeze_secondary_cpus(). > When discussing the error path and ret value that we not restore play_dead, > I will try to analyze the difference between native_play_dead and resume_play_dead, > and sev_es_play_dead [the all possiable three value], and I see some mwait and hlt > way difference.[maybe it happens as disable the cpu failed and goes to Enable_cpus > path in func:resume_target_kernel in hibernate.c? ] Is that it desgin to do and we can > move it to a common place in hibernate.c and left some comments ? Why not leave it as is?
diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c index 63230ff8cf4f..023cf9421467 100644 --- a/arch/x86/power/cpu.c +++ b/arch/x86/power/cpu.c @@ -297,9 +297,6 @@ static void __noreturn resume_play_dead(void) int hibernate_resume_nonboot_cpu_disable(void) { - void (*play_dead)(void) = smp_ops.play_dead; - int ret; - /* * Ensure that MONITOR/MWAIT will not be used in the "play dead" loop * during hibernate image restoration, because it is likely that the @@ -316,13 +313,11 @@ int hibernate_resume_nonboot_cpu_disable(void) * resume) sleep afterwards, and the resumed kernel will decide itself * what to do with them. */ - ret = cpuhp_smt_enable(); + int ret = cpuhp_smt_enable(); if (ret) return ret; smp_ops.play_dead = resume_play_dead; - ret = freeze_secondary_cpus(0); - smp_ops.play_dead = play_dead; - return ret; + return freeze_secondary_cpus(0); } #endif