Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752872AbcLFIg6 (ORCPT ); Tue, 6 Dec 2016 03:36:58 -0500 Received: from mail-wj0-f195.google.com ([209.85.210.195]:34503 "EHLO mail-wj0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752162AbcLFIg4 (ORCPT ); Tue, 6 Dec 2016 03:36:56 -0500 Date: Tue, 6 Dec 2016 09:36:51 +0100 From: Ingo Molnar To: Peter Zijlstra Cc: Linus Torvalds , Vegard Nossum , Dave Jones , Chris Mason , Jens Axboe , Andy Lutomirski , Andy Lutomirski , Al Viro , Josef Bacik , David Sterba , linux-btrfs , Linux Kernel , Dave Chinner Subject: Re: bio linked list corruption. Message-ID: <20161206083651.GB13195@gmail.com> References: <20161123193419.pq7adje2eanky2wx@codemonkey.org.uk> <20161123195845.iphzr7ac4mu5ewjt@codemonkey.org.uk> <20161206081659.GV3092@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161206081659.GV3092@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1548 Lines: 59 * Peter Zijlstra wrote: > $ git grep DECLARE_WAIT_QUEUE_HEAD_ONSTACK | wc -l > 28 This debug facility looks sensible. A couple of minor suggestions: > --- a/include/linux/wait.h > +++ b/include/linux/wait.h > @@ -39,6 +39,9 @@ struct wait_bit_queue { > struct __wait_queue_head { > spinlock_t lock; > struct list_head task_list; > +#ifdef CONFIG_DEBUG_WAITQUEUE > + int onstack; > +#endif The structure will pack better in the debug-enabled case if 'onstack' is next to 'lock', as spinlock_t is 4 bytes on many architectures. > -#define __WAIT_QUEUE_HEAD_INITIALIZER(name) { \ > +#ifdef CONFIG_DEBUG_WAITQUEUE > +#define ___WAIT_QUEUE_ONSTACK(onstack) .onstack = (onstack), > +#else > +#define ___WAIT_QUEUE_ONSTACK(onstack) > +#endif Please help readers by showing the internal structure of the definition: #ifdef CONFIG_DEBUG_WAITQUEUE # define ___WAIT_QUEUE_ONSTACK(onstack) .onstack = (onstack), #else # define ___WAIT_QUEUE_ONSTACK(onstack) #endif > +static inline void prepare_debug(wait_queue_head_t *q, wait_queue_t *wait) > +{ > +#ifdef CONFIG_DEBUG_WAITQUEUE > + WARN_ON_ONCE(q->onstack && wait->func == autoremove_wake_function) > +#endif > +} I'd name this debug_waitqueue_check() or such - as the 'prepare' is a bit misleadig (we don't prepare debugging, we do the debug check here). > +config DEBUG_WAITQUEUE > + bool "Debug waitqueue" > + depends on DEBUG_KERNEL I'd name it DEBUG_SCHED_WAITQUEUE=y and I'd also make it depend on CONFIG_DEBUG_SCHED. LGTM otherwise! Thanks, Ingo