| 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
| ||
|
Message-ID: <2bd42110-a37f-4da1-bc9a-194f3ff909df@amd.com>
Date: Fri, 14 Mar 2025 23:34:44 +0530
From: K Prateek Nayak <kprateek.nayak@....com>
To: Aaron Lu <ziqianlu@...edance.com>
CC: Chengming Zhou <chengming.zhou@...ux.dev>, 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>,
<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>, Chuyi Zhou <zhouchuyi@...edance.com>, John
Stultz <jstultz@...gle.com>
Subject: Re: [RFC PATCH 2/7] sched/fair: Handle throttle path for task based
throttle
(+jstultz for some clarification around rq->donor)
Hello Aaron,
On 3/14/2025 5:17 PM, Aaron Lu wrote:
> Hi Prateek,
>
> On Fri, Mar 14, 2025 at 03:56:26PM +0530, K Prateek Nayak wrote:
>> Hello Aaron,
>>
>> On 3/14/2025 3:12 PM, Aaron Lu wrote:
>>>> Then then if we pick a task from a throttled cfs_rq subtree, we can setup task work
>>>> for it, so we don't botter with the delayed_dequeue task case that Prateek mentioned.
>>> If we add a check point in pick time, maybe we can also avoid the check
>>> in enqueue time. One thing I'm thinking is, for a task, it may be picked
>>> multiple times with only a single enqueue so if we do the check in pick,
>>> the overhead can be larger?
>>
>> I think it can be minimized to a good extent. Something like:
>
> I see, thanks for the illustration.
>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index d646451d617c..ba6571368840 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -5942,6 +5942,9 @@ static inline bool task_has_throttle_work(struct task_struct *p)
>> static inline void task_throttle_setup_work(struct task_struct *p)
>> {
>> + if (task_has_throttle_work(p))
>> + return;
>> +
>> /*
>> * Kthreads and exiting tasks don't return to userspace, so adding the
>> * work is pointless
>> @@ -5949,9 +5952,6 @@ static inline void task_throttle_setup_work(struct task_struct *p)
>> if ((p->flags & (PF_EXITING | PF_KTHREAD)))
>> return;
>> - if (task_has_throttle_work(p))
>> - return;
>> -
>> task_work_add(p, &p->sched_throttle_work, TWA_RESUME);
>> }
>> @@ -6000,12 +6000,6 @@ static int tg_throttle_down(struct task_group *tg, void *data)
>> node = rb_next(node);
>> }
>> - /* curr is not in the timeline tree */
>> - if (cfs_rq->curr && entity_is_task(cfs_rq->curr)) {
>> - p = task_of(cfs_rq->curr);
>> - task_throttle_setup_work(p);
>> - }
>> -
>
> Should we also remove adding throttle work for those tasks in
> cfs_rq->tasks_timeline?
Yes. I don't think that is required with this approach suggested by
Chengming.
>
>> return 0;
>> }
>> @@ -6049,6 +6043,18 @@ static void throttle_cfs_rq(struct cfs_rq *cfs_rq)
>> SCHED_WARN_ON(cfs_rq->throttled_clock);
>> if (cfs_rq->nr_queued)
>> cfs_rq->throttled_clock = rq_clock(rq);
>> +
>> + /*
>> + * If cfs_rq->curr is set, check if current task is queued
>> + * and set up the throttle work proactively.
>> + */
>> + if (cfs_rq->curr) {
>> + struct task_struct *p = rq->donor; /* scheduling context with proxy */
>
> I'll have to check what rq->donor means.
Essentially "rq->donor" is the scheduling context (where the time
accounting happens) whereas "rq->curr" may point to a mutex-holder that
is running with the context of "rq->donor" to unblock itself as quickly
as possible.
If we experience a throttle, it is because the time accounting to
"rq->donor" hierarchy couldn't fetch anymore bandwidth.
TBH, adding the task work to either of them doesn't make sense since
the "rq->donor" will be picked to run again once mutex-holder does a
handoff and the throttle work can be added then too.
> I think the point is to proactively add throttle work for rq->curr if
> rq->curr is in this throttled hierarchy? Because the only check point to
> add throttle work will be at pick time and curr will probably not be
> picked anytime soon.
John knows these bits best and maybe he can correct me if I'm wrong
here. For the time being, rq->curr and rq->donor both point to the
same task but that will change when Part-2 of proxy execution series
lands. We can just clarify these bits now to avoid any conflicts later.
--
Thanks and Regards,
Prateek
>
> Thanks,
> Aaron
>
>> +
>> + if (task_on_rq_queued(p))
>> + task_throttle_setup_work(p);
>> + }
>> +
>> return;
>> }
>> @@ -8938,6 +8944,13 @@ pick_next_task_fair(struct rq *rq, struct task_struct *prev, struct rq_flags *rf
>> struct sched_entity *pse = &prev->se;
>> struct cfs_rq *cfs_rq;
>> + /*
>> + * Check if throttle work needs to be setup when
>> + * switching to a different task.
>> + */
>> + if (throttled_hierarchy(cfs_rq_of(se)))
>> + task_throttle_setup_work(p);
>> +
>> while (!(cfs_rq = is_same_group(se, pse))) {
>> int se_depth = se->depth;
>> int pse_depth = pse->depth;
>> @@ -13340,6 +13353,9 @@ static void set_next_task_fair(struct rq *rq, struct task_struct *p, bool first)
>> account_cfs_rq_runtime(cfs_rq, 0);
>> }
>> + if (throttled_hierarchy(cfs_rq_of(se)))
>> + task_throttle_setup_work(p);
>> +
>> __set_next_task_fair(rq, p, first);
>> }
>> --
>>
>> .. with the additional plumbing in place of course.
>>
>> --
>> Thanks and Regards,
>> Prateek
>>
Powered by blists - more mailing lists