From patchwork Fri Jan 17 15:57:33 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: George Dunlap X-Patchwork-Id: 11339509 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 16F7814B7 for ; Fri, 17 Jan 2020 15:59:05 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E6F912064C for ; Fri, 17 Jan 2020 15:59:04 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=citrix.com header.i=@citrix.com header.b="iRHYGB9a" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E6F912064C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=citrix.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1isU0g-0002cx-QH; Fri, 17 Jan 2020 15:58:10 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1isU0g-0002cE-An for xen-devel@lists.xenproject.org; Fri, 17 Jan 2020 15:58:10 +0000 X-Inumbo-ID: 1d97c410-3942-11ea-9fd7-bc764e2007e4 Received: from esa4.hc3370-68.iphmx.com (unknown [216.71.155.144]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 1d97c410-3942-11ea-9fd7-bc764e2007e4; Fri, 17 Jan 2020 15:57:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=citrix.com; s=securemail; t=1579276671; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=pxpaa2v2iad66B8kvb2k9dwGuWp9mnfKF/5hDp1VZVU=; b=iRHYGB9aQURE0woiDrk4rukAe+2ORQEKKya4iOlbqFEsVxLPkAZPvvIg abaxn3JlBdpETYwZJCnp2+Ex9Qjf1YDJZtiIrf7MyPvCj8TbBMj00nDc7 m6kN3iY1QIGFNoEE881h8R8VyGxe0pAeGwTPH3Duf2hzTV3QsWaalnXwZ w=; Authentication-Results: esa4.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=george.dunlap@citrix.com; spf=Pass smtp.mailfrom=George.Dunlap@citrix.com; spf=None smtp.helo=postmaster@mail.citrix.com Received-SPF: None (esa4.hc3370-68.iphmx.com: no sender authenticity information available from domain of george.dunlap@citrix.com) identity=pra; client-ip=162.221.158.21; receiver=esa4.hc3370-68.iphmx.com; envelope-from="George.Dunlap@citrix.com"; x-sender="george.dunlap@citrix.com"; x-conformance=sidf_compatible Received-SPF: Pass (esa4.hc3370-68.iphmx.com: domain of George.Dunlap@citrix.com designates 162.221.158.21 as permitted sender) identity=mailfrom; client-ip=162.221.158.21; receiver=esa4.hc3370-68.iphmx.com; envelope-from="George.Dunlap@citrix.com"; x-sender="George.Dunlap@citrix.com"; x-conformance=sidf_compatible; x-record-type="v=spf1"; x-record-text="v=spf1 ip4:209.167.231.154 ip4:178.63.86.133 ip4:195.66.111.40/30 ip4:85.115.9.32/28 ip4:199.102.83.4 ip4:192.28.146.160 ip4:192.28.146.107 ip4:216.52.6.88 ip4:216.52.6.188 ip4:162.221.158.21 ip4:162.221.156.83 ip4:168.245.78.127 ~all" Received-SPF: None (esa4.hc3370-68.iphmx.com: no sender authenticity information available from domain of postmaster@mail.citrix.com) identity=helo; client-ip=162.221.158.21; receiver=esa4.hc3370-68.iphmx.com; envelope-from="George.Dunlap@citrix.com"; x-sender="postmaster@mail.citrix.com"; x-conformance=sidf_compatible IronPort-SDR: 73DUylGE7OTi2kjhTiwjs0kUdavzLkKjlBwWO5PulM05vsl5yAJUd1s2bn/FI+6Qi99mKSqag3 frXyY0jkxh3H6cPMWWNvNo4GuAc2l0tveOYRGu7Xwj+R3rB+JMsdKhYFSqGyT7oKK0sTsBtvjI zaSicGOfI29W4PMDFpDzceoA4pv4oJ4ye7U22C2pKHcUqDlJa/1SuqSxmq14RJI2UNsbIrDtDO EkayD3ZXZKukIUI3n50ykYzGwvA5u7xfNxyVeChHuOqFsy/TstQSopbgWKeZgx9htxKlWT3QRB MCw= X-SBRS: 2.7 X-MesageID: 11674366 X-Ironport-Server: esa4.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.70,330,1574139600"; d="scan'208";a="11674366" From: George Dunlap To: Date: Fri, 17 Jan 2020 15:57:33 +0000 Message-ID: <20200117155734.1067550-7-george.dunlap@citrix.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200117155734.1067550-1-george.dunlap@citrix.com> References: <20200117155734.1067550-1-george.dunlap@citrix.com> MIME-Version: 1.0 Subject: [Xen-devel] [PATCH v3 7/8] golang/xenlight: Notify xenlight of SIGCHLD X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Nick Rosbrook , Ian Jackson , George Dunlap Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" libxl forks external processes and waits for them to complete; it therefore needs to be notified when children exit. In absence of instructions to the contrary, libxl sets up its own SIGCHLD handlers. Golang always unmasks and handles SIGCHLD itself. libxl thankfully notices this and throws an assert() rather than clobbering SIGCHLD handlers. Tell libxl that we'll be responsible for getting SIGCHLD notifications to it. Arrange for a channel in the context to receive notifications on SIGCHLD, and set up a goroutine that will pass these on to libxl. NB that every libxl context needs a notification; so multiple contexts will each spin up their own goroutine when opening a context, and shut it down on close. libxl also wants to hold on to a const pointer to xenlight_childproc_hooks rather than do a copy; so make a global structure in C space and initialize it once on library creation. While here, add a few comments to make the context set-up a bit easier to follow. Signed-off-by: George Dunlap Reviewed-by: Ian Jackson --- v2: - Fix unsafe libxl_childproc_hooks pointer behavior - Close down the SIGCHLD handler first, and make sure it's exited before closing the context - Explicitly decide to have a separate goroutine per ctx NB that due to a bug in libxl, this will hang without Ian's "[PATCH v2 00/10] libxl: event: Fix hang for some applications" series. CC: Nick Rosbrook CC: Ian Jackson --- tools/golang/xenlight/xenlight.go | 72 ++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go index 662b266250..c462e4bb42 100644 --- a/tools/golang/xenlight/xenlight.go +++ b/tools/golang/xenlight/xenlight.go @@ -20,6 +20,8 @@ package xenlight #cgo LDFLAGS: -lxenlight -lyajl -lxentoollog #include #include + +libxl_childproc_hooks xenlight_childproc_hooks; */ import "C" @@ -33,6 +35,9 @@ import "C" import ( "fmt" + "os" + "os/signal" + "syscall" "unsafe" ) @@ -72,10 +77,49 @@ func (e Error) Error() string { return fmt.Sprintf("libxl error: %d", e) } +func init() { + // libxl for some reason wants to: + // 1. Retain a copy to this pointer as long as the context is open, and + // 2. Not free it when it's done + // + // Rather than alloc and free multiple copies, just keep a single + // static copy in the C space (since C code isn't allowed to retain pointers + // to go code), and initialize it once. + C.xenlight_childproc_hooks.chldowner = C.libxl_sigchld_owner_mainloop +} + // Context represents a libxl_ctx. type Context struct { - ctx *C.libxl_ctx - logger *C.xentoollog_logger_stdiostream + ctx *C.libxl_ctx + logger *C.xentoollog_logger_stdiostream + sigchld chan os.Signal + sigchldDone chan bool +} + +// Golang always unmasks SIGCHLD, and internally has ways of +// distributing SIGCHLD to multiple recipients. libxl has provision +// for this model: just tell it when a SIGCHLD happened, and it will +// look after its own processes. +// +// This should "play nicely" with other users of SIGCHLD as long as +// they don't reap libxl's processes. +// +// Every context needs to be notified on each SIGCHLD; so spin up a +// new goroutine for each context. If there are a large number of contexts, +// this means each context will be woken up looking through its own list of children. +// +// The alternate would be to register a fork callback, such that the +// xenlight package can make a single list of all children, and only +// notify the specific libxl context(s) that have children woken. But +// it's not clear to me this will be much more work than having the +// xenlight go library do the same thing; doing it in separate go +// threads has the potential to do it in parallel. Leave that as an +// optimization for later if it turns out to be a bottleneck. +func sigchldHandler(ctx *Context) { + for _ = range ctx.sigchld { + go C.libxl_childproc_sigchld_occurred(ctx.ctx) + } + close(ctx.sigchldDone) } // NewContext returns a new Context. @@ -89,19 +133,43 @@ func NewContext() (ctx *Context, err error) { } }() + // Create a logger ctx.logger = C.xtl_createlogger_stdiostream(C.stderr, C.XTL_DEBUG, 0) + // Allocate a context ret := C.libxl_ctx_alloc(&ctx.ctx, C.LIBXL_VERSION, 0, (*C.xentoollog_logger)(unsafe.Pointer(ctx.logger))) if ret != 0 { return ctx, Error(ret) } + // Tell libxl that we'll be dealing with SIGCHLD... + C.libxl_childproc_setmode(ctx.ctx, &C.xenlight_childproc_hooks, nil) + + // ...and arrange to keep that promise. + ctx.sigchld = make(chan os.Signal, 2) + ctx.sigchldDone = make(chan bool, 1) + signal.Notify(ctx.sigchld, syscall.SIGCHLD) + + go sigchldHandler(ctx) + return ctx, nil } // Close closes the Context. func (ctx *Context) Close() error { + // Tell our SIGCHLD notifier to shut down, and wait for it to exit + // before we free the context. + if ctx.sigchld == nil { + signal.Stop(ctx.sigchld) + close(ctx.sigchld) + + <-ctx.sigchldDone + + ctx.sigchld = nil + ctx.sigchldDone = nil + } + if ctx.ctx != nil { ret := C.libxl_ctx_free(ctx.ctx) if ret != 0 {