Re: [Qemu-devel] [PATCH v2 2/3] migration: use the free page reporting feature from balloon

[Wang, Wei W]

On Monday, February 26, 2018 1:07 PM, Wei Wang wrote:
> On 02/09/2018 07:50 PM, Dr. David Alan Gilbert wrote:
> > * Wei Wang (address@hidden) wrote:
> >> Use the free page reporting feature from the balloon device to clear
> >> the bits corresponding to guest free pages from the dirty bitmap, so
> >> that the free memory are not sent.
> >>
> >> Signed-off-by: Wei Wang <address@hidden>
> >> CC: Michael S. Tsirkin <address@hidden>
> >> CC: Juan Quintela <address@hidden>
> >> ---
> >>   migration/ram.c | 24 ++++++++++++++++++++----
> >>   1 file changed, 20 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/migration/ram.c b/migration/ram.c index d6f462c..4fe16d2
> >> 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -49,6 +49,7 @@
> >>   #include "qemu/rcu_queue.h"
> >>   #include "migration/colo.h"
> >>   #include "migration/block.h"
> >> +#include "sysemu/balloon.h"
> >>
> >>   /***********************************************************/
> >>   /* ram save/restore */
> >> @@ -206,6 +207,10 @@ struct RAMState {
> >>       uint32_t last_version;
> >>       /* We are in the first round */
> >>       bool ram_bulk_stage;
> >> +    /* The feature, skipping the transfer of free pages, is supported */
> >> +    bool free_page_support;
> >> +    /* Skip the transfer of free pages in the bulk stage */
> >> +    bool free_page_done;
> >>       /* How many times we have dirty too many pages */
> >>       int dirty_rate_high_cnt;
> >>       /* these variables are used for bitmap sync */ @@ -773,7 +778,7
> >> @@ unsigned long migration_bitmap_find_dirty(RAMState *rs, RAMBlock
> *rb,
> >>       unsigned long *bitmap = rb->bmap;
> >>       unsigned long next;
> >>
> >> -    if (rs->ram_bulk_stage && start > 0) {
> >> +    if (rs->ram_bulk_stage && start > 0 && !rs->free_page_support) {
> >>           next = start + 1;
> >>       } else {
> >>           next = find_next_bit(bitmap, size, start); @@ -1653,6
> >> +1658,8 @@ static void ram_state_reset(RAMState *rs)
> >>       rs->last_page = 0;
> >>       rs->last_version = ram_list.version;
> >>       rs->ram_bulk_stage = true;
> >> +    rs->free_page_support = balloon_free_page_support();
> >> +    rs->free_page_done = false;
> >>   }
> >>
> >>   #define MAX_WAIT 50 /* ms, half buffered_file limit */ @@ -2135,7
> >> +2142,7 @@ static int ram_state_init(RAMState **rsp)
> >>       return 0;
> >>   }
> >>
> >> -static void ram_list_init_bitmaps(void)
> >> +static void ram_list_init_bitmaps(RAMState *rs)
> >>   {
> >>       RAMBlock *block;
> >>       unsigned long pages;
> >> @@ -2145,7 +2152,11 @@ static void ram_list_init_bitmaps(void)
> >>           QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
> >>               pages = block->max_length >> TARGET_PAGE_BITS;
> >>               block->bmap = bitmap_new(pages);
> >> -            bitmap_set(block->bmap, 0, pages);
> >> +            if (rs->free_page_support) {
> >> +                bitmap_set(block->bmap, 1, pages);
> > I don't understand how it makes sense to do that here; ignoring
> > anything ese it means that migration_dirty_pages is wrong which could
> > end up with migration finishing before all real pages are sent.
> >
> 
> The bulk stage treats all the pages as dirty pages, so we set all the bits to 
> "1",
> this is needed by this optimization feature, because the free pages reported
> from the guest can then be directly cleared from the bitmap (we don't need
> any more bitmaps to record free pages).
> 

Sorry, there was a misunderstanding of the bitmap_set API (thought it was used 
to set all the bits to 1 or 0). So the above change isn't needed actually. Btw, 
this doesn't affect the results I reported.

Best,
Wei