From patchwork Tue Mar 14 21:23:29 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Xu, Anthony" X-Patchwork-Id: 738941 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3vjSRM23Nyz9s2Q for ; Wed, 15 Mar 2017 08:24:13 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="key not found in DNS" (0-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="LYQxg0/B"; dkim-atps=neutral Received: from localhost ([::1]:33551 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cntvK-0002Cc-8g for incoming@patchwork.ozlabs.org; Tue, 14 Mar 2017 17:24:06 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56741) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cntus-0002CL-6j for qemu-devel@nongnu.org; Tue, 14 Mar 2017 17:23:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cntup-0004C1-0u for qemu-devel@nongnu.org; Tue, 14 Mar 2017 17:23:38 -0400 Received: from mga14.intel.com ([192.55.52.115]:49476) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cntuo-0004B5-JD for qemu-devel@nongnu.org; Tue, 14 Mar 2017 17:23:34 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=intel.com; i=@intel.com; q=dns/txt; s=intel; t=1489526614; x=1521062614; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=+THe110CM/pRN8Apuby9ah/rlNGfUUdCtJ6/jgn7nF8=; b=LYQxg0/BWey07P6xGqpvCGOq1OGjTV8Njtmx2MLSPzBxXGwO9zgFBgYt r25nFlM0sCaZAyHcM6daz2OeyeJo3A==; Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Mar 2017 14:23:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,165,1486454400"; d="scan'208";a="944302815" Received: from orsmsx103.amr.corp.intel.com ([10.22.225.130]) by orsmga003.jf.intel.com with ESMTP; 14 Mar 2017 14:23:31 -0700 Received: from orsmsx158.amr.corp.intel.com (10.22.240.20) by ORSMSX103.amr.corp.intel.com (10.22.225.130) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 14 Mar 2017 14:23:31 -0700 Received: from orsmsx112.amr.corp.intel.com ([169.254.3.204]) by ORSMSX158.amr.corp.intel.com ([169.254.10.135]) with mapi id 14.03.0248.002; Tue, 14 Mar 2017 14:23:30 -0700 From: "Xu, Anthony" To: Paolo Bonzini Thread-Topic: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB Thread-Index: AQHSmW5s7VgxEgkfD0KNa0Uxoqhl0qGOUJAAgAAAp8AZksd/6v842HEggADaOoD///owIA== Date: Tue, 14 Mar 2017 21:23:29 +0000 Message-ID: <4712D8F4B26E034E80552F30A67BE0B1A1561C@ORSMSX112.amr.corp.intel.com> References: <1489158897-9206-1-git-send-email-yang.zhong@intel.com> <1489158897-9206-2-git-send-email-yang.zhong@intel.com> <53522014-8f70-9358-7d60-8b6c491c9611@redhat.com> <4712D8F4B26E034E80552F30A67BE0B1A0F91A@ORSMSX112.amr.corp.intel.com> <920352018.1797331.1489251855464.JavaMail.zimbra@redhat.com> <4712D8F4B26E034E80552F30A67BE0B1A13045@ORSMSX112.amr.corp.intel.com> <991a0f3d-5e1f-d17c-a8f3-a42c02ac2f17@redhat.com> In-Reply-To: <991a0f3d-5e1f-d17c-a8f3-a42c02ac2f17@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTI0ZDk2NDctNWI2Yy00MzI5LWFmNTMtMDM1YWY4Nzg2MDJlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6ImYwOFNFM1ROS2N2cUtsUUF1RDRXOFFlYXlvQ1RGSWJ0RmZIMnpnQnJrMmM9In0= x-originating-ip: [10.22.254.139] MIME-Version: 1.0 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 192.55.52.115 Subject: Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "Zhong, Yang" , "Peng, Chao P" , "qemu-devel@nongnu.org" Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" > -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: Tuesday, March 14, 2017 3:15 AM > To: Xu, Anthony > Cc: Zhong, Yang ; qemu-devel@nongnu.org; Peng, > Chao P > Subject: Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from > 12252kB to 2752KB > > > > On 14/03/2017 06:14, Xu, Anthony wrote: > > Below functions are registered in RCU thread > > address_space_dispatch_free, > > do_address_space_destroy > > flatview_unref > > reclaim_ramblock, > > qht_map_destroy, > > migration_bitmap_free > > > > first three are address space related, should work without global lock per > above analysis. > > The rest are very simple, seems doesn't need global lock. > > flatview_unref can call object_unref and thus reach: Okay, flatview_unref is the one you worried about, Flatview_unref is registered as a RCU callback only in address_space_update_topology, Strangely, it is registered as a RCU callback, and is called directly in this function. Basially flatview_unref is called twice for old view. There are some comments, but it is not clear to me, is this a bug or by design? Is flatview_destroy called in current thread or RCU thread? static void address_space_update_topology(AddressSpace *as) { FlatView *old_view = address_space_get_flatview(as); FlatView *new_view = generate_memory_topology(as->root); address_space_update_topology_pass(as, old_view, new_view, false); address_space_update_topology_pass(as, old_view, new_view, true); /* Writes are protected by the BQL. */ atomic_rcu_set(&as->current_map, new_view); call_rcu(old_view, flatview_unref, rcu); /* Note that all the old MemoryRegions are still alive up to this * point. This relieves most MemoryListeners from the need to * ref/unref the MemoryRegions they get---unless they use them * outside the iothread mutex, in which case precise reference * counting is necessary. */ flatview_unref(old_view); address_space_update_ioeventfds(as); } Let me split the patch, Do you think below patch is correct? > > - all QOM instance_finalize callbacks > > - all QOM property release callbacks > > In turn, of QOM property release callbacks the more important ones are > release_drive (which calls blockdev_auto_del and blk_detach_dev) and > release_chr (which calls qemu_chr_fe_deinit). > > Your patch is incorrect, sorry. If it were that simple, it would have > been done already... > > Paolo --- a/memory.c +++ b/memory.c @@ -1503,15 +1503,9 @@ static void memory_region_finalize(Object *obj) * and cause an infinite loop. */ mr->enabled = false; - memory_region_transaction_begin(); - while (!QTAILQ_EMPTY(&mr->subregions)) { - MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions); - memory_region_del_subregion(mr, subregion); - } - memory_region_transaction_commit(); - + assert(QTAILQ_EMPTY(&mr->subregions)); mr->destructor(mr); - memory_region_clear_coalescing(m + assert(QTAILQ_EMPTY(&mr->coalesced)); g_free((char *)mr->name); g_free(mr->ioeventfds); } Then let us take a look at flatview_unref, memory_region_unref may call any QOM finalize through Object_unref(mr->owner), that's your concern, I got it. It is way too complicated to look at each QOM object release callbacks and each QOM property release callbacks. I gave up this path. How about fall back to synchronize_rcu? As for address space, the RCU read lock is used to protect PhysPageMap, but not the regular MemoryRegions, because that are not allocated in PhysPageMap build. Subpage MemoryRegion is an exception, because that is allocated in PhysPageMap build. The MemoryRegions returned from address_space_translate are regular MemoryRegions, so address_space_write_continue and address_space_read_continue don't need RCU read lock. Below patch reclaim address space in synchronized way, it reduces heap size from ~12MB to ~3MB. You need to apply this patch with above patch. And when address_space_dispatch_free is called it already holds the global lock, we don't need to acquire the global lock in address_space_dispatch_free. Please review this patch. Thanks, Anthony diff --git a/cputlb.c b/cputlb.c index 6c39927..98bd21f 100644 --- a/cputlb.c +++ b/cputlb.c @@ -347,6 +347,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, tlb_add_large_page(env, vaddr, size); } + rcu_read_lock(); sz = size; section = address_space_translate_for_iotlb(cpu, asidx, paddr, &xlat, &sz); assert(sz >= TARGET_PAGE_SIZE); @@ -406,6 +407,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, } else { te->addr_write = -1; } + rcu_read_unlock(); } /* Add a new TLB entry, but without specifying the memory diff --git a/exec.c b/exec.c index 6fa337b..446d622 100644 --- a/exec.c +++ b/exec.c @@ -455,7 +455,7 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr, IOMMUTLBEntry iotlb = {0}; MemoryRegionSection *section; MemoryRegion *mr; - + rcu_read_lock(); for (;;) { AddressSpaceDispatch *d = atomic_rcu_read(&as->dispatch); section = address_space_lookup_region(d, addr, false); @@ -477,7 +477,7 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr, | (addr & iotlb.addr_mask)); as = iotlb.target_as; } - + rcu_read_unlock(); return iotlb; } @@ -490,6 +490,7 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr, MemoryRegionSection *section; MemoryRegion *mr; + rcu_read_lock(); for (;;) { AddressSpaceDispatch *d = atomic_rcu_read(&as->dispatch); section = address_space_translate_internal(d, addr, &addr, plen, true); @@ -517,6 +518,7 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr, } *xlat = addr; + rcu_read_unlock(); return mr; } @@ -2413,7 +2415,8 @@ static void mem_commit(MemoryListener *listener) atomic_rcu_set(&as->dispatch, next); if (cur) { - call_rcu(cur, address_space_dispatch_free, rcu); + synchronize_rcu(); + address_space_dispatch_free(cur); } } @@ -2459,7 +2462,8 @@ void address_space_destroy_dispatch(AddressSpace *as) atomic_rcu_set(&as->dispatch, NULL); if (d) { - call_rcu(d, address_space_dispatch_free, rcu); + synchronize_rcu(); + address_space_dispatch_free(d); } } @@ -2686,12 +2690,10 @@ MemTxResult address_space_write(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, MemTxResult result = MEMTX_OK; if (len > 0) { - rcu_read_lock(); l = len; mr = address_space_translate(as, addr, &addr1, &l, true); result = address_space_write_continue(as, addr, attrs, buf, len, addr1, l, mr); - rcu_read_unlock(); } return result; @@ -2776,12 +2778,10 @@ MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr, MemTxResult result = MEMTX_OK; if (len > 0) { - rcu_read_lock(); l = len; mr = address_space_translate(as, addr, &addr1, &l, false); result = address_space_read_continue(as, addr, attrs, buf, len, addr1, l, mr); - rcu_read_unlock(); } return result; diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index febe519..3557ac5 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -914,8 +914,6 @@ void vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write) IOMMUTLBEntry iotlb; uint64_t uaddr, len; - rcu_read_lock(); - iotlb = address_space_get_iotlb_entry(dev->vdev->dma_as, iova, write); if (iotlb.target_as != NULL) { @@ -936,7 +934,7 @@ void vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write) } } out: - rcu_read_unlock(); + return; } static int vhost_virtqueue_start(struct vhost_dev *dev,