[PATCH] timekeeping: Move debug sleep time accounting outside spinlock

[PATCH] timekeeping: Move debug sleep time accounting outside spinlock

[Haofeng Li]

From: Haofeng Li <lihaofeng@kylinos.cn>

The function tk_debug_account_sleep_time() was called inside a spinlock
in __timekeeping_inject_sleeptime(), which could lead to potential
deadlocks, particularly when CONFIG_DEBUG_PM_SLEEP is enabled.

To prevent this, move the call to tk_debug_account_sleep_time() outside
the raw spinlock. Additionally, add a check to ensure it is only called
with a valid time delta, avoiding unnecessary debug accounting when the
time value is zero or invalid.

The same change is applied to timekeeping_inject_sleeptime64() and
timekeeping_resume() to maintain consistency and improve stability across
the timekeeping subsystem.

Signed-off-by: Haofeng Li <lihaofeng@kylinos.cn>
---
 kernel/time/timekeeping.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index b6974fce800c..3c6eb5220149 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1865,7 +1865,6 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
 	tk_xtime_add(tk, delta);
 	tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, *delta));
 	tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
-	tk_debug_account_sleep_time(delta);
 }
 
 #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE)
@@ -1926,6 +1925,9 @@ void timekeeping_inject_sleeptime64(const struct timespec64 *delta)
 		timekeeping_update_from_shadow(&tk_core, TK_UPDATE_ALL);
 	}
 
+	if (timespec64_valid_strict(delta))
+		tk_debug_account_sleep_time(delta);
+
 	/* Signal hrtimers about time change */
 	clock_was_set(CLOCK_SET_WALL | CLOCK_SET_BOOT);
 }
@@ -1986,7 +1988,8 @@ void timekeeping_resume(void)
 	timekeeping_update_from_shadow(&tk_core, TK_CLOCK_WAS_SET);
 	raw_spin_unlock_irqrestore(&tk_core.lock, flags);
 
-	touch_softlockup_watchdog();
+	if (inject_sleeptime && timespec64_valid_strict(&ts_delta))
+		tk_debug_account_sleep_time(&ts_delta);
 
 	/* Resume the clockevent device(s) and hrtimers */
 	tick_resume();
-- 
2.25.1

Re: [PATCH] timekeeping: Move debug sleep time accounting outside spinlock

[Thomas Gleixner]

On Tue, Sep 09 2025 at 18:23, Haofeng Li wrote:
> @@ -1986,7 +1988,8 @@ void timekeeping_resume(void)
>  	timekeeping_update_from_shadow(&tk_core, TK_CLOCK_WAS_SET);
>  	raw_spin_unlock_irqrestore(&tk_core.lock, flags);
>  
> -	touch_softlockup_watchdog();

Removing this is related to the problem at hand in which way and how is
that even correct to begin with?

Thanks,

        tglx

Re: [PATCH] timekeeping: Move debug sleep time accounting outside spinlock

[Thomas Gleixner]

On Tue, Sep 09 2025 at 18:23, Haofeng Li wrote:
> From: Haofeng Li <lihaofeng@kylinos.cn>
>
> The function tk_debug_account_sleep_time() was called inside a spinlock
> in __timekeeping_inject_sleeptime(), which could lead to potential
> deadlocks, particularly when CONFIG_DEBUG_PM_SLEEP is enabled.

How so exactly?

I don't see how that would deadlock, but I might miss something. So
please explain it properly instead of handwaving about it.

Thanks,

        tglx

Re: [PATCH WITHDRAWN] timekeeping: Move debug sleep time accounting outside spinlock

[Haofeng Li]

>How so exactly?

>I don't see how that would deadlock, but I might miss something. So
>please explain it properly instead of handwaving about it.

>Thanks,

>        tglx


Thank you for your detailed feedback and for pointing out the issues with my previous analysis. I sincerely apologize for the oversight and any confusion it may have caused. You were absolutely right to question the reasoning, and upon re-examining the code with a fresher perspective, I realize my initial explanation was incomplete and incorrect.

Regarding your specific questions:

Removal of touch_softlockup_watchdog():
I now understand that this change was unrelated to the problem at hand. The removal was mistakenly included in the context of addressing the potential deadlock issue, which was not justified. This line should not have been touched.

Potential Deadlock in tk_debug_account_sleep_time():
Your skepticism about the deadlock risk was entirely valid. After further analysis, I confirmed that the function tk_debug_account_sleep_time()—specifically, the pm_deferred_pr_dbg() macro and its underlying printk_deferred() implementation—does not acquire any locks that could cause a deadlock, even when called within a spinlock-protected context. The deferred printk mechanism is designed to be safe in such scenarios, as it avoids locking and instead uses per-CPU buffers to handle logging.

