Re: [PATCH v3 0/3] Introduce per-task latency_nice for scheduler hints

[chris hyser]

On 2/19/20 9:15 AM, chris hyser wrote:
> On 2/19/20 5:09 AM, Parth Shah wrote:
> > Hi Chris,
> >
> > On 2/19/20 4:30 AM, chris hyser wrote:
> > > On 2/17/20 3:57 AM, Parth Shah wrote:
> > > > On 1/16/20 5:32 PM, Parth Shah wrote:
> > > > > This is the 3rd revision of the patch set to introduce
> > > > > latency_{nice/tolerance} as a per task attribute.
> > > > >
> > > > > The previous version can be found at:
> > > > > v1: https://lkml.org/lkml/2019/11/25/151
> > > > > v2: https://lkml.org/lkml/2019/12/8/10
> > > > >
> > > > > Changes in this revision are:
> > > > > v2 -> v3:
> > > > > - This series changes the longer attribute name to "latency_nice" as per
> > > > > ÂÂÂ the comment from Dietmar Eggemann https://lkml.org/lkml/2019/12/5/394
> > > > > v1 -> v2:
> > > > > - Addressed comments from Qais Yousef
> > > > > - As per suggestion from Dietmar, moved content from newly created
> > > > > ÂÂÂ include/linux/sched/latency_tolerance.h to kernel/sched/sched.h
> > > > > - Extend sched_setattr() to support latency_tolerance in tools headers UAPI
> > > > >
> > > > >
> > > > > Introduction:
> > > > > ==============
> > > > > This patch series introduces a new per-task attribute latency_nice to
> > > > > provide the scheduler hints about the latency requirements of the task [1].
> > > > >
> > > > > Latency_nice is a ranged attribute of a task with the value ranging
> > > > > from [-20, 19] both inclusive which makes it align with the task nice
> > > > > value.
> > > > >
> > > > > The value should provide scheduler hints about the relative latency
> > > > > requirements of tasks, meaning the task with "latency_nice = -20"
> > > > > should have lower latency requirements than compared to those tasks with
> > > > > higher values. Similarly a task with "latency_nice = 19" can have higher
> > > > > latency and hence such tasks may not care much about latency.
> > > > >
> > > > > The default value is set to 0. The usecases discussed below can use this
> > > > > range of [-20, 19] for latency_nice for the specific purpose. This
> > > > > patch does not implement any use cases for such attribute so that any
> > > > > change in naming or range does not affect much to the other (future)
> > > > > patches using this. The actual use of latency_nice during task wakeup
> > > > > and load-balancing is yet to be coded for each of those usecases.
> > > > >
> > > > > As per my view, this defined attribute can be used in following ways for a
> > > > > some of the usecases:
> > > > > 1 Reduce search scan time for select_idle_cpu():
> > > > > - Reduce search scans for finding idle CPU for a waking task with lower
> > > > > ÂÂÂ latency_nice values.
> > > > >
> > > > > 2 TurboSched:
> > > > > - Classify the tasks with higher latency_nice values as a small
> > > > > ÂÂÂ background task given that its historic utilization is very low, for
> > > > > ÂÂÂ which the scheduler can search for more number of cores to do task
> > > > >  packing. A task with a latency_nice >= some_threshold (e.g, == 19)
> > > > > ÂÂÂ and util <= 12.5% can be background tasks.
> > > > >
> > > > > 3 Optimize AVX512 based workload:
> > > > > - Bias scheduler to not put a task having (latency_nice == -20) on a
> > > > > ÂÂÂ core occupying AVX512 based workload.
> > > > >
> > > > >
> > > > > Series Organization:
> > > > > ====================
> > > > > - Patch 1: Add new attribute latency_nice to task_struct.
> > > > > - Patch 2: Clone parent task's attribute to the child task on fork
> > > > > - Patch 3: Add support for sched_{set,get}attr syscall to modify
> > > > > ÂÂÂÂÂÂÂÂÂÂÂÂ latency_nice of the task
> > > > >
> > > > >
> > > > > The patch series can be applied on tip/sched/core at the
> > > > > commit 804d402fb6f6 ("sched/rt: Make RT capacity-aware")
> > > > >
> > > > >
> > > > > References:
> > > > > ============
> > > > > [1]. Usecases for the per-task latency-nice attribute,
> > > > > ÂÂÂÂÂÂ https://lkml.org/lkml/2019/9/30/215
> > > > > [2]. Task Latency-nice, "Subhra Mazumdar",
> > > > > ÂÂÂÂÂÂ https://lkml.org/lkml/2019/8/30/829
> > > > > [3]. Introduce per-task latency_tolerance for scheduler hints,
> > > > > ÂÂÂÂÂÂ https://lkml.org/lkml/2019/12/8/10
> > > > >
> > > > >
> > > > > Parth Shah (3):
> > > > > ÂÂÂ sched: Introduce latency-nice as a per-task attribute
> > > > > ÂÂÂ sched/core: Propagate parent task's latency requirements to the child
> > > > > ÂÂÂÂÂ task
> > > > > ÂÂÂ sched: Allow sched_{get,set}attr to change latency_nice of the task
> > > > >
> > > > > ÂÂ include/linux/sched.hÂÂÂÂÂÂÂÂÂÂÂ |Â 1 +
> > > > > ÂÂ include/uapi/linux/sched.hÂÂÂÂÂÂ |Â 4 +++-
> > > > > ÂÂ include/uapi/linux/sched/types.h | 19 +++++++++++++++++++
> > > > > ÂÂ kernel/sched/core.cÂÂÂÂÂÂÂÂÂÂÂÂÂ | 21 +++++++++++++++++++++
> > > > > ÂÂ kernel/sched/sched.hÂÂÂÂÂÂÂÂÂÂÂÂ | 18 ++++++++++++++++++
> > > > > ÂÂ tools/include/uapi/linux/sched.h |Â 4 +++-
> > > > > ÂÂ 6 files changed, 65 insertions(+), 2 deletions(-)
> > > >
> > > > Its been a long time and few revisions since the beginning of the
> > > > discussion around the latency-nice. Hence thought of asking if there is/are
> > > > any further work that needs to be done for adding latency-nice attribute or
> > > > am I missing any piece in here?
> > >
> > > All, I was asked to take a look at the original latency_nice patchset.
> > > First, to clarify objectives, Oracle is not interested in trading
> > > throughput for latency. What we found is that the DB has specific tasks
> > > which do very little but need to do this as absolutely quickly as possible,
> > > ie extreme latency sensitivity. Second, the key to latency reduction in the
> > > task wakeup path seems to be limiting variations of "idle cpu" search. The
> > > latter particularly interests me as an example of "platform size based
> > > latency" which I believe to be important given all the varying size VMs and
> > > containers.
> > >
> > > Parth, I've been using your v3 patchset as the basis of an investigation
> > > into the measurable effects of short-circuiting this search. I'm not quite
> > > ready to put anything out, but the patchset is working well. The only
> >
> > That's a good news as you are able to get a usecase of this patch-set.
> >
> > > feedback I have is that currently non-root can set the value negative which
> > > is inconsistent with 'nice' and I would think a security hole.
> >
> > I would assume you mean 'latency_nice' here.
> >
> > ÂFrom my testing, I was not able to set values for any root owned task's
> > latency_nice value by the non-root user. Also, my patch-set just piggybacks
> > on the already existing sched_setattr syscall and hence it should not allow
> > non-root user to do any modifications. Can you confirm this by changing
> > nice (renice) value of a root task from non-root user.
> >
> > I have done the sanity check in the code and thinking where it could
> > possibly have gone wrong. So, can you please specify what values were you
> > able to set outside the [-20, 19] range?
>
> The checks prevent being outside that range. But negative numbers -20 to -1 did not need root. Let me dig some more. I 
> verified this explicitly before sending the email so something is up.

I went digging. This is absolutely repeatable. I checked that I do not unknowingly have CAP_SYS_NICE as a user. So 
first, are we tying latency_nice to CAP_SYS_NICE? Seems like a reasonable thing, but not sure I saw this stated 
anywhere. Second, the only capability checked in __sched_setscheduler() in the patch I have is CAP_SYS_NICE and those 
checks will not return a -EPERM for a negative latency_tolerance (in the code, aka latency_nice). Do I have the correct 
version of the code? Am I missing something?

-chrish