[PATCH] fix the issue that the tick_nohz_get_sleep_length() function could return a negative value

[PATCH] fix the issue that the tick_nohz_get_sleep_length() function could return a negative value

[Zhou Ti (x2019cwm)]

Fix the issue that the tick_nohz_get_sleep_length() function could return
a negative value.

The variable "next_event" has a small possibility to be smaller than the
variable "now" during running. Since both the menu idle governor and the
teo idle governor use u64 to store the return value of the function, this
may result in an extremely large and invalid value.

One can easily detect the existence of this issue by using printk to
output a warning.

Signed-off-by: Ti Zhou <x2019cwm@stfx.ca>
---
--- tip/kernel/time/tick-sched.c.orig   2021-01-20 05:34:25.151325912 -0400
+++ tip/kernel/time/tick-sched.c        2021-01-20 07:09:15.060980886 -0400
@@ -1156,6 +1156,9 @@ ktime_t tick_nohz_get_sleep_length(ktime
        next_event = min_t(u64, next_event,
                           hrtimer_next_event_without(&ts->sched_timer));

+       if (unlikely(next_event < now))
+               next_event = now;
+
        return ktime_sub(next_event, now);
 }

Re: [PATCH] fix the issue that the tick_nohz_get_sleep_length() function could return a negative value

[Randy Dunlap]

On 1/20/21 10:58 AM, Zhou Ti (x2019cwm) wrote:
> Fix the issue that the tick_nohz_get_sleep_length() function could return
> a negative value.
> 
> The variable "next_event" has a small possibility to be smaller than the
> variable "now" during running. Since both the menu idle governor and the
> teo idle governor use u64 to store the return value of the function, this
> may result in an extremely large and invalid value.
> 
> One can easily detect the existence of this issue by using printk to
> output a warning.
> 
> Signed-off-by: Ti Zhou <x2019cwm@stfx.ca>
> ---
> --- tip/kernel/time/tick-sched.c.orig   2021-01-20 05:34:25.151325912 -0400
> +++ tip/kernel/time/tick-sched.c        2021-01-20 07:09:15.060980886 -0400
> @@ -1156,6 +1156,9 @@ ktime_t tick_nohz_get_sleep_length(ktime
>         next_event = min_t(u64, next_event,
>                            hrtimer_next_event_without(&ts->sched_timer));
> 
> +       if (unlikely(next_event < now))
> +               next_event = now;
> +
>         return ktime_sub(next_event, now);
>  }
> 

Hi,

You need to send this to some maintainer who can apply/merge it.
Just sending it to a mailing list is not sufficient.

thanks.
-- 
~Randy

Re: [PATCH] fix the issue that the tick_nohz_get_sleep_length() function could return a negative value

[Frederic Weisbecker]

On Wed, Jan 20, 2021 at 11:49:38PM +0000, Zhou Ti (x2019cwm) wrote:
> Fix the issue that the tick_nohz_get_sleep_length() function could return a 
> negative value.
> 
> The variable "dev->next_event" has a small possibility to be smaller than 
> the variable "now" during running, which would result in a negative value 
> of "*delta_next". The variable "next_event" also has a small posibility to 
> be smaller than the variable "now". Both case could lead to a negative 
> return of function tick_nohz_get_sleep_length().
> 
> Signed-off-by: Ti Zhou <x2019cwm@stfx.ca>
> ---
> --- tip/kernel/time/tick-sched.c.orig	2021-01-20 05:34:25.151325912 -0400
> +++ tip/kernel/time/tick-sched.c	2021-01-20 19:44:28.238538380 -0400
> @@ -1142,6 +1142,9 @@ ktime_t tick_nohz_get_sleep_length(ktime
>  
>  	*delta_next = ktime_sub(dev->next_event, now);
>  
> +	if (unlikely(*delta_next < 0))
> +		*delta_next = 0;
> +
>  	if (!can_stop_idle_tick(cpu, ts))
>  		return *delta_next;
>  
> @@ -1156,6 +1159,9 @@ ktime_t tick_nohz_get_sleep_length(ktime
>  	next_event = min_t(u64, next_event,
>  			   hrtimer_next_event_without(&ts->sched_timer));
>  
> +	if (unlikely(next_event < now))
> +		next_event = now;
> +
>  	return ktime_sub(next_event, now);
>  }
>  

I have reworked the changelog that way and queued. I'll post a
series including it after the merge window.

Thanks!

---
From: "Zhou Ti (x2019cwm)" <x2019cwm@stfx.ca>
Date: Wed, 20 Jan 2021 23:49:38 +0000
Subject: [PATCH] tick/nohz: Prevent tick_nohz_get_sleep_length() from
 returning negative value

If the hardware clock happens to fire its interrupts late, two possible
issues can happen while calling tick_nohz_get_sleep_length(). Either:

1) The next clockevent device event is due past the last idle entry time.

