MIME-Version: 1.0
References: <20230128001639.3510083-1-tj@kernel.org> <20230128001639.3510083-28-tj@kernel.org>
In-Reply-To: <20230128001639.3510083-28-tj@kernel.org>
From:   Josh Don <joshdon@google.com>
Date:   Mon, 30 Jan 2023 13:38:15 -0800
Message-ID: <CABk29Nt2-CCGnogpfEgJ3ZDk5Esk04n6EwsAqpw_vdeVfKuFUQ@mail.gmail.com>
Subject: Re: [PATCH 27/30] sched_ext: Implement core-sched support
To:     Tejun Heo <tj@kernel.org>
Cc:     torvalds@linux-foundation.org, mingo@redhat.com,
        peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
        bristot@redhat.com, vschneid@redhat.com, ast@kernel.org,
        daniel@iogearbox.net, andrii@kernel.org, martin.lau@kernel.org,
        brho@google.com, pjt@google.com, derkling@google.com,
        haoluo@google.com, dvernet@meta.com, dschatzberg@meta.com,
        dskarlat@cs.cmu.edu, riel@surriel.com,
        linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
        kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Tejun,

On Fri, Jan 27, 2023 at 4:17 PM Tejun Heo <tj@kernel.org> wrote:
>
> The core-sched support is composed of the following parts:

Thanks, this looks pretty reasonable overall.

One meta comment is that I think we can shortcircuit from
touch_core_sched when we have sched_core_disabled().

Reviewed-by: Josh Don <joshdon@google.com>

> +                       /*
> +                        * While core-scheduling, rq lock is shared among
> +                        * siblings but the debug annotations and rq clock
> +                        * aren't. Do pinning dance to transfer the ownership.
> +                        */
> +                       WARN_ON_ONCE(__rq_lockp(rq) != __rq_lockp(srq));
> +                       rq_unpin_lock(rq, rf);
> +                       rq_pin_lock(srq, &srf);
> +
> +                       update_rq_clock(srq);

Unfortunate that we have to do this superfluous update; maybe we can
save/restore the clock flags from before the pinning shenanigans?

> +static struct task_struct *pick_task_scx(struct rq *rq)
> +{
> +       struct task_struct *curr = rq->curr;
> +       struct task_struct *first = first_local_task(rq);
> +
> +       if (curr->scx.flags & SCX_TASK_QUEUED) {
> +               /* is curr the only runnable task? */
> +               if (!first)
> +                       return curr;
> +
> +               /*
> +                * Does curr trump first? We can always go by core_sched_at for
> +                * this comparison as it represents global FIFO ordering when
> +                * the default core-sched ordering is in used and local-DSQ FIFO
> +                * ordering otherwise.
> +                */
> +               if (curr->scx.slice && time_before64(curr->scx.core_sched_at,
> +                                                    first->scx.core_sched_at))
> +                       return curr;

So is this to handle the case where we have something running on 'rq'
to match the cookie of our sibling (which had priority), but now we
want to switch to running the first thing in the local queue, which
has a different cookie (and is now the highest priority entity)? Maybe
being slightly more specific in the comment would help :)