* Amit Shah (amit.s...@redhat.com) wrote:
> On (Wed) 16 Sep 2015 [19:11:54], Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilb...@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 <dgilb...@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 / dgilb...@redhat.com / Manchester, UK
