[Qemu-devel] [PATCH 1/1] Count used RAMBlock pages for migration_dirty_pages

[Qemu-devel] [PATCH 1/1] Count used RAMBlock pages for migration_dirty_pages

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This is a fix for a bug* triggered by a migration after hot unplugging
a few virtio-net NICs, that caused migration never to converge, because
'migration_dirty_pages' is incorrectly initialised.

'migration_dirty_pages' is used as a tally of the number of outstanding
dirty pages, to give the migration code an idea of how much more data
will need to be transferred, and thus whether it can end the iterative
phase.

It was initialised to the total size of the RAMBlock address space,
however hotunplug can leave this space sparse, and hence
migration_dirty_pages ended up too large.

Note that the code tries to be careful when counting to deal with
RAMBlocks that share the same end/start page - I don't know
if this is actually possible and it does complicate the code,
but since there was other code that dealt with unaligned RAMBlocks
it seemed possible.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

(* https://bugzilla.redhat.com/show_bug.cgi?id=1074913 )
---
 arch_init.c | 41 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index f18f42e..ef0e98d 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -727,11 +727,8 @@ static void reset_ram_globals(void)
 static int ram_save_setup(QEMUFile *f, void *opaque)
 {
     RAMBlock *block;
-    int64_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS;
+    int64_t ram_bitmap_pages;
 
-    migration_bitmap = bitmap_new(ram_pages);
-    bitmap_set(migration_bitmap, 0, ram_pages);
-    migration_dirty_pages = ram_pages;
     mig_throttle_on = false;
     dirty_rate_high_cnt = 0;
 
@@ -770,6 +767,42 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     bytes_transferred = 0;
     reset_ram_globals();
 
+    ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS;
+    migration_bitmap = bitmap_new(ram_bitmap_pages);
+    bitmap_set(migration_bitmap, 0, ram_bitmap_pages);
+    /*
+     * Count the total number of pages used by ram blocks. We clear the dirty
+     * bit for the start/end of each ramblock as we go so that we don't double
+     * count ramblocks that have overlapping pages - at entry the whole dirty
+     * bitmap is set.
+     */
+    migration_dirty_pages = 0;
+    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
+        uint64_t block_pages = 0;
+        ram_addr_t saddr, eaddr;
+
+        saddr = block->mr->ram_addr;
+        eaddr = saddr + block->length - 1;
+        saddr /= TARGET_PAGE_SIZE;
+        eaddr /= TARGET_PAGE_SIZE;
+
+        /* Do the end pages of the range, that might be shared with others */
+        block_pages += test_and_clear_bit(saddr, migration_bitmap);
+        block_pages += test_and_clear_bit(eaddr, migration_bitmap);
+
+        if ((saddr + 1) < eaddr) {
+            block_pages += eaddr - (saddr + 1);
+        }
+        migration_dirty_pages += block_pages;
+        /*fprintf(stderr, "ram_save_setup: %s s/e=%zx/%zx page s/e=%zx/%zx"
+                " bp=%zu\n",
+                block->idstr, block->mr->ram_addr,
+                block->mr->ram_addr+block->length-1,
+                saddr, eaddr, block_pages); */
+    }
+    /* and set the bits again that got used for our overlap check */
+    bitmap_set(migration_bitmap, 0, ram_bitmap_pages);
+
     memory_global_dirty_log_start();
     migration_bitmap_sync();
     qemu_mutex_unlock_iothread();
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH 1/1] Count used RAMBlock pages for migration_dirty_pages

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> This is a fix for a bug* triggered by a migration after hot unplugging
> a few virtio-net NICs, that caused migration never to converge, because
> 'migration_dirty_pages' is incorrectly initialised.

Good catch.

