Re: [PATCH v5 7/7] sched/fair: Fair server interface

From: Daniel Bristot de Oliveira <bristot@kernel.org>
To: Joel Fernandes <joel@joelfernandes.org>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Juri Lelli <juri.lelli@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	Valentin Schneider <vschneid@redhat.com>,
	linux-kernel@vger.kernel.org,
	Luca Abeni <luca.abeni@santannapisa.it>,
	Tommaso Cucinotta <tommaso.cucinotta@santannapisa.it>,
	Thomas Gleixner <tglx@linutronix.de>,
	Vineeth Pillai <vineeth@bitbyteword.org>,
	Shuah Khan <skhan@linuxfoundation.org>,
	Phil Auld <pauld@redhat.com>
Subject: Re: [PATCH v5 7/7] sched/fair: Fair server interface
Date: Wed, 14 Feb 2024 15:23:35 +0100	[thread overview]
Message-ID: <091ca2ea-202d-4685-92ea-529186a94f0a@kernel.org> (raw)
In-Reply-To: <8cbf4bcd-431b-466f-b62d-ee03932e97f5@joelfernandes.org>

On 2/13/24 03:13, Joel Fernandes wrote:
> 
> 
> On 11/4/2023 6:59 AM, Daniel Bristot de Oliveira wrote:
>> Add an interface for fair server setup on debugfs.
>>
>> Each rq have three files under /sys/kernel/debug/sched/rq/CPU{ID}:
>>
>>  - fair_server_runtime: set runtime in ns
>>  - fair_server_period: set period in ns
>>  - fair_server_defer: on/off for the defer mechanism
> 
> Btw Daniel, there is an interesting side-effect of this interface having runtime
> and period in 2 separate files :)
> 
> Say I want to set a CPU to 5ms / 10ms.
> 
> I cannot set either period or runtime to 5ms or 10ms directly.
> 
> I have to first set period to 100ms, then set runtime to 50ms, then set period
> to 50ms, then set runtime to 5ms, then finally set period to 10ms.

Hummm yeah I could reproduce that, it seems that it is not even a problem of having
two files, but a bug in the logic, I will have a look.

> The reason seems to be because otherwise runtime / period will not be
> accomodated and will cause dl_overflow issues.
> 
> I'd suggest providing both runtime and period in the same interface to make it
> more easier to use. However, for the testing I am going with what we have.
> 
> Also a request:
> 
> I was wondering if a new version of the last 3 patches could be posted to
> LKML or shared in a tree somewhere. I am trying to sync to mainline and
> rebase our latest fixes on top of that, however it is difficult to do because
> these 3 patches are in bit of a flux (example the discussion between you and
> Peter about update_curr()). What's the best way to move forward with rebasing
> our fix contributions?

Juri and I chat about, and we think it is a good thing to re-send this patch set,
including a fix I have to it (to avoid regression wrt rt throttling), explaining
these things in the mailing list so peter will be able to follow the discussion.

I still need to finish testing, and to make a proper cover page with all updates, the
latest thing is here (tm):

https://git.kernel.org/pub/scm/linux/kernel/git/bristot/linux.git/log/?h=dl_server_v6

It is based on peter's sched/more. I will probably re-send it today or tomorrow,
but at least you can have a look at it.

Another reason to send it is to get the regression test machinery running....

 I am going with the sched/more in Peter's queue.git
> unless you/Peter prefer something else. And I added your update_curr()
> suggestion onto that, let me know if you disagree with it:
> 
> @@ -1173,6 +1171,8 @@ static void update_curr(struct cfs_rq *cfs_rq)
> 
>         if (entity_is_task(curr))
>                 update_curr_task(task_of(curr), delta_exec);
> +       else
> +               dl_server_update(&rq_of(cfs_rq)->fair_server, delta_exec);
> 
>         account_cfs_rq_runtime(cfs_rq, delta_exec);
>  }

That part of the code was optimized by peter during the last round of discussions.

It is like this now:

------------ %< -----------
-       if (entity_is_task(curr))
-               update_curr_task(task_of(curr), delta_exec);
+       if (entity_is_task(curr)) {
+               struct task_struct *p = task_of(curr);
+               update_curr_task(p, delta_exec);
+               /*
+                * Any fair task that runs outside of fair_server should
+                * account against fair_server such that it can account for
+                * this time and possibly avoid running this period.
+                */
+               if (p->dl_server != &rq->fair_server)
+                       dl_server_update(&rq->fair_server, delta_exec);
+       }
------------ >% -----------

It is not straightforward to understand... but the ideia is:

if it is a task, and the server is ! of the fair server, discount time
directly from the fair server. This also means that if dl_server is NULL
(the server is not enabled) it will discount time from the fair server.

-- Daniel

> thanks,
> 
>  - Joel