[PATCH] x86/tsc: avoid system instability in hibernation System

[PATCH] x86/tsc: avoid system instability in hibernation System

[Atharva Tiwari]

instability are seen during resume from hibernation when system is under heavy CPU load. this is caused by the lack of update of sched clock data

Signed-off-by: Atharva Tiwari <evepolonium@gmail.com>
---
 arch/x86/kernel/tsc.c       | 27 +++++++++++++++++++++++++++
 include/linux/sched/clock.h |  5 +++++
 kernel/sched/clock.c        |  4 ++--
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 67aeaba4ba9c..1879ae5b49c8 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -15,6 +15,7 @@
 #include <linux/timex.h>
 #include <linux/static_key.h>
 #include <linux/static_call.h>
+#include <linux/suspend.h>
 
 #include <asm/hpet.h>
 #include <asm/timer.h>
@@ -1599,3 +1600,29 @@ unsigned long calibrate_delay_is_known(void)
 	return 0;
 }
 #endif
+static int tsc_pm_notifier(struct notifier_block *notifier,
+                          unsigned long pm_event, void *unused)
+{
+	switch (pm_event) {
+	case PM_HIBERNATION_PREPARE:
+		clear_sched_clock_stable();
+		break;
+	case PM_POST_HIBERNATION:
+		/* Set back to the default */
+		if (!check_tsc_unstable())
+			set_sched_clock_stable();
+		break;
+	}
+	return 0;
+};
+
+static struct notifier_block tsc_pm_notifier_block = {
+       .notifier_call = tsc_pm_notifier,
+};
+
+static int tsc_setup_pm_notifier(void)
+{
+       return register_pm_notifier(&tsc_pm_notifier_block);
+}
+
+subsys_initcall(tsc_setup_pm_notifier);
diff --git a/include/linux/sched/clock.h b/include/linux/sched/clock.h
index 196f0ca351a2..811b8ebb57a5 100644
--- a/include/linux/sched/clock.h
+++ b/include/linux/sched/clock.h
@@ -41,6 +41,10 @@ static inline void clear_sched_clock_stable(void)
 {
 }
 
+static inline void set_sched_clock_stable(void)
+{
+}
+
 static inline void sched_clock_idle_sleep_event(void)
 {
 }
@@ -65,6 +69,7 @@ static __always_inline u64 local_clock(void)
 }
 #else
 extern int sched_clock_stable(void);
+extern void set_sched_clock_stable(void);
 extern void clear_sched_clock_stable(void);
 
 /*
diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
index a09655b48140..efe8f2b69657 100644
--- a/kernel/sched/clock.c
+++ b/kernel/sched/clock.c
@@ -114,7 +114,7 @@ notrace static void __scd_stamp(struct sched_clock_data *scd)
 	scd->tick_raw = sched_clock();
 }
 
-notrace static void __set_sched_clock_stable(void)
+notrace void set_sched_clock_stable(void)
 {
 	struct sched_clock_data *scd;
 
@@ -234,7 +234,7 @@ static int __init sched_clock_init_late(void)
 	smp_mb(); /* matches {set,clear}_sched_clock_stable() */
 
 	if (__sched_clock_stable_early)
-		__set_sched_clock_stable();
+		set_sched_clock_stable();
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH] x86/tsc: avoid system instability in hibernation System

[Thomas Gleixner]

On Mon, Dec 16 2024 at 16:06, Atharva Tiwari wrote:
> instability are seen during resume from hibernation when system is under heavy CPU load. this is caused by the lack of update of sched clock data

Neither the subject nor the change log text make any sense. (formatting ignored)

Aside of that you still have not answered the question from Peter:

  https://lore.kernel.org/all/20241210123516.GP8562@noisy.programming.kicks-ass.net

and you keep resending the same patch over and over without any
explanation about the underlying problem.

Actually after Peter asked you to provide details, you reduced the
information in the changelog and resent the thing twice within a few
hours. The second time with a even more broken changelog. Then five days
later you repeat the exercise with a resend of the second variant.

May I ask you to read Documentation/process/* to figure out how this
works?

You can resend this as much as you want, as long as you don't provide
answers to the questions asked, this is going nowhere.

Let me ask you more detailed questions:

> +static int tsc_pm_notifier(struct notifier_block *notifier,
> +                          unsigned long pm_event, void *unused)
> +{
> +	switch (pm_event) {
> +	case PM_HIBERNATION_PREPARE:
> +		clear_sched_clock_stable();

This marks a stable sched clock unstable, which means that the simple
fast path of reading the clock:

      sched_clock_noinstr() + __sched_clock_offset;

is disabled and the system has to update the sched clock data for no
good reason.

Questions:

   1) What has this to do with heavy CPU load?

   2) What has this to do with the system not updating sched clock data,
      especially as a stable sched clock does not require any sched clock
      data updates at all?

   3) Can you provide dmesg output or any other evidence which backs up
      your reasoning to mark sched clock unstable when preparing for
      hibernation?

Thanks,

        tglx