> 'migration_dirty_pages' is used as a tally of the number of outstanding
> dirty pages, to give the migration code an idea of how much more data
> will need to be transferred, and thus whether it can end the iterative
> phase.
>
> It was initialised to the total size of the RAMBlock address space,
> however hotunplug can leave this space sparse, and hence
> migration_dirty_pages ended up too large.
>
> Note that the code tries to be careful when counting to deal with
> RAMBlocks that share the same end/start page - I don't know
> if this is actually possible and it does complicate the code,
> but since there was other code that dealt with unaligned RAMBlocks
> it seemed possible.

Couldn't we just check at block addition that it dont' overlap?

What code do you mean?

My understanding is that the "normal" way of creating new RAMBlocks is
with qemu_ram_alloc_from_ptr(), and my reading is that block never
overlap.  (Important words of the sentence: "my reading").

>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> (* https://bugzilla.redhat.com/show_bug.cgi?id=1074913 )
> ---
>  arch_init.c | 41 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index f18f42e..ef0e98d 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -727,11 +727,8 @@ static void reset_ram_globals(void)
>  static int ram_save_setup(QEMUFile *f, void *opaque)
>  {
>      RAMBlock *block;
> -    int64_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> +    int64_t ram_bitmap_pages;
>  
> -    migration_bitmap = bitmap_new(ram_pages);
> -    bitmap_set(migration_bitmap, 0, ram_pages);
> -    migration_dirty_pages = ram_pages;
>      mig_throttle_on = false;
>      dirty_rate_high_cnt = 0;
>  
> @@ -770,6 +767,42 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      bytes_transferred = 0;
>      reset_ram_globals();
>  
> +    ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> +    migration_bitmap = bitmap_new(ram_bitmap_pages);
> +    bitmap_set(migration_bitmap, 0, ram_bitmap_pages);
> +    /*
> +     * Count the total number of pages used by ram blocks. We clear the dirty
> +     * bit for the start/end of each ramblock as we go so that we don't double
> +     * count ramblocks that have overlapping pages - at entry the whole dirty
> +     * bitmap is set.
> +     */
> +    migration_dirty_pages = 0;
> +    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> +        uint64_t block_pages = 0;
> +        ram_addr_t saddr, eaddr;
> +
> +        saddr = block->mr->ram_addr;
> +        eaddr = saddr + block->length - 1;

If my assumtpion is true:  block->lenght-1 / TARGET_PAGE_SIZE (rounded
up) should be enough, no?

Reason for this is that migration bitmap handling is already slow, and
we are adding a whole two passes here?

Later, Juan.

Re: [Qemu-devel] [PATCH 1/1] Count used RAMBlock pages for migration_dirty_pages

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > This is a fix for a bug* triggered by a migration after hot unplugging
> > a few virtio-net NICs, that caused migration never to converge, because
> > 'migration_dirty_pages' is incorrectly initialised.
> 
> Good catch.
> 
> > 'migration_dirty_pages' is used as a tally of the number of outstanding
> > dirty pages, to give the migration code an idea of how much more data
> > will need to be transferred, and thus whether it can end the iterative
> > phase.
> >
> > It was initialised to the total size of the RAMBlock address space,
> > however hotunplug can leave this space sparse, and hence
> > migration_dirty_pages ended up too large.
> >
> > Note that the code tries to be careful when counting to deal with
> > RAMBlocks that share the same end/start page - I don't know
> > if this is actually possible and it does complicate the code,
> > but since there was other code that dealt with unaligned RAMBlocks
> > it seemed possible.
> 
> Couldn't we just check at block addition that it dont' overlap?
> 
> What code do you mean?
> 
> My understanding is that the "normal" way of creating new RAMBlocks is
> with qemu_ram_alloc_from_ptr(), and my reading is that block never
> overlap.  (Important words of the sentence: "my reading").

I don't think they overlap, but I worry that the end of one block
and the start of the next might be on the same page.
The code that got me worried was migration_bitmap_sync_range
that seemd to be general; but actually that's worrying about 64bit words
not pages.
What happens with things like '/rom@etc/table-loader' which is only
4k on x86 when they are on boxes with bigger target_page.


> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> > (* https://bugzilla.redhat.com/show_bug.cgi?id=1074913 )
> > ---
> >  arch_init.c | 41 +++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 37 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch_init.c b/arch_init.c
> > index f18f42e..ef0e98d 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -727,11 +727,8 @@ static void reset_ram_globals(void)
> >  static int ram_save_setup(QEMUFile *f, void *opaque)
> >  {
> >      RAMBlock *block;
> > -    int64_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> > +    int64_t ram_bitmap_pages;
> >  
> > -    migration_bitmap = bitmap_new(ram_pages);
> > -    bitmap_set(migration_bitmap, 0, ram_pages);
> > -    migration_dirty_pages = ram_pages;
> >      mig_throttle_on = false;
> >      dirty_rate_high_cnt = 0;
> >  
> > @@ -770,6 +767,42 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> >      bytes_transferred = 0;
> >      reset_ram_globals();
> >  
> > +    ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> > +    migration_bitmap = bitmap_new(ram_bitmap_pages);
> > +    bitmap_set(migration_bitmap, 0, ram_bitmap_pages);
> > +    /*
> > +     * Count the total number of pages used by ram blocks. We clear the dirty
> > +     * bit for the start/end of each ramblock as we go so that we don't double
> > +     * count ramblocks that have overlapping pages - at entry the whole dirty
> > +     * bitmap is set.
> > +     */
> > +    migration_dirty_pages = 0;
> > +    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> > +        uint64_t block_pages = 0;
> > +        ram_addr_t saddr, eaddr;
> > +
> > +        saddr = block->mr->ram_addr;
> > +        eaddr = saddr + block->length - 1;
> 
> If my assumtpion is true:  block->lenght-1 / TARGET_PAGE_SIZE (rounded
> up) should be enough, no?
> 
> Reason for this is that migration bitmap handling is already slow, and
> we are adding a whole two passes here?

Oh yes, if we are sure the ramblocks never meet on the same page all this
becomes a lot simpler (indeed it was a lot simpler yesterday before I got
worried about them touching).

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/1] Count used RAMBlock pages for migration_dirty_pages

[Paolo Bonzini]

Il 21/03/2014 14:22, Dr. David Alan Gilbert ha scritto:
> I don't think they overlap, but I worry that the end of one block
> and the start of the next might be on the same page.
> The code that got me worried was migration_bitmap_sync_range
> that seemd to be general; but actually that's worrying about 64bit words
> not pages.
> What happens with things like '/rom@etc/table-loader' which is only
> 4k on x86 when they are on boxes with bigger target_page.

Do you mean bigger host page?

RAM sizes are always rounded up to target page size:

     size = TARGET_PAGE_ALIGN(size);

If I understand correctly all that matters here is ram addresses have no 
remainder WRT target page sizes.

Host page sizes matter only for KVM (see hpratio in 
cpu_physical_memory_set_dirty_lebitmap in include/exec/ram_addr.h).  In 
this case dirtying a 4K area also dirties 12K of adjacent ram_addr_t. 
Things could go wrong if those ram_addr_t's are not part of any 
RAMBlock, because in that case you will loop endlessly (I think).  But 
this is not something your patch introduces.

So it looks like your patch could also fix the problem Juan reported at 
http://article.gmane.org/gmane.comp.emulators.qemu/247462 -- but perhaps 
only on hosts where !KVM || TARGET_PAGE_SIZE==getpagesize().

Paolo

Re: [Qemu-devel] [PATCH 1/1] Count used RAMBlock pages for migration_dirty_pages

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Il 21/03/2014 14:22, Dr. David Alan Gilbert ha scritto:
> >I don't think they overlap, but I worry that the end of one block
> >and the start of the next might be on the same page.
> >The code that got me worried was migration_bitmap_sync_range
> >that seemd to be general; but actually that's worrying about 64bit words
> >not pages.
> >What happens with things like '/rom@etc/table-loader' which is only
> >4k on x86 when they are on boxes with bigger target_page.
> 
> Do you mean bigger host page?

Yes.

> RAM sizes are always rounded up to target page size:
> 
>     size = TARGET_PAGE_ALIGN(size);
> 
> If I understand correctly all that matters here is ram addresses
> have no remainder WRT target page sizes.

OK, so I can simplify it back to last nights code from before
I started worrying about it.

> Host page sizes matter only for KVM (see hpratio in
> cpu_physical_memory_set_dirty_lebitmap in include/exec/ram_addr.h).
> In this case dirtying a 4K area also dirties 12K of adjacent
> ram_addr_t. Things could go wrong if those ram_addr_t's are not part
> of any RAMBlock, because in that case you will loop endlessly (I
> think).  But this is not something your patch introduces.

No, actually I don't think it will loop; the code loops over all the
RAM blocks, looking only at the bitmap bits that RAMBlocks->length
cover; if the kernel sets bits outside the RAMBlocks then they'll
get ignored.

> So it looks like your patch could also fix the problem Juan reported
> at http://article.gmane.org/gmane.comp.emulators.qemu/247462 -- but
> perhaps only on hosts where !KVM || TARGET_PAGE_SIZE==getpagesize().

Possibly yes; I think that would cause gaps in the bitmap to have
the same effect as the hot unplug.

I'll rework the patch to my less paranoid version from yesterday
that treats bitmap bits as belonging to only one RAMBlock and repost.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/1] Count used RAMBlock pages for migration_dirty_pages

[Paolo Bonzini]

Il 21/03/2014 16:45, Dr. David Alan Gilbert ha scritto:
>> > So it looks like your patch could also fix the problem Juan reported
>> > at http://article.gmane.org/gmane.comp.emulators.qemu/247462 -- but
>> > perhaps only on hosts where !KVM || TARGET_PAGE_SIZE==getpagesize().
> Possibly yes; I think that would cause gaps in the bitmap to have
> the same effect as the hot unplug.

It would be nice to revive that patch, because without it you risk 
getting bad performance from migration of hotplugged memory.  On-board 
RAM typically gets low ram_addr_t's that are aligned, but the 128K (32 
pages) ROM and the 64K VGA BIOS will ruin the alignment and cause 
migration to use the slow paths to migrate hotplugged memory above them.

Paolo

> I'll rework the patch to my less paranoid version from yesterday
> that treats bitmap bits as belonging to only one RAMBlock and repost.

Re: [Qemu-devel] [PATCH 1/1] Count used RAMBlock pages for migration_dirty_pages

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 21/03/2014 16:45, Dr. David Alan Gilbert ha scritto:
>>> > So it looks like your patch could also fix the problem Juan reported
>>> > at http://article.gmane.org/gmane.comp.emulators.qemu/247462 -- but
>>> > perhaps only on hosts where !KVM || TARGET_PAGE_SIZE==getpagesize().
>> Possibly yes; I think that would cause gaps in the bitmap to have
>> the same effect as the hot unplug.
>
> It would be nice to revive that patch, because without it you risk
> getting bad performance from migration of hotplugged memory.  On-board
> RAM typically gets low ram_addr_t's that are aligned, but the 128K (32
> pages) ROM and the 64K VGA BIOS will ruin the alignment and cause
> migration to use the slow paths to migrate hotplugged memory above
> them.

