This is mostly a revert of baa9be4ff.
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 <[email protected]>
---
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 3fb986c52825..24b5e12efb40 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4930,14 +4930,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);
}
@@ -5051,10 +5049,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);
@@ -5064,9 +5058,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)
@@ -5075,7 +5066,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);
}
@@ -5217,7 +5207,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 db3a57675ccf..7198683b2869 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;
--
2.26.0.110.g2183baf09c-goog
On Fri, Apr 10, 2020 at 3:52 PM Josh Don <[email protected]> wrote:
>
> This is mostly a revert of baa9be4ff.
>
> 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 <[email protected]>
> ---
> 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 3fb986c52825..24b5e12efb40 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4930,14 +4930,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);
> }
>
> @@ -5051,10 +5049,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);
> @@ -5064,9 +5058,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)
> @@ -5075,7 +5066,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);
> }
>
> @@ -5217,7 +5207,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 db3a57675ccf..7198683b2869 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;
> --
> 2.26.0.110.g2183baf09c-goog
>
[email protected]
Hi Josh,
On Sat, Apr 11, 2020 at 07:01:45PM -0700 Josh Don wrote:
> On Fri, Apr 10, 2020 at 3:52 PM Josh Don <[email protected]> wrote:
> >
> > This is mostly a revert of baa9be4ff.
> >
> > 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 <[email protected]>
Nice. This was more or less a hack. It's good to be able to remove it.
I re-ran my test cases that originally caused the need for the
distribute_running. As expected, since there is no adding to the head
of the queue, the behavior is the same as before this series.
Reviewed-by: Phil Auld <[email protected]>
Tested-by: Phil Auld <[email protected]>
Cheers,
Phil
> > ---
> > 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 3fb986c52825..24b5e12efb40 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4930,14 +4930,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);
> > }
> >
> > @@ -5051,10 +5049,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);
> > @@ -5064,9 +5058,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)
> > @@ -5075,7 +5066,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);
> > }
> >
> > @@ -5217,7 +5207,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 db3a57675ccf..7198683b2869 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;
> > --
> > 2.26.0.110.g2183baf09c-goog
> >
>
> [email protected]
>
--
On Fri, Apr 10, 2020 at 03:52:08PM -0700, Josh Don wrote:
> This is mostly a revert of baa9be4ff.
That's written like:
"This is mostly a revert of commit:
baa9be4ffb55 ("sched/fair: Fix throttle_list starvation with low CFS quota")"
Fixed that for you.
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 <[email protected]>
AuthorDate: Fri, 10 Apr 2020 15:52:08 -07:00
Committer: Peter Zijlstra <[email protected]>
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 <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Phil Auld <[email protected]>
Tested-by: Phil Auld <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
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);
}
@@ -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;
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 <[email protected]>
> > AuthorDate: Fri, 10 Apr 2020 15:52:08 -07:00
> > Committer: Peter Zijlstra <[email protected]>
> > 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 <[email protected]>
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > Reviewed-by: Phil Auld <[email protected]>
> > Tested-by: Phil Auld <[email protected]>
> > Link: https://lkml.kernel.org/r/[email protected]
> > ---
> > 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?
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;
>
--
Hi Tao,
On Mon, Jun 8, 2020 at 4:01 PM Tao Zhou <[email protected]> wrote:
> 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, I missed the now-useless lock acquire in my revert.
s/elimite/eliminate
Reviewed-by: Josh Don <[email protected]>
Best,
Josh
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 <[email protected]>
> > > > AuthorDate: Fri, 10 Apr 2020 15:52:08 -07:00
> > > > Committer: Peter Zijlstra <[email protected]>
> > > > 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 <[email protected]>
> > > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > > > Reviewed-by: Phil Auld <[email protected]>
> > > > Tested-by: Phil Auld <[email protected]>
> > > > Link: https://lkml.kernel.org/r/[email protected]
> > > > ---
> > > > 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 <[email protected]>
> 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 <[email protected]>
>
> 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;
> > >
> >
> > --
>
--