From patchwork Fri Jan 10 13:28:58 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ian Jackson X-Patchwork-Id: 11327395 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 620FA930 for ; Fri, 10 Jan 2020 13:30:19 +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 329342077C for ; Fri, 10 Jan 2020 13:30:19 +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="hL8RPhpl" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 329342077C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=eu.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 1ipuLs-00022V-Bn; Fri, 10 Jan 2020 13:29:24 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1ipuLq-000220-UE for xen-devel@lists.xenproject.org; Fri, 10 Jan 2020 13:29:22 +0000 X-Inumbo-ID: 2ebce332-33ad-11ea-b89f-bc764e2007e4 Received: from esa3.hc3370-68.iphmx.com (unknown [216.71.145.155]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 2ebce332-33ad-11ea-b89f-bc764e2007e4; Fri, 10 Jan 2020 13:29:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=citrix.com; s=securemail; t=1578662950; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version; bh=tldUzIpqW259HNlP4sDhwClv5zBMPQrjoeXdLGkijiY=; b=hL8RPhplTbVLIUPN9+yytOTF9KHem8Ev0TFN4mp9N5cE13XOlFREPiaJ bmWmahBA6OZAjEOKmDwpDyi0w6zGnBTnBV/XDQnw7iNtTp37IA6gs2M4n qZIR5JLNDZspmve4eDdC1fX3zR3O9xBZ4bzEsSMNpvZr646YSdiZuNjx/ 8=; Authentication-Results: esa3.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=ian.jackson@eu.citrix.com; spf=Pass smtp.mailfrom=Ian.Jackson@citrix.com; spf=None smtp.helo=postmaster@mail.citrix.com Received-SPF: None (esa3.hc3370-68.iphmx.com: no sender authenticity information available from domain of ian.jackson@eu.citrix.com) identity=pra; client-ip=162.221.158.21; receiver=esa3.hc3370-68.iphmx.com; envelope-from="Ian.Jackson@citrix.com"; x-sender="ian.jackson@eu.citrix.com"; x-conformance=sidf_compatible Received-SPF: Pass (esa3.hc3370-68.iphmx.com: domain of Ian.Jackson@citrix.com designates 162.221.158.21 as permitted sender) identity=mailfrom; client-ip=162.221.158.21; receiver=esa3.hc3370-68.iphmx.com; envelope-from="Ian.Jackson@citrix.com"; x-sender="Ian.Jackson@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 (esa3.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=esa3.hc3370-68.iphmx.com; envelope-from="Ian.Jackson@citrix.com"; x-sender="postmaster@mail.citrix.com"; x-conformance=sidf_compatible IronPort-SDR: f8IU3UlIfR4o62I84iI1o5RbjLKf9uOL+JmrpXc1MHa2QtN/WDB6fYCJXa3Z44Qh6mV0ncvRYT vmkCWzwmFvjL307KSroTTFW5uTTl2y0s33dp/DR6E5Jy5cZ8HnBI3XqEcFlvwS5K8LZWpknGGA wTldvoSnqSsAdTQOw1vs54VOaGDsxkyLMUKMLo/esH4P6lcE7ugHLYdMdBlVdof5Aah50thLu9 5hEmor/ge8n7N865UQi+HAO4D270JEgmBo0Kd+e4HHTWdPTwqtn5p0YsYCnoM0eHas/flidarn 2vk= X-SBRS: 2.7 X-MesageID: 10724855 X-Ironport-Server: esa3.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.69,417,1571716800"; d="scan'208";a="10724855" From: Ian Jackson To: Date: Fri, 10 Jan 2020 13:28:58 +0000 Message-ID: <20200110132902.29295-5-ian.jackson@eu.citrix.com> X-Mailer: git-send-email 2.11.0 In-Reply-To: <20200110132902.29295-1-ian.jackson@eu.citrix.com> References: <20200110132902.29295-1-ian.jackson@eu.citrix.com> MIME-Version: 1.0 Subject: [Xen-devel] [PATCH 4/8] libxl: event: Fix hang when mixing blocking and eventy calls 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: Anthony PERARD , Ian Jackson , George Dunlap , Wei Liu Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" If the application calls libxl with ao_how==0 and also makes calls like _occurred, libxl will sometimes get stuck. The bug happens as follows (for example): Thread A libxl_do_thing(,ao_how==0) libxl_do_thing starts, sets up some callbacks libxl_do_thing exit path calls AO_INPROGRESS libxl__ao_inprogress goes into event loop eventloop_iteration sleeps on: - do_thing's current fd set - sigchld pipe if applicable - its poller Thread B libxl_something_occurred the something is to do with do_thing, above do_thing_next_callback does some more work do_thing_next_callback becomes interested in fd N thread B returns to application Note that nothing wakes up thread A. A is not listening on fd N. So do_thing_* will not spot when fd N signals. do_thing will not make further timely progress. If there is no timeout thread A will never wake up. The problem here occurs because thread A is waiting on an out of date osevent set. We need the following property: whenever any thread is blocking in the libxl event loop, at least one thread must be using an up to date osevent set. It is OK for all but one threads to have stale event sets, because so long as one waiting thread has the right event set, any actually interesting event will, if nothing else, wake that "right" thread up. It will then make some progress and/or, if it exits, ensure that some other thread becomes the "right" thread. There is also the possibility that a thread might block waiting for libxl osevents but outside libxl, eg if the application used libxl_osevent_beforepoll. We will deal with that in a moment. Reported-by: George Dunlap Signed-off-by: Ian Jackson --- tools/libxl/libxl_event.c | 72 +++++++++++++++++++++++++++++++++++++------- tools/libxl/libxl_internal.h | 33 ++++++++++++++++---- 2 files changed, 88 insertions(+), 17 deletions(-) diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c index be37e12bb0..18db091a6e 100644 --- a/tools/libxl/libxl_event.c +++ b/tools/libxl/libxl_event.c @@ -36,6 +36,42 @@ static libxl__ao *ao_nested_root(libxl__ao *ao); static void ao__check_destroy(libxl_ctx *ctx, libxl__ao *ao); +static void pollers_note_osevent_added(libxl_ctx *ctx) { + libxl__poller *poller; + LIBXL_LIST_FOREACH(poller, &ctx->pollers_active, active_entry) + poller->osevents_added = 1; +} + +void libxl__egc_cleanup_1_baton(libxl__egc *egc) +{ + EGC_GC; + libxl__poller *search, *wake=0; + + LIBXL_LIST_FOREACH(search, &CTX->pollers_active, active_entry) { + if (search == CTX->poller_app) + /* This one is special. We can't give it the baton. */ + continue; + if (!search->osevents_added) + /* This poller is up to date and will wake up as needed. */ + return; + if (!wake) + wake = search; + } + + if (!wake) + /* no-one in libxl waiting for any events */ + return; + + libxl__poller_wakeup(egc, wake); + + wake->osevents_added = 0; + /* This serves to make _1_baton idempotent. It is OK even though + * that poller may currently be sleeping on only old osevents, + * because it is going to wake up because we've just prodded it, + * and it pick up new osevents on its next iteration (or pass + * on the baton). */ +} + /* * The counter osevent_in_hook is used to ensure that the application * honours the reentrancy restriction documented in libxl_event.h. @@ -194,6 +230,7 @@ int libxl__ev_fd_register(libxl__gc *gc, libxl__ev_fd *ev, ev->func = func; LIBXL_LIST_INSERT_HEAD(&CTX->efds, ev, entry); + pollers_note_osevent_added(CTX); rc = 0; @@ -214,6 +251,8 @@ int libxl__ev_fd_modify(libxl__gc *gc, libxl__ev_fd *ev, short events) rc = OSEVENT_HOOK(fd,modify, noop, ev->fd, &ev->nexus->for_app_reg, events); if (rc) goto out; + if ((events & ~ev->events)) + pollers_note_osevent_added(CTX); ev->events = events; rc = 0; @@ -315,6 +354,7 @@ static int time_register_finite(libxl__gc *gc, libxl__ev_time *ev, LIBXL_TAILQ_INSERT_SORTED(&CTX->etimes, entry, ev, evsearch, /*empty*/, timercmp(&ev->abs, &evsearch->abs, >)); + pollers_note_osevent_added(CTX); return 0; } @@ -1121,6 +1161,7 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller, *nfds_io = used; poller->fds_deregistered = 0; + poller->osevents_added = 0; libxl__ev_time *etime = LIBXL_TAILQ_FIRST(&CTX->etimes); if (etime) { @@ -1444,7 +1485,7 @@ static void egc_run_callbacks(libxl__egc *egc) } } -void libxl__egc_cleanup(libxl__egc *egc) +void libxl__egc_cleanup_2_ul_cb_gc(libxl__egc *egc) { EGC_GC; egc_run_callbacks(egc); @@ -1754,13 +1795,15 @@ int libxl_event_wait(libxl_ctx *ctx, libxl_event **event_r, rc = eventloop_iteration(egc, poller); if (rc) goto out; - /* we unlock and cleanup the egc each time we go through this loop, - * so that (a) we don't accumulate garbage and (b) any events - * which are to be dispatched by callback are actually delivered - * in a timely fashion. + /* we unlock and cleanup the egc each time we go through this + * loop, so that (a) we don't accumulate garbage and (b) any + * events which are to be dispatched by callback are actually + * delivered in a timely fashion. _1_baton will be + * called to pass the baton iff we actually leave; otherwise + * we are still carrying it. */ CTX_UNLOCK; - libxl__egc_cleanup(egc); + libxl__egc_cleanup_2_ul_cb_gc(egc); CTX_LOCK; } @@ -2033,10 +2076,12 @@ int libxl__ao_inprogress(libxl__ao *ao, * synchronous cancellation ability. */ } + /* The call to egc..1_baton is below, only if we are leaving. */ CTX_UNLOCK; - libxl__egc_cleanup(&egc); + libxl__egc_cleanup_2_ul_cb_gc(&egc); CTX_LOCK; } + libxl__egc_cleanup_1_baton(&egc); } else { rc = 0; } @@ -2053,6 +2098,9 @@ int libxl__ao_inprogress(libxl__ao *ao, static int ao__abort(libxl_ctx *ctx, libxl__ao *parent) /* Temporarily unlocks ctx, which must be locked exactly once on entry. */ { + libxl__egc egc; + LIBXL_INIT_EGC(egc,ctx); + int rc; ao__manip_enter(parent); @@ -2073,9 +2121,6 @@ static int ao__abort(libxl_ctx *ctx, libxl__ao *parent) /* We keep calling abort hooks until there are none left */ while (!LIBXL_LIST_EMPTY(&parent->abortables)) { - libxl__egc egc; - LIBXL_INIT_EGC(egc,ctx); - assert(!parent->complete); libxl__ao_abortable *abrt = LIBXL_LIST_FIRST(&parent->abortables); @@ -2088,8 +2133,9 @@ static int ao__abort(libxl_ctx *ctx, libxl__ao *parent) "ao %p: abrt=%p: aborting", parent, abrt->ao); abrt->callback(&egc, abrt, ERROR_ABORTED); + /* The call to egc..1_baton is in the out block below. */ libxl__ctx_unlock(ctx); - libxl__egc_cleanup(&egc); + libxl__egc_cleanup_2_ul_cb_gc(&egc); libxl__ctx_lock(ctx); } @@ -2097,6 +2143,10 @@ static int ao__abort(libxl_ctx *ctx, libxl__ao *parent) out: ao__manip_leave(ctx, parent); + /* The call to egc..2_ul_cb_gc is above. This is sufficient + * because only code inside the loop adds anything to the egc, and + * we ensures that the egc is clean when we leave the loop. */ + libxl__egc_cleanup_1_baton(&egc); return rc; } diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 983fffac7a..b9c4031863 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -634,9 +634,23 @@ struct libxl__poller { * event is deregistered, we set the fds_deregistered of all non-idle * pollers. So afterpoll can tell whether any POLLNVAL is * plausibly due to an fd being closed and reopened. + * + * Additionally, we record whether any fd or time event sources + * have been registered. This is necessary because sometimes we + * need to wake up the only libxl thread stuck in + * eventloop_iteration so that it will pick up new fds or earlier + * timeouts. osevents_added is cleared by beforepoll, and set by + * fd or timeout event registration. When we are about to leave + * libxl (strictly, when we are about to give up an egc), we check + * whether there are any pollers. If there are, then at least one + * of them must have osevents_added clear. If not, we wake up the + * first one on the list. Any entry on pollers_active constitutes + * a promise to also make this check, so the baton will never be + * dropped. */ LIBXL_LIST_ENTRY(libxl__poller) active_entry; bool fds_deregistered; + bool osevents_added; }; struct libxl__gc { @@ -2350,7 +2364,10 @@ _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc); LIBXL_STAILQ_INIT(&(egc).ev_immediates); \ } while(0) -_hidden void libxl__egc_cleanup(libxl__egc *egc); +_hidden void libxl__egc_cleanup_1_baton(libxl__egc *egc); + /* Passes the baton for added osevents. See comment for + * osevents_added in struct libxl__poller. */ +_hidden void libxl__egc_cleanup_2_ul_cb_gc(libxl__egc *egc); /* Frees memory allocated within this egc's gc, and and report all * occurred events via callback, if applicable. May reenter the * application; see restrictions above. The ctx must be UNLOCKED. */ @@ -2361,9 +2378,11 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc); libxl__egc egc[1]; LIBXL_INIT_EGC(egc[0],ctx); \ EGC_GC -#define EGC_FREE libxl__egc_cleanup(egc) - -#define CTX_UNLOCK_EGC_FREE do{ CTX_UNLOCK; EGC_FREE; }while(0) +#define CTX_UNLOCK_EGC_FREE do{ \ + libxl__egc_cleanup_1_baton(egc); \ + CTX_UNLOCK; \ + libxl__egc_cleanup_2_ul_cb_gc(egc); \ + }while(0) /* @@ -2468,8 +2487,9 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc); #define AO_INPROGRESS ({ \ libxl_ctx *ao__ctx = libxl__gc_owner(&ao->gc); \ + /* __ao_inprogress will do egc..1_baton if needed */ \ CTX_UNLOCK; \ - EGC_FREE; \ + libxl__egc_cleanup_2_ul_cb_gc(egc); \ CTX_LOCK; \ int ao__rc = libxl__ao_inprogress(ao, \ __FILE__, __LINE__, __func__); \ @@ -2481,8 +2501,9 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc); libxl_ctx *ao__ctx = libxl__gc_owner(&ao->gc); \ assert(rc); \ libxl__ao_create_fail(ao); \ + libxl__egc_cleanup_1_baton(egc); \ libxl__ctx_unlock(ao__ctx); /* gc is now invalid */ \ - EGC_FREE; \ + libxl__egc_cleanup_2_ul_cb_gc(egc); \ (rc); \ })