From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56744) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQuVN-0002RI-TZ for qemu-devel@nongnu.org; Wed, 24 Oct 2012 02:31:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TQuVK-0005x9-8X for qemu-devel@nongnu.org; Wed, 24 Oct 2012 02:31:53 -0400 Received: from mail-we0-f173.google.com ([74.125.82.173]:54184) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQuVJ-0005x5-Ty for qemu-devel@nongnu.org; Wed, 24 Oct 2012 02:31:50 -0400 Received: by mail-we0-f173.google.com with SMTP id t11so81346wey.4 for ; Tue, 23 Oct 2012 23:31:49 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <50868F34.2030505@redhat.com> 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> <50868F34.2030505@redhat.com> From: liu ping fan Date: Wed, 24 Oct 2012 14:31:28 +0800 Message-ID: Content-Type: text/plain; charset=ISO-8859-1 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: Avi Kivity , Jan Kiszka Cc: Stefan Hajnoczi , Marcelo Tosatti , "qemu-devel@nongnu.org" , Anthony Liguori , Paolo Bonzini On Tue, Oct 23, 2012 at 8:36 PM, Avi Kivity wrote: > 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. > Yes, now the updater of physmap based on mem_map_lock, and the same it will be for readers. >>> >>> @@ -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. > Oh, here is a bug, need unref. As to unbalanced refcount, it will be adopted for virtio-blk listener (not implement in this patchset) Regards, pingfan > -- > error compiling committee.c: too many arguments to function >