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

[Dr. David Alan Gilbert]

* Amit Shah (address@hidden) wrote:
> 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.


Changed to:
----
ram_debug_dump_bitmap: Dump a migration bitmap as text

Useful for debugging the migration bitmap and other bitmaps
of the same format (including the sentmap in postcopy).

The bitmap is printed to stderr.
Lines that are all the expected value are excluded so the output
can be quite compact for many bitmaps.
----

I'm not sure it's that useful to endusers; during the migration the bitmap
can be quite a mix and thus the 'exclude' doesn't help much so the output
can be quite big.  Sanidhya Kashyap's series that did repeated logging
is more useful as a tool to find out why things aren't converging.

> 
> > 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.

Done.

> > +/*
> > + * '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?

Replaced by:
/*
 * 'expected' is the value you expect the bitmap mostly to be full
 * of; it won't bother printing lines that are all this value.
 * If 'todump' is null the migration bitmap is dumped.
 */

> > + */
> > +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.

Done, oddly checkpatch didn't moan - I don't see why.

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

Done; again checkpatch doesn't moan.

Thanks, 

Dave

> 
>               Amit
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK