Re: [Qemu-devel] [PATCH v6 17/47] ram_debug_dump_bitmap: Dump a migration bitmap as text

[Amit Shah]

On (Tue) 14 Apr 2015 [18:03:43], Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
> 
> Misses out lines that are all the expected value so the output
> can be quite compact depending on the circumstance.

s/Misses out lines/Prints out lines to stderr/

s/the expected/an expected/

?

Also, some sentence explaining why this is being added - like helps
with debugging when <something> goes wrong, and how it saves time.  I
really think this is a good function to have, and if we can somehow
use this output to even create a visualisation of how far ahead we are
in the migration process, users can see fancy output which tells them
how fast things are converging (in precopy), and how fast the guest is
updating the memory vs throttling applied, etc.

> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  arch_init.c                   | 40 +++++++++++++++++++++++++++++++++++++++-
>  include/migration/migration.h |  1 +
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 3a21f0e..2b0cd18 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -833,13 +833,51 @@ static void reset_ram_globals(void)
>  
>  #define MAX_WAIT 50 /* ms, half buffered_file limit */
>  
> -
>  /* Each of ram_save_setup, ram_save_iterate and ram_save_complete has
>   * long-running RCU critical section.  When rcu-reclaims in the code
>   * start to become numerous it will be necessary to reduce the
>   * granularity of these critical sections.
>   */

This new function should go above this comment.

> +/*
> + * 'expected' is the value you expect the bitmap mostly to be full
> + * of and it won't bother printing lines that are all this value
> + * if 'todump' is null the migration bitmap is dumped.

Missing punctuation?  The last line there is a new sentence, isn't it?

> + */
> +void ram_debug_dump_bitmap(unsigned long *todump, bool expected)
> +{
> +    int64_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> +
> +    int64_t cur;
> +    int64_t linelen = 128;
> +    char linebuf[129];
> +
> +    if (!todump) {
> +        todump = migration_bitmap;
> +    }
> +
> +    for (cur = 0; cur < ram_pages; cur += linelen) {
> +        int64_t curb;
> +        bool found = false;
> +        /*
> +         * Last line; catch the case where the line length
> +         * is longer than remaining ram
> +         */
> +        if (cur+linelen > ram_pages) {

spacing is off: whitespace around '+'.  (I didn't run checkpatch,
though.)  Similar below too.

> +            linelen = ram_pages - cur;
> +        }
> +        for (curb = 0; curb < linelen; curb++) {
> +            bool thisbit = test_bit(cur+curb, todump);

whitespace around +

                Amit