nohz: Update tick instead of restarting tick in tick_nohz_idle_exit()

nohz: Update tick instead of restarting tick in tick_nohz_idle_exit()

[Yunfeng Ye]

In realtime scenarios, the "nohz_full" parameter is configured. Tick
interference is not expected when there is only one realtime thread.
But when the idle thread is switched to the realtime thread, the tick
timer is restarted always.

So on the nohz full mode, it is unnecessary to restart the tick timer
when there is only one realtime thread. Adding can_stop_full_tick()
before restarting the tick, if it return true, keep tick stopped.

Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
---
 kernel/time/tick-sched.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index cc7cba20382e..f664c7735cab 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1208,6 +1208,20 @@ void tick_nohz_idle_restart_tick(void)
 		__tick_nohz_idle_restart_tick(ts, ktime_get());
 }

+static void tick_nohz_update_tick(struct tick_sched *ts, ktime_t now)
+{
+#ifdef CONFIG_NO_HZ_FULL
+	int cpu = smp_processor_id();
+
+	if (tick_nohz_full_cpu(cpu) && can_stop_full_tick(cpu, ts)) {
+		tick_nohz_stop_sched_tick(ts, cpu);
+		tick_nohz_account_idle_ticks(ts);
+		return;
+	}
+#endif
+	__tick_nohz_idle_restart_tick(ts, now);
+}
+
 /**
  * tick_nohz_idle_exit - restart the idle tick from the idle task
  *
@@ -1237,7 +1251,7 @@ void tick_nohz_idle_exit(void)
 		tick_nohz_stop_idle(ts, now);

 	if (tick_stopped)
-		__tick_nohz_idle_restart_tick(ts, now);
+		tick_nohz_update_tick(ts, now);

 	local_irq_enable();
 }
-- 
2.18.4

Re: nohz: Update tick instead of restarting tick in tick_nohz_idle_exit()

[Frederic Weisbecker]

On Mon, Nov 23, 2020 at 09:22:08PM +0800, Yunfeng Ye wrote:
> In realtime scenarios, the "nohz_full" parameter is configured. Tick
> interference is not expected when there is only one realtime thread.
> But when the idle thread is switched to the realtime thread, the tick
> timer is restarted always.
> 
> So on the nohz full mode, it is unnecessary to restart the tick timer
> when there is only one realtime thread. Adding can_stop_full_tick()
> before restarting the tick, if it return true, keep tick stopped.
> 
> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>

We can indeed stop the tick and avoid it to be re-armed needlessly at this
point.

I'm taking your patch, I may just edit it a little and resend it.

Thanks!

Re: nohz: Update tick instead of restarting tick in tick_nohz_idle_exit()

[Yunfeng Ye]

On 2020/11/27 20:15, Frederic Weisbecker wrote:
> On Mon, Nov 23, 2020 at 09:22:08PM +0800, Yunfeng Ye wrote:
>> In realtime scenarios, the "nohz_full" parameter is configured. Tick
>> interference is not expected when there is only one realtime thread.
>> But when the idle thread is switched to the realtime thread, the tick
>> timer is restarted always.
>>
>> So on the nohz full mode, it is unnecessary to restart the tick timer
>> when there is only one realtime thread. Adding can_stop_full_tick()
>> before restarting the tick, if it return true, keep tick stopped.
>>
>> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
> 
> We can indeed stop the tick and avoid it to be re-armed needlessly at this
> point.
> 
> I'm taking your patch, I may just edit it a little and resend it.
> 
Ok, thanks.

> Thanks!
> .
> 

Re: nohz: Update tick instead of restarting tick in tick_nohz_idle_exit()

[Frederic Weisbecker]

On Mon, Nov 23, 2020 at 09:22:08PM +0800, Yunfeng Ye wrote:
> In realtime scenarios, the "nohz_full" parameter is configured. Tick
> interference is not expected when there is only one realtime thread.
> But when the idle thread is switched to the realtime thread, the tick
> timer is restarted always.
> 
> So on the nohz full mode, it is unnecessary to restart the tick timer
> when there is only one realtime thread. Adding can_stop_full_tick()
> before restarting the tick, if it return true, keep tick stopped.
> 
> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>

Hi,

After reworking the codebase a bit, I've edited your patch and
changelog and then queued it. I'll post it in a series after the
merge window. See the result:

---
From: Yunfeng Ye <yeyunfeng@huawei.com>
Date: Tue, 9 Feb 2021 23:59:19 +0100
Subject: [PATCH] tick/nohz: Conditionally restart tick on idle exit

In nohz_full mode, switching from idle to a task will unconditionally
issue a tick restart. If the task is alone in the runqueue or is the
highest priority, the tick will fire once then eventually stop. But that
alone is still undesired noise.

Therefore, only restart the tick on idle exit when it's strictly
necessary.

Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/time/tick-sched.c | 44 ++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 3e272490fe2e..79796286a4ba 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -923,24 +923,28 @@ static void tick_nohz_restart_sched_tick(struct tick_sched *ts, ktime_t now)
 	tick_nohz_restart(ts, now);
 }
 
-static void tick_nohz_full_update_tick(struct tick_sched *ts)
+static void __tick_nohz_full_update_tick(struct tick_sched *ts,
+					 ktime_t now)
 {
 #ifdef CONFIG_NO_HZ_FULL
-	int cpu;
+	int cpu = smp_processor_id();
 
+	if (can_stop_full_tick(cpu, ts))
+		tick_nohz_stop_sched_tick(ts, cpu);
+	else if (ts->tick_stopped)
+		tick_nohz_restart_sched_tick(ts, now);
+#endif
+}
+
+static void tick_nohz_full_update_tick(struct tick_sched *ts)
+{
 	if (!tick_nohz_full_this_cpu())
 		return;
 
 	if (!ts->tick_stopped && ts->nohz_mode == NOHZ_MODE_INACTIVE)
 		return;
 
-	cpu = smp_processor_id();
-
-	if (can_stop_full_tick(cpu, ts))
-		tick_nohz_stop_sched_tick(ts, cpu);
-	else if (ts->tick_stopped)
-		tick_nohz_restart_sched_tick(ts, ktime_get());
-#endif
+	__tick_nohz_full_update_tick(ts, ktime_get());
 }
 
 static bool can_stop_idle_tick(int cpu, struct tick_sched *ts)
@@ -1210,18 +1214,24 @@ static void tick_nohz_account_idle_ticks(struct tick_sched *ts)
 #endif
 }
 
-static void __tick_nohz_idle_restart_tick(struct tick_sched *ts, ktime_t now)
+void tick_nohz_idle_restart_tick(void)
 {
-	tick_nohz_restart_sched_tick(ts, now);
-	tick_nohz_account_idle_ticks(ts);
+	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
+
+	if (ts->tick_stopped) {
+		tick_nohz_restart_sched_tick(ts, ktime_get());
+		tick_nohz_account_idle_ticks(ts);
+	}
 }
 
-void tick_nohz_idle_restart_tick(void)
+static void tick_nohz_idle_update_tick(struct tick_sched *ts, ktime_t now)
 {
-	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
+	if (tick_nohz_full_this_cpu())
+		__tick_nohz_full_update_tick(ts, now);
+	else
+		tick_nohz_restart_sched_tick(ts, now);
 
-	if (ts->tick_stopped)
-		__tick_nohz_idle_restart_tick(ts, ktime_get());
+	tick_nohz_account_idle_ticks(ts);
 }
 
 /**
@@ -1253,7 +1263,7 @@ void tick_nohz_idle_exit(void)
 		tick_nohz_stop_idle(ts, now);
 
 	if (tick_stopped)
-		__tick_nohz_idle_restart_tick(ts, now);
+		tick_nohz_idle_update_tick(ts, now);
 
 	local_irq_enable();
 }
-- 
2.25.1

Re: nohz: Update tick instead of restarting tick in tick_nohz_idle_exit()

[Yunfeng Ye]

On 2021/2/19 20:16, Frederic Weisbecker wrote:
> On Mon, Nov 23, 2020 at 09:22:08PM +0800, Yunfeng Ye wrote:
>> In realtime scenarios, the "nohz_full" parameter is configured. Tick
>> interference is not expected when there is only one realtime thread.
>> But when the idle thread is switched to the realtime thread, the tick
>> timer is restarted always.
>>
>> So on the nohz full mode, it is unnecessary to restart the tick timer
>> when there is only one realtime thread. Adding can_stop_full_tick()
>> before restarting the tick, if it return true, keep tick stopped.
>>
>> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
> 
> Hi,
> 
> After reworking the codebase a bit, I've edited your patch and
> changelog and then queued it. I'll post it in a series after the
> merge window. See the result:
> 
Ok, Thanks. My first version patch which not be sent is a little like this.

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index cc7cba20382e..e793c8f675e6 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -908,21 +908,24 @@ static void tick_nohz_restart_sched_tick(struct tick_sched *ts, ktime_t now)
        tick_nohz_restart(ts, now);
 }

-static void tick_nohz_full_update_tick(struct tick_sched *ts)
+static int tick_nohz_full_update_tick(struct tick_sched *ts, ktime_t now)
 {
 #ifdef CONFIG_NO_HZ_FULL
        int cpu = smp_processor_id();

        if (!tick_nohz_full_cpu(cpu))
-               return;
+               return -EINVAL;

        if (!ts->tick_stopped && ts->nohz_mode == NOHZ_MODE_INACTIVE)
-               return;
+               return -EINVAL;

        if (can_stop_full_tick(cpu, ts))
                tick_nohz_stop_sched_tick(ts, cpu);
        else if (ts->tick_stopped)
-               tick_nohz_restart_sched_tick(ts, ktime_get());
+               tick_nohz_restart_sched_tick(ts, now);
+       return 0;
+#else
+       return -EINVAL;
 #endif
 }

@@ -1080,7 +1083,7 @@ void tick_nohz_irq_exit(void)
        if (ts->inidle)
                tick_nohz_start_idle(ts);
        else
-               tick_nohz_full_update_tick(ts);
+               tick_nohz_full_update_tick(ts, ktime_get());
 }

 /**
@@ -1208,6 +1211,13 @@ void tick_nohz_idle_restart_tick(void)
                __tick_nohz_idle_restart_tick(ts, ktime_get());
 }

+static void tick_nohz_update_tick(struct tick_sched *ts, ktime_t now)
+{
+       if (tick_nohz_full_update_tick(ts) < 0)
+               tick_nohz_restart_sched_tick(ts, now);
+       tick_nohz_account_idle_ticks(ts);
+}
+
 /**
  * tick_nohz_idle_exit - restart the idle tick from the idle task
  *
@@ -1237,7 +1247,7 @@ void tick_nohz_idle_exit(void)
                tick_nohz_stop_idle(ts, now);

        if (tick_stopped)
-               __tick_nohz_idle_restart_tick(ts, now);
+               tick_nohz_update_tick(ts, now);

        local_irq_enable();
 }


> ---
> From: Yunfeng Ye <yeyunfeng@huawei.com>
> Date: Tue, 9 Feb 2021 23:59:19 +0100
> Subject: [PATCH] tick/nohz: Conditionally restart tick on idle exit
> 
> In nohz_full mode, switching from idle to a task will unconditionally
> issue a tick restart. If the task is alone in the runqueue or is the
> highest priority, the tick will fire once then eventually stop. But that
> alone is still undesired noise.
> 
> Therefore, only restart the tick on idle exit when it's strictly
> necessary.
> 
> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> ---
>  kernel/time/tick-sched.c | 44 ++++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 3e272490fe2e..79796286a4ba 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -923,24 +923,28 @@ static void tick_nohz_restart_sched_tick(struct tick_sched *ts, ktime_t now)
>  	tick_nohz_restart(ts, now);
>  }
>  
> -static void tick_nohz_full_update_tick(struct tick_sched *ts)
> +static void __tick_nohz_full_update_tick(struct tick_sched *ts,
> +					 ktime_t now)
>  {
>  #ifdef CONFIG_NO_HZ_FULL
> -	int cpu;
> +	int cpu = smp_processor_id();
>  
> +	if (can_stop_full_tick(cpu, ts))
> +		tick_nohz_stop_sched_tick(ts, cpu);
> +	else if (ts->tick_stopped)
> +		tick_nohz_restart_sched_tick(ts, now);
> +#endif
> +}
> +
> +static void tick_nohz_full_update_tick(struct tick_sched *ts)
> +{
>  	if (!tick_nohz_full_this_cpu())
>  		return;
>  
>  	if (!ts->tick_stopped && ts->nohz_mode == NOHZ_MODE_INACTIVE)
>  		return;
>  
> -	cpu = smp_processor_id();
> -
> -	if (can_stop_full_tick(cpu, ts))
> -		tick_nohz_stop_sched_tick(ts, cpu);
> -	else if (ts->tick_stopped)
> -		tick_nohz_restart_sched_tick(ts, ktime_get());
> -#endif
> +	__tick_nohz_full_update_tick(ts, ktime_get());
>  }
>  
>  static bool can_stop_idle_tick(int cpu, struct tick_sched *ts)
> @@ -1210,18 +1214,24 @@ static void tick_nohz_account_idle_ticks(struct tick_sched *ts)
>  #endif
>  }
>  
> -static void __tick_nohz_idle_restart_tick(struct tick_sched *ts, ktime_t now)
> +void tick_nohz_idle_restart_tick(void)
>  {
> -	tick_nohz_restart_sched_tick(ts, now);
> -	tick_nohz_account_idle_ticks(ts);
> +	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
> +
> +	if (ts->tick_stopped) {
> +		tick_nohz_restart_sched_tick(ts, ktime_get());
> +		tick_nohz_account_idle_ticks(ts);
> +	}
>  }
>  
> -void tick_nohz_idle_restart_tick(void)
> +static void tick_nohz_idle_update_tick(struct tick_sched *ts, ktime_t now)
>  {
> -	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
> +	if (tick_nohz_full_this_cpu())
> +		__tick_nohz_full_update_tick(ts, now);
> +	else
> +		tick_nohz_restart_sched_tick(ts, now);
>  
> -	if (ts->tick_stopped)
> -		__tick_nohz_idle_restart_tick(ts, ktime_get());
> +	tick_nohz_account_idle_ticks(ts);
>  }
>  
>  /**
> @@ -1253,7 +1263,7 @@ void tick_nohz_idle_exit(void)
>  		tick_nohz_stop_idle(ts, now);
>  
>  	if (tick_stopped)
> -		__tick_nohz_idle_restart_tick(ts, now);
> +		tick_nohz_idle_update_tick(ts, now);
>  
>  	local_irq_enable();
>  }
>