Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220311161406.23497-1-vincent.guittot@linaro.org> <20220311161406.23497-6-vincent.guittot@linaro.org>
In-Reply-To: <20220311161406.23497-6-vincent.guittot@linaro.org>
From:   Josh Don <joshdon@google.com>
Date:   Mon, 14 Mar 2022 17:53:18 -0700
Message-ID: <CABk29Numf595wF6PiLd673TmFt6AV9qTEsWP7tSf2xPOKcGgLQ@mail.gmail.com>
Subject: Re: [RFC 5/6] sched/fair: Take into account latency nice at wakeup
To:     Vincent Guittot <vincent.guittot@linaro.org>
Cc:     Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Benjamin Segall <bsegall@google.com>,
        Mel Gorman <mgorman@suse.de>,
        linux-kernel <linux-kernel@vger.kernel.org>, parth@linux.ibm.com,
        Qais Yousef <qais.yousef@arm.com>,
        "Hyser,Chris" <chris.hyser@oracle.com>,
        Pavan Kondeti <pkondeti@codeaurora.org>,
        Valentin Schneider <valentin.schneider@arm.com>,
        patrick.bellasi@matbug.net, David.Laight@aculab.com,
        Paul Turner <pjt@google.com>, pavel@ucw.cz,
        Tejun Heo <tj@kernel.org>,
        Dhaval Giani <dhaval.giani@oracle.com>, qperret@google.com,
        Tim Chen <tim.c.chen@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Vincent,

On Fri, Mar 11, 2022 at 8:21 AM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
[snip]
> +
> +static void check_preempt_from_idle(struct cfs_rq *cfs, struct sched_entity *se)
> +{
> +       struct sched_entity *next;
> +
> +       if (se->latency_weight <= 0)
> +               return;
> +
> +       if (cfs->nr_running <= 1)
> +               return;

I don't quite understand this nr_running check.

> +       /*
> +        * When waking from idle, we don't need to check to preempt at wakeup
> +        * the idle thread and don't set next buddy as a candidate for being
> +        * picked in priority.
> +        * In case of simultaneous wakeup from idle, the latency sensitive tasks
> +        * lost opportunity to preempt non sensitive tasks which woke up
> +        * simultaneously.
> +        */
> +
> +       if (cfs->next)
> +               next = cfs->next;
> +       else
> +               next = __pick_first_entity(cfs);
> +
> +       if (next && wakeup_preempt_entity(next, se) == 1)
> +               set_next_buddy(se);
> +}
> +

What's the motivation to do this with the next buddy vs using wakeup
entity placement to achieve a similar result? The latter would also
more generically work when we aren't transitioning from idle. It also
doesn't suffer from some slight weirdness here in the interaction with
core scheduling, where rq->curr can be idle despite the presence of
runnable tasks if the cpu is forced idle.