Re: [PATCH v2 1/4] kvm: Dynamically adjust the rate of dirty ring reaper thread

[Peter Xu <peterx@redhat.com>]

On Sat, Apr 02, 2022 at 03:28:13PM +0800, Chongyun Wu wrote:
> > > +{
> > > +    float ratio = 0.0;
> > > +    uint64_t sleep_time = 1000000;
> > > +    enum KVMDirtyRingReaperRunLevel run_level_want;
> > > +    enum KVMDirtyRingReaperSpeedControl speed_control;
> > > +
> > > +    /*
> > > +     * When the number of dirty pages collected exceeds
> > > +     * the given percentage of the ring size,the speed
> > > +     * up action will be triggered.
> > > +     */
> > > +    s->reaper.ratio_adjust_threshold = 0.1;
> > > +    s->reaper.stable_count_threshold = 5;
> > > +
> > > +    ratio = (float)dirty_count / s->kvm_dirty_ring_size;
> > > +
> > > +    if (s->reaper.ring_full_cnt > ring_full_cnt_last) {
> > > +        /* If get a new ring full need speed up reaper thread */
> > > +        if (s->reaper.run_level != KVM_DIRTY_RING_REAPER_RUN_MAX_SPEED) {
> > > +            s->reaper.run_level++;
> > > +        }
> > > +    } else {
> > > +        /*
> > > +         * If get more dirty pages this loop and this status continus
> > > +         * for many times try to speed up reaper thread.
> > > +         * If the status is stable and need to decide which speed need
> > > +         * to use.
> > > +         */
> > > +        if (ratio < s->reaper.ratio_adjust_threshold) {
> > > +            run_level_want = KVM_DIRTY_RING_REAPER_RUN_NORMAL;
> > > +        } else if (ratio < s->reaper.ratio_adjust_threshold * 2) {
> > > +            run_level_want = KVM_DIRTY_RING_REAPER_RUN_FAST1;
> > > +        } else if (ratio < s->reaper.ratio_adjust_threshold * 3) {
> > > +            run_level_want = KVM_DIRTY_RING_REAPER_RUN_FAST2;
> > > +        } else if (ratio < s->reaper.ratio_adjust_threshold * 4) {
> > > +            run_level_want = KVM_DIRTY_RING_REAPER_RUN_FAST3;
> > > +        } else if (ratio < s->reaper.ratio_adjust_threshold * 5) {
> > > +            run_level_want = KVM_DIRTY_RING_REAPER_RUN_FAST4;
> > > +        } else {
> > > +            run_level_want = KVM_DIRTY_RING_REAPER_RUN_MAX_SPEED;
> > > +        }
> > > +
> > > +        /* Get if need speed up or slow down */
> > > +        if (run_level_want > s->reaper.run_level) {
> > > +            speed_control = KVM_DIRTY_RING_REAPER_SPEED_CONTROL_UP;
> > > +            *speed_down_cnt = 0;
> > > +        } else if (run_level_want < s->reaper.run_level) {
> > > +            speed_control = KVM_DIRTY_RING_REAPER_SPEED_CONTROL_DOWN;
> > > +            *speed_down_cnt++;
> > > +        } else {
> > > +            speed_control = KVM_DIRTY_RING_REAPER_SPEED_CONTROL_KEEP;
> > > +        }
> > > +
> > > +        /* Control reaper thread run in sutiable run speed level */
> > > +        if (speed_control == KVM_DIRTY_RING_REAPER_SPEED_CONTROL_UP) {
> > > +            /* If need speed up do not check its stable just do it */
> > > +            s->reaper.run_level++;
> > > +        } else if (speed_control ==
> > > +            KVM_DIRTY_RING_REAPER_SPEED_CONTROL_DOWN) {
> > > +            /* If need speed down we should filter this status */
> > > +            if (*speed_down_cnt > s->reaper.stable_count_threshold) {
> > > +                s->reaper.run_level--;
> > > +            }
> > > +        }
> > > +    }
> > > +
> > > +    /* Set the actual running rate of the reaper */
> > > +    switch (s->reaper.run_level) {
> > > +    case KVM_DIRTY_RING_REAPER_RUN_NORMAL:
> > > +        sleep_time = 1000000;
> > > +        break;
> > > +    case KVM_DIRTY_RING_REAPER_RUN_FAST1:
> > > +        sleep_time = 500000;
> > > +        break;
> > > +    case KVM_DIRTY_RING_REAPER_RUN_FAST2:
> > > +        sleep_time = 250000;
> > > +        break;
> > > +    case KVM_DIRTY_RING_REAPER_RUN_FAST3:
> > > +        sleep_time = 125000;
> > > +        break;
> > > +    case KVM_DIRTY_RING_REAPER_RUN_FAST4:
> > > +        sleep_time = 100000;
> > > +        break;
> > > +    case KVM_DIRTY_RING_REAPER_RUN_MAX_SPEED:
> > > +        sleep_time = 80000;
> > > +        break;
> > > +    default:
> > > +        sleep_time = 1000000;
> > > +        error_report("Bad reaper thread run level, use default");
> > > +    }
> > > +
> > > +    return sleep_time;
> > > +}
> > > +I think how to calculate the sleep time needs discuussion,
> > > including why
> > we define 5 levels, why we choose the time constants and in what
> > scenarios this algo works fine.
> 
> 
> > 
> > The other thing is i still think it's nicer we have the simplest
> > algorithm firstly, which should be very easy to verify.

Chongyun,

I saw that you left some empty line above but didn't really answer this
question.  I am with Yong.. there're just too many magic numbers.

I don't have a strong opinion on dropping all of them, maybe your point is
you did tons of testing and you found these numbers are the best (though I
doubt it.. but if it's true please let me know) but at least I think it can
be put into a structure like:

  struct dirty_ring_interval_table {
    /* This can be 0->5; we really don't need those macros.. */
    int level;
    /* This marks over which dirty ratio we use this level */
    float threshold_ratio;
    /* This is the time to sleep if this level is chosen */
    int sleep_us;
    ...
  };

IIUC the whole logic above has a major point in that we shrink the sleep
time more aggresively than growing, I think it can be done much easier than
this algorithm, e.g., we mark a global threshold of say 0.8 dirty ratio out
of the ring size, then:

  (1) if dirty ratio > 0.8 we half the sleep time (delay /= 2)
  (2) if dirty ratio <= 0.2 we increase the sleep time (delay += 20ms)

We also give a limit to the sleep time, say max 1sec min 100ms.  Would that
also work very well already with e.g. 20 LOCs rather than 100+ with lots of
magics and if/else's?

Thanks,

-- 
Peter Xu