Re: [PATCH] hrtimer: Interrupt storm on clock_settime

From: Thomas Gleixner <tglx@linutronix.de>
To: Mikael Beckius <mikael.beckius@windriver.com>,
	linux-kernel@vger.kernel.org
Cc: Anna-Maria Behnsen <anna-maria@linutronix.de>
Subject: Re: [PATCH] hrtimer: Interrupt storm on clock_settime
Date: Fri, 05 Feb 2021 16:45:42 +0100	[thread overview]
Message-ID: <87r1lu8qmx.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20210128140208.30264-1-mikael.beckius@windriver.com>

On Thu, Jan 28 2021 at 15:02, Mikael Beckius wrote:

> During clock_settime absolute realtime timers may get updated to expire
> sooner in absolute monotonic time but if hrtimer_force_reprogram is
> called as part of a clock_settime and the next hard hrtimer expires
> before the next soft hrtimer softirq_expires_next will not be updated
> to reflect this change (assuming the realtime timer is a soft timer).
>
> This means that if the next soft hrtimer expires before
> softirq_expires_next but after now no soft hrtimer interrupt will be
> raised in hrtimer_interrupt which will instead retry tick_program_event
> three times before forcing a tick_program_event using a very short delay
> entering hrtimer_interrupt again almost immediately repeating the
> process over and over again until now exceeds softirq_expires_next and a
> soft hrtimer interrupt is finally raised.

Duh.

> This patch aims to solve this by always updating softirq_expires_next if
> a soft hrtimer exists.

  git grep 'This patch' Documentation/process/

> Signed-off-by: Mikael Beckius <mikael.beckius@windriver.com>
> ---
>  kernel/time/hrtimer.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index 743c852e10f2..e4c233f404b1 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -633,7 +633,7 @@ hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal)
>  	 */
>  	expires_next = __hrtimer_get_next_event(cpu_base, HRTIMER_ACTIVE_ALL);
>  
> -	if (cpu_base->next_timer && cpu_base->next_timer->is_soft) {
> +	if (cpu_base->softirq_next_timer) {

>  		/*
>  		 * When the softirq is activated, hrtimer has to be
>  		 * programmed with the first hard hrtimer because soft
> @@ -643,7 +643,8 @@ hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal)
>  			expires_next = __hrtimer_get_next_event(cpu_base,
>  								HRTIMER_ACTIVE_HARD);
>  		else
> -			cpu_base->softirq_expires_next = expires_next;
> +			cpu_base->softirq_expires_next = __hrtimer_get_next_event(cpu_base,
> +								HRTIMER_ACTIVE_SOFT);

That works, but we can spare the double scan completely and make the
code understandable. See below.

Thanks,

        tglx
---

--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -626,26 +626,25 @@ static inline int hrtimer_hres_active(vo
 static void
 hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal)
 {
-	ktime_t expires_next;
+	ktime_t expires_next, soft = KTIME_MAX;
 
 	/*
-	 * Find the current next expiration time.
+	 * If soft interrupt has already been activated, ignore the soft
+	 * base. It will be handled in the already raised soft interrupt.
 	 */
-	expires_next = __hrtimer_get_next_event(cpu_base, HRTIMER_ACTIVE_ALL);
-
-	if (cpu_base->next_timer && cpu_base->next_timer->is_soft) {
+	if (!cpu_base->softirq_activated) {
+		soft = __hrtimer_get_next_event(cpu_base, HRTIMER_ACTIVE_SOFT);
 		/*
-		 * When the softirq is activated, hrtimer has to be
-		 * programmed with the first hard hrtimer because soft
-		 * timer interrupt could occur too late.
+		 * Update the soft expiry time. clock_settime() might have
+		 * affected it.
 		 */
-		if (cpu_base->softirq_activated)
-			expires_next = __hrtimer_get_next_event(cpu_base,
-								HRTIMER_ACTIVE_HARD);
-		else
-			cpu_base->softirq_expires_next = expires_next;
+		cpu_base->softirq_expires_next = soft;
 	}
 
+	expires_next = __hrtimer_get_next_event(cpu_base, HRTIMER_ACTIVE_HARD);
+	if (expires_next > soft)
+		expires_next = soft;
+
 	if (skip_equal && expires_next == cpu_base->expires_next)
 		return;
 
