From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50149) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzPDZ-00054p-Ow for qemu-devel@nongnu.org; Wed, 26 Oct 2016 10:30:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzPDV-0006H4-EE for qemu-devel@nongnu.org; Wed, 26 Oct 2016 10:30:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51516) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bzPDV-0006FF-6F for qemu-devel@nongnu.org; Wed, 26 Oct 2016 10:30:09 -0400 References: <20161024092151.32386-1-haozhong.zhang@intel.com> <20161024092151.32386-3-haozhong.zhang@intel.com> <20161025195028.GX5057@thinpad.lan.raisama.net> <20161026055648.cjsvoyhq65qrpmnm@hz-desktop> <20161026074929.rd4qn5tdrzfgpuf4@hz-desktop> <20161026141737.GY5057@thinpad.lan.raisama.net> From: Paolo Bonzini Message-ID: <74c0c8ce-e148-d0ac-92b7-9629d02f518e@redhat.com> Date: Wed, 26 Oct 2016 16:30:02 +0200 MIME-Version: 1.0 In-Reply-To: <20161026141737.GY5057@thinpad.lan.raisama.net> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] hostmem-file: allow option 'size' optional List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost , qemu-devel@nongnu.org, Igor Mammedov , Peter Crosthwaite , Richard Henderson , Xiao Guangrong On 26/10/2016 16:17, Eduardo Habkost wrote: > On Wed, Oct 26, 2016 at 03:49:29PM +0800, Haozhong Zhang wrote: >> On 10/26/16 13:56 +0800, Haozhong Zhang wrote: >>> On 10/25/16 17:50 -0200, Eduardo Habkost wrote: >>>> On Mon, Oct 24, 2016 at 05:21:51PM +0800, Haozhong Zhang wrote: >>>>> + if (memory) { >>>>> + memory = memory ?: file_size; >>>> >>>> This doesn't make sense to me. You already checked if memory is >>>> zero above, and now you are checking if it's zero again. >>>> file_size is never going to be used here. >>>> >>>>> + memory_region_set_size(block->mr, memory); >>>>> + memory = HOST_PAGE_ALIGN(memory); >>>>> + block->used_length = memory; >>>>> + block->max_length = memory; >>>> >>>> This is fragile: it duplicates the logic that initializes >>>> used_length and max_length in qemu_ram_alloc_*(). >>>> >>>> Maybe it's better to keep the file-size-probing magic inside >>>> hostmem-file.c, and always give a non-zero size to >>>> memory_region_init_ram_from_file(). >>>> >>> >>> Yes, I can move the logic in above if-statement to qemu_ram_alloc_from_file(). >>> >> >> Maybe no. Two reasons: >> 1) Patch 1 has some code related to file size and I'd like to put all >> logic related to file size in the same function. >> 2) file_ram_alloc() has the logic to open/create/close the backend file >> and handle errors in this process, which also needs to move to >> qemu_ram_alloc_from_file() if file-size-probing is moved. > > I'd argue to move the whole file creation/opening logic to > hostmem-file.c. But it wouldn't be a small amount of work, so > your points make sense. > > The plan below could work, but I would like it to get feedback > from the Memory API maintainer (Paolo). Yes, it makes sense. Paolo >> >> Instead, I'd >> 1) keep all logic related to file size in file_ram_alloc() >> >> 2) let file_ram_alloc() return the value of 'memory', e.g. >> static void *file_ram_alloc(RAMBlock *block, >> - ram_addr_t *memory, >> + ram_addr_t memory, >> const char *path, >> Error **errp) >> { >> ... >> // other usages of 'memory' are changed as well >> - memory = ROUND_UP(*memory, block->page_size); + *memory = >> ROUND_UP(*memory, block->page_size); >> ... >> } >> 3) in qemu_ram_alloc_from_file(), move setting >> block->used_length/max_length after calling file_ram_alloc(), >> e.g. >> RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr, >> bool share, const char *mem_path, >> Error **errp) >> { >> ... >> size = HOST_PAGE_ALIGN(size); >> new_block = g_malloc0(sizeof(*new_block)); >> new_block->mr = mr; >> - new_block->used_length = size; >> - new_block->max_length = size; >> new_block->flags = share ? RAM_SHARED : 0; >> - new_block->host = file_ram_alloc(new_block, size, >> + new_block->host = file_ram_alloc(new_block, &size, >> mem_path, errp); >> if (!new_block->host) { >> g_free(new_block); >> return NULL; >> } >> + new_block->used_length = size; >> + new_block->max_length = size; >> ... >> } >> >