Date:   Thu, 20 Feb 2020 14:04:16 +0530
From:   Parth Shah <parth@...ux.ibm.com>
To:     chris hyser <chris.hyser@...cle.com>, vincent.guittot@...aro.org,
        patrick.bellasi@...bug.net, valentin.schneider@....com,
        dhaval.giani@...cle.com, dietmar.eggemann@....com
Cc:     linux-kernel@...r.kernel.org, peterz@...radead.org,
        mingo@...hat.com, qais.yousef@....com, pavel@....cz,
        qperret@...rret.net, David.Laight@...LAB.COM, pjt@...gle.com,
        tj@...nel.org
Subject: Re: [PATCH v3 0/3] Introduce per-task latency_nice for scheduler
 hints



On 2/19/20 11:53 PM, chris hyser wrote:
> 
> 
> 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?

You are right. I have not added permission checks for setting the
latency_nice value. For the task_nice, non-root user has no permission to
set the value lower than the current value which is not the case with the
latency_nice.

In order to align with the permission checks like task_nice, I will add the
check similar to task_nice and send out the v4 of the series soon.


Thanks for pointing out.
- Parth

> 
> -chrish
> 

Powered by blists - more mailing lists