[{"id":1764389,"web_url":"http://patchwork.ozlabs.org/comment/1764389/","msgid":"<20170906212338.GD25558@flamenco>","list_archive_url":null,"date":"2017-09-06T21:23:38","subject":"Re: [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control\n\tinterface","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 20:59:02 +0300, Lluís Vilanova wrote:\n> Signed-off-by: Lluís Vilanova \n> ---\n> Makefile | 5 +++\n> configure | 1 +\n> instrument/Makefile.objs | 2 +\n> instrument/control.c | 28 +++++++++++++++++\n> instrument/control.h | 44 +++++++++++++++++++++++++++\n> instrument/control.inc.h | 25 ++++++++++++++++\n> instrument/error.h | 28 +++++++++++++++++\n> instrument/events.h | 37 +++++++++++++++++++++++\n> instrument/events.inc.h | 11 +++++++\n\nAm I the only one who finds this control vs. events division confusing?\n\nAlso, do we need all these many files, even for the public API?\n\nAnd why the .inc's?\n\nThanks,\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=\"a+cvGEoL\"; \n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"abnpb4VS\"; \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 3xnc602qybz9s7f\n\tfor ;\n\tThu, 7 Sep 2017 07:24:08 +1000 (AEST)","from localhost ([::1]:37824 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dphnq-0004vf-Hb\n\tfor incoming@patchwork.ozlabs.org; Wed, 06 Sep 2017 17:24:06 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:36325)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dphnU-0004v9-Gt\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 17:23:48 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dphnP-0005yK-Rw\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 17:23:44 -0400","from out1-smtp.messagingengine.com ([66.111.4.25]:42479)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dphnP-0005y9-NM\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 17:23:39 -0400","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id 63E4E20D63;\n\tWed, 6 Sep 2017 17:23:39 -0400 (EDT)","from frontend1 ([10.202.2.160])\n\tby compute4.internal (MEProxy); Wed, 06 Sep 2017 17:23:39 -0400","from localhost (flamenco.cs.columbia.edu [128.59.20.216])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 13C017F9A8;\n\tWed, 6 Sep 2017 17:23:39 -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=MgDcv5I+2r3bwgQ\n\ttcL8Xlq7H1RqTnwejdFwYaFGkL6c=; b=a+cvGEoLqLGoDeKQs8CvBO1CzmIrsp4\n\t/gpZtGcnMqrjtjYI1/PAZyd8RIFTdqlfatx+MX7B/471HZlAUCX1X3qjE+miSixB\n\tNBHq3ZOj5G0jj00WEqTLDcWXrOgH4WXDOrD6rlBEdFsilq+hj7H6bsd2RSuQA8fc\n\t7MpTPW6YgG2w=","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=MgDcv5I+2r3bwgQtcL8Xlq7H1RqTnwejdFwYaFGkL6c=; b=abnpb4VS\n\t20RSMkdt1GEbO6FUctoQ/FjYzAw5tvv3iNv/ttp4les6rpXwyj2BGo6z7wstgClk\n\td4FrKCy/tmFvVkFhJ+Utfp3FCqs3rNh/qesPVtrBCgroCDPEfMJbocqhr0uqXMJI\n\tky3XAtsNgg+NSeHfCSpmHMkZQ8Ik82Ge/VqlpTUA8e9QrX0dKTfnMACEkSSJOOmj\n\tQ33gl1YNMIRYVbJWvRrUakpYeURe+/xhc6bcnzmNO6PPd+G0sMyvd531WPv4cS7Z\n\t6+uCeGtkEJIlk5NSmJsCln9hdKD2lxqn1b7f4hGqiR5oqxikUh24jCm/RxQ4l/5N\n\tUbdEIamDIJcMVg=="],"X-ME-Sender":"","X-Sasl-enc":"Y6ffA0iYwYWl6n8alnoKW18TYcuOtCkguk0gx1bcZRtJ 1504733019","Date":"Wed, 6 Sep 2017 17:23:38 -0400","From":"\"Emilio G. Cota\" ","To":"=?iso-8859-1?q?Llu=EDs?= Vilanova ","Message-ID":"<20170906212338.GD25558@flamenco>","References":"<150471856141.24907.274176769201097378.stgit@frigg.lan>\n\t<150472074219.24907.5510718414753398145.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":"<150472074219.24907.5510718414753398145.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 09/20] instrument: Add basic control\n\tinterface","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":"Paolo Bonzini , qemu-devel@nongnu.org,\n\tStefan Hajnoczi ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1764409,"web_url":"http://patchwork.ozlabs.org/comment/1764409/","msgid":"<20170906215711.GA18214@flamenco>","list_archive_url":null,"date":"2017-09-06T21:57:11","subject":"Re: [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control\n\tinterface","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 20:59:02 +0300, Lluís Vilanova wrote:\n> Signed-off-by: Lluís Vilanova \n> ---\n(snip)\n> +QI_VPUBLIC void qi_set_fini(qi_fini_fn fn, void *data)\n> +{\n> + ERROR_IF(!instr_get_state(), \"called outside instrumentation\");\n> + instr_set_event(fini_fn, fn);\n> + instr_set_event(fini_data, data);\n> +}\n\nWhy are these QI_VPUBLIC attributes here? Those are useful for DSO's, not\nfor executables --by using -rdynamic, all non-static symbols in the\nexecutable are already visible.\n\n> diff --git a/instrument/control.h b/instrument/control.h\n> new file mode 100644\n> index 0000000000..f2b085f69b\n> --- /dev/null\n> +++ b/instrument/control.h\n(snip)\n> + * Instrumentation state of current host thread. Used to ensure instrumentation\n> + * clients use QEMU's API only in expected points.\n> + */\n> +typedef enum {\n> + INSTR_STATE_DISABLE,\n> + INSTR_STATE_ENABLE,\n> +} InstrState;\n\nI find this unnecessarily ugly for the little gain we get, i.e. asserts against\ncalling API code from QEMU.. seems unlikely to me (although admittedly I think\nthe qemu-internal API is unnecessarily complex/verbose, so maybe\nyou're better off with these checks).\n\n(snip)\n> +/**\n> + * instr_get_event:\n> + *\n> + * Get value set by instrumentation library.\n> + */\n> +#define instr_get_event(name) \\\n> + atomic_load_acquire(&instr_event__ ## name)\n> +\n> +/**\n> + * instr_get_event:\n> + *\n> + * Set value from instrumentation library.\n> + */\n> +#define instr_set_event(name, fn) \\\n> + atomic_store_release(&instr_event__ ## name, fn)\n\nThis isn't enough to decide whether to call instrumentation, especially for\nTCG. We need TB's to know what to call, and update that mask with async\nwork, just like we do with tracing. Check out my alternative patchset.\n\nAlso, a single function pointer cannot work for more than one plugin. But\nI see you have an XXX when there's more than one plugin, so it's OK for now.\nI used RCU lists for this, which at least gives you a time in the future\nat which things become visible/invisible by other threads -- this is important\nwhen unloading an instrumenter, since you don't want to clear important stuff\n(e.g. dlclose) before you're sure no further callbacks to it are possible.\n[no, the atomic_acquire/release isn't enough!]\n\n(snip)\n> diff --git a/instrument/load.c b/instrument/load.c\n> index a57401102a..e180f03429 100644\n> --- a/instrument/load.c\n> +++ b/instrument/load.c\n> @@ -11,6 +11,8 @@\n> #include \"qemu-common.h\"\n> \n> #include \n> +#include \"instrument/control.h\"\n> +#include \"instrument/events.h\"\n> #include \"instrument/load.h\"\n> #include \"qemu/config-file.h\"\n> #include \"qemu/error-report.h\"\n> @@ -105,8 +107,11 @@ InstrLoadError instr_load(const char * path, int argc, const char ** argv,\n> res = INSTR_LOAD_DLERROR;\n> goto err;\n> }\n> + instr_set_event(fini_fn, NULL);\n> \n> + instr_set_state(INSTR_STATE_ENABLE);\n> main_res = main_cb(argc, argv);\n> + instr_set_state(INSTR_STATE_DISABLE);\n> \n> if (main_res != 0) {\n> res = INSTR_LOAD_ERROR;\n> @@ -136,6 +141,14 @@ InstrUnloadError instr_unload(int64_t handle_id)\n> goto out;\n> }\n> \n> + qi_fini_fn fini_fn = instr_get_event(fini_fn);\n> + if (fini_fn) {\n> + void *fini_data = instr_get_event(fini_data);\n> + fini_fn(fini_data);\n> + }\n> +\n> + instr_set_event(fini_fn, NULL);\n> +\n\nIs fini really that useful? Doesn't the tool just die with QEMU once QEMU exits?\nAt the end of the day, the tool could register its own atexit hook.\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=\"mga54Nrn\"; \n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"iTGnhnLL\"; \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 3xncrm62Hkz9s8J\n\tfor ;\n\tThu, 7 Sep 2017 07:57:42 +1000 (AEST)","from localhost ([::1]:37916 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpiKJ-0006ni-EA\n\tfor incoming@patchwork.ozlabs.org; Wed, 06 Sep 2017 17:57:39 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:43625)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpiJy-0006na-AA\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 17:57:19 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpiJu-0000yb-Dp\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 17:57:18 -0400","from out1-smtp.messagingengine.com ([66.111.4.25]:52619)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dpiJu-0000yO-6f\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 17:57:14 -0400","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id 9DD2220D6D;\n\tWed, 6 Sep 2017 17:57:11 -0400 (EDT)","from frontend1 ([10.202.2.160])\n\tby compute4.internal (MEProxy); Wed, 06 Sep 2017 17:57:11 -0400","from localhost (flamenco.cs.columbia.edu [128.59.20.216])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 4F3577E75E;\n\tWed, 6 Sep 2017 17:57:11 -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=Mrv0CK8RzZNroo4\n\tDZlSSCYZ+U4rtZ3LnxWZNs4EW9IY=; b=mga54NrntAik9hrmiVe5aeeaGvksSAu\n\t89XoJPHHrmbJjqFFopI6l7EqmF/zQpmgye3RrVdV2QWrsV/zDcnHZhXpbHoZhshp\n\trttZD8N4M3AvYTK1a37dZTlBS1sAhwlnF0MebKAYndTSuWeAq5L35JyvtRqEMn/b\n\t+ljUmBr73KYc=","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=Mrv0CK8RzZNroo4DZlSSCYZ+U4rtZ3LnxWZNs4EW9IY=; b=iTGnhnLL\n\te4GpzfTm08tlJwXNjBco92R79lXh22MYDmIhMvQ7Ky92pvMgI6p6RrJ1RPFdB9fS\n\t3JsSvr6eWbyEbE1VvTyfDlBVrprq1LriFlt0yjxEtcwtOXPJpYaJi+zKp+pmCVCe\n\tcfsYbJKCwu/hdq7XIsjYyJWrNcRvF+92F8UQtGG5LlQxvg4jPwFnu6PjFHqaiih9\n\t+H7ADELKOH+Whr7FG8sw7dzg95QlC3jw+Rc/NzbA3/5kZ9d3uRLr8SD6oLaPDDwA\n\tGdDQWG4YVS+CdfP5oD4eaC3ahGSNH1SctvPJNQ1EewBA/qYFbM0MHTgWdGOvdqUA\n\tHhNhYqXcn1kNcw=="],"X-ME-Sender":"","X-Sasl-enc":"pRIPY/B2rn7MaU5q6d2JuvglJ6pwFAE/1rLxwag0SLFy 1504735031","Date":"Wed, 6 Sep 2017 17:57:11 -0400","From":"\"Emilio G. Cota\" ","To":"=?iso-8859-1?q?Llu=EDs?= Vilanova ","Message-ID":"<20170906215711.GA18214@flamenco>","References":"<150471856141.24907.274176769201097378.stgit@frigg.lan>\n\t<150472074219.24907.5510718414753398145.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":"<150472074219.24907.5510718414753398145.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 09/20] instrument: Add basic control\n\tinterface","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":"Paolo Bonzini , qemu-devel@nongnu.org,\n\tStefan Hajnoczi ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1766020,"web_url":"http://patchwork.ozlabs.org/comment/1766020/","msgid":"<87fubuxkso.fsf@frigg.lan>","list_archive_url":null,"date":"2017-09-10T22:15:19","subject":"Re: [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control\n\tinterface","submitter":{"id":9099,"url":"http://patchwork.ozlabs.org/api/people/9099/","name":"Lluís Vilanova","email":"vilanova@ac.upc.edu"},"content":"Emilio G Cota writes:\n\n> On Wed, Sep 06, 2017 at 20:59:02 +0300, Lluís Vilanova wrote:\n>> Signed-off-by: Lluís Vilanova \n>> ---\n>> Makefile | 5 +++\n>> configure | 1 +\n>> instrument/Makefile.objs | 2 +\n>> instrument/control.c | 28 +++++++++++++++++\n>> instrument/control.h | 44 +++++++++++++++++++++++++++\n>> instrument/control.inc.h | 25 ++++++++++++++++\n>> instrument/error.h | 28 +++++++++++++++++\n>> instrument/events.h | 37 +++++++++++++++++++++++\n>> instrument/events.inc.h | 11 +++++++\n\n> Am I the only one who finds this control vs. events division confusing?\n\nControl is only for controlling instrumentation, and the header is used mainly\ninside the instrumentation directory. Wheread the events header is later going\nto be included in every file that needs to trigger an instrumentation event.\n\n> Also, do we need all these many files, even for the public API?\n\nThe only other header, error.h, is later used in many other files.\n\n\n> And why the .inc's?\n\nTo keep tidy headers with documentation, and the implementation details stashed\naway on a separate file (like in the case of trace/).\n\n\n> Thanks,\n> \t\tE.\n\n\nCheers,\n Lluis","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=)","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 3xr54V1v41z9sBd\n\tfor ;\n\tMon, 11 Sep 2017 08:16:22 +1000 (AEST)","from localhost ([::1]:54448 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1drAWa-0005RU-0Y\n\tfor incoming@patchwork.ozlabs.org; Sun, 10 Sep 2017 18:16:20 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:36933)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1drAVr-0005DO-UP\n\tfor qemu-devel@nongnu.org; Sun, 10 Sep 2017 18:15:36 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1drAVn-0005Kg-Lc\n\tfor qemu-devel@nongnu.org; Sun, 10 Sep 2017 18:15:35 -0400","from roura.ac.upc.edu ([147.83.33.10]:36849 helo=roura.ac.upc.es)\n\tby eggs.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1drAVn-0005K4-AK\n\tfor qemu-devel@nongnu.org; Sun, 10 Sep 2017 18:15:31 -0400","from correu-2.ac.upc.es (correu-2.ac.upc.es [147.83.30.92])\n\tby roura.ac.upc.es (8.13.8/8.13.8) with ESMTP id v8AMFPiC025934;\n\tMon, 11 Sep 2017 00:15:25 +0200","from localhost (unknown [31.210.187.58])\n\tby correu-2.ac.upc.es (Postfix) with ESMTPSA id 52D7025B;\n\tMon, 11 Sep 2017 00:15:20 +0200 (CEST)"],"From":"=?utf-8?q?Llu=C3=ADs_Vilanova?= ","To":"\"Emilio G. Cota\" ","References":"<150471856141.24907.274176769201097378.stgit@frigg.lan>\n\t<150472074219.24907.5510718414753398145.stgit@frigg.lan>\n\t<20170906212338.GD25558@flamenco>","Mail-Followup-To":"\"Emilio G. Cota\" , qemu-devel@nongnu.org, \n\tEric Blake ,\n\tStefan Hajnoczi , \n\tPaolo Bonzini ","Date":"Mon, 11 Sep 2017 01:15:19 +0300","In-Reply-To":"<20170906212338.GD25558@flamenco> (Emilio G. Cota's message of\n\t\"Wed, 6 Sep 2017 17:23:38 -0400\")","Message-ID":"<87fubuxkso.fsf@frigg.lan>","User-Agent":"Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.6.x [fuzzy]","X-Received-From":"147.83.33.10","Subject":"Re: [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control\n\tinterface","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":"Paolo Bonzini , qemu-devel@nongnu.org,\n\tStefan Hajnoczi ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1766022,"web_url":"http://patchwork.ozlabs.org/comment/1766022/","msgid":"<87y3pmuo9b.fsf@frigg.lan>","list_archive_url":null,"date":"2017-09-10T23:28:48","subject":"Re: [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control\n\tinterface","submitter":{"id":9099,"url":"http://patchwork.ozlabs.org/api/people/9099/","name":"Lluís Vilanova","email":"vilanova@ac.upc.edu"},"content":"Emilio G Cota writes:\n\n> On Wed, Sep 06, 2017 at 20:59:02 +0300, Lluís Vilanova wrote:\n>> Signed-off-by: Lluís Vilanova \n>> ---\n> (snip)\n>> +QI_VPUBLIC void qi_set_fini(qi_fini_fn fn, void *data)\n>> +{\n>> + ERROR_IF(!instr_get_state(), \"called outside instrumentation\");\n>> + instr_set_event(fini_fn, fn);\n>> + instr_set_event(fini_data, data);\n>> +}\n\n> Why are these QI_VPUBLIC attributes here? Those are useful for DSO's, not\n> for executables --by using -rdynamic, all non-static symbols in the\n> executable are already visible.\n\nThat's because I'm using -rdynamic, -fvisibility=hidden and these attributes all\ntogether to limit what symbols are available to dlopen()'ed libs (new series\nversion will have the visibility compiler flag I forgot to send).\n\n\n>> diff --git a/instrument/control.h b/instrument/control.h\n>> new file mode 100644\n>> index 0000000000..f2b085f69b\n>> --- /dev/null\n>> +++ b/instrument/control.h\n> (snip)\n>> + * Instrumentation state of current host thread. Used to ensure instrumentation\n>> + * clients use QEMU's API only in expected points.\n>> + */\n>> +typedef enum {\n>> + INSTR_STATE_DISABLE,\n>> + INSTR_STATE_ENABLE,\n>> +} InstrState;\n\n> I find this unnecessarily ugly for the little gain we get, i.e. asserts against\n> calling API code from QEMU.. seems unlikely to me (although admittedly I think\n> the qemu-internal API is unnecessarily complex/verbose, so maybe\n> you're better off with these checks).\n\nIt ensures only QEMU's threads now calling a library function are allowed to use\nthe public API. In a later patch, it also ensures that TCG-generating functions\n(e.g., qi_event_gen_guest_mem_before_exec) can only be called from\ntranslation-time library functions.\n\n\n> (snip)\n>> +/**\n>> + * instr_get_event:\n>> + *\n>> + * Get value set by instrumentation library.\n>> + */\n>> +#define instr_get_event(name) \\\n>> + atomic_load_acquire(&instr_event__ ## name)\n>> +\n>> +/**\n>> + * instr_get_event:\n>> + *\n>> + * Set value from instrumentation library.\n>> + */\n>> +#define instr_set_event(name, fn) \\\n>> + atomic_store_release(&instr_event__ ## name, fn)\n\n> This isn't enough to decide whether to call instrumentation, especially for\n> TCG. We need TB's to know what to call, and update that mask with async\n> work, just like we do with tracing. Check out my alternative patchset.\n\nWhat do you mean by TB's need to know what to call?\n\n> Also, a single function pointer cannot work for more than one plugin. But\n> I see you have an XXX when there's more than one plugin, so it's OK for now.\n> I used RCU lists for this, which at least gives you a time in the future\n> at which things become visible/invisible by other threads -- this is important\n> when unloading an instrumenter, since you don't want to clear important stuff\n> (e.g. dlclose) before you're sure no further callbacks to it are possible.\n> [no, the atomic_acquire/release isn't enough!]\n\nIn principle, this together with the API checks above and kicking all vCPUs to\nflush TBs before unloading a library (I'm still looking at last bit) should\nensure libs can be safely unloaded. This should work for guest code events, but\nI still need to look at other events like vCPU hotplug (is it asynchronous?), or\nother events we might want triggered outside the vCPU loops.\n\nFlushing all TBs is also on my todo to support immediate event availability when\nloading a library and setting a translation-time callback.\n\nAlso, I'm not sure if we should support the complexity and performance penalty\nof more than one library at a time. Right now, the QAPI commands expose support\nfor more than one just for future-proofing (as suggested by Richard Henderson,\nif I remember correctly).\n\n\n\n> (snip)\n>> diff --git a/instrument/load.c b/instrument/load.c\n>> index a57401102a..e180f03429 100644\n>> --- a/instrument/load.c\n>> +++ b/instrument/load.c\n>> @@ -11,6 +11,8 @@\n>> #include \"qemu-common.h\"\n>> \n>> #include \n>> +#include \"instrument/control.h\"\n>> +#include \"instrument/events.h\"\n>> #include \"instrument/load.h\"\n>> #include \"qemu/config-file.h\"\n>> #include \"qemu/error-report.h\"\n>> @@ -105,8 +107,11 @@ InstrLoadError instr_load(const char * path, int argc, const char ** argv,\n>> res = INSTR_LOAD_DLERROR;\n>> goto err;\n>> }\n>> + instr_set_event(fini_fn, NULL);\n>> \n>> + instr_set_state(INSTR_STATE_ENABLE);\n>> main_res = main_cb(argc, argv);\n>> + instr_set_state(INSTR_STATE_DISABLE);\n>> \n>> if (main_res != 0) {\n>> res = INSTR_LOAD_ERROR;\n>> @@ -136,6 +141,14 @@ InstrUnloadError instr_unload(int64_t handle_id)\n>> goto out;\n>> }\n>> \n>> + qi_fini_fn fini_fn = instr_get_event(fini_fn);\n>> + if (fini_fn) {\n>> + void *fini_data = instr_get_event(fini_data);\n>> + fini_fn(fini_data);\n>> + }\n>> +\n>> + instr_set_event(fini_fn, NULL);\n>> +\n\n> Is fini really that useful? Doesn't the tool just die with QEMU once QEMU exits?\n\nThe lib can be unloaded at any time (in softmmu mode).\n\n\n> At the end of the day, the tool could register its own atexit hook.\n\nI'm aware of that, but:\n\n* Passing a pointer when registering the callback avoids global variables.\n* I thought that qi_set_fini() would be more discoverable than\n dlclose()+atexit().\n\n\nThanks!\n Lluis","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=)","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 3xr6hr3z6Cz9s7f\n\tfor ;\n\tMon, 11 Sep 2017 09:29:29 +1000 (AEST)","from localhost ([::1]:54611 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1drBfK-0001z7-2d\n\tfor incoming@patchwork.ozlabs.org; Sun, 10 Sep 2017 19:29:26 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:48561)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1drBf0-0001yu-2L\n\tfor qemu-devel@nongnu.org; Sun, 10 Sep 2017 19:29:07 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1drBew-0006bh-Rv\n\tfor qemu-devel@nongnu.org; Sun, 10 Sep 2017 19:29:06 -0400","from roura.ac.upc.es ([147.83.33.10]:49647)\n\tby eggs.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1drBew-0006aq-DU\n\tfor qemu-devel@nongnu.org; Sun, 10 Sep 2017 19:29:02 -0400","from correu-1.ac.upc.es (correu-1.ac.upc.es [147.83.30.91])\n\tby roura.ac.upc.es (8.13.8/8.13.8) with ESMTP id v8ANSueY027197;\n\tMon, 11 Sep 2017 01:28:56 +0200","from localhost (unknown [31.210.187.58])\n\tby correu-1.ac.upc.es (Postfix) with ESMTPSA id 985BA96A;\n\tMon, 11 Sep 2017 01:28:50 +0200 (CEST)"],"From":"=?utf-8?q?Llu=C3=ADs_Vilanova?= ","To":"\"Emilio G. Cota\" ","References":"<150471856141.24907.274176769201097378.stgit@frigg.lan>\n\t<150472074219.24907.5510718414753398145.stgit@frigg.lan>\n\t<20170906215711.GA18214@flamenco>","Mail-Followup-To":"\"Emilio G. Cota\" , qemu-devel@nongnu.org, \n\tEric Blake ,\n\tStefan Hajnoczi , \n\tPaolo Bonzini ","Date":"Mon, 11 Sep 2017 02:28:48 +0300","In-Reply-To":"<20170906215711.GA18214@flamenco> (Emilio G. Cota's message of\n\t\"Wed, 6 Sep 2017 17:57:11 -0400\")","Message-ID":"<87y3pmuo9b.fsf@frigg.lan>","User-Agent":"Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.6.x [fuzzy]","X-Received-From":"147.83.33.10","Subject":"Re: [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control\n\tinterface","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":"Paolo Bonzini , qemu-devel@nongnu.org,\n\tStefan Hajnoczi ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]