Agreed.  I had forgot that optimization that we had to get out.

The problem (for my use case) was VGA, that was dirtying continously
memory that was not aligned.  Size was not big, but was using bit-by-bit
operations, when we could use bitmap ones.

Later, Juan.

Re: [Qemu-devel] [PATCH 1/1] Count used RAMBlock pages for migration_dirty_pages

[陈梁]

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> This is a fix for a bug* triggered by a migration after hot unplugging
> a few virtio-net NICs, that caused migration never to converge, because
> 'migration_dirty_pages' is incorrectly initialised.
> 
> 'migration_dirty_pages' is used as a tally of the number of outstanding
> dirty pages, to give the migration code an idea of how much more data
> will need to be transferred, and thus whether it can end the iterative
> phase.
> 
> It was initialised to the total size of the RAMBlock address space,
> however hotunplug can leave this space sparse, and hence
> migration_dirty_pages ended up too large.
> 
> Note that the code tries to be careful when counting to deal with
> RAMBlocks that share the same end/start page - I don't know
> if this is actually possible and it does complicate the code,
> but since there was other code that dealt with unaligned RAMBlocks
> it seemed possible.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> (* https://bugzilla.redhat.com/show_bug.cgi?id=1074913 )
> ---
> arch_init.c | 41 +++++++++++++++++++++++++++++++++++++----
> 1 file changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index f18f42e..ef0e98d 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -727,11 +727,8 @@ static void reset_ram_globals(void)
> static int ram_save_setup(QEMUFile *f, void *opaque)
> {
>     RAMBlock *block;
> -    int64_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> +    int64_t ram_bitmap_pages;
> 
> -    migration_bitmap = bitmap_new(ram_pages);
> -    bitmap_set(migration_bitmap, 0, ram_pages);
> -    migration_dirty_pages = ram_pages;
>     mig_throttle_on = false;
>     dirty_rate_high_cnt = 0;
> 
> @@ -770,6 +767,42 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>     bytes_transferred = 0;
>     reset_ram_globals();
> 
> +    ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> +    migration_bitmap = bitmap_new(ram_bitmap_pages);
> +    bitmap_set(migration_bitmap, 0, ram_bitmap_pages);
> +    /*
> +     * Count the total number of pages used by ram blocks. We clear the dirty
> +     * bit for the start/end of each ramblock as we go so that we don't double
> +     * count ramblocks that have overlapping pages - at entry the whole dirty
> +     * bitmap is set.
> +     */
> +    migration_dirty_pages = 0;
> +    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> +        uint64_t block_pages = 0;
> +        ram_addr_t saddr, eaddr;
> +
> +        saddr = block->mr->ram_addr;
> +        eaddr = saddr + block->length - 1;
> +        saddr /= TARGET_PAGE_SIZE;
> +        eaddr /= TARGET_PAGE_SIZE;
> +
> +        /* Do the end pages of the range, that might be shared with others */
> +        block_pages += test_and_clear_bit(saddr, migration_bitmap);
> +        block_pages += test_and_clear_bit(eaddr, migration_bitmap);
> +
> +        if ((saddr + 1) < eaddr) {
> +            block_pages += eaddr - (saddr + 1);
> +        }
> +        migration_dirty_pages += block_pages;
> +        /*fprintf(stderr, "ram_save_setup: %s s/e=%zx/%zx page s/e=%zx/%zx"
> +                " bp=%zu\n",
> +                block->idstr, block->mr->ram_addr,
> +                block->mr->ram_addr+block->length-1,
> +                saddr, eaddr, block_pages); */
> +    }
> +    /* and set the bits again that got used for our overlap check */
> +    bitmap_set(migration_bitmap, 0, ram_bitmap_pages);

maybe it should like this:
bitmap_set(migration_bitmap, 0, last_ram_offset() >> TARGET_PAGE_BITS);
Because there are some space sparse.
> +
>     memory_global_dirty_log_start();
>     migration_bitmap_sync();
>     qemu_mutex_unlock_iothread();
> -- 
> 1.8.5.3
> 
> 

Re: [Qemu-devel] [PATCH 1/1] Count used RAMBlock pages for migration_dirty_pages

[Dr. David Alan Gilbert]

* ???? (chenliang0016@icloud.com) wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > This is a fix for a bug* triggered by a migration after hot unplugging
> > a few virtio-net NICs, that caused migration never to converge, because
> > 'migration_dirty_pages' is incorrectly initialised.
> > 
> > 'migration_dirty_pages' is used as a tally of the number of outstanding
> > dirty pages, to give the migration code an idea of how much more data
> > will need to be transferred, and thus whether it can end the iterative
> > phase.
> > 
> > It was initialised to the total size of the RAMBlock address space,
> > however hotunplug can leave this space sparse, and hence
> > migration_dirty_pages ended up too large.
> > 
> > Note that the code tries to be careful when counting to deal with
> > RAMBlocks that share the same end/start page - I don't know
> > if this is actually possible and it does complicate the code,
> > but since there was other code that dealt with unaligned RAMBlocks
> > it seemed possible.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > (* https://bugzilla.redhat.com/show_bug.cgi?id=1074913 )
> > ---
> > arch_init.c | 41 +++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 37 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch_init.c b/arch_init.c
> > index f18f42e..ef0e98d 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -727,11 +727,8 @@ static void reset_ram_globals(void)
> > static int ram_save_setup(QEMUFile *f, void *opaque)
> > {
> >     RAMBlock *block;
> > -    int64_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> > +    int64_t ram_bitmap_pages;
> > 
> > -    migration_bitmap = bitmap_new(ram_pages);
> > -    bitmap_set(migration_bitmap, 0, ram_pages);
> > -    migration_dirty_pages = ram_pages;
> >     mig_throttle_on = false;
> >     dirty_rate_high_cnt = 0;
> > 
> > @@ -770,6 +767,42 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> >     bytes_transferred = 0;
> >     reset_ram_globals();
> > 
> > +    ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> > +    migration_bitmap = bitmap_new(ram_bitmap_pages);
> > +    bitmap_set(migration_bitmap, 0, ram_bitmap_pages);
> > +    /*
> > +     * Count the total number of pages used by ram blocks. We clear the dirty
> > +     * bit for the start/end of each ramblock as we go so that we don't double
> > +     * count ramblocks that have overlapping pages - at entry the whole dirty
> > +     * bitmap is set.
> > +     */
> > +    migration_dirty_pages = 0;
> > +    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> > +        uint64_t block_pages = 0;
> > +        ram_addr_t saddr, eaddr;
> > +
> > +        saddr = block->mr->ram_addr;
> > +        eaddr = saddr + block->length - 1;
> > +        saddr /= TARGET_PAGE_SIZE;
> > +        eaddr /= TARGET_PAGE_SIZE;
> > +
> > +        /* Do the end pages of the range, that might be shared with others */
> > +        block_pages += test_and_clear_bit(saddr, migration_bitmap);
> > +        block_pages += test_and_clear_bit(eaddr, migration_bitmap);
> > +
> > +        if ((saddr + 1) < eaddr) {
> > +            block_pages += eaddr - (saddr + 1);
> > +        }
> > +        migration_dirty_pages += block_pages;
> > +        /*fprintf(stderr, "ram_save_setup: %s s/e=%zx/%zx page s/e=%zx/%zx"
> > +                " bp=%zu\n",
> > +                block->idstr, block->mr->ram_addr,
> > +                block->mr->ram_addr+block->length-1,
> > +                saddr, eaddr, block_pages); */
> > +    }
> > +    /* and set the bits again that got used for our overlap check */
> > +    bitmap_set(migration_bitmap, 0, ram_bitmap_pages);
> 
> maybe it should like this:
> bitmap_set(migration_bitmap, 0, last_ram_offset() >> TARGET_PAGE_BITS);
> Because there are some space sparse.

No, ram_bitmap_pages is already equal to last_ram_offset() >> TARGET_PAGE_BITS
The loop has been calculating migration_dirty_pages and hasn't touched ram_bitmap_pages.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK