Re: [PATCH v2 07/11] migration: Implement dirty-limit convergence algo

[Hyman]

在 2022/11/30 7:17, Peter Xu 写道:
> On Mon, Nov 21, 2022 at 11:26:39AM -0500, huangy81@chinatelecom.cn wrote:
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 86950a1..096b61a 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -240,6 +240,7 @@ void migration_cancel(const Error *error)
> >       if (error) {
> >           migrate_set_error(current_migration, error);
> >       }
> > +    qmp_cancel_vcpu_dirty_limit(false, -1, NULL);
>
> Disable it only if migrate_dirty_limit() is true?  It seems okay if the
> admin wants to use dirtylimit separately from migration.
Ok.
> >       migrate_fd_cancel(current_migration);
> >   }
>
> [...]
>
> > @@ -1148,22 +1175,31 @@ static void migration_trigger_throttle(RAMState *rs)
> >       uint64_t bytes_dirty_period = rs->num_dirty_pages_period * 
> > TARGET_PAGE_SIZE;
> >       uint64_t bytes_dirty_threshold = bytes_xfer_period * threshold / 100;
> >   
> > -    /* During block migration the auto-converge logic incorrectly detects
> > -     * that ram migration makes no progress. Avoid this by disabling the
> > -     * throttling logic during the bulk phase of block migration. */
> > -    if (migrate_auto_converge() && !blk_mig_bulk_active()) {
> > -        /* The following detection logic can be refined later. For now:
> > -           Check to see if the ratio between dirtied bytes and the approx.
> > -           amount of bytes that just got transferred since the last time
> > -           we were in this routine reaches the threshold. If that happens
> > -           twice, start or increase throttling. */
> > -
> > -        if ((bytes_dirty_period > bytes_dirty_threshold) &&
> > -            (++rs->dirty_rate_high_cnt >= 2)) {
> > +    /*
> > +     * The following detection logic can be refined later. For now:
> > +     * Check to see if the ratio between dirtied bytes and the approx.
> > +     * amount of bytes that just got transferred since the last time
> > +     * we were in this routine reaches the threshold. If that happens
> > +     * twice, start or increase throttling.
> > +     */
> > +
> > +    if ((bytes_dirty_period > bytes_dirty_threshold) &&
> > +        (++rs->dirty_rate_high_cnt >= 2)) {
> > +        rs->dirty_rate_high_cnt = 0;
> > +        /*
> > +         * During block migration the auto-converge logic incorrectly detects
> > +         * that ram migration makes no progress. Avoid this by disabling the
> > +         * throttling logic during the bulk phase of block migration
> > +         */
> > +
> > +        if (migrate_auto_converge() && !blk_mig_bulk_active()) {
>
> Does dirtylimit cap needs to check blk_mig_bulk_active() too?  I assume
> that check was used to ignore the bulk block migration phase where major
> bandwidth will be consumed by block migrations so the measured bandwidth is
> not accurate.  IIUC it applies to dirtylimit too.Indeed, i'll add this next 
> version.
>
> >               trace_migration_throttle();
> > -            rs->dirty_rate_high_cnt = 0;
> >               mig_throttle_guest_down(bytes_dirty_period,
> >                                       bytes_dirty_threshold);
> > +        } else if (migrate_dirty_limit() &&
> > +                   kvm_dirty_ring_enabled() &&
> > +                   migration_is_active(s)) {
>
> Is "kvm_dirty_ring_enabled()" and "migration_is_active(s)" check helpful?
> Can we only rely on migrate_dirty_limit() alone?
In qmp_set_vcpu_dirty_limit, it checks if kvm enabled and dirty ring 
size set. When "dirty-limit" capability set, we also check this in 
migrate_caps_check, so kvm_dirty_ring_enabled can be dropped indeed.

As for migration_is_active, dirty-limit can be set anytime and migration 
is active already in the path. It also can be dropped.

I'll fix this next version.
>