Re: [PATCH v2 06/15] sched: server: Don't start hrtick for DL server tasks

From: Daniel Bristot de Oliveira <bristot@redhat.com>
To: "Joel Fernandes (Google)" <joel@joelfernandes.org>,
	linux-kernel@vger.kernel.org, Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Juri Lelli <juri.lelli@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
	Valentin Schneider <vschneid@redhat.com>
Cc: Suleiman Souhlal <suleiman@google.com>,
	Youssef Esmat <youssefesmat@google.com>,
	David Vernet <void@manifault.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Paul E . McKenney" <paulmck@kernel.org>,
	joseph.salisbury@canonical.com,
	Luca Abeni <luca.abeni@santannapisa.it>,
	Tommaso Cucinotta <tommaso.cucinotta@santannapisa.it>,
	Vineeth Pillai <vineeth@bitbyteword.org>,
	Shuah Khan <skhan@linuxfoundation.org>,
	Phil Auld <pauld@redhat.com>
Subject: Re: [PATCH v2 06/15] sched: server: Don't start hrtick for DL server tasks
Date: Fri, 5 Apr 2024 10:49:29 +0200	[thread overview]
Message-ID: <a9dd5c5e-0095-48f9-9628-d5901d469ff3@redhat.com> (raw)
In-Reply-To: <20240313012451.1693807-7-joel@joelfernandes.org>

On 3/13/24 02:24, Joel Fernandes (Google) wrote:
> From: Suleiman Souhlal <suleiman@google.com>
> 
> Otherwise, we might start it even for tasks in a sched class that should
> have it off.
> 
> Signed-off-by: Suleiman Souhlal <suleiman@google.com>
> ---
>  kernel/sched/deadline.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 8fafe3f8b59c..5adfc15803c3 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -2325,11 +2325,12 @@ static struct task_struct *pick_next_task_dl(struct rq *rq)
>  	if (!p)
>  		return p;
>  
> -	if (!p->dl_server)
> +	if (!p->dl_server) {
>  		set_next_task_dl(rq, p, true);
>  
> -	if (hrtick_enabled(rq))
> -		start_hrtick_dl(rq, &p->dl);
> +		if (hrtick_enabled(rq))
> +			start_hrtick_dl(rq, &p->dl);
> +	}

The rational for having hrtick for the dl server too is the following:

This hrtick is reponsible for adding a hr timer for throttling. The throttling
serves as a protection for the other tasks, to avoid them missing their deadlines
because the current task overun for too long. Like, a task with 200us of runtime
actually running for 1 ms because of the non-hr-tick.

For example, let's get the case we have at red hat, where we want to use the
dl server to provide a minimum bandwidth to avoid starvation, keeping the noise
on real-time tasks low.

On this case, we will set the runtime for the fair server with number as low
as 20 us... 40 us. With hrtick, when the fair server is enqueued, it will be
throttle in the us scale... Without the hrtick, the server can run for an entire
tick before being throttled.

here is an example of this scenario using osnoise with/withoutout arming the hrtick
for the server that was set for 20 us of runtime:

There is a kernel compilation in CPU 1. Then osnoise is dispatched as fifo,
on CPU 1.

 # osnoise -c 1 -P f:1

removing hrtick:
 ############
 duration:   0 00:02:00 | time is in us
 CPU Period       Runtime        Noise  % CPU Aval   Max Noise   Max Single          HW          NMI          IRQ      Softirq       Thread
   1 #119       119000000         6482    99.99455        1220         1009           0            0          244           59           50
 ############

See max single noise... it when longer than the 20 us i've set...

With hrtick:
 ############
  duration:   0 00:02:30 | time is in us
  CPU Period       Runtime        Noise  % CPU Aval   Max Noise   Max Single          HW          NMI          IRQ      Softirq       Thread
    1 #149       149000472         3730    99.99749          33           33           0            0          641            3          155
 ############

the max single goes down to 33 us. It is not exactly 20 because there is
one timer to enqueue the server, and another timer to throttle it. Still,
the granularity is in the same order.

So, maybe, what we can do is to get it back to hrtick_enabled_dl() instead
of hrtick_enabled(), to have it only when HRTICK_DL is set.

am I missing a point?

-- Daniel