Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CAKfTPtCZ5CttibZ9o18woLiQgzFeKCBMDAGrhMd3kbgX2qs29Q@mail.gmail.com>
 <E5C05A33-06E4-4627-8108-145752D72A24@joelfernandes.org> <CAKfTPtBZ8xfoP4=W1K5vaBBm5b05Ak9ojAdJeb2hvs5W=W3gmw@mail.gmail.com>
In-Reply-To: <CAKfTPtBZ8xfoP4=W1K5vaBBm5b05Ak9ojAdJeb2hvs5W=W3gmw@mail.gmail.com>
From:   Joel Fernandes <joel@joelfernandes.org>
Date:   Wed, 30 Nov 2022 03:09:51 +0000
Message-ID: <CAEXW_YRpYx3VC+PdM2J=QhZipcy59A46YnjyE0zpUQe3c4Ck8A@mail.gmail.com>
Subject: Re: [PATCH 5/9] sched/fair: Take into account latency priority at wakeup
To:     Vincent Guittot <vincent.guittot@linaro.org>
Cc:     mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com,
        dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
        mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com,
        linux-kernel@vger.kernel.org, parth@linux.ibm.com,
        qyousef@layalina.io, chris.hyser@oracle.com,
        patrick.bellasi@matbug.net, David.Laight@aculab.com,
        pjt@google.com, pavel@ucw.cz, tj@kernel.org, qperret@google.com,
        tim.c.chen@linux.intel.com, joshdon@google.com, timj@gnu.org,
        kprateek.nayak@amd.com, yu.c.chen@intel.com,
        youssefesmat@chromium.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Vincent,

On Tue, Nov 29, 2022 at 5:21 PM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
[...]
> > >>> }
> > >>>
> > >>> /*
> > >>> @@ -7544,7 +7558,7 @@ static int __sched_setscheduler(struct task_s=
truct *p,
> > >>>                if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)
> > >>>                        goto change;
> > >>>                if (attr->sched_flags & SCHED_FLAG_LATENCY_NICE &&
> > >>> -                   attr->sched_latency_nice !=3D p->latency_nice)
> > >>> +                   attr->sched_latency_nice !=3D LATENCY_TO_NICE(p=
->latency_prio))
> > >>>                        goto change;
> > >>>
> > >>>                p->sched_reset_on_fork =3D reset_on_fork;
> > >>> @@ -8085,7 +8099,7 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, st=
ruct sched_attr __user *, uattr,
> > >>>        get_params(p, &kattr);
> > >>>        kattr.sched_flags &=3D SCHED_FLAG_ALL;
> > >>>
> > >>> -       kattr.sched_latency_nice =3D p->latency_nice;
> > >>> +       kattr.sched_latency_nice =3D LATENCY_TO_NICE(p->latency_pri=
o);
> > >>>
> > >>> #ifdef CONFIG_UCLAMP_TASK
> > >>>        /*
> > >>> @@ -11294,6 +11308,20 @@ const u32 sched_prio_to_wmult[40] =3D {
> > >>>  /*  15 */ 119304647, 148102320, 186737708, 238609294, 286331153,
> > >>> };
> > >>>
> > >>> +/*
> > >>> + * latency weight for wakeup preemption
> > >>> + */
> > >>> +const int sched_latency_to_weight[40] =3D {
> > >>> + /* -20 */     -1024,     -973,     -922,      -870,      -819,
> > >>> + /* -15 */      -768,     -717,     -666,      -614,      -563,
> > >>> + /* -10 */      -512,     -461,     -410,      -358,      -307,
> > >>> + /*  -5 */      -256,     -205,     -154,      -102,       -51,
> > >>> + /*   0 */         0,       51,      102,       154,       205,
> > >>> + /*   5 */       256,      307,      358,       410,       461,
> > >>> + /*  10 */       512,      563,      614,       666,       717,
> > >>> + /*  15 */       768,      819,      870,       922,       973,
> > >>> +};
> > >>> +
> > >>
> > >> The table is linear. You could approximate this as: weight =3D nice =
* 51
> > >> since it is a linear scale and do the conversion in place.
> > >>
> > >> Or, since the only place you are using the latency_to_weight is in
> > >> set_latency_offset(), can we drop the sched_latency_to_weight array
> > >> and simplify as follows?
> > >
> > > It's also used in cgroup patch and keeps a coherency between
> > > nice/weight an latency_nice/offset so I prefer
> >
> > I dont think it=E2=80=99s a valid comparison as nice/weight conversion =
are non linear and over there a table makes sense: weight =3D 1024 / 1.25 ^=
 nice
> >
> > > keeping current
> > > implementation
> >
> > I could be missing something, but, since its a linear scale, why does c=
group need weight at all? Just store nice directly. Why would that not work=
?
> >
> > In the end the TG and SE has the latency offset in the struct, that is =
all you care about. All the conversion back and forth is unnecessary, as it=
 is a linear scale and just increases LOC and takes more memory to store li=
near arrays.
> >
> > Again I could be missing something and I will try to play with your ser=
ies and see if I can show you what I mean (or convince myself it=E2=80=99s =
needed).
>
> I get what you mean but I think that having an array gives latitude to
> adjust this internal offset mapping at a minimum cost of a const array

Ok that makes sense. If you feel like there might be updates in the
future to this mapping array (like changing the constants as you
mentioned), then I am Ok with us keeping it.

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

I am excited about your series, the CFS latency issues have been
thorny. This feels like a step forward in the right direction. Cheers,

 - Joel