From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48534) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YW5oW-0008DM-SZ for qemu-devel@nongnu.org; Thu, 12 Mar 2015 12:18:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YW5oR-0005ty-Rq for qemu-devel@nongnu.org; Thu, 12 Mar 2015 12:18:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42621) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YW5oR-0005tj-Jl for qemu-devel@nongnu.org; Thu, 12 Mar 2015 12:18:19 -0400 From: Juan Quintela In-Reply-To: <20150224103731.GD26558@grmbl.mre> (Amit Shah's message of "Tue, 24 Feb 2015 16:07:31 +0530") References: <1423778591-12590-1-git-send-email-quintela@redhat.com> <1423778591-12590-5-git-send-email-quintela@redhat.com> <20150224103731.GD26558@grmbl.mre> Date: Thu, 12 Mar 2015 16:48:43 +0100 Message-ID: <87mw3ip678.fsf@neno.neno> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 4/6] save_xbzrle_page: change calling convention Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: liang.z.li@intel.com, qemu-devel@nongnu.org Amit Shah wrote: > On (Thu) 12 Feb 2015 [23:03:09], Juan Quintela wrote: >> Add a parameter to pass the number of bytes written, and make it return >> the number of pages written instead. >> >> Signed-off-by: Juan Quintela >> --- >> arch_init.c | 44 +++++++++++++++++++++++++------------------- >> 1 file changed, 25 insertions(+), 19 deletions(-) >> >> diff --git a/arch_init.c b/arch_init.c >> index f243c6e..834f40c 100644 >> --- a/arch_init.c >> +++ b/arch_init.c >> @@ -352,11 +352,27 @@ static void xbzrle_cache_zero_page(ram_addr_t current_addr) >> >> #define ENCODING_FLAG_XBZRLE 0x1 >> >> +/** >> + * save_xbzrle_page: compress and send current page >> + * >> + * Returns: 1 means that we wrote the page >> + * 0 means that page is identical to the one already sent >> + * -1 means that xbzrle would be longer than normal >> + * >> + * @f: QEMUFile where to send the data >> + * @current_data: >> + * @current_addr: >> + * @block: block that contains the page we want to send >> + * @offset: offset inside the block for the page >> + * @last_stage: if we are at the completion stage >> + * @bytes_transferred: increase it with the number of transferred bytes >> + */ >> static int save_xbzrle_page(QEMUFile *f, uint8_t **current_data, >> ram_addr_t current_addr, RAMBlock *block, >> - ram_addr_t offset, int cont, bool last_stage) >> + ram_addr_t offset, int cont, bool last_stage, >> + uint64_t *bytes_transferred) >> { >> - int encoded_len = 0, bytes_sent = -1; >> + int encoded_len = 0, bytes_xbzrle; >> uint8_t *prev_cached_page; >> >> if (!cache_is_cached(XBZRLE.cache, current_addr, bitmap_sync_count)) { > > This patch exposes a bug in here: > > if (!cache_is_cached(XBZRLE.cache, current_addr, bitmap_sync_count)) { > acct_info.xbzrle_cache_miss++; > if (!last_stage) { > if (cache_insert(XBZRLE.cache, current_addr, *current_data, > bitmap_sync_count) == -1) { > return -1; > } else { > /* update *current_data when the page has been > inserted into cache */ > *current_data = get_cached_data(XBZRLE.cache, current_addr); > } > } > return -1; > } > > The return -1 should not be done for a successful insert of page in > cache. Why? Return -1 means: >> + * -1 means that xbzrle would be longer than normal in the case that page was not on the cache, we just sent the page as normal page, that is what we want. > > Also, return -1 for both cache overflow and cache miss are perhaps not > relevant for the caller but the doctext needs to be updated. -1 means: send the full page, please. I am not sure that caller want to know if it is because the cache was full or because the page was not in cache, or because the page was not "comprhensible". Later, Juan.