From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59292) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFnwx-0006iw-0r for qemu-devel@nongnu.org; Tue, 18 Feb 2014 11:55:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFnwr-00006P-06 for qemu-devel@nongnu.org; Tue, 18 Feb 2014 11:55:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:32865) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFnwq-000058-Nw for qemu-devel@nongnu.org; Tue, 18 Feb 2014 11:55:08 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s1IGt7UQ011310 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 18 Feb 2014 11:55:07 -0500 From: Juan Quintela In-Reply-To: <1392320685-20609-1-git-send-email-dgilbert@redhat.com> (David Alan Gilbert's message of "Thu, 13 Feb 2014 19:44:45 +0000") References: <1392320685-20609-1-git-send-email-dgilbert@redhat.com> Date: Tue, 18 Feb 2014 17:55:05 +0100 Message-ID: <874n3wcqdy.fsf@elfo.mitica> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH] Fix two XBZRLE corruption issues Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert (git)" Cc: owasserm@redhat.com, qemu-devel@nongnu.org "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); > +} > + Can we use a cache_insert_zero() page to avoid the copy? Right now, we are inserting a zero page on the cache > #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); Shouldn't it be easy to just create a: cache_forget(current_addr) function and call it a day? I told that because if there is a different page on that slot on the cache, we are dropping it for no help? > } 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. > + */ The problem here wasn't that we weren't forgotten it? If we forgot it, data is not stalled to start with? And yes, I understand that if next change to the page is just writting a few bytes it is better to know that the page has been sent as all zeros. But if we don't resent the page, we have just dropped one item in the cache for no good reason. Ideas? >>From now on, this is about xbzrle, not about previous page. Further thought of the day: perhaps it is a good idea when sending a page to check if it is shorter from a zero page or from the previous page? And once that we are looking into this: In function save_xbzrle_page() } else if (encoded_len == -1) { DPRINTF("Overflow\n"); acct_info.xbzrle_overflows++; /* update data in the cache */ memcpy(prev_cached_page, current_data, TARGET_PAGE_SIZE); ^^^^^^^^^^^^ return -1; } Why aren't we using XBZRLE.current_buf there? > 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;