From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39238) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a4OL6-0007mo-HI for qemu-devel@nongnu.org; Thu, 03 Dec 2015 02:30:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a4OL3-00025g-9i for qemu-devel@nongnu.org; Thu, 03 Dec 2015 02:30:04 -0500 Received: from szxga03-in.huawei.com ([119.145.14.66]:27788) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a4OL1-00024G-SK for qemu-devel@nongnu.org; Thu, 03 Dec 2015 02:30:01 -0500 References: <1448357149-17572-1-git-send-email-zhang.zhanghailiang@huawei.com> <1448357149-17572-15-git-send-email-zhang.zhanghailiang@huawei.com> <20151201181905.GC31209@work-vm> <565FED03.3060108@huawei.com> From: Hailiang Zhang Message-ID: <565FEF5B.4070204@huawei.com> Date: Thu, 3 Dec 2015 15:29:31 +0800 MIME-Version: 1.0 In-Reply-To: <565FED03.3060108@huawei.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH COLO-Frame v11 14/39] ram: Split host_from_stream_offset() into two helper functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: lizhijian@cn.fujitsu.com, quintela@redhat.com, yunhong.jiang@intel.com, eddie.dong@intel.com, peter.huangpeng@huawei.com, qemu-devel@nongnu.org, arei.gonglei@huawei.com, stefanha@redhat.com, amit.shah@redhat.com, hongyang.yang@easystack.cn On 2015/12/3 15:19, Hailiang Zhang wrote: > On 2015/12/2 2:19, Dr. David Alan Gilbert wrote: >> * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote: >>> Split host_from_stream_offset() into two parts: >>> One is to get ram block, which the block idstr may be get from migration >>> stream, the other is to get hva (host) address from block and the offset. >>> >>> Signed-off-by: zhanghailiang >> >> OK, I see why you're doing this from the next patch. >> >>> --- >>> v11: >>> - New patch >>> --- >>> migration/ram.c | 29 +++++++++++++++++++++-------- >>> 1 file changed, 21 insertions(+), 8 deletions(-) >>> >>> diff --git a/migration/ram.c b/migration/ram.c >>> index cfe78aa..a161620 100644 >>> --- a/migration/ram.c >>> +++ b/migration/ram.c >>> @@ -2136,9 +2136,9 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) >>> * offset: Offset within the block >>> * flags: Page flags (mostly to see if it's a continuation of previous block) >>> */ >>> -static inline void *host_from_stream_offset(QEMUFile *f, >>> - ram_addr_t offset, >>> - int flags) >>> +static inline RAMBlock *ram_block_from_stream(QEMUFile *f, >>> + ram_addr_t offset, >>> + int flags) >>> { >>> static RAMBlock *block = NULL; >>> char id[256]; >>> @@ -2150,22 +2150,31 @@ static inline void *host_from_stream_offset(QEMUFile *f, >>> return NULL; >>> } >>> >>> - return block->host + offset; >>> + return block; >>> } >>> - >>> len = qemu_get_byte(f); >>> qemu_get_buffer(f, (uint8_t *)id, len); >>> id[len] = 0; >>> >>> block = qemu_ram_block_by_name(id); >>> if (block && block->max_length > offset) { >>> - return block->host + offset; >>> + return block; >>> } >>> >>> error_report("Can't find block %s", id); >>> return NULL; >>> } >>> >>> +static inline void *host_from_ram_block_offset(RAMBlock *block, >>> + ram_addr_t offset) >>> +{ >>> + if (!block) { >>> + return NULL; >>> + } >>> + >>> + return block->host + offset; >>> +} >> >> That's almost the same as ramblock_ptr in include/exec/ram_addr.h, but >> it assert's rather than doing NULL on errors. >> > > Hmm, i didn't notice that before. > >> I'm not sure about this, but can I suggest: >> >> ram_block_from_stream(QEMUFile *f, int flags) >> >> doesn't have the offset; just finds the block and handles the CONT. >> >> bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset); >> >> actually does the check; put this in exec.c, and declare it in include/exec/ram_addr.h >> >> void *ramblock_ptr_try(RAMBlock *block, ram_addr_t offset) >> which returns NULL if offset_in_ramblock fails, and otherwise returns the result >> of ramblock_ptr - again put that in include/exec/ram_addr.h >> > > That sounds good. I will add the ramblock_ptr_try() and keep the ramblock_ptr(). They Keeping the name host_from_ram_block_offset() seems better, it corresponds with the name colo_cache_from_block_offset() in next patch. ;) > have different behavior when the check failed. I'm not supposed to break it. > >> (I'm not sure about this - I almost suggested changing ramblock_ptr to not do >> the checks, and just add a call to assert(offset_in_ramblock) before each use, but >> that sounded too painful). >> > > Yes, that's not so good. > >> Hmm - we check here for block->max_length > offset - where as the check in >> ram_addr.h is used_length - I wonder if we should be using used_length? >> > > Er, i think we should use used_length instead of max_length. > > Thanks, > zhanghailiang > >> Dave >> >>> + >>> /* >>> * If a page (or a whole RDMA chunk) has been >>> * determined to be zero, then zap it. >>> @@ -2310,7 +2319,9 @@ static int ram_load_postcopy(QEMUFile *f) >>> trace_ram_load_postcopy_loop((uint64_t)addr, flags); >>> place_needed = false; >>> if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE)) { >>> - host = host_from_stream_offset(f, addr, flags); >>> + RAMBlock *block = ram_block_from_stream(f, addr, flags); >>> + >>> + host = host_from_ram_block_offset(block, addr); >>> if (!host) { >>> error_report("Illegal RAM offset " RAM_ADDR_FMT, addr); >>> ret = -EINVAL; >>> @@ -2441,7 +2452,9 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) >>> >>> if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE | >>> RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) { >>> - host = host_from_stream_offset(f, addr, flags); >>> + RAMBlock *block = ram_block_from_stream(f, addr, flags); >>> + >>> + host = host_from_ram_block_offset(block, addr); >>> if (!host) { >>> error_report("Illegal RAM offset " RAM_ADDR_FMT, addr); >>> ret = -EINVAL; >>> -- >>> 1.8.3.1 >>> >>> >> -- >> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >> >> . >> >