Re: [PATCH v2] sched/rt: fix call to cpufreq_update_util

[Vincent Guittot]

Hi Peter,

On 17 May 2018 at 22:12, Vincent Guittot <vincent.guittot@xxxxxxxxxx> 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
b29bbbbff0d6 ('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(&rq->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.
>
>
>
>