linux-kernel - Re: [RFC PATCH v2 1/7] sched/fair: Add related data structure for task based throttle

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]

Message-ID: <b730d177-db05-44c1-b2c8-19290286f79d@amd.com>
Date: Mon, 14 Apr 2025 19:07:43 +0530
From: K Prateek Nayak <kprateek.nayak@....com>
To: Aaron Lu <ziqianlu@...edance.com>
CC: Valentin Schneider <vschneid@...hat.com>, Ben Segall <bsegall@...gle.com>,
	Peter Zijlstra <peterz@...radead.org>, Josh Don <joshdon@...gle.com>, Ingo
 Molnar <mingo@...hat.com>, Vincent Guittot <vincent.guittot@...aro.org>, Xi
 Wang <xii@...gle.com>, <linux-kernel@...r.kernel.org>, Juri Lelli
	<juri.lelli@...hat.com>, Dietmar Eggemann <dietmar.eggemann@....com>, Steven
 Rostedt <rostedt@...dmis.org>, Mel Gorman <mgorman@...e.de>, Chengming Zhou
	<chengming.zhou@...ux.dev>, Chuyi Zhou <zhouchuyi@...edance.com>, Jan Kiszka
	<jan.kiszka@...mens.com>
Subject: Re: [RFC PATCH v2 1/7] sched/fair: Add related data structure for
 task based throttle

Hello Aaron,

On 4/14/2025 5:25 PM, Aaron Lu wrote:
> On Mon, Apr 14, 2025 at 09:28:36AM +0530, K Prateek Nayak wrote:
>> Hello Aaron,
>>
>> On 4/9/2025 5:37 PM, Aaron Lu wrote:
>>> From: Valentin Schneider <vschneid@...hat.com>
>>>
>>> Add related data structures for this new throttle functionality.
>>>
>>> Signed-off-by: Valentin Schneider <vschneid@...hat.com>
>>> Signed-off-by: Aaron Lu <ziqianlu@...edance.com>
>>> ---
>>>    include/linux/sched.h |  4 ++++
>>>    kernel/sched/core.c   |  3 +++
>>>    kernel/sched/fair.c   | 12 ++++++++++++
>>>    kernel/sched/sched.h  |  2 ++
>>>    4 files changed, 21 insertions(+)
>>>
>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>> index f96ac19828934..0b55c79fee209 100644
>>> --- a/include/linux/sched.h
>>> +++ b/include/linux/sched.h
>>> @@ -880,6 +880,10 @@ struct task_struct {
>>>    #ifdef CONFIG_CGROUP_SCHED
>>>    	struct task_group		*sched_task_group;
>>> +#ifdef CONFIG_CFS_BANDWIDTH
>>> +	struct callback_head		sched_throttle_work;
>>> +	struct list_head		throttle_node;
>>
>> Since throttled tasks are fully dequeued before placing on the
>> "throttled_limbo_list", is it possible to reuse "p->se.group_node"?
> 
> I think it might be possible.
> 
>> Currently, it is used to track the task on "rq->cfs_tasks" and during
>> load-balancing when moving a bunch of tasks between CPUs but since a
>> fully throttled task is not tracked by either, it should be safe to
>> reuse this bit (CONFIG_DEBUG_LIST will scream if I'm wrong) and save
>> up on some space in the  task_struct.
>>
>> Thoughts?
> 
> Is it that adding throttle_node would cause task_struct to just cross a
> cacheline boundary? :-)
> 
> Or it's mainly a concern that system could have many tasks and any saving
> in task_struct is worth to try?

Mostly this :)

> 
> I can see reusing another field would cause task_is_throttled() more
> obscure to digest and implement, but I think it is doable.

I completely overlooked task_is_throttled() use-case. I think the
current implementation is much cleaner in that aspect; no need to
overload "p->se.group_node" and over-complicate this.

If we really want some space saving , declaring a "unsigned char
sched_throttled" in the hole next to "sched_delayed" would be cleaner
but I'd wait on Valentin and Peter's comments before going down that
path.

> 
> Thanks,
> Aaron

-- 
Thanks and Regards,
Prateek