The confusion arose because I was referencing an older version of the codebase that lacked the commit 099f1c84c005 ("printk: introduce per-cpu safe_print seq buffer"). This commit introduced critical improvements to the deferred printk functionality, ensuring it remains lock-free and safe for use in contexts like this. In the older codebase, a similar scenario could indeed lead to deadlocks, but in the current code, the implementation is robust and free of such risks.

Again, I apologize for the oversight and the time spent addressing this. I greatly appreciate your patience and thorough review, which helped me correct my misunderstanding. 

Thank you for your guidance and for holding the code to a high standard.

Best regards,

	lihaofeng

Re: [PATCH] timekeeping: Move debug sleep time accounting outside spinlock

[kernel test robot]

Hi Haofeng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tip/timers/core]
[also build test WARNING on linus/master v6.17-rc5 next-20250909]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Haofeng-Li/timekeeping-Move-debug-sleep-time-accounting-outside-spinlock/20250909-183705
base:   tip/timers/core
patch link:    https://lore.kernel.org/r/tencent_6FBD7FB2B5EDA57B1481766C52482D033008%40qq.com
patch subject: [PATCH] timekeeping: Move debug sleep time accounting outside spinlock
config: m68k-allnoconfig (https://download.01.org/0day-ci/archive/20250910/202509101047.oTfSjXyE-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250910/202509101047.oTfSjXyE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509101047.oTfSjXyE-lkp@intel.com/

All warnings (new ones prefixed by >>):

   kernel/time/timekeeping.c: In function 'timekeeping_resume':
>> kernel/time/timekeeping.c:1986:55: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
    1986 |                 tk_debug_account_sleep_time(&ts_delta);
         |                                                       ^


vim +/if +1986 kernel/time/timekeeping.c

  1929	
  1930	/**
  1931	 * timekeeping_resume - Resumes the generic timekeeping subsystem.
  1932	 */
  1933	void timekeeping_resume(void)
  1934	{
  1935		struct timekeeper *tks = &tk_core.shadow_timekeeper;
  1936		struct clocksource *clock = tks->tkr_mono.clock;
  1937		struct timespec64 ts_new, ts_delta;
  1938		bool inject_sleeptime = false;
  1939		u64 cycle_now, nsec;
  1940		unsigned long flags;
  1941	
  1942		read_persistent_clock64(&ts_new);
  1943	
  1944		clockevents_resume();
  1945		clocksource_resume();
  1946	
  1947		raw_spin_lock_irqsave(&tk_core.lock, flags);
  1948	
  1949		/*
  1950		 * After system resumes, we need to calculate the suspended time and
  1951		 * compensate it for the OS time. There are 3 sources that could be
  1952		 * used: Nonstop clocksource during suspend, persistent clock and rtc
  1953		 * device.
  1954		 *
  1955		 * One specific platform may have 1 or 2 or all of them, and the
  1956		 * preference will be:
  1957		 *	suspend-nonstop clocksource -> persistent clock -> rtc
  1958		 * The less preferred source will only be tried if there is no better
  1959		 * usable source. The rtc part is handled separately in rtc core code.
  1960		 */
  1961		cycle_now = tk_clock_read(&tks->tkr_mono);
  1962		nsec = clocksource_stop_suspend_timing(clock, cycle_now);
  1963		if (nsec > 0) {
  1964			ts_delta = ns_to_timespec64(nsec);
  1965			inject_sleeptime = true;
  1966		} else if (timespec64_compare(&ts_new, &timekeeping_suspend_time) > 0) {
  1967			ts_delta = timespec64_sub(ts_new, timekeeping_suspend_time);
  1968			inject_sleeptime = true;
  1969		}
  1970	
  1971		if (inject_sleeptime) {
  1972			suspend_timing_needed = false;
  1973			__timekeeping_inject_sleeptime(tks, &ts_delta);
  1974		}
  1975	
  1976		/* Re-base the last cycle value */
  1977		tks->tkr_mono.cycle_last = cycle_now;
  1978		tks->tkr_raw.cycle_last  = cycle_now;
  1979	
  1980		tks->ntp_error = 0;
  1981		timekeeping_suspended = 0;
  1982		timekeeping_update_from_shadow(&tk_core, TK_CLOCK_WAS_SET);
  1983		raw_spin_unlock_irqrestore(&tk_core.lock, flags);
  1984	
  1985		if (inject_sleeptime && timespec64_valid_strict(&ts_delta))
> 1986			tk_debug_account_sleep_time(&ts_delta);
  1987	
  1988		/* Resume the clockevent device(s) and hrtimers */
  1989		tick_resume();
  1990		/* Notify timerfd as resume is equivalent to clock_was_set() */
  1991		timerfd_resume();
  1992	}
  1993	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki