[{"id":1770208,"web_url":"http://patchwork.ozlabs.org/comment/1770208/","msgid":"","list_archive_url":null,"date":"2017-09-18T14:37:31","subject":"Re: [Qemu-devel] [PATCH qemu v3 11/13] memory: Share FlatView's and\n\tdispatch trees between address spaces","submitter":{"id":2701,"url":"http://patchwork.ozlabs.org/api/people/2701/","name":"Paolo Bonzini","email":"pbonzini@redhat.com"},"content":"On 18/09/2017 12:17, Alexey Kardashevskiy wrote:\n> + physmr = memory_region_unalias_entire(old_view->root);\n> +\n> + key = !physmr->enabled ? 0 : physmr;\n> + new_view = (FlatView *) g_hash_table_lookup(views, key);\n> + if (new_view) {\n> + continue;\n> + }\n> +\n> + new_view = generate_memory_topology(physmr);\n> +\n> + new_view->dispatch = address_space_dispatch_new(new_view);\n> + for (i = 0; i < new_view->nr; i++) {\n> + MemoryRegionSection mrs =\n> + section_from_flat_range(&new_view->ranges[i], new_view);\n> + flatview_add_to_dispatch(new_view, &mrs);\n> + }\n> + address_space_dispatch_compact(new_view->dispatch);\n> +\n> + g_hash_table_insert(views, key, new_view);\n> + }\n> +\n> + /* Replace FVs in ASes */\n> + QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {\n> + old_view = as->current_map;\n> + physmr = memory_region_unalias_entire(old_view->root);\n> +\n> + key = !physmr->enabled ? 0 : physmr;\n\nThis is duplicate, perhaps memory_region_unalias_entire should do it\ninstead? Does it make sense for flatview->root to be NULL, or does it\nbreak something else? If something breaks, disregard the remaining\ncomments.\n\n(BTW, maybe you can rename the function to memory_region_get_flatview_root).\n\n> \n> +\n> + /* Unref FVs from temporary table */\n> + g_hash_table_foreach_remove(views, flatview_unref_g_hash, 0);\n> + g_hash_table_unref(views);\n> }\n\nYou can avoid g_hash_table_foreach_remove and also flatview_unref_g_hash\ninstead use g_hash_table_new_full (casting flatview_unref to\nGDestroyNotify should work fine).\n\n> \n> @@ -2690,13 +2721,6 @@ AddressSpace *address_space_init_shareable(MemoryRegion *root, const char *name)\n> {\n> AddressSpace *as;\n> \n> - QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {\n> - if (root == as->root && as->malloced) {\n> - as->ref_count++;\n> - return as;\n> - }\n> - }\n> -\n> as = g_malloc0(sizeof *as);\n> address_space_init(as, root, name);\n> as->malloced = true;\n\nIs this on purpose because it's now pointless? Maybe it should be\npointed out in the commit message.\n\nPaolo","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=pbonzini@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xwpX6073Mz9s7F\n\tfor ;\n\tTue, 19 Sep 2017 00:38:14 +1000 (AEST)","from localhost ([::1]:37021 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dtxBc-0004HU-4l\n\tfor incoming@patchwork.ozlabs.org; Mon, 18 Sep 2017 10:38:12 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:54794)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dtxB6-0004GD-EJ\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 10:37:41 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dtxB2-0004ly-61\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 10:37:40 -0400","from mx1.redhat.com ([209.132.183.28]:41900)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dtxB1-0004lN-Vw\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 10:37:36 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 1F357356D3;\n\tMon, 18 Sep 2017 14:37:34 +0000 (UTC)","from [10.32.181.85] (unknown [10.32.181.85])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 870A95D6A6;\n\tMon, 18 Sep 2017 14:37:33 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 1F357356D3","To":"Alexey Kardashevskiy , qemu-devel@nongnu.org","References":"<20170918101709.30421-1-aik@ozlabs.ru>\n\t<20170918101709.30421-12-aik@ozlabs.ru>","From":"Paolo Bonzini ","Message-ID":"","Date":"Mon, 18 Sep 2017 16:37:31 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170918101709.30421-12-aik@ozlabs.ru>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.30]);\n\tMon, 18 Sep 2017 14:37:34 +0000 (UTC)","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH qemu v3 11/13] memory: Share FlatView's and\n\tdispatch trees between address spaces","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]