lkml.org 
[lkml]   [2019]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] sched/fair: scale quota and period without losing quota/period ratio precision
    On Thu, Oct 03, 2019 at 05:12:43PM -0700 Xuewei Zhang wrote:
    > quota/period ratio is used to ensure a child task group won't get more
    > bandwidth than the parent task group, and is calculated as:
    > normalized_cfs_quota() = [(quota_us << 20) / period_us]
    >
    > If the quota/period ratio was changed during this scaling due to
    > precision loss, it will cause inconsistency between parent and child
    > task groups. See below example:
    > A userspace container manager (kubelet) does three operations:
    > 1) Create a parent cgroup, set quota to 1,000us and period to 10,000us.
    > 2) Create a few children cgroups.
    > 3) Set quota to 1,000us and period to 10,000us on a child cgroup.
    >
    > These operations are expected to succeed. However, if the scaling of
    > 147/128 happens before step 3), quota and period of the parent cgroup
    > will be changed:
    > new_quota: 1148437ns, 1148us
    > new_period: 11484375ns, 11484us
    >
    > And when step 3) comes in, the ratio of the child cgroup will be 104857,
    > which will be larger than the parent cgroup ratio (104821), and will
    > fail.
    >
    > Scaling them by a factor of 2 will fix the problem.
    >
    > Fixes: 2e8e19226398 ("sched/fair: Limit sched_cfs_period_timer() loop to avoid hard lockup")
    > Signed-off-by: Xuewei Zhang <xueweiz@google.com>


    I managed to get it to trigger the second case. It took 50,000 children (20x my initial tests).

    [ 1367.850630] cfs_period_timer[cpu11]: period too short, scaling up (new cfs_period_us = 4340, cfs_quota_us = 250000)
    [ 1370.390832] cfs_period_timer[cpu11]: period too short, scaling up (new cfs_period_us = 8680, cfs_quota_us = 500000)
    [ 1372.914689] cfs_period_timer[cpu11]: period too short, scaling up (new cfs_period_us = 17360, cfs_quota_us = 1000000)
    [ 1375.447431] cfs_period_timer[cpu11]: period too short, scaling up (new cfs_period_us = 34720, cfs_quota_us = 2000000)
    [ 1377.982785] cfs_period_timer[cpu11]: period too short, scaling up (new cfs_period_us = 69440, cfs_quota_us = 4000000)
    [ 1380.481702] cfs_period_timer[cpu11]: period too short, scaling up (new cfs_period_us = 138880, cfs_quota_us = 8000000)
    [ 1382.894692] cfs_period_timer[cpu11]: period too short, scaling up (new cfs_period_us = 277760, cfs_quota_us = 16000000)
    [ 1385.264872] cfs_period_timer[cpu11]: period too short, scaling up (new cfs_period_us = 555520, cfs_quota_us = 32000000)
    [ 1393.965140] cfs_period_timer[cpu11]: period too short, but cannot scale up without losing precision (cfs_period_us = 555520, cfs_quota_us = 32000000)

    I suspect going higher could cause the original lockup, but that'd be the case with the old code as well.
    And this also gets us out of it faster.


    Tested-by: Phil Auld <pauld@redhat.com>


    Cheers,
    Phil


    > ---
    > kernel/sched/fair.c | 36 ++++++++++++++++++++++--------------
    > 1 file changed, 22 insertions(+), 14 deletions(-)
    >
    > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
    > index 83ab35e2374f..b3d3d0a231cd 100644
    > --- a/kernel/sched/fair.c
    > +++ b/kernel/sched/fair.c
    > @@ -4926,20 +4926,28 @@ static enum hrtimer_restart sched_cfs_period_timer(struct hrtimer *timer)
    > if (++count > 3) {
    > u64 new, old = ktime_to_ns(cfs_b->period);
    >
    > - new = (old * 147) / 128; /* ~115% */
    > - new = min(new, max_cfs_quota_period);
    > -
    > - cfs_b->period = ns_to_ktime(new);
    > -
    > - /* since max is 1s, this is limited to 1e9^2, which fits in u64 */
    > - cfs_b->quota *= new;
    > - cfs_b->quota = div64_u64(cfs_b->quota, old);
    > -
    > - pr_warn_ratelimited(
    > - "cfs_period_timer[cpu%d]: period too short, scaling up (new cfs_period_us %lld, cfs_quota_us = %lld)\n",
    > - smp_processor_id(),
    > - div_u64(new, NSEC_PER_USEC),
    > - div_u64(cfs_b->quota, NSEC_PER_USEC));
    > + /*
    > + * Grow period by a factor of 2 to avoid lossing precision.
    > + * Precision loss in the quota/period ratio can cause __cfs_schedulable
    > + * to fail.
    > + */
    > + new = old * 2;
    > + if (new < max_cfs_quota_period) {
    > + cfs_b->period = ns_to_ktime(new);
    > + cfs_b->quota *= 2;
    > +
    > + pr_warn_ratelimited(
    > + "cfs_period_timer[cpu%d]: period too short, scaling up (new cfs_period_us = %lld, cfs_quota_us = %lld)\n",
    > + smp_processor_id(),
    > + div_u64(new, NSEC_PER_USEC),
    > + div_u64(cfs_b->quota, NSEC_PER_USEC));
    > + } else {
    > + pr_warn_ratelimited(
    > + "cfs_period_timer[cpu%d]: period too short, but cannot scale up without losing precision (cfs_period_us = %lld, cfs_quota_us = %lld)\n",
    > + smp_processor_id(),
    > + div_u64(old, NSEC_PER_USEC),
    > + div_u64(cfs_b->quota, NSEC_PER_USEC));
    > + }
    >
    > /* reset count so we don't come right back in here */
    > count = 0;
    > --
    > 2.23.0.581.g78d2f28ef7-goog
    >

    --

    \
     
     \ /
      Last update: 2019-10-07 17:15    [W:4.361 / U:3.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site