| 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
| ||
|
Date: Mon, 8 Jun 2020 20:28:11 -0400
From: Phil Auld <pauld@...hat.com>
To: Tao Zhou <ouwen210@...mail.com>
Cc: linux-kernel@...r.kernel.org, Josh Don <joshdon@...gle.com>,
"Peter Zijlstra (Intel)" <peterz@...radead.org>,
x86 <x86@...nel.org>
Subject: Re: [tip: sched/core] sched/fair: Remove distribute_running from CFS
bandwidth
On Tue, Jun 09, 2020 at 07:05:38AM +0800 Tao Zhou wrote:
> Hi Phil,
>
> On Mon, Jun 08, 2020 at 10:53:04AM -0400, Phil Auld wrote:
> > On Sun, Jun 07, 2020 at 09:25:58AM +0800 Tao Zhou wrote:
> > > Hi,
> > >
> > > On Fri, May 01, 2020 at 06:22:12PM -0000, tip-bot2 for Josh Don wrote:
> > > > The following commit has been merged into the sched/core branch of tip:
> > > >
> > > > Commit-ID: ab93a4bc955b3980c699430bc0b633f0d8b607be
> > > > Gitweb: https://git.kernel.org/tip/ab93a4bc955b3980c699430bc0b633f0d8b607be
> > > > Author: Josh Don <joshdon@...gle.com>
> > > > AuthorDate: Fri, 10 Apr 2020 15:52:08 -07:00
> > > > Committer: Peter Zijlstra <peterz@...radead.org>
> > > > CommitterDate: Thu, 30 Apr 2020 20:14:38 +02:00
> > > >
> > > > sched/fair: Remove distribute_running from CFS bandwidth
> > > >
> > > > This is mostly a revert of commit:
> > > >
> > > > baa9be4ffb55 ("sched/fair: Fix throttle_list starvation with low CFS quota")
> > > >
> > > > The primary use of distribute_running was to determine whether to add
> > > > throttled entities to the head or the tail of the throttled list. Now
> > > > that we always add to the tail, we can remove this field.
> > > >
> > > > The other use of distribute_running is in the slack_timer, so that we
> > > > don't start a distribution while one is already running. However, even
> > > > in the event that this race occurs, it is fine to have two distributions
> > > > running (especially now that distribute grabs the cfs_b->lock to
> > > > determine remaining quota before assigning).
> > > >
> > > > Signed-off-by: Josh Don <joshdon@...gle.com>
> > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> > > > Reviewed-by: Phil Auld <pauld@...hat.com>
> > > > Tested-by: Phil Auld <pauld@...hat.com>
> > > > Link: https://lkml.kernel.org/r/20200410225208.109717-3-joshdon@google.com
> > > > ---
> > > > kernel/sched/fair.c | 13 +------------
> > > > kernel/sched/sched.h | 1 -
> > > > 2 files changed, 1 insertion(+), 13 deletions(-)
> > > >
> > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > > index 0c13a41..3d6ce75 100644
> > > > --- a/kernel/sched/fair.c
> > > > +++ b/kernel/sched/fair.c
> > > > @@ -4931,14 +4931,12 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
> > > > /*
> > > > * This check is repeated as we release cfs_b->lock while we unthrottle.
> > > > */
> > > > - while (throttled && cfs_b->runtime > 0 && !cfs_b->distribute_running) {
> > > > - cfs_b->distribute_running = 1;
> > > > + while (throttled && cfs_b->runtime > 0) {
> > > > raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> > > > /* we can't nest cfs_b->lock while distributing bandwidth */
> > > > distribute_cfs_runtime(cfs_b);
> > > > raw_spin_lock_irqsave(&cfs_b->lock, flags);
> > > >
> > > > - cfs_b->distribute_running = 0;
> > > > throttled = !list_empty(&cfs_b->throttled_cfs_rq);
> > > > }
> > > >
> > > > @@ -5052,10 +5050,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> > > > /* confirm we're still not at a refresh boundary */
> > > > raw_spin_lock_irqsave(&cfs_b->lock, flags);
> > > > cfs_b->slack_started = false;
> > > > - if (cfs_b->distribute_running) {
> > > > - raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> > > > - return;
> > > > - }
> > > >
> > > > if (runtime_refresh_within(cfs_b, min_bandwidth_expiration)) {
> > > > raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> > > > @@ -5065,9 +5059,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> > > > if (cfs_b->quota != RUNTIME_INF && cfs_b->runtime > slice)
> > > > runtime = cfs_b->runtime;
> > > >
> > > > - if (runtime)
> > > > - cfs_b->distribute_running = 1;
> > > > -
> > > > raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> > > >
> > > > if (!runtime)
> > > > @@ -5076,7 +5067,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> > > > distribute_cfs_runtime(cfs_b);
> > > >
> > > > raw_spin_lock_irqsave(&cfs_b->lock, flags);
> > > > - cfs_b->distribute_running = 0;
> > > > raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> > > > }
> > >
> > > When I read the tip code, I found nothing between above lock/unlock.
> > > This commit removed distribute_running. Is there any reason to remain
> > > that lock/unlock there ? I feel that it is not necessary now, no ?
> > >
> >
> > Yeah, that looks pretty useless :)
> >
> > Do you want to spin up a patch?
>
> Thanks for your reply, I prepared a patch for that indeed.
>
> Attached below:
>
> From 9ce12d6ab5542041e5adab7b200874c789cfd6e6 Mon Sep 17 00:00:00 2001
> From: Tao Zhou <ouwen210@...mail.com>
> Date: Sat, 6 Jun 2020 23:08:58 +0800
> Subject: [PATCH] sched/fair: remove the residual cfs_b lock/unlock
>
> After commit ab93a4bc955b, cfs_b->distribute_running is not used and
> removed. The lock/unlock protecting it are not removed and remain in
> the code. One benefit of removing them is that it can elimite the code
> size a little.
>
> Fixes: ab93a4bc955b ("sched/fair: Remove distribute_running from CFS bandwidth")
> ---
> kernel/sched/fair.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 35f4cc024dcf..cc2e1e839e03 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5089,9 +5089,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> return;
>
> distribute_cfs_runtime(cfs_b);
> -
> - raw_spin_lock_irqsave(&cfs_b->lock, flags);
> - raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> }
>
> /*
> --
Thanks Tao.
Reviewed-by: Phil Auld <pauld@...hat.com>
>
> Thanks,
> Tao
>
> > Cheers,
> > Phil
> >
> >
> > > Thanks
> > >
> > > > @@ -5218,7 +5208,6 @@ void init_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
> > > > cfs_b->period_timer.function = sched_cfs_period_timer;
> > > > hrtimer_init(&cfs_b->slack_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > > > cfs_b->slack_timer.function = sched_cfs_slack_timer;
> > > > - cfs_b->distribute_running = 0;
> > > > cfs_b->slack_started = false;
> > > > }
> > > >
> > > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > > > index db3a576..7198683 100644
> > > > --- a/kernel/sched/sched.h
> > > > +++ b/kernel/sched/sched.h
> > > > @@ -349,7 +349,6 @@ struct cfs_bandwidth {
> > > >
> > > > u8 idle;
> > > > u8 period_active;
> > > > - u8 distribute_running;
> > > > u8 slack_started;
> > > > struct hrtimer period_timer;
> > > > struct hrtimer slack_timer;
> > >
> >
> > --
>
--
Powered by blists - more mailing lists