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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 14 Feb 2024 15:23:35 +0100
From: Daniel Bristot de Oliveira <bristot@...nel.org>
To: Joel Fernandes <joel@...lfernandes.org>, Ingo Molnar <mingo@...hat.com>,
 Peter Zijlstra <peterz@...radead.org>, Juri Lelli <juri.lelli@...hat.com>,
 Vincent Guittot <vincent.guittot@...aro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@....com>,
 Steven Rostedt <rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>,
 Mel Gorman <mgorman@...e.de>, Daniel Bristot de Oliveira
 <bristot@...hat.com>, Valentin Schneider <vschneid@...hat.com>,
 linux-kernel@...r.kernel.org, Luca Abeni <luca.abeni@...tannapisa.it>,
 Tommaso Cucinotta <tommaso.cucinotta@...tannapisa.it>,
 Thomas Gleixner <tglx@...utronix.de>,
 Vineeth Pillai <vineeth@...byteword.org>,
 Shuah Khan <skhan@...uxfoundation.org>, Phil Auld <pauld@...hat.com>
Subject: Re: [PATCH v5 7/7] sched/fair: Fair server interface

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