| Message ID | 1442427114-5075-3-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
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; :-) > +/* > + * 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). 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? > + found = find_dirty_block(f, &pss, &again); > + > + if (found) { Amit
* 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
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; struct CompressParam { bool start; @@ -917,6 +917,59 @@ static int ram_save_compressed_page(QEMUFile *f, RAMBlock *block, return pages; } +/* + * 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; + } +} + /** * 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; + found = find_dirty_block(f, &pss, &again); + + if (found) { if (compression_switch && migrate_use_compression()) { pages = ram_save_compressed_page(f, pss.block, pss.offset, last_stage, @@ -978,10 +1014,9 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage, /* if page is unmodified, continue to the next */ if (pages > 0) { last_sent_block = pss.block; - break; } } - } + } while (!pages && again); last_seen_block = pss.block; last_offset = pss.offset;