From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55962) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQdiZ-0001RE-8A for qemu-devel@nongnu.org; Tue, 23 Oct 2012 08:36:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TQdiT-0002uV-7f for qemu-devel@nongnu.org; Tue, 23 Oct 2012 08:36:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:19580) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQdiS-0002uM-Vu for qemu-devel@nongnu.org; Tue, 23 Oct 2012 08:36:17 -0400 Message-ID: <50868F34.2030505@redhat.com> Date: Tue, 23 Oct 2012 14:36:04 +0200 From: Avi Kivity MIME-Version: 1.0 References: <1350897839-29593-1-git-send-email-pingfank@linux.vnet.ibm.com> <1350897839-29593-8-git-send-email-pingfank@linux.vnet.ibm.com> <508689B0.5060904@siemens.com> In-Reply-To: <508689B0.5060904@siemens.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [patch v4 07/16] memory: make mmio dispatch able to be out of biglock List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Liu Ping Fan , Stefan Hajnoczi , Marcelo Tosatti , "qemu-devel@nongnu.org" , Anthony Liguori , Paolo Bonzini On 10/23/2012 02:12 PM, Jan Kiszka wrote: > On 2012-10-22 11:23, Liu Ping Fan wrote: >> Without biglock, we try to protect the mr by increase refcnt. >> If we can inc refcnt, go backward and resort to biglock. >> >> Another point is memory radix-tree can be flushed by another >> thread, so we should get the copy of terminal mr to survive >> from such issue. >> >> + >> void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf, >> int len, int is_write) >> { >> @@ -3413,14 +3489,28 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf, >> uint8_t *ptr; >> uint32_t val; >> target_phys_addr_t page; >> - MemoryRegionSection *section; >> + MemoryRegionSection *section, obj_mrs; >> + int safe_ref; >> >> while (len > 0) { >> page = addr & TARGET_PAGE_MASK; >> l = (page + TARGET_PAGE_SIZE) - addr; >> if (l > len) >> l = len; >> - section = phys_page_find(page >> TARGET_PAGE_BITS); >> + qemu_mutex_lock(&mem_map_lock); >> + safe_ref = phys_page_lookup(page, &obj_mrs); >> + qemu_mutex_unlock(&mem_map_lock); >> + if (safe_ref == 0) { >> + qemu_mutex_lock_iothread(); >> + qemu_mutex_lock(&mem_map_lock); >> + /* At the 2nd try, mem map can change, so need to judge it again */ >> + safe_ref = phys_page_lookup(page, &obj_mrs); >> + qemu_mutex_unlock(&mem_map_lock); >> + if (safe_ref > 0) { >> + qemu_mutex_unlock_iothread(); >> + } >> + } >> + section = &obj_mrs; >> >> if (is_write) { >> if (!memory_region_is_ram(section->mr)) { >> @@ -3491,10 +3581,16 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf, >> qemu_put_ram_ptr(ptr); >> } >> } >> + >> + memory_region_section_unref(&obj_mrs); > > The mapping cannot change from not-referenced to reference-counted while > we were dispatching? I mean the case where we found not ref callback on > entry and took the big lock, but now there is an unref callback. We drop the big lock in that case, so we end up in the same situation. > >> len -= l; >> buf += l; >> addr += l; >> + if (safe_ref == 0) { >> + qemu_mutex_unlock_iothread(); >> + } >> } >> + >> } >> >> /* used for ROM loading : can write in RAM and ROM */ >> @@ -3504,14 +3600,18 @@ void cpu_physical_memory_write_rom(target_phys_addr_t addr, >> int l; >> uint8_t *ptr; >> target_phys_addr_t page; >> - MemoryRegionSection *section; >> + MemoryRegionSection *section, mr_obj; >> >> while (len > 0) { >> page = addr & TARGET_PAGE_MASK; >> l = (page + TARGET_PAGE_SIZE) - addr; >> if (l > len) >> l = len; >> - section = phys_page_find(page >> TARGET_PAGE_BITS); >> + >> + qemu_mutex_lock(&mem_map_lock); >> + phys_page_lookup(page, &mr_obj); >> + qemu_mutex_unlock(&mem_map_lock); >> + section = &mr_obj; > > But here we don't care about the return code of phys_page_lookup and all > related topics? Because we assume the BQL is held? Reminds me that we > will need some support for assert(qemu_mutex_is_locked(&lock)). I guess it's better to drop that assumption than to have asymmetric APIs. >> >> @@ -4239,9 +4345,12 @@ bool virtio_is_big_endian(void) >> #ifndef CONFIG_USER_ONLY >> bool cpu_physical_memory_is_io(target_phys_addr_t phys_addr) >> { >> - MemoryRegionSection *section; >> + MemoryRegionSection *section, mr_obj; >> >> - section = phys_page_find(phys_addr >> TARGET_PAGE_BITS); >> + qemu_mutex_lock(&mem_map_lock); >> + phys_page_lookup(phys_addr, &mr_obj); >> + qemu_mutex_unlock(&mem_map_lock); >> + section = &mr_obj; > > Err, no unref needed here? Need _ref in the name to remind reviewers that it leaves the refcount unbalanced. -- error compiling committee.c: too many arguments to function