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

[Amit Shah]

On (Wed) 16 Sep 2015 [19:11:54], Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
> 
> 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 <address@hidden>
> ---
>  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