[{"id":1769073,"web_url":"http://patchwork.ozlabs.org/comment/1769073/","msgid":"<165a7fa9-6ebb-3e95-2d95-7df2f550f1de@redhat.com>","list_archive_url":null,"date":"2017-09-15T09:25:59","subject":"Re: [Qemu-devel] [PATCH qemu v2 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 15/09/2017 10:40, Alexey Kardashevskiy wrote:\n> +\n> +static bool flatview_can_share(FlatView *old_view, FlatView *new_view)\n> +{\n> + MemoryRegion *old_root = memory_region_unalias_entire(old_view->root);\n> + MemoryRegion *new_root = memory_region_unalias_entire(new_view->root);\n> +\n> + if (old_root == new_root) {\n> + return true;\n> + }\n> +\n> + if (!old_root->enabled && !new_root->enabled) {\n> + return true;\n> + }\n> +\n> + return false;\n> +}\n> +\n\nI think you can improve this by keeping the root in the address space\n(removing the previous patch). Instead, the FlatView's root can be the\none with resolved aliases. Then old_root is just old_view->root, and if\nan empty FlatView has a NULL root this just becomes\n\n the_other_view->root == memory_region_unalias_entire(as->root);\n\n> \n> +\n> + /* Build list of unique FlatViews, FV::root is the key */\n> + QTAILQ_FOREACH(old_view, &flat_views, flat_views_link) {\n> + found = false;\n> + QTAILQ_FOREACH(new_view, &fvs_tmp, flat_views_link) {\n> + if (flatview_can_share(old_view, new_view)) {\n> + found = true;\n> + break;\n> + }\n> + }\n> + if (found) {\n> + continue;\n> + }\n> +\n> + new_view = generate_memory_topology(old_view->root);\n> + flatview_render_new(old_view, new_view);\n> + QTAILQ_INSERT_TAIL(&fvs_tmp, new_view, flat_views_link);\n> + }\n\nI don't understand the algorithm here. Why is it visiting &flat_views\nrather than the list of address spaces? I would have thought it enough\nto do\n\n for each address space\n if there is a (new) flat view that we can share\n add a reference\n else\n render a new flat view and add it to fvs_tmp\n\n flat_views = fvs_tmp;\n\n for each address space\n old_view = address_space_to_flatview(as);\n find the new flat view to use in fvs_tmp\n address_space_update_topology_pass(..., false);\n address_space_update_topology_pass(..., true);\n QTAILQ_REMOVE(&old_view->address_spaces, ...);\n atomic_rcu_set(&as->current_map, new_view);\n flatview_unref(old_view);\n QTAILQ_INSERT_TAIL(&new_view->address_spaces, ...);\n\n\nIt's also not clear to me what you need the FlatView's address_spaces\nlist for. (It's probably something trivial that I'm missing, or maybe\nit goes away with the previous change).\n\n> \n> 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 == address_space_root(as) && 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> +\n> return as;\n> }\n> \n\nThis belongs in the next patch; leaving as->malloced in\ndo_address_space_destroy and the reference count in\naddress_space_destroy is not a big complication.\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-mx01.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx01.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 3xtqm20jxHz9rxj\n\tfor ;\n\tFri, 15 Sep 2017 19:26:42 +1000 (AEST)","from localhost ([::1]:52219 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dsmtU-0001Dt-3g\n\tfor incoming@patchwork.ozlabs.org; Fri, 15 Sep 2017 05:26:40 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:56259)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dsmsw-0001Cw-GR\n\tfor qemu-devel@nongnu.org; Fri, 15 Sep 2017 05:26:07 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dsmss-0003sj-Gk\n\tfor qemu-devel@nongnu.org; Fri, 15 Sep 2017 05:26:06 -0400","from mx1.redhat.com ([209.132.183.28]:45222)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dsmss-0003sF-7u\n\tfor qemu-devel@nongnu.org; Fri, 15 Sep 2017 05:26:02 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\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 4725A81DFD;\n\tFri, 15 Sep 2017 09:26:01 +0000 (UTC)","from [10.36.117.159] (ovpn-117-159.ams2.redhat.com [10.36.117.159])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 9C27C78C1F;\n\tFri, 15 Sep 2017 09:26:00 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 4725A81DFD","To":"Alexey Kardashevskiy , qemu-devel@nongnu.org","References":"<20170915084030.40988-1-aik@ozlabs.ru>\n\t<20170915084030.40988-12-aik@ozlabs.ru>","From":"Paolo Bonzini ","Message-ID":"<165a7fa9-6ebb-3e95-2d95-7df2f550f1de@redhat.com>","Date":"Fri, 15 Sep 2017 11:25:59 +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":"<20170915084030.40988-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.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.25]);\n\tFri, 15 Sep 2017 09:26:01 +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 v2 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"}},{"id":1769531,"web_url":"http://patchwork.ozlabs.org/comment/1769531/","msgid":"<14f70953-dc95-6845-04be-07ee1615117e@ozlabs.ru>","list_archive_url":null,"date":"2017-09-16T00:58:40","subject":"Re: [Qemu-devel] [PATCH qemu v2 11/13] memory: Share FlatView's and\n\tdispatch trees between address spaces","submitter":{"id":7621,"url":"http://patchwork.ozlabs.org/api/people/7621/","name":"Alexey Kardashevskiy","email":"aik@ozlabs.ru"},"content":"On 15/09/17 19:25, Paolo Bonzini wrote:\n> On 15/09/2017 10:40, Alexey Kardashevskiy wrote:\n>> +\n>> +static bool flatview_can_share(FlatView *old_view, FlatView *new_view)\n>> +{\n>> + MemoryRegion *old_root = memory_region_unalias_entire(old_view->root);\n>> + MemoryRegion *new_root = memory_region_unalias_entire(new_view->root);\n>> +\n>> + if (old_root == new_root) {\n>> + return true;\n>> + }\n>> +\n>> + if (!old_root->enabled && !new_root->enabled) {\n>> + return true;\n>> + }\n>> +\n>> + return false;\n>> +}\n>> +\n> \n> I think you can improve this by keeping the root in the address space\n> (removing the previous patch). Instead, the FlatView's root can be the\n> one with resolved aliases. Then old_root is just old_view->root, and if\n> an empty FlatView has a NULL root this just becomes\n> \n> the_other_view->root == memory_region_unalias_entire(as->root);\n\nOk!\n\n\n> \n>>\n>> +\n>> + /* Build list of unique FlatViews, FV::root is the key */\n>> + QTAILQ_FOREACH(old_view, &flat_views, flat_views_link) {\n>> + found = false;\n>> + QTAILQ_FOREACH(new_view, &fvs_tmp, flat_views_link) {\n>> + if (flatview_can_share(old_view, new_view)) {\n>> + found = true;\n>> + break;\n>> + }\n>> + }\n>> + if (found) {\n>> + continue;\n>> + }\n>> +\n>> + new_view = generate_memory_topology(old_view->root);\n>> + flatview_render_new(old_view, new_view);\n>> + QTAILQ_INSERT_TAIL(&fvs_tmp, new_view, flat_views_link);\n>> + }\n> \n> I don't understand the algorithm here. Why is it visiting &flat_views\n> rather than the list of address spaces? I would have thought it enough\n> to do\n> \n> for each address space\n> if there is a (new) flat view that we can share\n> add a reference\n> else\n> render a new flat view and add it to fvs_tmp\n> \n> flat_views = fvs_tmp;\n> \n> for each address space\n> old_view = address_space_to_flatview(as);\n> find the new flat view to use in fvs_tmp\n> address_space_update_topology_pass(..., false);\n> address_space_update_topology_pass(..., true);\n> QTAILQ_REMOVE(&old_view->address_spaces, ...);\n> atomic_rcu_set(&as->current_map, new_view);\n> flatview_unref(old_view);\n> QTAILQ_INSERT_TAIL(&new_view->address_spaces, ...);\n> \n> \n> It's also not clear to me what you need the FlatView's address_spaces\n> list for. (It's probably something trivial that I'm missing, or maybe\n> it goes away with the previous change).\n\n\nIt is trivial - the first version added a global list of FVs and shared\nthem among ASes. Every transactoion_commit would produce a new FV and\nreplace it in all ASes which old FV was shared among. The decision whether\nto share FV or not was made in address_space_init() which a very different\nplace in code.\n\nIn v2 I moved the sharing decision to the commit part and noooow having a\nglobal list of FVs and ASes list in each FV seems redundant so I'll remove\nit in v3, thanks for pointing out :)\n\nAnd I'll probably drop FV allocation in address_space_init as it is going\nto be rebuild at commit time anyway.\n\n\n> \n>>\n>> 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 == address_space_root(as) && 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>> +\n>> return as;\n>> }\n>> \n> \n> This belongs in the next patch;\n\nBy \"this\" you mean removal of \"malloced\", not the AS loop above, right?\n\n> leaving as->malloced in\n> do_address_space_destroy and the reference count in\n> address_space_destroy is not a big complication.\n\n\nBut why would we want to leave them anyway?\n\n\nthanks for the quick review!","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=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ozlabs-ru.20150623.gappssmtp.com\n\theader.i=@ozlabs-ru.20150623.gappssmtp.com\n\theader.b=\"RxZE8sOS\"; dkim-atps=neutral"],"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 3xvDS63ltQz9t16\n\tfor ;\n\tSat, 16 Sep 2017 10:59:17 +1000 (AEST)","from localhost ([::1]:55519 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dt1Rx-0003xs-LZ\n\tfor incoming@patchwork.ozlabs.org; Fri, 15 Sep 2017 20:59:13 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:46566)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dt1Ra-0003xh-Oa\n\tfor qemu-devel@nongnu.org; Fri, 15 Sep 2017 20:58:51 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dt1RX-0001UN-MU\n\tfor qemu-devel@nongnu.org; Fri, 15 Sep 2017 20:58:50 -0400","from mail-pg0-x22a.google.com ([2607:f8b0:400e:c05::22a]:44150)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16)\n\t(Exim 4.71) (envelope-from ) id 1dt1RX-0001TE-BC\n\tfor qemu-devel@nongnu.org; Fri, 15 Sep 2017 20:58:47 -0400","by mail-pg0-x22a.google.com with SMTP id j16so2391852pga.1\n\tfor ; Fri, 15 Sep 2017 17:58:46 -0700 (PDT)","from [192.168.10.22] (124-171-134-202.dyn.iinet.net.au.\n\t[124.171.134.202]) by smtp.googlemail.com with ESMTPSA id\n\th82sm5150315pfd.148.2017.09.15.17.58.43\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 15 Sep 2017 17:58:44 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ozlabs-ru.20150623.gappssmtp.com; s=20150623;\n\th=subject:to:references:from:message-id:date:user-agent:mime-version\n\t:in-reply-to:content-language:content-transfer-encoding;\n\tbh=rxOqyKkJivQPjP64rfMreeJPF1U0p7AXhp3e1MQRKoA=;\n\tb=RxZE8sOSC2Qs0P1yxkuzturm+dyEGqvOp6kEzneAeafepLFTFTMz/QFF0Ldx4MyOkp\n\tZ41kimxHPMN7QAQNGKjCQM82qUypSlm4//8bSvYfSbQIb9qMwJTJN3pVQnhUe85a+FEH\n\t4oU9zzsGyVrvNQ+xIf/awiVVAxa4Xez4CYgk0lylgk+pbdiw2Sa9KWxwZGy2qFhOlWKk\n\tzyi9jEg0fTa6Q/OI0k43qRQTIzVdc7JxI94hlG6bnQOaFvpqa43omogyqDC+EDzPtzR4\n\tW7bxUWercbTZ1eK4sEHTyXpZybwW7GCiQ0TgCik1mxtTHHPf/0/CL/KkVpp/BOihOcjE\n\tY5Nw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=rxOqyKkJivQPjP64rfMreeJPF1U0p7AXhp3e1MQRKoA=;\n\tb=R4dOaaFCwimqQXtB4t5mAy0pGruT9SsB2Md2ruhv7QaacVeF/dXSnQOYAhzrlf/phi\n\tzOAw5pSn4jseIPSSK+jvaWhk9I36zcLzN7/nfzDlijNN5wxzMfDz7P/OTv73fqUeSg0y\n\tfX/6zMgdEBbZXjY5E/j4UA4DnkvWpc4QsnFe8MVbcOzST9NKTAXF0fYncymYF+ipKiev\n\tc3KThvf3OH9b2ar6dfvUwY/gRy1zWw6ikDAADEk7cfx1jbP+94xw1bi831cEpPoHwIHc\n\ti0BnRhuxI4gLEV7pAp5VMDODIKQqgGoOnZWdp3vyi4w4zJoywNE69rZ4zaoo8ybnuwwR\n\tW1JA==","X-Gm-Message-State":"AHPjjUhu/Eg5R9NyaG27wgoV6H2PXrlRJ1oLtD+CLMe8ZzXgMORx6ybG\n\tVjG7wRUwDbhhv4/ix8I=","X-Google-Smtp-Source":"ADKCNb6U3q4425RnG6lTzwAmvWHEs2+qRd+V4lZIxzpj1/ziCV5IJvYb/ltV71jy6AUoXSs6LW5ajQ==","X-Received":"by 10.98.144.142 with SMTP id q14mr21319004pfk.303.1505523525139;\n\tFri, 15 Sep 2017 17:58:45 -0700 (PDT)","To":"Paolo Bonzini , qemu-devel@nongnu.org","References":"<20170915084030.40988-1-aik@ozlabs.ru>\n\t<20170915084030.40988-12-aik@ozlabs.ru>\n\t<165a7fa9-6ebb-3e95-2d95-7df2f550f1de@redhat.com>","From":"Alexey Kardashevskiy ","Message-ID":"<14f70953-dc95-6845-04be-07ee1615117e@ozlabs.ru>","Date":"Sat, 16 Sep 2017 10:58:40 +1000","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":"<165a7fa9-6ebb-3e95-2d95-7df2f550f1de@redhat.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-AU","Content-Transfer-Encoding":"7bit","X-detected-operating-system":"by eggs.gnu.org: Genre and OS details not\n\trecognized.","X-Received-From":"2607:f8b0:400e:c05::22a","Subject":"Re: [Qemu-devel] [PATCH qemu v2 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"}}]