Count used RAMBlock pages for migration_dirty_pages
PATCH 1/1 Count used RAMBlock pages for migration_dirty_pages
From: "Dr. David Alan Gilbert" <[email protected]>
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.
(* https://bugzilla.redhat.com/show_bug.cgi?id=1074913 )
--- arch_init.c | 41 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 4 deletions(-)
arch_init.c +37 -4
@@ -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 ();
Signature · 1 line
1.8.5.3
Quoted 5 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
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.
Quoted 9 lines
'
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_pagesended up too large.
Quoted 5 lines
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").
Quoted 35 lines
Signed-off-by Dr. David Alan Gilbert <[email protected]>(* https://bugzilla.redhat.com/show_bug.cgi?id=1074913 )
--- arch_init.c | 41 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 4 deletions(-)arch_init.c +7
-4@@ -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
arch_init.c static int ram_save_setup(QEMUFile *f, void *opaque) · line 785
Quoted 10 lines
arch_init.c +10
-0+* 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.
Quoted 24 lines · * Juan Quintela ([email protected]) wrote:
Quoted 5 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
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.
Quoted 9 lines
'
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_pagesended up too large.Quoted 5 lines
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.
Quoted 7 lines
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.
Quoted 48 lines
Quoted 34 lines
Signed-off-by Dr. David Alan Gilbert <[email protected]>(* https://bugzilla.redhat.com/show_bug.cgi?id=1074913 )
--- arch_init.c | 41 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 4 deletions(-)arch_init.c +7
-4@@ -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 arch_init.c static int ram_save_setup(QEMUFile *f, void *opaque) · line 785
Quoted 10 lines
arch_init.c +10
-0+* 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?
Quoted 2 lines
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 / [email protected] / Manchester, UK
Il 21/03/2014 14:22, Dr. David Alan Gilbert ha scritto:
Quoted 7 lines
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 wasmigration_bitmap_sync_rangethat 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 biggertarget_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
Quoted 10 lines · * Paolo Bonzini ([email protected]) wrote:
Il 21/03/2014 14:22, Dr. David Alan Gilbert ha scritto:
Quoted 7 lines
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 wasmigration_bitmap_sync_rangethat 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 biggertarget_page.Do you mean bigger host page?
Yes.
Quoted 6 lines
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.
Quoted 6 lines
Host page sizes matter only for KVM (see hpratio in
cpu_physical_memory_set_dirty_lebitmapininclude/exec/ram_addr.h). In this case dirtying a 4K area also dirties 12K of adjacentram_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.
Quoted 3 lines
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 / [email protected] / Manchester, UK
Il 21/03/2014 16:45, Dr. David Alan Gilbert ha scritto:
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
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
Quoted 2 lines
I'll rework the patch to my less paranoid version from yesterday that treats bitmap bits as belonging to only one RAMBlock and repost.
Quoted 7 lines · Paolo Bonzini
Il 21/03/2014 16:45, Dr. David Alan Gilbert ha scritto:
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
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.
Quoted 6 lines
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.
Quoted 75 lines
From: "Dr. David Alan Gilbert" <[email protected]>
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_pagesended 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 <[email protected]>(* https://bugzilla.redhat.com/show_bug.cgi?id=1074913 )
---arch_init.c| 41 +++++++++++++++++++++++++++++++++++++----1 file changed, 37 insertions(+), 4 deletions(-)arch_init.c +26
-4@@ -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 );
arch_init.c static int ram_save_setup(QEMUFile *f, void *opaque) · line 804
Quoted 10 lines
arch_init.c +10
-0+ } + 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.
Quoted 8 lines
arch_init.c +1
-0+memory_global_dirty_log_start ();migration_bitmap_sync ();qemu_mutex_unlock_iothread ();Signature · 1 line
1.8.5.3
Quoted 81 lines · * ???? ([email protected]) wrote:
Quoted 81 lines
From: "Dr. David Alan Gilbert" <[email protected]>
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_pagesended 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 <[email protected]>(* https://bugzilla.redhat.com/show_bug.cgi?id=1074913 )
---arch_init.c| 41 +++++++++++++++++++++++++++++++++++++----1 file changed, 37 insertions(+), 4 deletions(-)arch_init.c +32
-4@@ -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,
Quoted 8 lines
arch_init.c static int ram_save_setup(QEMUFile *f, void *opaque) · line 804
Quoted 4 lines
arch_init.c +4
-0+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 / [email protected] / Manchester, UK