Re: [Qemu-devel] [PATCH 2/2] ram_find_and_save_block: Split out the finding

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Amit Shah (amit.shah@redhat.com) wrote:
> On (Wed) 16 Sep 2015 [19:11:54], Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Split out the finding of the dirty page and all the wrap detection
> > into a separate function since it was getting a bit hairy.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  migration/ram.c | 87 ++++++++++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 61 insertions(+), 26 deletions(-)
> > 
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 1fadf52..d2982e9 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -236,7 +236,7 @@ struct PageSearchStatus {
> >      /* Set once we wrap around */
> >      bool         complete_round;
> >  };
> > -typdef struct PageSearchStatus PageSearchStatus;
> > +typedef struct PageSearchStatus PageSearchStatus;
> 
> :-)

Ahem - I didn't retest after I split it into two subpatches; thanks; I'll recut this soon.

> > +/*
> > + * Find the next dirty page and update any state associated with
> > + * the search process.
> > + *
> > + * Returns: True if a page is found
> > + *
> > + * @f: Current migration stream.
> > + * @pss: Data about the state of the current dirty page scan.
> > + * @*again: Set to false if the search has scanned the whole of RAM
> > + */
> > +static bool find_dirty_block(QEMUFile *f, PageSearchStatus *pss,
> > +                             bool *again)
> > +{
> > +    pss->offset = migration_bitmap_find_and_reset_dirty(pss->block,
> > +                                                       pss->offset);
> > +    if (pss->complete_round && pss->block == last_seen_block &&
> > +        pss->offset >= last_offset) {
> > +        /*
> > +         * We've been once around the RAM and haven't found anything
> > +         * give up.
> > +         */
> > +        *again = false;
> > +        return false;
> > +    }
> > +    if (pss->offset >= pss->block->used_length) {
> > +        /* Didn't find anything in this RAM Block */
> > +        pss->offset = 0;
> > +        pss->block = QLIST_NEXT_RCU(pss->block, next);
> > +        if (!pss->block) {
> > +            /* Hit the end of the list */
> > +            pss->block = QLIST_FIRST_RCU(&ram_list.blocks);
> > +            /* Flag that we've looped */
> > +            pss->complete_round = true;
> > +            ram_bulk_stage = false;
> > +            if (migrate_use_xbzrle()) {
> > +                /* If xbzrle is on, stop using the data compression at this
> > +                 * point. In theory, xbzrle can do better than compression.
> > +                 */
> > +                flush_compressed_data(f);
> > +                compression_switch = false;
> > +            }
> > +        }
> > +        /* Didn't find anything this time, but try again on the new block */
> > +        *again = true;
> > +        return false;
> > +    } else {
> > +        /* Can go around again, but... */
> > +        *again = true;
> > +        /* We've found something so probably don't need to */
> > +        return true;
> 
> These comments are very good.  Had you not introduced them, I'd have
> recommended to just set *again to true before the if; and get the
> 'return true' case handled earlier so that all the nesting too goes
> off (each case has a return, so no point in continuing with if..else).

Yes, I needed to add them to help me understand what was going on.

> Also, the dance with the return value and the 'again' is also slightly
> complex -- but I doubt it can be made any simpler.
> 
> > +    }
> > +}
> > +
> >  /**
> >   * ram_find_and_save_block: Finds a dirty page and sends it to f
> >   *
> > @@ -935,6 +988,7 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
> >  {
> >      PageSearchStatus pss;
> >      int pages = 0;
> > +    bool again, found;
> >  
> >      pss.block = last_seen_block;
> >      pss.offset = last_offset;
> > @@ -943,29 +997,11 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
> >      if (!pss.block)
> >          pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
> >  
> > -    while (true) {
> > -        pss.offset = migration_bitmap_find_and_reset_dirty(pss.block,
> > -                                                           pss.offset);
> > -        if (pss.complete_round && pss.block == last_seen_block &&
> > -            pss.offset >= last_offset) {
> > -            break;
> > -        }
> > -        if (pss.offset >= pss.block->used_length) {
> > -            pss.offset = 0;
> > -            pss.block = QLIST_NEXT_RCU(pss.block, next);
> > -            if (!pss.block) {
> > -                pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
> > -                pss.complete_round = true;
> > -                ram_bulk_stage = false;
> > -                if (migrate_use_xbzrle()) {
> > -                    /* If xbzrle is on, stop using the data compression at this
> > -                     * point. In theory, xbzrle can do better than compression.
> > -                     */
> > -                    flush_compressed_data(f);
> > -                    compression_switch = false;
> > -                }
> > -            }
> > -        } else {
> > +    do {
> > +        again = true;
> 
> This assignment isn't needed -- did any tool complain?

No, it just seemed neater; I'll remove it when I repost.

Dave

> 
> > +        found = find_dirty_block(f, &pss, &again);
> > +
> > +        if (found) {
> 
> 
> 		Amit
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK