* [PATCH] sched/fair: Fix the wrong throttled clock time for cfs_rq_clock_task() @ 2016-05-10 13:03 Xunlei Pang 2016-05-10 18:19 ` bsegall 2016-06-03 10:47 ` [tip:sched/core] " tip-bot for Xunlei Pang 0 siblings, 2 replies; 6+ messages in thread From: Xunlei Pang @ 2016-05-10 13:03 UTC (permalink / raw) To: linux-kernel Cc: Peter Zijlstra, Juri Lelli, Ingo Molnar, Steven Rostedt, Xunlei Pang Two minor fixes for cfs_rq_clock_task(). 1) If cfs_rq is currently being throttled, we need to subtract the cfs throttled clock time. 2) Make "throttled_clock_task_time" update SMP unrelated. Now UP cases need it as well. Signed-off-by: Xunlei Pang <xlpang@redhat.com> --- kernel/sched/fair.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 1708729e..fb80a12 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -3655,7 +3655,7 @@ static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg) static inline u64 cfs_rq_clock_task(struct cfs_rq *cfs_rq) { if (unlikely(cfs_rq->throttle_count)) - return cfs_rq->throttled_clock_task; + return cfs_rq->throttled_clock_task - cfs_rq->throttled_clock_task_time; return rq_clock_task(rq_of(cfs_rq)) - cfs_rq->throttled_clock_task_time; } @@ -3793,13 +3793,11 @@ static int tg_unthrottle_up(struct task_group *tg, void *data) struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)]; cfs_rq->throttle_count--; -#ifdef CONFIG_SMP if (!cfs_rq->throttle_count) { /* adjust cfs_rq_clock_task() */ cfs_rq->throttled_clock_task_time += rq_clock_task(rq) - cfs_rq->throttled_clock_task; } -#endif return 0; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/fair: Fix the wrong throttled clock time for cfs_rq_clock_task() 2016-05-10 13:03 [PATCH] sched/fair: Fix the wrong throttled clock time for cfs_rq_clock_task() Xunlei Pang @ 2016-05-10 18:19 ` bsegall 2016-05-11 6:49 ` Peter Zijlstra 2016-06-03 10:47 ` [tip:sched/core] " tip-bot for Xunlei Pang 1 sibling, 1 reply; 6+ messages in thread From: bsegall @ 2016-05-10 18:19 UTC (permalink / raw) To: Xunlei Pang Cc: linux-kernel, Peter Zijlstra, Juri Lelli, Ingo Molnar, Steven Rostedt, pjt Xunlei Pang <xlpang@redhat.com> writes: > Two minor fixes for cfs_rq_clock_task(). > 1) If cfs_rq is currently being throttled, we need to subtract the cfs > throttled clock time. > > 2) Make "throttled_clock_task_time" update SMP unrelated. Now UP cases > need it as well. > > Signed-off-by: Xunlei Pang <xlpang@redhat.com> > --- > kernel/sched/fair.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 1708729e..fb80a12 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -3655,7 +3655,7 @@ static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg) > static inline u64 cfs_rq_clock_task(struct cfs_rq *cfs_rq) > { > if (unlikely(cfs_rq->throttle_count)) > - return cfs_rq->throttled_clock_task; > + return cfs_rq->throttled_clock_task - cfs_rq->throttled_clock_task_time; > > return rq_clock_task(rq_of(cfs_rq)) - cfs_rq->throttled_clock_task_time; > } > @@ -3793,13 +3793,11 @@ static int tg_unthrottle_up(struct task_group *tg, void *data) > struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)]; > > cfs_rq->throttle_count--; > -#ifdef CONFIG_SMP > if (!cfs_rq->throttle_count) { > /* adjust cfs_rq_clock_task() */ > cfs_rq->throttled_clock_task_time += rq_clock_task(rq) - > cfs_rq->throttled_clock_task; > } > -#endif > > return 0; > } [Cc: pjt@google.com] This looks reasonable to me (at least the first part; I'm not certain why the CONFIG_SMP ifdef was put in place). ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/fair: Fix the wrong throttled clock time for cfs_rq_clock_task() 2016-05-10 18:19 ` bsegall @ 2016-05-11 6:49 ` Peter Zijlstra 2016-05-12 3:36 ` Xunlei Pang 0 siblings, 1 reply; 6+ messages in thread From: Peter Zijlstra @ 2016-05-11 6:49 UTC (permalink / raw) To: bsegall Cc: Xunlei Pang, linux-kernel, Juri Lelli, Ingo Molnar, Steven Rostedt, pjt On Tue, May 10, 2016 at 11:19:44AM -0700, bsegall@google.com wrote: > Xunlei Pang <xlpang@redhat.com> writes: > > > Two minor fixes for cfs_rq_clock_task(). > > 1) If cfs_rq is currently being throttled, we need to subtract the cfs > > throttled clock time. > > > > 2) Make "throttled_clock_task_time" update SMP unrelated. Now UP cases > > need it as well. > > > > Signed-off-by: Xunlei Pang <xlpang@redhat.com> > > --- > > kernel/sched/fair.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > index 1708729e..fb80a12 100644 > > --- a/kernel/sched/fair.c > > +++ b/kernel/sched/fair.c > > @@ -3655,7 +3655,7 @@ static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg) > > static inline u64 cfs_rq_clock_task(struct cfs_rq *cfs_rq) > > { > > if (unlikely(cfs_rq->throttle_count)) > > - return cfs_rq->throttled_clock_task; > > + return cfs_rq->throttled_clock_task - cfs_rq->throttled_clock_task_time; > > > > return rq_clock_task(rq_of(cfs_rq)) - cfs_rq->throttled_clock_task_time; > > } The alternative is obviously to do the subtraction in tg_throttle_down(), were we set ->throttled_clock_task. > > @@ -3793,13 +3793,11 @@ static int tg_unthrottle_up(struct task_group *tg, void *data) > > struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)]; > > > > cfs_rq->throttle_count--; > > -#ifdef CONFIG_SMP > > if (!cfs_rq->throttle_count) { > > /* adjust cfs_rq_clock_task() */ > > cfs_rq->throttled_clock_task_time += rq_clock_task(rq) - > > cfs_rq->throttled_clock_task; > > } > > -#endif > > > > return 0; > > } > > [Cc: pjt@google.com] > > This looks reasonable to me (at least the first part; I'm not > certain why the CONFIG_SMP ifdef was put in place). 64660c864f46 ("sched: Prevent interactions with throttled entities") Introduced it, because at that time it was about updating shares, which is only present on SMP. Then: f1b17280efbd ("sched: Maintain runnable averages across throttled periods") Added the clock thing inside it, and: 82958366cfea ("sched: Replace update_shares weight distribution with per-entity computation") took out the shares update and left the clock update, resulting in the current code. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/fair: Fix the wrong throttled clock time for cfs_rq_clock_task() 2016-05-11 6:49 ` Peter Zijlstra @ 2016-05-12 3:36 ` Xunlei Pang 2016-05-30 9:37 ` Xunlei Pang 0 siblings, 1 reply; 6+ messages in thread From: Xunlei Pang @ 2016-05-12 3:36 UTC (permalink / raw) To: Peter Zijlstra, bsegall Cc: Xunlei Pang, linux-kernel, Juri Lelli, Ingo Molnar, Steven Rostedt, pjt On 2016/05/11 at 14:49, Peter Zijlstra wrote: > On Tue, May 10, 2016 at 11:19:44AM -0700, bsegall@google.com wrote: >> Xunlei Pang <xlpang@redhat.com> writes: >> >>> Two minor fixes for cfs_rq_clock_task(). >>> 1) If cfs_rq is currently being throttled, we need to subtract the cfs >>> throttled clock time. >>> >>> 2) Make "throttled_clock_task_time" update SMP unrelated. Now UP cases >>> need it as well. >>> >>> Signed-off-by: Xunlei Pang <xlpang@redhat.com> >>> --- >>> kernel/sched/fair.c | 4 +--- >>> 1 file changed, 1 insertion(+), 3 deletions(-) >>> >>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >>> index 1708729e..fb80a12 100644 >>> --- a/kernel/sched/fair.c >>> +++ b/kernel/sched/fair.c >>> @@ -3655,7 +3655,7 @@ static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg) >>> static inline u64 cfs_rq_clock_task(struct cfs_rq *cfs_rq) >>> { >>> if (unlikely(cfs_rq->throttle_count)) >>> - return cfs_rq->throttled_clock_task; >>> + return cfs_rq->throttled_clock_task - cfs_rq->throttled_clock_task_time; >>> >>> return rq_clock_task(rq_of(cfs_rq)) - cfs_rq->throttled_clock_task_time; >>> } > The alternative is obviously to do the subtraction in > tg_throttle_down(), were we set ->throttled_clock_task. It is possible, but throttled_clock_task is a timestamp, I think doing it here is semantically better. > >>> @@ -3793,13 +3793,11 @@ static int tg_unthrottle_up(struct task_group *tg, void *data) >>> struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)]; >>> >>> cfs_rq->throttle_count--; >>> -#ifdef CONFIG_SMP >>> if (!cfs_rq->throttle_count) { >>> /* adjust cfs_rq_clock_task() */ >>> cfs_rq->throttled_clock_task_time += rq_clock_task(rq) - >>> cfs_rq->throttled_clock_task; >>> } >>> -#endif >>> >>> return 0; >>> } >> [Cc: pjt@google.com] >> >> This looks reasonable to me (at least the first part; I'm not >> certain why the CONFIG_SMP ifdef was put in place). > 64660c864f46 ("sched: Prevent interactions with throttled entities") > > Introduced it, because at that time it was about updating shares, which > is only present on SMP. Then: > > f1b17280efbd ("sched: Maintain runnable averages across throttled periods") > > Added the clock thing inside it, and: > > 82958366cfea ("sched: Replace update_shares weight distribution with per-entity computation") > > took out the shares update and left the clock update, resulting in the > current code. > > Thanks, Xunlei ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/fair: Fix the wrong throttled clock time for cfs_rq_clock_task() 2016-05-12 3:36 ` Xunlei Pang @ 2016-05-30 9:37 ` Xunlei Pang 0 siblings, 0 replies; 6+ messages in thread From: Xunlei Pang @ 2016-05-30 9:37 UTC (permalink / raw) To: Peter Zijlstra, bsegall Cc: Xunlei Pang, linux-kernel, Juri Lelli, Ingo Molnar, Steven Rostedt, pjt Ping On 2016/05/12 at 11:36, Xunlei Pang wrote: > On 2016/05/11 at 14:49, Peter Zijlstra wrote: >> On Tue, May 10, 2016 at 11:19:44AM -0700, bsegall@google.com wrote: >>> Xunlei Pang <xlpang@redhat.com> writes: >>> >>>> Two minor fixes for cfs_rq_clock_task(). >>>> 1) If cfs_rq is currently being throttled, we need to subtract the cfs >>>> throttled clock time. >>>> >>>> 2) Make "throttled_clock_task_time" update SMP unrelated. Now UP cases >>>> need it as well. >>>> >>>> Signed-off-by: Xunlei Pang <xlpang@redhat.com> >>>> --- >>>> kernel/sched/fair.c | 4 +--- >>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>> >>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >>>> index 1708729e..fb80a12 100644 >>>> --- a/kernel/sched/fair.c >>>> +++ b/kernel/sched/fair.c >>>> @@ -3655,7 +3655,7 @@ static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg) >>>> static inline u64 cfs_rq_clock_task(struct cfs_rq *cfs_rq) >>>> { >>>> if (unlikely(cfs_rq->throttle_count)) >>>> - return cfs_rq->throttled_clock_task; >>>> + return cfs_rq->throttled_clock_task - cfs_rq->throttled_clock_task_time; >>>> >>>> return rq_clock_task(rq_of(cfs_rq)) - cfs_rq->throttled_clock_task_time; >>>> } >> The alternative is obviously to do the subtraction in >> tg_throttle_down(), were we set ->throttled_clock_task. > It is possible, but throttled_clock_task is a timestamp, I think doing it here is semantically better. > >>>> @@ -3793,13 +3793,11 @@ static int tg_unthrottle_up(struct task_group *tg, void *data) >>>> struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)]; >>>> >>>> cfs_rq->throttle_count--; >>>> -#ifdef CONFIG_SMP >>>> if (!cfs_rq->throttle_count) { >>>> /* adjust cfs_rq_clock_task() */ >>>> cfs_rq->throttled_clock_task_time += rq_clock_task(rq) - >>>> cfs_rq->throttled_clock_task; >>>> } >>>> -#endif >>>> >>>> return 0; >>>> } >>> [Cc: pjt@google.com] >>> >>> This looks reasonable to me (at least the first part; I'm not >>> certain why the CONFIG_SMP ifdef was put in place). >> 64660c864f46 ("sched: Prevent interactions with throttled entities") >> >> Introduced it, because at that time it was about updating shares, which >> is only present on SMP. Then: >> >> f1b17280efbd ("sched: Maintain runnable averages across throttled periods") >> >> Added the clock thing inside it, and: >> >> 82958366cfea ("sched: Replace update_shares weight distribution with per-entity computation") >> >> took out the shares update and left the clock update, resulting in the >> current code. >> >> > Thanks, > Xunlei > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:sched/core] sched/fair: Fix the wrong throttled clock time for cfs_rq_clock_task() 2016-05-10 13:03 [PATCH] sched/fair: Fix the wrong throttled clock time for cfs_rq_clock_task() Xunlei Pang 2016-05-10 18:19 ` bsegall @ 2016-06-03 10:47 ` tip-bot for Xunlei Pang 1 sibling, 0 replies; 6+ messages in thread From: tip-bot for Xunlei Pang @ 2016-06-03 10:47 UTC (permalink / raw) To: linux-tip-commits Cc: peterz, rostedt, mingo, torvalds, hpa, linux-kernel, tglx, xlpang, juri.lelli, efault Commit-ID: 1a99ae3f00d3c7c7885ee529ac9a874b19caa0cf Gitweb: http://git.kernel.org/tip/1a99ae3f00d3c7c7885ee529ac9a874b19caa0cf Author: Xunlei Pang <xlpang@redhat.com> AuthorDate: Tue, 10 May 2016 21:03:18 +0800 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Fri, 3 Jun 2016 09:18:56 +0200 sched/fair: Fix the wrong throttled clock time for cfs_rq_clock_task() Two minor fixes for cfs_rq_clock_task(): 1) If cfs_rq is currently being throttled, we need to subtract the cfs throttled clock time. 2) Make "throttled_clock_task_time" update SMP unrelated. Now UP cases need it as well. Signed-off-by: Xunlei Pang <xlpang@redhat.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Juri Lelli <juri.lelli@arm.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Mike Galbraith <efault@gmx.de> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/1462885398-14724-1-git-send-email-xlpang@redhat.com Signed-off-by: Ingo Molnar <mingo@kernel.org> --- kernel/sched/fair.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 218f8e8..1e87bb6 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -3688,7 +3688,7 @@ static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg) static inline u64 cfs_rq_clock_task(struct cfs_rq *cfs_rq) { if (unlikely(cfs_rq->throttle_count)) - return cfs_rq->throttled_clock_task; + return cfs_rq->throttled_clock_task - cfs_rq->throttled_clock_task_time; return rq_clock_task(rq_of(cfs_rq)) - cfs_rq->throttled_clock_task_time; } @@ -3826,13 +3826,11 @@ static int tg_unthrottle_up(struct task_group *tg, void *data) struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)]; cfs_rq->throttle_count--; -#ifdef CONFIG_SMP if (!cfs_rq->throttle_count) { /* adjust cfs_rq_clock_task() */ cfs_rq->throttled_clock_task_time += rq_clock_task(rq) - cfs_rq->throttled_clock_task; } -#endif return 0; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-03 10:48 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-05-10 13:03 [PATCH] sched/fair: Fix the wrong throttled clock time for cfs_rq_clock_task() Xunlei Pang 2016-05-10 18:19 ` bsegall 2016-05-11 6:49 ` Peter Zijlstra 2016-05-12 3:36 ` Xunlei Pang 2016-05-30 9:37 ` Xunlei Pang 2016-06-03 10:47 ` [tip:sched/core] " tip-bot for Xunlei Pang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox