[{"id":1764396,"web_url":"http://patchwork.ozlabs.org/comment/1764396/","msgid":"<20170906213641.GE25558@flamenco>","list_archive_url":null,"date":"2017-09-06T21:36:41","subject":"Re: [Qemu-devel] [PATCH v4 11/20] instrument: Track vCPUs","submitter":{"id":65690,"url":"http://patchwork.ozlabs.org/api/people/65690/","name":"Emilio Cota","email":"cota@braap.org"},"content":"On Wed, Sep 06, 2017 at 21:07:06 +0300, Lluís Vilanova wrote:\n> Keep a translation between instrumentation's QICPU and CPUState objects to avoid\n> exposing QEMU's internals to instrumentation clients.\n> \n> Signed-off-by: Lluís Vilanova \n(snip)\n> diff --git a/instrument/control.c b/instrument/control.c\n> index 2c2781beeb..83453ea561 100644\n> --- a/instrument/control.c\n> +++ b/instrument/control.c\n> @@ -13,10 +13,32 @@\n> #include \"instrument/load.h\"\n> #include \"instrument/qemu-instr/control.h\"\n> #include \"instrument/qemu-instr/visibility.h\"\n> +#include \"qom/cpu.h\"\n> +\n> \n> __thread InstrState instr_cur_state;\n> \n> \n> +unsigned int instr_cpus_count;\n> +CPUState **instr_cpus;\n> +\n> +void instr_cpu_add(CPUState *vcpu)\n> +{\n> + unsigned int idx = vcpu->cpu_index;\n> + if (idx >= instr_cpus_count) {\n> + instr_cpus_count = idx + 1;\n> + instr_cpus = realloc(instr_cpus, sizeof(*instr_cpus) * instr_cpus_count);\n> + }\n> + instr_cpus[idx] = vcpu;\n> +}\n> +\n> +void instr_cpu_remove(CPUState *vcpu)\n> +{\n> + unsigned int idx = vcpu->cpu_index;\n> + instr_cpus[idx] = NULL;\n> +}\n\ninstr_cpus_count and instr_cpus are both modified with cpu_list_lock.\nHowever, other readers do not acquire this same lock. This gets messy\nwhen you try to implement something like \"vcpu_for_each\", which is\nvery useful when you load an instrumentation tool once the program\nis running (otherwise the tool cannot know for sure what the vCPUs\nare). This vcpu_for_each would also have to take cpu_list_lock, for\notherwise it'd miss new threads being added. As you can imagine this\ngets messy pretty quickly, given that you have your own lock here.\nI went with having my own hash table (a list would be fine too),\nprotected with the same \"instr_lock\" you have (i.e. the recursive mutex).\n\nAn unrelated comment: your usage of get/set/put confuses me. I'd expect those\nto work on refcounted items; instead you use them for instance to hide a cast\n(cpu_set) or to create/destroy (e.g. handle_get/put).\n\n\t\tE.","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\" (1024-bit key;\n\tunprotected) header.d=braap.org header.i=@braap.org\n\theader.b=\"BWZIyONj\"; \n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"FG/kAVTB\"; \n\tdkim-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 3xncPQ3kF4z9t2R\n\tfor ;\n\tThu, 7 Sep 2017 07:37:29 +1000 (AEST)","from localhost ([::1]:37879 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpi0j-0001Ea-Nx\n\tfor incoming@patchwork.ozlabs.org; Wed, 06 Sep 2017 17:37:25 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:39487)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpi0A-0001BT-IX\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 17:36:55 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpi05-0007MH-RF\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 17:36:50 -0400","from out1-smtp.messagingengine.com ([66.111.4.25]:36701)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dpi05-0007Jm-IU\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 17:36:45 -0400","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id 9EA5221AAA;\n\tWed, 6 Sep 2017 17:36:42 -0400 (EDT)","from frontend2 ([10.202.2.161])\n\tby compute4.internal (MEProxy); Wed, 06 Sep 2017 17:36:42 -0400","from localhost (flamenco.cs.columbia.edu [128.59.20.216])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 5BB9F24335;\n\tWed, 6 Sep 2017 17:36:42 -0400 (EDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=braap.org; h=cc\n\t:content-transfer-encoding:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=+xTH3HsJ56b8Ddz\n\tD8wN9GTsenpsZrAwjPd0X/Pw5eYE=; b=BWZIyONjAzcFTZU5OWHFE1MFNsvNyO6\n\t1qR6EHMlOO+SNsw4PaEq5zF1fIY7+itCwpWHdHmI5PPcxW0k5XSbFItb0QW0aTtn\n\tbGEfkVtgv0Wj3hmGoFYqnvcOEgZ5kDd4rEJ7rxwG291zubovfqRGFrae8xVOpXBQ\n\tTEfqaZEw45us=","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-transfer-encoding:content-type\n\t:date:from:in-reply-to:message-id:mime-version:references\n\t:subject:to:x-me-sender:x-me-sender:x-sasl-enc:x-sasl-enc; s=\n\tfm1; bh=+xTH3HsJ56b8DdzD8wN9GTsenpsZrAwjPd0X/Pw5eYE=; b=FG/kAVTB\n\tprzsX2EJyh88xZikXNTbI1zzQEqgAzmdg1HyM/8Z6NmxSUGyqhzt5M0h1gXuY1RF\n\tum2MYW4hzYYAtjxk1K5kGnpFKkhFGt+MLjBZh+y/mQDeQdXKJHJm93e0Y5DwocSo\n\tN/psQMelSKGc+BEj/pEwmAJtjHFL7yi8kg8ZlO04w/RUL1/O9mJUMDK9r6WBZCk+\n\t8b5WXd2Fs4NIopjQ29w3NhNM44ZeZO2A8uZNsDROKyVFre5+h7OZ7DVs+NqbaED0\n\t9EYtZWOl2O1CtZnKZv6QqCysfikGZqbfWCCknucINyZpWyAMzgCyDs4x4z+To4mR\n\tym70Oo5ulXM1NA=="],"X-ME-Sender":"","X-Sasl-enc":"S1c4xXNuN0AtYwfwJfF4B79z0xSz6lY3B05FCnOz4JuS 1504733802","Date":"Wed, 6 Sep 2017 17:36:41 -0400","From":"\"Emilio G. Cota\" ","To":"=?iso-8859-1?q?Llu=EDs?= Vilanova ","Message-ID":"<20170906213641.GE25558@flamenco>","References":"<150471856141.24907.274176769201097378.stgit@frigg.lan>\n\t<150472122675.24907.11597641982841030964.stgit@frigg.lan>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<150472122675.24907.11597641982841030964.stgit@frigg.lan>","User-Agent":"Mutt/1.5.24 (2015-08-30)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"66.111.4.25","Subject":"Re: [Qemu-devel] [PATCH v4 11/20] instrument: Track vCPUs","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","Cc":"qemu-devel@nongnu.org, Stefan Hajnoczi ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]