lkml.org 
[lkml]   [2019]   [Aug]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH] sched/fair: don't assign runtime for throttled cfs_rq
    From
    Date
    On 14/08/2019 19:00, Liangyan wrote:
    > do_sched_cfs_period_timer() will refill cfs_b runtime and call
    > distribute_cfs_runtime() to unthrottle cfs_rq, sometimes cfs_b->runtime
    > will allocate all quota to one cfs_rq incorrectly.

    > This will cause other cfs_rq can't get runtime and will be throttled.
    > We find that one throttled cfs_rq has non-negative
    > cfs_rq->runtime_remaining and cause an unexpetced cast from s64 to u64
    > in snippet: distribute_cfs_runtime() {
    > runtime = -cfs_rq->runtime_remaining + 1; }.

    > This cast will cause that runtime will be a large number and
    > cfs_b->runtime will be subtracted to be zero at last.
    >

    I'm a complete CFS bandwidth noob but let me give this a try...


    -Wconversion does pick this up (turning this thing on made me understand
    why it's not on by default)

    kernel/sched/fair.c: In function ‘distribute_cfs_runtime’:
    kernel/sched/fair.c:4633:13: warning: conversion to ‘u64’ {aka ‘long long unsigned int’} from ‘s64’ {aka ‘long long int’} may change the sign of the result [-Wsign-conversion]
    runtime = -cfs_rq->runtime_remaining + 1;
    ^
    kernel/sched/fair.c:4638:29: warning: conversion to ‘long long unsigned int’ from ‘s64’ {aka ‘long long int’} may change the sign of the result [-Wsign-conversion]
    cfs_rq->runtime_remaining += runtime;
    ^~


    The thing is we have a !cfs_rq_throttled() check just before the snippet
    you're calling out, so AFAICT cfs_rq->runtime_remaining has to be <= 0
    there (otherwise this cfs_rq wouldn't be throttled).

    I doubt you can get this to fire, but just to be sure...
    -----8<-----
    diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
    index bbd90adabe2a..836948a3ae23 100644
    --- a/kernel/sched/fair.c
    +++ b/kernel/sched/fair.c
    @@ -4630,6 +4630,8 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b, u64 remaining)
    if (!cfs_rq_throttled(cfs_rq))
    goto next;

    + WARN_ON(cfs_rq->runtime_remaining > 0);
    +
    runtime = -cfs_rq->runtime_remaining + 1;
    if (runtime > remaining)
    runtime = remaining;
    ----->8-----
    Other than those signed/unsigned shenanigans, I only see one other scenario
    that leads to a cfs_rq getting allocated all the remaining runtime: its
    .runtime_remaining just has to be greater or equal (in absolute value)
    than the remaining runtime.

    If that's happening consistently, I suppose that could be due to long
    delays between update_curr_fair() calls, but I can't think right why that
    would happen.

    > This commit prevents cfs_rq to be assgined new runtime if it has been
    > throttled to avoid the above incorrect type cast.
    >
    > Signed-off-by: Liangyan <liangyan.peng@linux.alibaba.com>
    > ---
    > kernel/sched/fair.c | 2 ++
    > 1 file changed, 2 insertions(+)
    >
    > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
    > index 81fd8a2a605b..b14d67d28138 100644
    > --- a/kernel/sched/fair.c
    > +++ b/kernel/sched/fair.c
    > @@ -4068,6 +4068,8 @@ static void __account_cfs_rq_runtime(struct cfs_rq *cfs_rq, u64 delta_exec)
    > if (likely(cfs_rq->runtime_remaining > 0))
    > return;
    >
    > + if (cfs_rq->throttled)
    > + return;
    > /*
    > * if we're unable to extend our runtime we resched so that the active
    > * hierarchy can be throttled
    >

    \
     
     \ /
      Last update: 2019-08-15 18:37    [W:3.260 / U:0.888 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site