From patchwork Fri Jun 2 15:29:34 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Roman Pen X-Patchwork-Id: 9762853 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B9D7160365 for ; Fri, 2 Jun 2017 15:30:49 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A0BCA28506 for ; Fri, 2 Jun 2017 15:30:49 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 920CB2852E; Fri, 2 Jun 2017 15:30:49 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 6930F28506 for ; Fri, 2 Jun 2017 15:30:48 +0000 (UTC) Received: from localhost ([::1]:50286 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dGoXH-0000eu-3b for patchwork-qemu-devel@patchwork.kernel.org; Fri, 02 Jun 2017 11:30:47 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44795) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dGoWT-0000cu-CN for qemu-devel@nongnu.org; Fri, 02 Jun 2017 11:29:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dGoWS-00070x-0i for qemu-devel@nongnu.org; Fri, 02 Jun 2017 11:29:57 -0400 Received: from mail-it0-x22f.google.com ([2607:f8b0:4001:c0b::22f]:34203) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dGoWR-00070T-PX for qemu-devel@nongnu.org; Fri, 02 Jun 2017 11:29:55 -0400 Received: by mail-it0-x22f.google.com with SMTP id m47so4639281iti.1 for ; Fri, 02 Jun 2017 08:29:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=profitbricks-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=OKifP1iOXnfZ9z40na20nXLHuGvZbsAp/f3LXwYVwoQ=; b=errHQvLWiiZw7WxmqGRzo4d6RLZuORygDpGznLPXGge/uLfoGcGThP5wh7gNs1m83w /VxLr8OMiXsy6NjdxM2kqyM5nEKZfm2tgaKZTEPR4n0OnBWSoMjDvV4cNGJ3YBlosvYh 0F8hRwf+IbadZSq5TnuEud/qkQd47QQRSMlR0eNzPN3YIeJTdNH2k7YLnQQ7UMu1VRPi EPgH6MQf/EJ+BsXdleQvLd8izdlzGCGrD9kbm3ypeC1O0kcXf6P7VdASKHFx+ugHuvT1 e9tu/MWUliPAeuLbQeBmauYh4ToIpvy+bsBmlqTPfTIUx1vkhyEbjfDKLRpn8PmgQxD+ lzoA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=OKifP1iOXnfZ9z40na20nXLHuGvZbsAp/f3LXwYVwoQ=; b=KctKvLa3r+MXXd4yklORyvytdO+MTYIW7cvxkPciSZcfBQIJ7fIOIxXPOXM21Qskp+ WeCho/olAi94ZylJsJlI82Hyr+9U5hBaqtYgFv1dZaFvCsRUN2T78ByIdb0zl3+4HTz/ p8PJ7dZdhJxPeKtyOhKk2Iht6kpTC/GdEVmbPHhTiIrN9fFPYVc3Q3PywZ0ldg23BscV YicPNzj7cXf2VF44qZ97KayMjdRelTNHhloMwgVohoqGCysuKtJAiQdLPt2g6CrScgjU rmZ8C/9OKB4fAIkT9zE9D8dIm0BZtEceRovV/yzIT8/9bj0k//8PvSvh277IZB5ifOnE q1Yw== X-Gm-Message-State: AODbwcCORmDcnOCyS9Y78tAJaWvDAVlNGSYWbgma2uk2w8DIe/U2JbsH zrprzcF5Sv+ZOENU9OFlZFiCAELKUMH5 X-Received: by 10.36.211.69 with SMTP id n66mr5310804itg.36.1496417394917; Fri, 02 Jun 2017 08:29:54 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.18.31 with HTTP; Fri, 2 Jun 2017 08:29:34 -0700 (PDT) In-Reply-To: <20170601160847.23720-1-roman.penyaev@profitbricks.com> References: <20170601160847.23720-1-roman.penyaev@profitbricks.com> From: Roman Penyaev Date: Fri, 2 Jun 2017 17:29:34 +0200 Message-ID: To: Roman Pen X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4001:c0b::22f Subject: Re: [Qemu-devel] [PATCH v3 1/1] coroutine-lock: do not touch coroutine after another one has been entered X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kevin Wolf , Paolo Bonzini , Fam Zheng , qemu-devel , Stefan Hajnoczi Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Jun 1, 2017 at 6:08 PM, Roman Pen wrote: [cut] > Thread 12 (Thread 0x7f57f2ffd700 (LWP 56426)): > #0 raise () from /lib/x86_64-linux-gnu/libc.so.6 > #1 abort () from /lib/x86_64-linux-gnu/libc.so.6 > #2 qemu_coroutine_enter (co=0x7f5804009af0) at qemu-coroutine.c:113 > #3 qemu_co_queue_run_restart (co=0x7f5804009a30) at qemu-coroutine-lock.c:60 > #4 qemu_coroutine_enter (co=0x7f5804009a30) at qemu-coroutine.c:119 > ^^^^^^^^^^^^^^^^^^ > WTF?? this is equal to elem from crashed thread > > #5 qemu_co_queue_run_restart (co=0x7f57e7f16ae0) at qemu-coroutine-lock.c:60 > #6 qemu_coroutine_enter (co=0x7f57e7f16ae0) at qemu-coroutine.c:119 > #7 qemu_co_queue_run_restart (co=0x7f5807e112a0) at qemu-coroutine-lock.c:60 > #8 qemu_coroutine_enter (co=0x7f5807e112a0) at qemu-coroutine.c:119 > #9 qemu_co_queue_run_restart (co=0x7f5807f17820) at qemu-coroutine-lock.c:60 > #10 qemu_coroutine_enter (co=0x7f5807f17820) at qemu-coroutine.c:119 > #11 qemu_co_queue_run_restart (co=0x7f57e7f18e10) at qemu-coroutine-lock.c:60 > #12 qemu_coroutine_enter (co=0x7f57e7f18e10) at qemu-coroutine.c:119 > #13 qemu_co_enter_next (queue=queue@entry=0x5598b1e742d0) at qemu-coroutine-lock.c:106 > #14 timer_cb (blk=0x5598b1e74280, is_write=) at throttle-groups.c:419 BTW, while chasing current bug I observed utterly long recursive backtraces with repeating: qemu_co_queue_run_restart qemu_coroutine_enter .... like in the current bug but much much longer. It is clear that each pair qemu_coroutine_enter(),qemu_co_queue_run_restart() is one request/coroutine, so in the worst case we can get stack depth equal to number of requests in flight which were throttled. But real number of requests with MQ virtio device can be very huge. When virtio device is created with num-queues=$VCPU we have requests in total equal to nr_requests x num_queues. Say if guest has 16 VCPUS, virtio block device can have 128x16 = 2048 requests in flight. If my calculations are correct each loop costs around 112 bytes on stack, so 2048x112 = 224K. Definitely not nice. Straightforward fix can be in keeping queue_wakeup on stack. I did just fast dirty @@ -101,7 +101,7 @@ static void coroutine_delete(Coroutine *co) qemu_coroutine_delete(co); } -void qemu_coroutine_enter(Coroutine *co) +void __qemu_coroutine_enter(Coroutine *co, CoroutineQueue *queue_wakeup) { Coroutine *self = qemu_coroutine_self(); CoroutineAction ret; @@ -114,17 +114,9 @@ void qemu_coroutine_enter(Coroutine *co) } co->caller = self; + co->co_queue_wakeup = queue_wakeup; ret = qemu_coroutine_switch(self, co, COROUTINE_ENTER); - qemu_co_queue_run_restart(co); - - /* - * BEWARE: in case of ret == COROUTINE_YIELD here at this point - * after qemu_co_queue_run_restart() 'co' can be already - * freed by other coroutine, which has entered 'co'. So - * be careful and do not touch it. - */ - switch (ret) { case COROUTINE_YIELD: return; @@ -137,6 +129,15 @@ void qemu_coroutine_enter(Coroutine *co) } } +void qemu_coroutine_enter(Coroutine *co) +{ + CoroutineQueue queue_wakeup = + QSIMPLEQ_HEAD_INITIALIZER(queue_wakeup); + + __qemu_coroutine_enter(co, &queue_wakeup); + qemu_co_queue_run_restart(&queue_wakeup); +} + void coroutine_fn qemu_coroutine_yield(void) { Coroutine *self = qemu_coroutine_self(); @@ -150,6 +151,7 @@ void coroutine_fn qemu_coroutine_yield(void) } self->caller = NULL; + self->co_queue_wakeup = NULL; qemu_coroutine_switch(self, to, COROUTINE_YIELD); } diff (below), but seems the idea should be clear. That will iron out recursion and save 8 bytes for each coroutine. -- Roman diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index e46a32d0fcc2..4f1a3278e432 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -65,10 +65,13 @@ typedef void coroutine_fn CoroutineEntry(void *opaque); */ Coroutine *qemu_coroutine_create(CoroutineEntry *entry, void *opaque); +typedef QSIMPLEQ_HEAD(CoroutineQueue, Coroutine) CoroutineQueue; + /** * Transfer control to a coroutine */ void qemu_coroutine_enter(Coroutine *coroutine); +void __qemu_coroutine_enter(Coroutine *coroutine, CoroutineQueue *queue_wakeup); /** * Transfer control back to a coroutine's caller diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h index 581a7f514075..edf1c0a4edbb 100644 --- a/include/qemu/coroutine_int.h +++ b/include/qemu/coroutine_int.h @@ -41,7 +41,7 @@ struct Coroutine { QSLIST_ENTRY(Coroutine) pool_next; /* Coroutines that should be woken up when we yield or terminate */ - QSIMPLEQ_HEAD(, Coroutine) co_queue_wakeup; + CoroutineQueue *co_queue_wakeup; QSIMPLEQ_ENTRY(Coroutine) co_queue_next; }; @@ -49,6 +49,6 @@ Coroutine *qemu_coroutine_new(void); void qemu_coroutine_delete(Coroutine *co); CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to, CoroutineAction action); -void coroutine_fn qemu_co_queue_run_restart(Coroutine *co); +void coroutine_fn qemu_co_queue_run_restart(CoroutineQueue *queue_wakeup); #endif diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c index 0db8e4fc6d98..9282e5b82824 100644 --- a/util/qemu-coroutine-lock.c +++ b/util/qemu-coroutine-lock.c @@ -50,27 +50,13 @@ void coroutine_fn qemu_co_queue_wait(CoQueue *queue) * invoked by the core coroutine code when the current coroutine yields or * terminates. */ -void qemu_co_queue_run_restart(Coroutine *co) +void qemu_co_queue_run_restart(CoroutineQueue *queue_wakeup) { Coroutine *next; - QSIMPLEQ_HEAD(, Coroutine) tmp_queue_wakeup = - QSIMPLEQ_HEAD_INITIALIZER(tmp_queue_wakeup); - - trace_qemu_co_queue_run_restart(co); - - /* - * We use temporal queue in order not to touch 'co' after entering - * 'next' coroutine. The thing is that 'next' coroutine can resume - * current 'co' coroutine and eventually terminate (free) it (see - * linux-aio.c: ioq_submit() where qemu_laio_process_completions() - * is invoked). The rule of thumb is simple: do not touch coroutine - * when you enter another one. - */ - QSIMPLEQ_CONCAT(&tmp_queue_wakeup, &co->co_queue_wakeup); - - while ((next = QSIMPLEQ_FIRST(&tmp_queue_wakeup))) { - QSIMPLEQ_REMOVE_HEAD(&tmp_queue_wakeup, co_queue_next); - qemu_coroutine_enter(next); + + while ((next = QSIMPLEQ_FIRST(queue_wakeup))) { + QSIMPLEQ_REMOVE_HEAD(queue_wakeup, co_queue_next); + __qemu_coroutine_enter(next, queue_wakeup); } } @@ -85,7 +71,7 @@ static bool qemu_co_queue_do_restart(CoQueue *queue, bool single) while ((next = QSIMPLEQ_FIRST(&queue->entries)) != NULL) { QSIMPLEQ_REMOVE_HEAD(&queue->entries, co_queue_next); - QSIMPLEQ_INSERT_TAIL(&self->co_queue_wakeup, next, co_queue_next); + QSIMPLEQ_INSERT_TAIL(self->co_queue_wakeup, next, co_queue_next); trace_qemu_co_queue_next(next); if (single) { break; diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c index 1a89e04238fa..6d280921c161 100644 --- a/util/qemu-coroutine.c +++ b/util/qemu-coroutine.c @@ -77,7 +77,7 @@ Coroutine *qemu_coroutine_create(CoroutineEntry *entry, void *opaque) co->entry = entry; co->entry_arg = opaque; - QSIMPLEQ_INIT(&co->co_queue_wakeup); + co->co_queue_wakeup = NULL; return co; }