or:

2) The last timekeeping update happened before the last idle entry time
   and the next timer callback expires past the last idle entry time.

Make sure that both cases are handled to avoid returning a negative
duration to the cpuidle governors.

Signed-off-by: Ti Zhou <x2019cwm@stfx.ca>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/time/tick-sched.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index e10a4af88737..22b6a46818cf 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1142,6 +1142,9 @@ ktime_t tick_nohz_get_sleep_length(ktime_t *delta_next)
 
 	*delta_next = ktime_sub(dev->next_event, now);
 
+	if (unlikely(*delta_next < 0))
+		*delta_next = 0;
+
 	if (!can_stop_idle_tick(cpu, ts))
 		return *delta_next;
 
@@ -1156,6 +1159,9 @@ ktime_t tick_nohz_get_sleep_length(ktime_t *delta_next)
 	next_event = min_t(u64, next_event,
 			   hrtimer_next_event_without(&ts->sched_timer));
 
+	if (unlikely(next_event < now))
+		next_event = now;
+
 	return ktime_sub(next_event, now);
 }
 
-- 
2.25.1

Re: [PATCH] fix the issue that the tick_nohz_get_sleep_length() function could return a negative value

[Frederic Weisbecker]

On Wed, Jan 20, 2021 at 11:49:38PM +0000, Zhou Ti (x2019cwm) wrote:
> Fix the issue that the tick_nohz_get_sleep_length() function could return a 
> negative value.
> 
> The variable "dev->next_event" has a small possibility to be smaller than 
> the variable "now" during running, which would result in a negative value 
> of "*delta_next". The variable "next_event" also has a small posibility to 
> be smaller than the variable "now". Both case could lead to a negative 
> return of function tick_nohz_get_sleep_length().

Makes sense, queued, thanks!

> 
> Signed-off-by: Ti Zhou <x2019cwm@stfx.ca>
> ---
> --- tip/kernel/time/tick-sched.c.orig	2021-01-20 05:34:25.151325912 -0400
> +++ tip/kernel/time/tick-sched.c	2021-01-20 19:44:28.238538380 -0400
> @@ -1142,6 +1142,9 @@ ktime_t tick_nohz_get_sleep_length(ktime
>  
>  	*delta_next = ktime_sub(dev->next_event, now);
>  
> +	if (unlikely(*delta_next < 0))
> +		*delta_next = 0;
> +
>  	if (!can_stop_idle_tick(cpu, ts))
>  		return *delta_next;
>  
> @@ -1156,6 +1159,9 @@ ktime_t tick_nohz_get_sleep_length(ktime
>  	next_event = min_t(u64, next_event,
>  			   hrtimer_next_event_without(&ts->sched_timer));
>  
> +	if (unlikely(next_event < now))
> +		next_event = now;
> +
>  	return ktime_sub(next_event, now);
>  }
>  

[PATCH] fix the issue that the tick_nohz_get_sleep_length() function could return a negative value

[Zhou Ti (x2019cwm)]

Fix the issue that the tick_nohz_get_sleep_length() function could return a 
negative value.

The variable "dev->next_event" has a small possibility to be smaller than 
the variable "now" during running, which would result in a negative value 
of "*delta_next". The variable "next_event" also has a small posibility to 
be smaller than the variable "now". Both case could lead to a negative 
return of function tick_nohz_get_sleep_length().

Signed-off-by: Ti Zhou <x2019cwm@stfx.ca>
---
--- tip/kernel/time/tick-sched.c.orig	2021-01-20 05:34:25.151325912 -0400
+++ tip/kernel/time/tick-sched.c	2021-01-20 19:44:28.238538380 -0400
@@ -1142,6 +1142,9 @@ ktime_t tick_nohz_get_sleep_length(ktime
 
 	*delta_next = ktime_sub(dev->next_event, now);
 
+	if (unlikely(*delta_next < 0))
+		*delta_next = 0;
+
 	if (!can_stop_idle_tick(cpu, ts))
 		return *delta_next;
 
@@ -1156,6 +1159,9 @@ ktime_t tick_nohz_get_sleep_length(ktime
 	next_event = min_t(u64, next_event,
 			   hrtimer_next_event_without(&ts->sched_timer));
 
+	if (unlikely(next_event < now))
+		next_event = now;
+
 	return ktime_sub(next_event, now);
 }