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;
:-)
> +/*
> + * 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
