From patchwork Thu May 9 20:06:33 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 10937597 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id CE40C76 for ; Thu, 9 May 2019 20:06:46 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id BEB8628BB4 for ; Thu, 9 May 2019 20:06:46 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B2C2B28BBE; Thu, 9 May 2019 20:06:46 +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=-5.2 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 40E7F28BB4 for ; Thu, 9 May 2019 20:06:46 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A8E7189B46; Thu, 9 May 2019 20:06:42 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-ed1-x541.google.com (mail-ed1-x541.google.com [IPv6:2a00:1450:4864:20::541]) by gabe.freedesktop.org (Postfix) with ESMTPS id 561B889A91 for ; Thu, 9 May 2019 20:06:41 +0000 (UTC) Received: by mail-ed1-x541.google.com with SMTP id f37so3101661edb.13 for ; Thu, 09 May 2019 13:06:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=2eMy81xQoynkvOjZ8yVubZa7SulddFEJzBcPEPrFGYQ=; b=qCE/UVg55UIzJuL/fAsmxIvseAmQkIAtI0a8FpJBTXo/S0b13ytYo4j6JoS/nBM48d o0+PAB1iGnySi2Y/wVfI0py7Be9DsiUj+U62WIe6fA2utB1SO1F6hmhd8FnORjImT0V4 +scB3wEWDamr37kmN3tutAn4J9bMIyilAHz4Mnp+P1uKwZ0Y/BHXaHJxd2LGpT3ynivi lDejcZ0Rv9wt6PyNqalq7BF58alPhKHnbHzuP7DudQreQ2p17ceMyWVmuIGAzAIexUM/ jsK94JxjRRrVoXbSOiIdd1q1vXo+ARHK7LjF9QOPJ1q5xXK8siUD8sbjnltvBZg22KsB DbQw== X-Gm-Message-State: APjAAAXBln/rU20OzsJ4elS7Diyf6SRoGqUjP0fXQI7rNdUCyt9at/Wi BjfthT62e8zAvg7rewYdX5HBY5NAo28= X-Google-Smtp-Source: APXvYqxj4+2Mrt199n9RPZfirgO2cTGlKahmfW3rytPUch4MQ27eM7Nxv4JvVHynhY78CelcoZKfCw== X-Received: by 2002:a50:a5ed:: with SMTP id b42mr6126084edc.178.1557432399557; Thu, 09 May 2019 13:06:39 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:569e:0:3106:d637:d723:e855]) by smtp.gmail.com with ESMTPSA id x30sm813241edd.74.2019.05.09.13.06.37 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 09 May 2019 13:06:38 -0700 (PDT) From: Daniel Vetter To: Intel Graphics Development Date: Thu, 9 May 2019 22:06:33 +0200 Message-Id: <20190509200633.19678-1-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20190509120903.28939-1-daniel.vetter@ffwll.ch> References: <20190509120903.28939-1-daniel.vetter@ffwll.ch> MIME-Version: 1.0 X-Mailman-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=2eMy81xQoynkvOjZ8yVubZa7SulddFEJzBcPEPrFGYQ=; b=Hjg39OzW1Svgec3a5DZNVclKDdeN2jAcbw2Ff17kMPtzSfVdFVjOOMpVM+8pvd/y1G GxzIE6WCwJDU7oO3C++iCeBly3eLKVezmFRH1W8G4HRxbWQVnSyyEVMFWFeojqEva+ls hsUSstHf3wZDSxBqOGTWOfE536dkZ5+uGGC4k= Subject: [Intel-gfx] [PATCH] kernel/locking/semaphore: use wake_q in up() X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Petr Mladek , John Ogness , Peter Zijlstra , Daniel Vetter , Will Deacon , linux-kernel@vger.kernel.org, DRI Development , Sergey Senozhatsky , Ingo Molnar , Steven Rostedt , Daniel Vetter Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP console_trylock, called from within printk, can be called from pretty much anywhere. Including try_to_wake_up. Note that this isn't common, usually the box is in pretty bad shape at that point already. But it really doesn't help when then lockdep jumps in and spams the logs, potentially obscuring the real backtrace we're really interested in. One case I've seen (slightly simplified backtrace): Call Trace: console_trylock+0xe/0x60 vprintk_emit+0xf1/0x320 printk+0x4d/0x69 __warn_printk+0x46/0x90 native_smp_send_reschedule+0x2f/0x40 check_preempt_curr+0x81/0xa0 ttwu_do_wakeup+0x14/0x220 try_to_wake_up+0x218/0x5f0 pollwake+0x6f/0x90 credit_entropy_bits+0x204/0x310 add_interrupt_randomness+0x18f/0x210 handle_irq+0x67/0x160 do_IRQ+0x5e/0x130 common_interrupt+0xf/0xf This alone isn't a problem, but the spinlock in the semaphore is also still held while waking up waiters (up() -> __up() -> try_to_wake_up() callchain), which then closes the runqueue vs. semaphore.lock loop, and upsets lockdep, which issues a circular locking splat to dmesg. Worse it upsets developers, since we don't want to spam dmesg with clutter when the machine is dying already. Fix this specific locking recursion by moving the wake_up_process out from under the semaphore.lock spinlock, using wake_q as recommended by Peter Zijlstra. As Petr Mladek points out this doesn't fix all the locking recursions in this area. If we actually recursive in the above callchain: + try_to_wake_up() # takes p->pi_lock + ttwu_remote() # takes rq lock + ttwu_do_wakeup() + check_preempt_curr() + native_smp_send_reschedule() + __warn_printk() + printk() + vprintk_emit() + console_trylock() # success + console_unlock() + up_console_sem() + up() # wait list in not empty + __up() + wake_up_process() + try_to_wake_up() Then there's any number of scheduler related locks will deadlock. Given that the kernel is dying already (the printk() in native_smp_send_reschedule() happens because we run on an offlined CPU) I think there's limited value in trying to fix this: - We haven't seen the actual deadlock in our CI, only lockdep complaining about the possibility. - The real issue is that the lockdep splat hides useful dmesg information we capture in e.g. pstore or on screen about the real cause of why the kernel is dying. - The console_unlock in the above callchain should have managed to get all the dmesg up to that point out already. Dying later on is somewhat ok - I've only seen this lockdep splat in pstore when the machine died anyway. Also cc'ing John Ogness since perhaps his printk rework fixes this all properly. v2: Ditch attempt to fix console_trylock. v3: Add a comment explaining why the taks we're waking won't disappear (Chris), and improve commit message to address review questions. v4: Use wake_q (Peter Z). Signed-off-by: Daniel Vetter Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Will Deacon Cc: Petr Mladek Cc: Sergey Senozhatsky Cc: Steven Rostedt Cc: Daniel Vetter Cc: John Ogness Cc: Chris Wilson Cc: linux-kernel@vger.kernel.org Signed-off-by: Daniel Vetter Reviewed-by: Petr Mladek --- kernel/locking/semaphore.c | 42 +++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c index 561acdd39960..7a6f33715688 100644 --- a/kernel/locking/semaphore.c +++ b/kernel/locking/semaphore.c @@ -33,12 +33,12 @@ #include #include #include +#include static noinline void __down(struct semaphore *sem); static noinline int __down_interruptible(struct semaphore *sem); static noinline int __down_killable(struct semaphore *sem); static noinline int __down_timeout(struct semaphore *sem, long timeout); -static noinline void __up(struct semaphore *sem); /** * down - acquire the semaphore @@ -169,6 +169,14 @@ int down_timeout(struct semaphore *sem, long timeout) } EXPORT_SYMBOL(down_timeout); +/* Functions for the contended case */ + +struct semaphore_waiter { + struct list_head list; + struct task_struct *task; + bool up; +}; + /** * up - release the semaphore * @sem: the semaphore to release @@ -179,24 +187,25 @@ EXPORT_SYMBOL(down_timeout); void up(struct semaphore *sem) { unsigned long flags; + struct semaphore_waiter *waiter; + DEFINE_WAKE_Q(wake_q); raw_spin_lock_irqsave(&sem->lock, flags); - if (likely(list_empty(&sem->wait_list))) + if (likely(list_empty(&sem->wait_list))) { sem->count++; - else - __up(sem); + } else { + waiter = list_first_entry(&sem->wait_list, + struct semaphore_waiter, list); + list_del(&waiter->list); + waiter->up = true; + wake_q_add(&wake_q, waiter->task); + } raw_spin_unlock_irqrestore(&sem->lock, flags); + + wake_up_q(&wake_q); } EXPORT_SYMBOL(up); -/* Functions for the contended case */ - -struct semaphore_waiter { - struct list_head list; - struct task_struct *task; - bool up; -}; - /* * Because this function is inlined, the 'state' parameter will be * constant, and thus optimised away by the compiler. Likewise the @@ -252,12 +261,3 @@ static noinline int __sched __down_timeout(struct semaphore *sem, long timeout) { return __down_common(sem, TASK_UNINTERRUPTIBLE, timeout); } - -static noinline void __sched __up(struct semaphore *sem) -{ - struct semaphore_waiter *waiter = list_first_entry(&sem->wait_list, - struct semaphore_waiter, list); - list_del(&waiter->list); - waiter->up = true; - wake_up_process(waiter->task); -}