[PATCH v2] sched/rt: fix call to cpufreq_update_util
With commit 8f111bc357aa ("cpufreq/schedutil: Rewrite CPUFREQ_RT support")
schedutil governor uses rq->rt.rt_nr_running to detect whether a RT task is
currently running on the CPU and to set frequency to max if necessary.
cpufreq_update_util() is called in enqueue/dequeue_top_rt_rq() but
rq->rt.rt_nr_running has not been updated yet when dequeue_top_rt_rq() is
called so schedutil still considers that a RT task is running when the
last task is dequeued. The update of rq->rt.rt_nr_running happens later
in dequeue_rt_stack().
In fact, we can take advantage of the sequence that the dequeue then
re-enqueue rt entities when a rt task is enqueued or dequeued;
As a result enqueue_top_rt_rq() is always called when a task is
enqueued or dequeued and also when groups are throttled or unthrottled.
The only place that not use enqueue_top_rt_rq() is when root rt_rq is
throttled.
Fixes: 8f111bc357aa ('cpufreq/schedutil: Rewrite CPUFREQ_RT support')
Cc: # v4.16+
Signed-off-by: Vincent Guittot
---
kernel/sched/cpufreq_schedutil.c | 2 +-
kernel/sched/rt.c| 16 ++--
kernel/sched/sched.h | 5 +
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 3cde464..c907fde 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -192,7 +192,7 @@ static unsigned long sugov_aggregate_util(struct sugov_cpu
*sg_cpu)
{
struct rq *rq = cpu_rq(sg_cpu->cpu);
- if (rq->rt.rt_nr_running)
+ if (rt_rq_is_runnable(>rt))
return sg_cpu->max;
/*
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 47556b0..5725670 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -508,8 +508,11 @@ static void sched_rt_rq_dequeue(struct rt_rq *rt_rq)
rt_se = rt_rq->tg->rt_se[cpu];
- if (!rt_se)
+ if (!rt_se) {
dequeue_top_rt_rq(rt_rq);
+ /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
+ cpufreq_update_util(rq_of_rt_rq(rt_rq), 0);
+ }
else if (on_rt_rq(rt_se))
dequeue_rt_entity(rt_se, 0);
}
@@ -1001,8 +1004,6 @@ dequeue_top_rt_rq(struct rt_rq *rt_rq)
sub_nr_running(rq, rt_rq->rt_nr_running);
rt_rq->rt_queued = 0;
- /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
- cpufreq_update_util(rq, 0);
}
static void
@@ -1014,11 +1015,14 @@ enqueue_top_rt_rq(struct rt_rq *rt_rq)
if (rt_rq->rt_queued)
return;
- if (rt_rq_throttled(rt_rq) || !rt_rq->rt_nr_running)
+
+ if (rt_rq_throttled(rt_rq))
return;
- add_nr_running(rq, rt_rq->rt_nr_running);
- rt_rq->rt_queued = 1;
+ if (rt_rq->rt_nr_running) {
+ add_nr_running(rq, rt_rq->rt_nr_running);
+ rt_rq->rt_queued = 1;
+ }
/* Kick cpufreq (see the comment in kernel/sched/sched.h). */
cpufreq_update_util(rq, 0);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 6601baf..27ddec3 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -609,6 +609,11 @@ struct rt_rq {
#endif
};
+static inline bool rt_rq_is_runnable(struct rt_rq *rt_rq)
+{
+ return rt_rq->rt_queued && rt_rq->rt_nr_running;
+}
+
/* Deadline class' related fields in a runqueue */
struct dl_rq {
/* runqueue is an rbtree, ordered by deadline */
--
2.7.4
[PATCH v2] sched/rt: fix call to cpufreq_update_util
With commit 8f111bc357aa ("cpufreq/schedutil: Rewrite CPUFREQ_RT support")
schedutil governor uses rq->rt.rt_nr_running to detect whether a RT task is
currently running on the CPU and to set frequency to max if necessary.
cpufreq_update_util() is called in enqueue/dequeue_top_rt_rq() but
rq->rt.rt_nr_running has not been updated yet when dequeue_top_rt_rq() is
called so schedutil still considers that a RT task is running when the
last task is dequeued. The update of rq->rt.rt_nr_running happens later
in dequeue_rt_stack().
In fact, we can take advantage of the sequence that the dequeue then
re-enqueue rt entities when a rt task is enqueued or dequeued;
As a result enqueue_top_rt_rq() is always called when a task is
enqueued or dequeued and also when groups are throttled or unthrottled.
The only place that not use enqueue_top_rt_rq() is when root rt_rq is
throttled.
Fixes: 8f111bc357aa ('cpufreq/schedutil: Rewrite CPUFREQ_RT support')
Cc: # v4.16+
Signed-off-by: Vincent Guittot
---
kernel/sched/cpufreq_schedutil.c | 2 +-
kernel/sched/rt.c| 16 ++--
kernel/sched/sched.h | 5 +
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 3cde464..c907fde 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -192,7 +192,7 @@ static unsigned long sugov_aggregate_util(struct sugov_cpu
*sg_cpu)
{
struct rq *rq = cpu_rq(sg_cpu->cpu);
- if (rq->rt.rt_nr_running)
+ if (rt_rq_is_runnable(>rt))
return sg_cpu->max;
/*
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 47556b0..5725670 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -508,8 +508,11 @@ static void sched_rt_rq_dequeue(struct rt_rq *rt_rq)
rt_se = rt_rq->tg->rt_se[cpu];
- if (!rt_se)
+ if (!rt_se) {
dequeue_top_rt_rq(rt_rq);
+ /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
+ cpufreq_update_util(rq_of_rt_rq(rt_rq), 0);
+ }
else if (on_rt_rq(rt_se))
dequeue_rt_entity(rt_se, 0);
}
@@ -1001,8 +1004,6 @@ dequeue_top_rt_rq(struct rt_rq *rt_rq)
sub_nr_running(rq, rt_rq->rt_nr_running);
rt_rq->rt_queued = 0;
- /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
- cpufreq_update_util(rq, 0);
}
static void
@@ -1014,11 +1015,14 @@ enqueue_top_rt_rq(struct rt_rq *rt_rq)
if (rt_rq->rt_queued)
return;
- if (rt_rq_throttled(rt_rq) || !rt_rq->rt_nr_running)
+
+ if (rt_rq_throttled(rt_rq))
return;
- add_nr_running(rq, rt_rq->rt_nr_running);
- rt_rq->rt_queued = 1;
+ if (rt_rq->rt_nr_running) {
+ add_nr_running(rq, rt_rq->rt_nr_running);
+ rt_rq->rt_queued = 1;
+ }
/* Kick cpufreq (see the comment in kernel/sched/sched.h). */
cpufreq_update_util(rq, 0);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 6601baf..27ddec3 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -609,6 +609,11 @@ struct rt_rq {
#endif
};
+static inline bool rt_rq_is_runnable(struct rt_rq *rt_rq)
+{
+ return rt_rq->rt_queued && rt_rq->rt_nr_running;
+}
+
/* Deadline class' related fields in a runqueue */
struct dl_rq {
/* runqueue is an rbtree, ordered by deadline */
--
2.7.4
Re: [PATCH v2] sched/rt: fix call to cpufreq_update_util
Hi Peter,
On 17 May 2018 at 22:12, Vincent Guittot wrote:
> Le Thursday 17 May 2018 à 11:32:06 (+0200), Peter Zijlstra a écrit :
>> On Thu, May 17, 2018 at 11:05:30AM +0200, Peter Zijlstra wrote:
>> > Hurm.. I think this is also wrong. See how dequeue_rt_stack() is also
>> > called from the enqueue path. Also, nothing calls cpufreq_update_util()
>> > on the throttle path now.
>
> Yes I missed the point that rt_entities were dequeued then re-enqueued when
> a task was either enqueued or dequeued.
>
> That's also means that enqueue_top_rt_rq() is always called when a task is
> enqueued or dequeued and also when groups are throttled or unthrottled
> In fact, the only point where it's not called, is when root rt_rq is
> throttled. So I have prepared the patch below which call cpufreq_update util
> in enqueue_top_rt_rq() and also when we throttle the root rt_rq.
> According to the tests I have done , it seems to work for enqueue/dequeue and
> throttle/unthrottle use cases.
>
> I have re-used your rt_rq_is_runnable() which takes into account rt_queued
What is the status for this problem ?
The patch below conflict with
b29ff0d6 ('sched/cpufreq: always consider blocked FAIR utilization')
that has been merged in tip/sched/core
Do you want me to rebase it ?
Regards,
Vincent
>
> ---
> kernel/sched/cpufreq_schedutil.c | 2 +-
> kernel/sched/rt.c| 16 ++--
> kernel/sched/sched.h | 5 +
> 3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c
> b/kernel/sched/cpufreq_schedutil.c
> index e13df95..1751f96 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -185,7 +185,7 @@ static unsigned long sugov_aggregate_util(struct
> sugov_cpu *sg_cpu)
> struct rq *rq = cpu_rq(sg_cpu->cpu);
> unsigned long util;
>
> - if (rq->rt.rt_nr_running) {
> + if (rt_rq_is_runnable(>rt)) {
> util = sg_cpu->max;
> } else {
> util = sg_cpu->util_dl;
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 7aef6b4..d6b9517 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -508,8 +508,11 @@ static void sched_rt_rq_dequeue(struct rt_rq *rt_rq)
>
> rt_se = rt_rq->tg->rt_se[cpu];
>
> - if (!rt_se)
> + if (!rt_se) {
> dequeue_top_rt_rq(rt_rq);
> + /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
> + cpufreq_update_util(rq_of_rt_rq(rt_rq), 0);
> + }
> else if (on_rt_rq(rt_se))
> dequeue_rt_entity(rt_se, 0);
> }
> @@ -1001,8 +1004,6 @@ dequeue_top_rt_rq(struct rt_rq *rt_rq)
> sub_nr_running(rq, rt_rq->rt_nr_running);
> rt_rq->rt_queued = 0;
>
> - /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
> - cpufreq_update_util(rq, 0);
> }
>
> static void
> @@ -1014,11 +1015,14 @@ enqueue_top_rt_rq(struct rt_rq *rt_rq)
>
> if (rt_rq->rt_queued)
> return;
> - if (rt_rq_throttled(rt_rq) || !rt_rq->rt_nr_running)
> +
> + if (rt_rq_throttled(rt_rq))
> return;
>
> - add_nr_running(rq, rt_rq->rt_nr_running);
> - rt_rq->rt_queued = 1;
> + if (rt_rq->rt_nr_running) {
> + add_nr_running(rq, rt_rq->rt_nr_running);
> + rt_rq->rt_queued = 1;
> + }
>
> /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
> cpufreq_update_util(rq, 0);
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index c9895d3..bbebcfe 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -609,6 +609,11 @@ struct rt_rq {
> #endif
> };
>
> +static inline bool rt_rq_is_runnable(struct rt_rq *rt_rq)
> +{
> + return rt_rq->rt_queued && rt_rq->rt_nr_running;
> +}
> +
> /* Deadline class' related fields in a runqueue */
> struct dl_rq {
> /* runqueue is an rbtree, ordered by deadline */
> --
> 2.7.4
>
>
>
>
>
>> >
>> > And talking about throttle; I think we wanted to check rt_queued in that
>> > case.
>>
>> Bah, clearly you also want to update on unthrottle.. but I think there's
>> more buggered wrt throtteling.
>
>
>
>
Re: [PATCH v2] sched/rt: fix call to cpufreq_update_util
Hi Peter,
On 17 May 2018 at 22:12, Vincent Guittot wrote:
> Le Thursday 17 May 2018 à 11:32:06 (+0200), Peter Zijlstra a écrit :
>> On Thu, May 17, 2018 at 11:05:30AM +0200, Peter Zijlstra wrote:
>> > Hurm.. I think this is also wrong. See how dequeue_rt_stack() is also
>> > called from the enqueue path. Also, nothing calls cpufreq_update_util()
>> > on the throttle path now.
>
> Yes I missed the point that rt_entities were dequeued then re-enqueued when
> a task was either enqueued or dequeued.
>
> That's also means that enqueue_top_rt_rq() is always called when a task is
> enqueued or dequeued and also when groups are throttled or unthrottled
> In fact, the only point where it's not called, is when root rt_rq is
> throttled. So I have prepared the patch below which call cpufreq_update util
> in enqueue_top_rt_rq() and also when we throttle the root rt_rq.
> According to the tests I have done , it seems to work for enqueue/dequeue and
> throttle/unthrottle use cases.
>
> I have re-used your rt_rq_is_runnable() which takes into account rt_queued
What is the status for this problem ?
The patch below conflict with
b29ff0d6 ('sched/cpufreq: always consider blocked FAIR utilization')
that has been merged in tip/sched/core
Do you want me to rebase it ?
Regards,
Vincent
>
> ---
> kernel/sched/cpufreq_schedutil.c | 2 +-
> kernel/sched/rt.c| 16 ++--
> kernel/sched/sched.h | 5 +
> 3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c
> b/kernel/sched/cpufreq_schedutil.c
> index e13df95..1751f96 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -185,7 +185,7 @@ static unsigned long sugov_aggregate_util(struct
> sugov_cpu *sg_cpu)
> struct rq *rq = cpu_rq(sg_cpu->cpu);
> unsigned long util;
>
> - if (rq->rt.rt_nr_running) {
> + if (rt_rq_is_runnable(>rt)) {
> util = sg_cpu->max;
> } else {
> util = sg_cpu->util_dl;
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 7aef6b4..d6b9517 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -508,8 +508,11 @@ static void sched_rt_rq_dequeue(struct rt_rq *rt_rq)
>
> rt_se = rt_rq->tg->rt_se[cpu];
>
> - if (!rt_se)
> + if (!rt_se) {
> dequeue_top_rt_rq(rt_rq);
> + /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
> + cpufreq_update_util(rq_of_rt_rq(rt_rq), 0);
> + }
> else if (on_rt_rq(rt_se))
> dequeue_rt_entity(rt_se, 0);
> }
> @@ -1001,8 +1004,6 @@ dequeue_top_rt_rq(struct rt_rq *rt_rq)
> sub_nr_running(rq, rt_rq->rt_nr_running);
> rt_rq->rt_queued = 0;
>
> - /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
> - cpufreq_update_util(rq, 0);
> }
>
> static void
> @@ -1014,11 +1015,14 @@ enqueue_top_rt_rq(struct rt_rq *rt_rq)
>
> if (rt_rq->rt_queued)
> return;
> - if (rt_rq_throttled(rt_rq) || !rt_rq->rt_nr_running)
> +
> + if (rt_rq_throttled(rt_rq))
> return;
>
> - add_nr_running(rq, rt_rq->rt_nr_running);
> - rt_rq->rt_queued = 1;
> + if (rt_rq->rt_nr_running) {
> + add_nr_running(rq, rt_rq->rt_nr_running);
> + rt_rq->rt_queued = 1;
> + }
>
> /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
> cpufreq_update_util(rq, 0);
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index c9895d3..bbebcfe 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -609,6 +609,11 @@ struct rt_rq {
> #endif
> };
>
> +static inline bool rt_rq_is_runnable(struct rt_rq *rt_rq)
> +{
> + return rt_rq->rt_queued && rt_rq->rt_nr_running;
> +}
> +
> /* Deadline class' related fields in a runqueue */
> struct dl_rq {
> /* runqueue is an rbtree, ordered by deadline */
> --
> 2.7.4
>
>
>
>
>
>> >
>> > And talking about throttle; I think we wanted to check rt_queued in that
>> > case.
>>
>> Bah, clearly you also want to update on unthrottle.. but I think there's
>> more buggered wrt throtteling.
>
>
>
>
Re: [PATCH v2] sched/rt: fix call to cpufreq_update_util
Le Thursday 17 May 2018 à 11:32:06 (+0200), Peter Zijlstra a écrit :
> On Thu, May 17, 2018 at 11:05:30AM +0200, Peter Zijlstra wrote:
> > Hurm.. I think this is also wrong. See how dequeue_rt_stack() is also
> > called from the enqueue path. Also, nothing calls cpufreq_update_util()
> > on the throttle path now.
Yes I missed the point that rt_entities were dequeued then re-enqueued when
a task was either enqueued or dequeued.
That's also means that enqueue_top_rt_rq() is always called when a task is
enqueued or dequeued and also when groups are throttled or unthrottled
In fact, the only point where it's not called, is when root rt_rq is
throttled. So I have prepared the patch below which call cpufreq_update util
in enqueue_top_rt_rq() and also when we throttle the root rt_rq.
According to the tests I have done , it seems to work for enqueue/dequeue and
throttle/unthrottle use cases.
I have re-used your rt_rq_is_runnable() which takes into account rt_queued
---
kernel/sched/cpufreq_schedutil.c | 2 +-
kernel/sched/rt.c| 16 ++--
kernel/sched/sched.h | 5 +
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index e13df95..1751f96 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -185,7 +185,7 @@ static unsigned long sugov_aggregate_util(struct sugov_cpu
*sg_cpu)
struct rq *rq = cpu_rq(sg_cpu->cpu);
unsigned long util;
- if (rq->rt.rt_nr_running) {
+ if (rt_rq_is_runnable(>rt)) {
util = sg_cpu->max;
} else {
util = sg_cpu->util_dl;
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 7aef6b4..d6b9517 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -508,8 +508,11 @@ static void sched_rt_rq_dequeue(struct rt_rq *rt_rq)
rt_se = rt_rq->tg->rt_se[cpu];
- if (!rt_se)
+ if (!rt_se) {
dequeue_top_rt_rq(rt_rq);
+ /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
+ cpufreq_update_util(rq_of_rt_rq(rt_rq), 0);
+ }
else if (on_rt_rq(rt_se))
dequeue_rt_entity(rt_se, 0);
}
@@ -1001,8 +1004,6 @@ dequeue_top_rt_rq(struct rt_rq *rt_rq)
sub_nr_running(rq, rt_rq->rt_nr_running);
rt_rq->rt_queued = 0;
- /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
- cpufreq_update_util(rq, 0);
}
static void
@@ -1014,11 +1015,14 @@ enqueue_top_rt_rq(struct rt_rq *rt_rq)
if (rt_rq->rt_queued)
return;
- if (rt_rq_throttled(rt_rq) || !rt_rq->rt_nr_running)
+
+ if (rt_rq_throttled(rt_rq))
return;
- add_nr_running(rq, rt_rq->rt_nr_running);
- rt_rq->rt_queued = 1;
+ if (rt_rq->rt_nr_running) {
+ add_nr_running(rq, rt_rq->rt_nr_running);
+ rt_rq->rt_queued = 1;
+ }
/* Kick cpufreq (see the comment in kernel/sched/sched.h). */
cpufreq_update_util(rq, 0);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index c9895d3..bbebcfe 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -609,6 +609,11 @@ struct rt_rq {
#endif
};
+static inline bool rt_rq_is_runnable(struct rt_rq *rt_rq)
+{
+ return rt_rq->rt_queued && rt_rq->rt_nr_running;
+}
+
/* Deadline class' related fields in a runqueue */
struct dl_rq {
/* runqueue is an rbtree, ordered by deadline */
--
2.7.4
> >
> > And talking about throttle; I think we wanted to check rt_queued in that
> > case.
>
> Bah, clearly you also want to update on unthrottle.. but I think there's
> more buggered wrt throtteling.
Re: [PATCH v2] sched/rt: fix call to cpufreq_update_util
Le Thursday 17 May 2018 à 11:32:06 (+0200), Peter Zijlstra a écrit :
> On Thu, May 17, 2018 at 11:05:30AM +0200, Peter Zijlstra wrote:
> > Hurm.. I think this is also wrong. See how dequeue_rt_stack() is also
> > called from the enqueue path. Also, nothing calls cpufreq_update_util()
> > on the throttle path now.
Yes I missed the point that rt_entities were dequeued then re-enqueued when
a task was either enqueued or dequeued.
That's also means that enqueue_top_rt_rq() is always called when a task is
enqueued or dequeued and also when groups are throttled or unthrottled
In fact, the only point where it's not called, is when root rt_rq is
throttled. So I have prepared the patch below which call cpufreq_update util
in enqueue_top_rt_rq() and also when we throttle the root rt_rq.
According to the tests I have done , it seems to work for enqueue/dequeue and
throttle/unthrottle use cases.
I have re-used your rt_rq_is_runnable() which takes into account rt_queued
---
kernel/sched/cpufreq_schedutil.c | 2 +-
kernel/sched/rt.c| 16 ++--
kernel/sched/sched.h | 5 +
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index e13df95..1751f96 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -185,7 +185,7 @@ static unsigned long sugov_aggregate_util(struct sugov_cpu
*sg_cpu)
struct rq *rq = cpu_rq(sg_cpu->cpu);
unsigned long util;
- if (rq->rt.rt_nr_running) {
+ if (rt_rq_is_runnable(>rt)) {
util = sg_cpu->max;
} else {
util = sg_cpu->util_dl;
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 7aef6b4..d6b9517 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -508,8 +508,11 @@ static void sched_rt_rq_dequeue(struct rt_rq *rt_rq)
rt_se = rt_rq->tg->rt_se[cpu];
- if (!rt_se)
+ if (!rt_se) {
dequeue_top_rt_rq(rt_rq);
+ /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
+ cpufreq_update_util(rq_of_rt_rq(rt_rq), 0);
+ }
else if (on_rt_rq(rt_se))
dequeue_rt_entity(rt_se, 0);
}
@@ -1001,8 +1004,6 @@ dequeue_top_rt_rq(struct rt_rq *rt_rq)
sub_nr_running(rq, rt_rq->rt_nr_running);
rt_rq->rt_queued = 0;
- /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
- cpufreq_update_util(rq, 0);
}
static void
@@ -1014,11 +1015,14 @@ enqueue_top_rt_rq(struct rt_rq *rt_rq)
if (rt_rq->rt_queued)
return;
- if (rt_rq_throttled(rt_rq) || !rt_rq->rt_nr_running)
+
+ if (rt_rq_throttled(rt_rq))
return;
- add_nr_running(rq, rt_rq->rt_nr_running);
- rt_rq->rt_queued = 1;
+ if (rt_rq->rt_nr_running) {
+ add_nr_running(rq, rt_rq->rt_nr_running);
+ rt_rq->rt_queued = 1;
+ }
/* Kick cpufreq (see the comment in kernel/sched/sched.h). */
cpufreq_update_util(rq, 0);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index c9895d3..bbebcfe 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -609,6 +609,11 @@ struct rt_rq {
#endif
};
+static inline bool rt_rq_is_runnable(struct rt_rq *rt_rq)
+{
+ return rt_rq->rt_queued && rt_rq->rt_nr_running;
+}
+
/* Deadline class' related fields in a runqueue */
struct dl_rq {
/* runqueue is an rbtree, ordered by deadline */
--
2.7.4
> >
> > And talking about throttle; I think we wanted to check rt_queued in that
> > case.
>
> Bah, clearly you also want to update on unthrottle.. but I think there's
> more buggered wrt throtteling.
Re: [PATCH v2] sched/rt: fix call to cpufreq_update_util
On Thu, May 17, 2018 at 11:05:30AM +0200, Peter Zijlstra wrote: > Hurm.. I think this is also wrong. See how dequeue_rt_stack() is also > called from the enqueue path. Also, nothing calls cpufreq_update_util() > on the throttle path now. > > And talking about throttle; I think we wanted to check rt_queued in that > case. Bah, clearly you also want to update on unthrottle.. but I think there's more buggered wrt throtteling.
Re: [PATCH v2] sched/rt: fix call to cpufreq_update_util
On Thu, May 17, 2018 at 11:05:30AM +0200, Peter Zijlstra wrote: > Hurm.. I think this is also wrong. See how dequeue_rt_stack() is also > called from the enqueue path. Also, nothing calls cpufreq_update_util() > on the throttle path now. > > And talking about throttle; I think we wanted to check rt_queued in that > case. Bah, clearly you also want to update on unthrottle.. but I think there's more buggered wrt throtteling.
Re: [PATCH v2] sched/rt: fix call to cpufreq_update_util
On Thu, May 17, 2018 at 09:16:43AM +0200, Vincent Guittot wrote:
> With commit
>
> 8f111bc357aa ("cpufreq/schedutil: Rewrite CPUFREQ_RT support")
>
> schedutil governor uses rq->rt.rt_nr_running to detect whether a RT task is
> currently running on the CPU and to set frequency to max if necessary.
>
> cpufreq_update_util() is called in enqueue/dequeue_top_rt_rq() but
> rq->rt.rt_nr_running as not been updated yet when dequeue_top_rt_rq() is
> called so schedutil still considers that a RT task is running when the
> last task is dequeued. The update of rq->rt.rt_nr_running happens later
> in dequeue_rt_stack()
>
> Fixes: 8f111bc357aa ('cpufreq/schedutil: Rewrite CPUFREQ_RT support')
> Cc: # v4.16+
> Signed-off-by: Vincent Guittot
> ---
> - v2:
> - remove blank line
>
> kernel/sched/rt.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 7aef6b4..a9f1119 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -1000,9 +1000,6 @@ dequeue_top_rt_rq(struct rt_rq *rt_rq)
>
> sub_nr_running(rq, rt_rq->rt_nr_running);
> rt_rq->rt_queued = 0;
> -
> - /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
> - cpufreq_update_util(rq, 0);
> }
>
> static void
> @@ -1288,6 +1285,9 @@ static void dequeue_rt_stack(struct sched_rt_entity
> *rt_se, unsigned int flags)
> if (on_rt_rq(rt_se))
> __dequeue_rt_entity(rt_se, flags);
> }
> +
> + /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
> + cpufreq_update_util(rq_of_rt_rq(rt_rq_of_se(back)), 0);
> }
Hurm.. I think this is also wrong. See how dequeue_rt_stack() is also
called from the enqueue path. Also, nothing calls cpufreq_update_util()
on the throttle path now.
And talking about throttle; I think we wanted to check rt_queued in that
case.
How about the below?
---
kernel/sched/cpufreq_schedutil.c | 2 +-
kernel/sched/rt.c| 24 ++--
kernel/sched/sched.h | 5 +
3 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index e13df951aca7..1751f9630e49 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -185,7 +185,7 @@ static unsigned long sugov_aggregate_util(struct sugov_cpu
*sg_cpu)
struct rq *rq = cpu_rq(sg_cpu->cpu);
unsigned long util;
- if (rq->rt.rt_nr_running) {
+ if (rt_rq_is_runnable(>rt)) {
util = sg_cpu->max;
} else {
util = sg_cpu->util_dl;
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 7aef6b4e885a..a1108a7da777 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -979,8 +979,14 @@ static void update_curr_rt(struct rq *rq)
if (sched_rt_runtime(rt_rq) != RUNTIME_INF) {
raw_spin_lock(_rq->rt_runtime_lock);
rt_rq->rt_time += delta_exec;
- if (sched_rt_runtime_exceeded(rt_rq))
+ if (sched_rt_runtime_exceeded(rt_rq)) {
+ struct rq *rq = rq_of_rt_rq(rt_rq);
+
+ if (>rt == rt_rq)
+ cpufreq_update_util(rq, 0);
+
resched_curr(rq);
+ }
raw_spin_unlock(_rq->rt_runtime_lock);
}
}
@@ -1000,9 +1006,6 @@ dequeue_top_rt_rq(struct rt_rq *rt_rq)
sub_nr_running(rq, rt_rq->rt_nr_running);
rt_rq->rt_queued = 0;
-
- /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
- cpufreq_update_util(rq, 0);
}
static void
@@ -1019,9 +1022,6 @@ enqueue_top_rt_rq(struct rt_rq *rt_rq)
add_nr_running(rq, rt_rq->rt_nr_running);
rt_rq->rt_queued = 1;
-
- /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
- cpufreq_update_util(rq, 0);
}
#if defined CONFIG_SMP
@@ -1330,6 +1330,8 @@ enqueue_task_rt(struct rq *rq, struct task_struct *p, int
flags)
if (!task_current(rq, p) && p->nr_cpus_allowed > 1)
enqueue_pushable_task(rq, p);
+
+ cpufreq_update_util(rq, 0);
}
static void dequeue_task_rt(struct rq *rq, struct task_struct *p, int flags)
@@ -1340,6 +1342,8 @@ static void dequeue_task_rt(struct rq *rq, struct
task_struct *p, int flags)
dequeue_rt_entity(rt_se, flags);
dequeue_pushable_task(rq, p);
+
+ cpufreq_update_util(rq, 0);
}
/*
@@ -1533,6 +1537,9 @@ pick_next_task_rt(struct rq *rq, struct task_struct
*prev, struct rq_flags *rf)
struct task_struct *p;
struct rt_rq *rt_rq = >rt;
+ if (!rt_rq->rt_queued)
+ return NULL;
+
if (need_pull_rt_task(rq, prev)) {
/*
* This is OK, because
Re: [PATCH v2] sched/rt: fix call to cpufreq_update_util
On Thu, May 17, 2018 at 09:16:43AM +0200, Vincent Guittot wrote:
> With commit
>
> 8f111bc357aa ("cpufreq/schedutil: Rewrite CPUFREQ_RT support")
>
> schedutil governor uses rq->rt.rt_nr_running to detect whether a RT task is
> currently running on the CPU and to set frequency to max if necessary.
>
> cpufreq_update_util() is called in enqueue/dequeue_top_rt_rq() but
> rq->rt.rt_nr_running as not been updated yet when dequeue_top_rt_rq() is
> called so schedutil still considers that a RT task is running when the
> last task is dequeued. The update of rq->rt.rt_nr_running happens later
> in dequeue_rt_stack()
>
> Fixes: 8f111bc357aa ('cpufreq/schedutil: Rewrite CPUFREQ_RT support')
> Cc: # v4.16+
> Signed-off-by: Vincent Guittot
> ---
> - v2:
> - remove blank line
>
> kernel/sched/rt.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 7aef6b4..a9f1119 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -1000,9 +1000,6 @@ dequeue_top_rt_rq(struct rt_rq *rt_rq)
>
> sub_nr_running(rq, rt_rq->rt_nr_running);
> rt_rq->rt_queued = 0;
> -
> - /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
> - cpufreq_update_util(rq, 0);
> }
>
> static void
> @@ -1288,6 +1285,9 @@ static void dequeue_rt_stack(struct sched_rt_entity
> *rt_se, unsigned int flags)
> if (on_rt_rq(rt_se))
> __dequeue_rt_entity(rt_se, flags);
> }
> +
> + /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
> + cpufreq_update_util(rq_of_rt_rq(rt_rq_of_se(back)), 0);
> }
Hurm.. I think this is also wrong. See how dequeue_rt_stack() is also
called from the enqueue path. Also, nothing calls cpufreq_update_util()
on the throttle path now.
And talking about throttle; I think we wanted to check rt_queued in that
case.
How about the below?
---
kernel/sched/cpufreq_schedutil.c | 2 +-
kernel/sched/rt.c| 24 ++--
kernel/sched/sched.h | 5 +
3 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index e13df951aca7..1751f9630e49 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -185,7 +185,7 @@ static unsigned long sugov_aggregate_util(struct sugov_cpu
*sg_cpu)
struct rq *rq = cpu_rq(sg_cpu->cpu);
unsigned long util;
- if (rq->rt.rt_nr_running) {
+ if (rt_rq_is_runnable(>rt)) {
util = sg_cpu->max;
} else {
util = sg_cpu->util_dl;
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 7aef6b4e885a..a1108a7da777 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -979,8 +979,14 @@ static void update_curr_rt(struct rq *rq)
if (sched_rt_runtime(rt_rq) != RUNTIME_INF) {
raw_spin_lock(_rq->rt_runtime_lock);
rt_rq->rt_time += delta_exec;
- if (sched_rt_runtime_exceeded(rt_rq))
+ if (sched_rt_runtime_exceeded(rt_rq)) {
+ struct rq *rq = rq_of_rt_rq(rt_rq);
+
+ if (>rt == rt_rq)
+ cpufreq_update_util(rq, 0);
+
resched_curr(rq);
+ }
raw_spin_unlock(_rq->rt_runtime_lock);
}
}
@@ -1000,9 +1006,6 @@ dequeue_top_rt_rq(struct rt_rq *rt_rq)
sub_nr_running(rq, rt_rq->rt_nr_running);
rt_rq->rt_queued = 0;
-
- /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
- cpufreq_update_util(rq, 0);
}
static void
@@ -1019,9 +1022,6 @@ enqueue_top_rt_rq(struct rt_rq *rt_rq)
add_nr_running(rq, rt_rq->rt_nr_running);
rt_rq->rt_queued = 1;
-
- /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
- cpufreq_update_util(rq, 0);
}
#if defined CONFIG_SMP
@@ -1330,6 +1330,8 @@ enqueue_task_rt(struct rq *rq, struct task_struct *p, int
flags)
if (!task_current(rq, p) && p->nr_cpus_allowed > 1)
enqueue_pushable_task(rq, p);
+
+ cpufreq_update_util(rq, 0);
}
static void dequeue_task_rt(struct rq *rq, struct task_struct *p, int flags)
@@ -1340,6 +1342,8 @@ static void dequeue_task_rt(struct rq *rq, struct
task_struct *p, int flags)
dequeue_rt_entity(rt_se, flags);
dequeue_pushable_task(rq, p);
+
+ cpufreq_update_util(rq, 0);
}
/*
@@ -1533,6 +1537,9 @@ pick_next_task_rt(struct rq *rq, struct task_struct
*prev, struct rq_flags *rf)
struct task_struct *p;
struct rt_rq *rt_rq = >rt;
+ if (!rt_rq->rt_queued)
+ return NULL;
+
if (need_pull_rt_task(rq, prev)) {
/*
* This is OK, because current is on_cpu, which avoids it being
@@ -1560,9
[PATCH v2] sched/rt: fix call to cpufreq_update_util
With commit 8f111bc357aa ("cpufreq/schedutil: Rewrite CPUFREQ_RT support")
schedutil governor uses rq->rt.rt_nr_running to detect whether a RT task is
currently running on the CPU and to set frequency to max if necessary.
cpufreq_update_util() is called in enqueue/dequeue_top_rt_rq() but
rq->rt.rt_nr_running as not been updated yet when dequeue_top_rt_rq() is
called so schedutil still considers that a RT task is running when the
last task is dequeued. The update of rq->rt.rt_nr_running happens later
in dequeue_rt_stack()
Fixes: 8f111bc357aa ('cpufreq/schedutil: Rewrite CPUFREQ_RT support')
Cc: # v4.16+
Signed-off-by: Vincent Guittot
---
- v2:
- remove blank line
kernel/sched/rt.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 7aef6b4..a9f1119 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1000,9 +1000,6 @@ dequeue_top_rt_rq(struct rt_rq *rt_rq)
sub_nr_running(rq, rt_rq->rt_nr_running);
rt_rq->rt_queued = 0;
-
- /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
- cpufreq_update_util(rq, 0);
}
static void
@@ -1288,6 +1285,9 @@ static void dequeue_rt_stack(struct sched_rt_entity
*rt_se, unsigned int flags)
if (on_rt_rq(rt_se))
__dequeue_rt_entity(rt_se, flags);
}
+
+ /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
+ cpufreq_update_util(rq_of_rt_rq(rt_rq_of_se(back)), 0);
}
static void enqueue_rt_entity(struct sched_rt_entity *rt_se, unsigned int
flags)
--
2.7.4
[PATCH v2] sched/rt: fix call to cpufreq_update_util
With commit 8f111bc357aa ("cpufreq/schedutil: Rewrite CPUFREQ_RT support")
schedutil governor uses rq->rt.rt_nr_running to detect whether a RT task is
currently running on the CPU and to set frequency to max if necessary.
cpufreq_update_util() is called in enqueue/dequeue_top_rt_rq() but
rq->rt.rt_nr_running as not been updated yet when dequeue_top_rt_rq() is
called so schedutil still considers that a RT task is running when the
last task is dequeued. The update of rq->rt.rt_nr_running happens later
in dequeue_rt_stack()
Fixes: 8f111bc357aa ('cpufreq/schedutil: Rewrite CPUFREQ_RT support')
Cc: # v4.16+
Signed-off-by: Vincent Guittot
---
- v2:
- remove blank line
kernel/sched/rt.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 7aef6b4..a9f1119 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1000,9 +1000,6 @@ dequeue_top_rt_rq(struct rt_rq *rt_rq)
sub_nr_running(rq, rt_rq->rt_nr_running);
rt_rq->rt_queued = 0;
-
- /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
- cpufreq_update_util(rq, 0);
}
static void
@@ -1288,6 +1285,9 @@ static void dequeue_rt_stack(struct sched_rt_entity
*rt_se, unsigned int flags)
if (on_rt_rq(rt_se))
__dequeue_rt_entity(rt_se, flags);
}
+
+ /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
+ cpufreq_update_util(rq_of_rt_rq(rt_rq_of_se(back)), 0);
}
static void enqueue_rt_entity(struct sched_rt_entity *rt_se, unsigned int
flags)
--
2.7.4
