From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54409) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WEFRh-00040G-6R for qemu-devel@nongnu.org; Fri, 14 Feb 2014 04:52:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WEFRa-0000Tx-Vm for qemu-devel@nongnu.org; Fri, 14 Feb 2014 04:52:33 -0500 Received: from mx1.redhat.com ([209.132.183.28]:22001) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WEFRa-0000T3-Mc for qemu-devel@nongnu.org; Fri, 14 Feb 2014 04:52:26 -0500 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s1E9qOPh025408 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 14 Feb 2014 04:52:24 -0500 Message-ID: <52FDE778.406@redhat.com> Date: Fri, 14 Feb 2014 11:52:56 +0200 From: Orit Wasserman MIME-Version: 1.0 References: <1392320685-20609-1-git-send-email-dgilbert@redhat.com> In-Reply-To: <1392320685-20609-1-git-send-email-dgilbert@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] Fix two XBZRLE corruption issues List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert (git)" , qemu-devel@nongnu.org Cc: quintela@redhat.com On 02/13/2014 09:44 PM, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" > > Push zero'd pages into the XBZRLE cache > A page that was cached by XBZRLE, zero'd and then XBZRLE'd again > was being compared against a stale cache value > > Don't use 'qemu_put_buffer_async' to put pages from the XBZRLE cache > Since the cache might change before the data hits the wire > > Signed-off-by: Dr. David Alan Gilbert > --- > arch_init.c | 64 ++++++++++++++++++++++++++++++++---------- > include/migration/page_cache.h | 2 +- > page_cache.c | 2 +- > 3 files changed, 51 insertions(+), 17 deletions(-) > > diff --git a/arch_init.c b/arch_init.c > index 80574a0..fe17279 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -122,7 +122,6 @@ static void check_guest_throttling(void); > #define RAM_SAVE_FLAG_XBZRLE 0x40 > /* 0x80 is reserved in migration.h start with 0x100 next */ > > - > static struct defconfig_file { > const char *filename; > /* Indicates it is an user config file (disabled by -no-user-config) */ > @@ -133,6 +132,7 @@ static struct defconfig_file { > { NULL }, /* end of list */ > }; > > +static const uint8_t ZERO_TARGET_PAGE[TARGET_PAGE_SIZE]; > > int qemu_read_default_config_files(bool userconfig) > { > @@ -273,6 +273,34 @@ static size_t save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset, > return size; > } > > +/* This is the last block that we have visited serching for dirty pages > + */ > +static RAMBlock *last_seen_block; > +/* This is the last block from where we have sent data */ > +static RAMBlock *last_sent_block; > +static ram_addr_t last_offset; > +static unsigned long *migration_bitmap; > +static uint64_t migration_dirty_pages; > +static uint32_t last_version; > +static bool ram_bulk_stage; > + > +/* Update the xbzrle cache to reflect a page that's been sent as all 0. > + * The important thing is that a stale (not-yet-0'd) page be replaced > + * by the new data. > + * As a bonus, if the page wasn't in the cache it gets added so that > + * when a small write is made into the 0'd page it gets XBZRLE sent > + */ > +static void xbzrle_cache_zero_page(ram_addr_t current_addr) > +{ > + if (ram_bulk_stage || !migrate_use_xbzrle()) { > + return; > + } > + > + /* We don't care if this fails to allocate a new cache page > + * as long as it updated an old one */ > + cache_insert(XBZRLE.cache, current_addr, ZERO_TARGET_PAGE); > +} > + > #define ENCODING_FLAG_XBZRLE 0x1 > > static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data, > @@ -329,18 +357,6 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data, > return bytes_sent; > } > > - > -/* This is the last block that we have visited serching for dirty pages > - */ > -static RAMBlock *last_seen_block; > -/* This is the last block from where we have sent data */ > -static RAMBlock *last_sent_block; > -static ram_addr_t last_offset; > -static unsigned long *migration_bitmap; > -static uint64_t migration_dirty_pages; > -static uint32_t last_version; > -static bool ram_bulk_stage; > - > static inline > ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr, > ram_addr_t start) > @@ -512,6 +528,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage) > } else { > int ret; > uint8_t *p; > + bool send_async = true; > int cont = (block == last_sent_block) ? > RAM_SAVE_FLAG_CONTINUE : 0; > > @@ -522,6 +539,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage) > ret = ram_control_save_page(f, block->offset, > offset, TARGET_PAGE_SIZE, &bytes_sent); > > + current_addr = block->offset + offset; > if (ret != RAM_SAVE_CONTROL_NOT_SUPP) { > if (ret != RAM_SAVE_CONTROL_DELAYED) { > if (bytes_sent > 0) { > @@ -536,19 +554,35 @@ static int ram_save_block(QEMUFile *f, bool last_stage) > RAM_SAVE_FLAG_COMPRESS); > qemu_put_byte(f, 0); > bytes_sent++; > + /* Must let xbzrle know, otherwise a previous (now 0'd) cached > + * page would be stale > + */ > + xbzrle_cache_zero_page(current_addr); > } else if (!ram_bulk_stage && migrate_use_xbzrle()) { > - current_addr = block->offset + offset; > bytes_sent = save_xbzrle_page(f, p, current_addr, block, > offset, cont, last_stage); > if (!last_stage) { > + /* We must send exactly what's in the xbzrle cache > + * even if the page wasn't xbzrle compressed, so that > + * it's right next time. > + */ > p = get_cached_data(XBZRLE.cache, current_addr); > + > + /* Can't send this cached data async, since the cache page > + * might get updated before it gets to the wire > + */ > + send_async = false; > } > } > > /* XBZRLE overflow or normal page */ > if (bytes_sent == -1) { > bytes_sent = save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE); > - qemu_put_buffer_async(f, p, TARGET_PAGE_SIZE); > + if (send_async) { > + qemu_put_buffer_async(f, p, TARGET_PAGE_SIZE); > + } else { > + qemu_put_buffer(f, p, TARGET_PAGE_SIZE); > + } > bytes_sent += TARGET_PAGE_SIZE; > acct_info.norm_pages++; > } > diff --git a/include/migration/page_cache.h b/include/migration/page_cache.h > index d156f0d..2d5ce2d 100644 > --- a/include/migration/page_cache.h > +++ b/include/migration/page_cache.h > @@ -66,7 +66,7 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr); > * @addr: page address > * @pdata: pointer to the page > */ > -int cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata); > +int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata); > > /** > * cache_resize: resize the page cache. In case of size reduction the extra > diff --git a/page_cache.c b/page_cache.c > index 3ef6ee7..b033681 100644 > --- a/page_cache.c > +++ b/page_cache.c > @@ -150,7 +150,7 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr) > return cache_get_by_addr(cache, addr)->it_data; > } > > -int cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata) > +int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata) > { > > CacheItem *it = NULL; > Reviewed-by: Orit Wasserman