From mboxrd@z Thu Jan 1 00:00:00 1970 From: Josh Triplett Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Date: Sat, 23 Dec 2017 01:39:11 -0800 Message-ID: <20171223093910.GB6160@localhost> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> <20171222123112.GA6401@bombadil.infradead.org> <20171222133634.GE6401@bombadil.infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20171222133634.GE6401@bombadil.infradead.org> Sender: owner-linux-mm@kvack.org To: Matthew Wilcox Cc: Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox , linux-sparse@vger.kernel.org List-Id: linux-sparse@vger.kernel.org +linux-sparse On Fri, Dec 22, 2017 at 05:36:34AM -0800, Matthew Wilcox wrote: > On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote: > > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote: > > > On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote: > > > > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we > > > > return as this code will never run. > > > > > > It does matter slightly, as Sparse does some (very limited) value-based > > > analyses. Let's future-proof it. > > > > > > > That said, if sparse supports the GNU syntax of ?: then I have no > > > > objection to doing that. > > > > > > Sparse does support that syntax. > > > > Great, I'll fix that and resubmit. > > Except the context imbalance warning comes back if I do. This is sparse > 0.5.1 (Debian's 0.5.1-2 package). -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Date: Sat, 23 Dec 2017 05:06:21 -0800 Message-ID: <20171223130621.GA3994@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> <20171222123112.GA6401@bombadil.infradead.org> <20171222133634.GE6401@bombadil.infradead.org> <20171223093910.GB6160@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20171223093910.GB6160@localhost> Sender: owner-linux-mm@kvack.org To: Josh Triplett Cc: Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox , linux-sparse@vger.kernel.org List-Id: linux-sparse@vger.kernel.org On Sat, Dec 23, 2017 at 01:39:11AM -0800, Josh Triplett wrote: > +linux-sparse Ehh ... we've probably trimmed too much to give linux-sparse a good summary. Here're the important lines from my patch: +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) + return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, start, end, + ptepp, pmdpp, ptlp)); This is supposed to be "If "c" is an error value, we don't have a lock, otherwise we have a lock". And to translate from linux-speak into sparse-speak: # define __acquire(x) __context__(x,1) Josh & Ross pointed out (quite correctly) that code which does something like if (foo()) return; will work with this, but code that does if (foo() < 0) return; will not because we're now returning 1 instead of -ENOMEM (for example). So they made the very sensible suggestion that I change the definition of __cond_lock to: # define __cond_lock_err(x,c) ((c) ?: ({ __acquire(x); 0; })) Unfortunately, when I do that, the context imbalance warning returns. As I said below, this is with sparse 0.5.1. > On Fri, Dec 22, 2017 at 05:36:34AM -0800, Matthew Wilcox wrote: > > On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote: > > > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote: > > > > On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote: > > > > > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we > > > > > return as this code will never run. > > > > > > > > It does matter slightly, as Sparse does some (very limited) value-based > > > > analyses. Let's future-proof it. > > > > > > > > > That said, if sparse supports the GNU syntax of ?: then I have no > > > > > objection to doing that. > > > > > > > > Sparse does support that syntax. > > > > > > Great, I'll fix that and resubmit. > > > > Except the context imbalance warning comes back if I do. This is sparse > > 0.5.1 (Debian's 0.5.1-2 package). > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: email@kvack.org -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 From: Luc Van Oostenryck Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Date: Wed, 27 Dec 2017 15:38:50 +0100 Message-ID: <20171227143850.nnuatshhezurbu7r@ltop.local> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> <20171222123112.GA6401@bombadil.infradead.org> <20171222133634.GE6401@bombadil.infradead.org> <20171223093910.GB6160@localhost> <20171223130621.GA3994@bombadil.infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20171223130621.GA3994@bombadil.infradead.org> Sender: owner-linux-mm@kvack.org To: Matthew Wilcox Cc: Josh Triplett , Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox , linux-sparse@vger.kernel.org List-Id: linux-sparse@vger.kernel.org On Sat, Dec 23, 2017 at 05:06:21AM -0800, Matthew Wilcox wrote: > On Sat, Dec 23, 2017 at 01:39:11AM -0800, Josh Triplett wrote: > > +linux-sparse > > Ehh ... we've probably trimmed too much to give linux-sparse a good summary. > > Here're the important lines from my patch: > > +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) > > + return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, start, end, > + ptepp, pmdpp, ptlp)); > > This is supposed to be "If "c" is an error value, we don't have a lock, > otherwise we have a lock". And to translate from linux-speak into > sparse-speak: > > # define __acquire(x) __context__(x,1) > > Josh & Ross pointed out (quite correctly) that code which does something like > > if (foo()) > return; > > will work with this, but code that does > > if (foo() < 0) > return; > > will not because we're now returning 1 instead of -ENOMEM (for example). > > So they made the very sensible suggestion that I change the definition > of __cond_lock to: > > # define __cond_lock_err(x,c) ((c) ?: ({ __acquire(x); 0; })) > > Unfortunately, when I do that, the context imbalance warning returns. > As I said below, this is with sparse 0.5.1. I think this __cond_lock_err() is now OK (but some comment about how its use is different from __cond_lock() would be welcome). For the context imbalance, I would really need a concrete example to be able to help more because it depends heavily on what the test is and what code is before and after. If you can point me to a tree, a .config and a specific warning, I'll be glad to take a look. -- Luc Van Oostenryck -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f69.google.com (mail-pg0-f69.google.com [74.125.83.69]) by kanga.kvack.org (Postfix) with ESMTP id 52DEB6B0038 for ; Tue, 19 Dec 2017 11:58:39 -0500 (EST) Received: by mail-pg0-f69.google.com with SMTP id y2so12799510pgv.8 for ; Tue, 19 Dec 2017 08:58:39 -0800 (PST) Received: from bombadil.infradead.org (bombadil.infradead.org. [65.50.211.133]) by mx.google.com with ESMTPS id v90si11475407pfk.397.2017.12.19.08.58.38 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 19 Dec 2017 08:58:38 -0800 (PST) From: Matthew Wilcox Subject: [PATCH 1/2] mm: Make follow_pte_pmd an inline Date: Tue, 19 Dec 2017 08:58:22 -0800 Message-Id: <20171219165823.24243-1-willy@infradead.org> Sender: owner-linux-mm@kvack.org List-ID: To: linux-kernel@vger.kernel.org, Ross Zwisler , Dave Hansen , linux-mm@kvack.org, Josh Triplett Cc: Matthew Wilcox From: Matthew Wilcox The one user of follow_pte_pmd (dax) emits a sparse warning because it doesn't know that follow_pte_pmd conditionally returns with the pte/pmd locked. The required annotation is already there; it's just in the wrong file. --- include/linux/mm.h | 15 ++++++++++++++- mm/memory.c | 16 +--------------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/include/linux/mm.h b/include/linux/mm.h index ea818ff739cd..94a9d2149bd6 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1314,7 +1314,7 @@ int copy_page_range(struct mm_struct *dst, struct mm_struct *src, struct vm_area_struct *vma); void unmap_mapping_range(struct address_space *mapping, loff_t const holebegin, loff_t const holelen, int even_cows); -int follow_pte_pmd(struct mm_struct *mm, unsigned long address, +int __follow_pte_pmd(struct mm_struct *mm, unsigned long address, unsigned long *start, unsigned long *end, pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp); int follow_pfn(struct vm_area_struct *vma, unsigned long address, @@ -1324,6 +1324,19 @@ int follow_phys(struct vm_area_struct *vma, unsigned long address, int generic_access_phys(struct vm_area_struct *vma, unsigned long addr, void *buf, int len, int write); +static inline int follow_pte_pmd(struct mm_struct *mm, unsigned long address, + unsigned long *start, unsigned long *end, + pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp) +{ + int res; + + /* (void) is needed to make gcc happy */ + (void) __cond_lock(*ptlp, + !(res = __follow_pte_pmd(mm, address, start, end, + ptepp, pmdpp, ptlp))); + return res; +} + static inline void unmap_shared_mapping_range(struct address_space *mapping, loff_t const holebegin, loff_t const holelen) { diff --git a/mm/memory.c b/mm/memory.c index cfaba6287702..cb433662af21 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -4201,7 +4201,7 @@ int __pmd_alloc(struct mm_struct *mm, pud_t *pud, unsigned long address) } #endif /* __PAGETABLE_PMD_FOLDED */ -static int __follow_pte_pmd(struct mm_struct *mm, unsigned long address, +int __follow_pte_pmd(struct mm_struct *mm, unsigned long address, unsigned long *start, unsigned long *end, pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp) { @@ -4278,20 +4278,6 @@ static inline int follow_pte(struct mm_struct *mm, unsigned long address, return res; } -int follow_pte_pmd(struct mm_struct *mm, unsigned long address, - unsigned long *start, unsigned long *end, - pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp) -{ - int res; - - /* (void) is needed to make gcc happy */ - (void) __cond_lock(*ptlp, - !(res = __follow_pte_pmd(mm, address, start, end, - ptepp, pmdpp, ptlp))); - return res; -} -EXPORT_SYMBOL(follow_pte_pmd); - /** * follow_pfn - look up PFN at a user virtual address * @vma: memory mapping -- 2.15.1 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f70.google.com (mail-pg0-f70.google.com [74.125.83.70]) by kanga.kvack.org (Postfix) with ESMTP id 554A36B0069 for ; Tue, 19 Dec 2017 11:58:39 -0500 (EST) Received: by mail-pg0-f70.google.com with SMTP id w5so12882963pgt.4 for ; Tue, 19 Dec 2017 08:58:39 -0800 (PST) Received: from bombadil.infradead.org (bombadil.infradead.org. [65.50.211.133]) by mx.google.com with ESMTPS id l5si10313357pgr.469.2017.12.19.08.58.37 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 19 Dec 2017 08:58:38 -0800 (PST) From: Matthew Wilcox Subject: [PATCH 2/2] Introduce __cond_lock_err Date: Tue, 19 Dec 2017 08:58:23 -0800 Message-Id: <20171219165823.24243-2-willy@infradead.org> In-Reply-To: <20171219165823.24243-1-willy@infradead.org> References: <20171219165823.24243-1-willy@infradead.org> Sender: owner-linux-mm@kvack.org List-ID: To: linux-kernel@vger.kernel.org, Ross Zwisler , Dave Hansen , linux-mm@kvack.org, Josh Triplett Cc: Matthew Wilcox From: Matthew Wilcox The __cond_lock macro expects the function to return 'true' if the lock was acquired and 'false' if it wasn't. We have another common calling convention in the kernel, which is returning 0 on success and an errno on failure. It's hard to use the existing __cond_lock macro for those kinds of functions, so introduce __cond_lock_err() and convert the two existing users. Signed-off-by: Matthew Wilcox --- include/linux/compiler_types.h | 2 ++ include/linux/mm.h | 9 ++------- mm/memory.c | 9 ++------- 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 6b79a9bba9a7..ff3c41c78efa 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -16,6 +16,7 @@ # define __acquire(x) __context__(x,1) # define __release(x) __context__(x,-1) # define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) # define __percpu __attribute__((noderef, address_space(3))) # define __rcu __attribute__((noderef, address_space(4))) # define __private __attribute__((noderef)) @@ -42,6 +43,7 @@ extern void __chk_io_ptr(const volatile void __iomem *); # define __acquire(x) (void)0 # define __release(x) (void)0 # define __cond_lock(x,c) (c) +# define __cond_lock_err(x,c) (c) # define __percpu # define __rcu # define __private diff --git a/include/linux/mm.h b/include/linux/mm.h index 94a9d2149bd6..2ccdc980296b 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1328,13 +1328,8 @@ static inline int follow_pte_pmd(struct mm_struct *mm, unsigned long address, unsigned long *start, unsigned long *end, pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp) { - int res; - - /* (void) is needed to make gcc happy */ - (void) __cond_lock(*ptlp, - !(res = __follow_pte_pmd(mm, address, start, end, - ptepp, pmdpp, ptlp))); - return res; + return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, start, end, + ptepp, pmdpp, ptlp)); } static inline void unmap_shared_mapping_range(struct address_space *mapping, diff --git a/mm/memory.c b/mm/memory.c index cb433662af21..92d58309cf45 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -4269,13 +4269,8 @@ int __follow_pte_pmd(struct mm_struct *mm, unsigned long address, static inline int follow_pte(struct mm_struct *mm, unsigned long address, pte_t **ptepp, spinlock_t **ptlp) { - int res; - - /* (void) is needed to make gcc happy */ - (void) __cond_lock(*ptlp, - !(res = __follow_pte_pmd(mm, address, NULL, NULL, - ptepp, NULL, ptlp))); - return res; + return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, NULL, NULL, + ptepp, NULL, ptlp)); } /** -- 2.15.1 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f71.google.com (mail-it0-f71.google.com [209.85.214.71]) by kanga.kvack.org (Postfix) with ESMTP id EE7306B0038 for ; Tue, 19 Dec 2017 12:05:46 -0500 (EST) Received: by mail-it0-f71.google.com with SMTP id a3so2566317itg.7 for ; Tue, 19 Dec 2017 09:05:46 -0800 (PST) Received: from smtprelay.hostedemail.com (smtprelay0102.hostedemail.com. [216.40.44.102]) by mx.google.com with ESMTPS id f32si2951110ioi.249.2017.12.19.09.05.45 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 19 Dec 2017 09:05:46 -0800 (PST) Message-ID: <1513703142.1234.53.camel@perches.com> Subject: Re: [PATCH 1/2] mm: Make follow_pte_pmd an inline From: Joe Perches Date: Tue, 19 Dec 2017 09:05:42 -0800 In-Reply-To: <20171219165823.24243-1-willy@infradead.org> References: <20171219165823.24243-1-willy@infradead.org> Content-Type: text/plain; charset="ISO-8859-1" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Matthew Wilcox , linux-kernel@vger.kernel.org, Ross Zwisler , Dave Hansen , linux-mm@kvack.org, Josh Triplett Cc: Matthew Wilcox On Tue, 2017-12-19 at 08:58 -0800, Matthew Wilcox wrote: > From: Matthew Wilcox > > The one user of follow_pte_pmd (dax) emits a sparse warning because > it doesn't know that follow_pte_pmd conditionally returns with the > pte/pmd locked. The required annotation is already there; it's just > in the wrong file. [] > diff --git a/include/linux/mm.h b/include/linux/mm.h [] > @@ -1324,6 +1324,19 @@ int follow_phys(struct vm_area_struct *vma, unsigned long address, > int generic_access_phys(struct vm_area_struct *vma, unsigned long addr, > void *buf, int len, int write); > > +static inline int follow_pte_pmd(struct mm_struct *mm, unsigned long address, > + unsigned long *start, unsigned long *end, > + pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp) > +{ > + int res; > + > + /* (void) is needed to make gcc happy */ > + (void) __cond_lock(*ptlp, > + !(res = __follow_pte_pmd(mm, address, start, end, > + ptepp, pmdpp, ptlp))); This seems obscure and difficult to read. Perhaps: res = __follow_pte_pmd(mm, address, start, end, ptepp, pmdpp, ptlp); (void)__cond_lock(*ptlp, !res); > + return res; > +} -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f200.google.com (mail-pf0-f200.google.com [209.85.192.200]) by kanga.kvack.org (Postfix) with ESMTP id 246F46B0038 for ; Tue, 19 Dec 2017 12:13:08 -0500 (EST) Received: by mail-pf0-f200.google.com with SMTP id u16so14885042pfh.7 for ; Tue, 19 Dec 2017 09:13:08 -0800 (PST) Received: from bombadil.infradead.org (bombadil.infradead.org. [65.50.211.133]) by mx.google.com with ESMTPS id z9si9486867pll.69.2017.12.19.09.13.07 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 19 Dec 2017 09:13:07 -0800 (PST) Date: Tue, 19 Dec 2017 09:12:54 -0800 From: Matthew Wilcox Subject: Re: [PATCH 1/2] mm: Make follow_pte_pmd an inline Message-ID: <20171219171254.GD30842@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <1513703142.1234.53.camel@perches.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1513703142.1234.53.camel@perches.com> Sender: owner-linux-mm@kvack.org List-ID: To: Joe Perches Cc: linux-kernel@vger.kernel.org, Ross Zwisler , Dave Hansen , linux-mm@kvack.org, Josh Triplett , Matthew Wilcox On Tue, Dec 19, 2017 at 09:05:42AM -0800, Joe Perches wrote: > On Tue, 2017-12-19 at 08:58 -0800, Matthew Wilcox wrote: > > + /* (void) is needed to make gcc happy */ > > + (void) __cond_lock(*ptlp, > > + !(res = __follow_pte_pmd(mm, address, start, end, > > + ptepp, pmdpp, ptlp))); > > This seems obscure and difficult to read. Perhaps: > > res = __follow_pte_pmd(mm, address, start, end, ptepp, pmdpp, ptlp); > (void)__cond_lock(*ptlp, !res); Patch 1 moves the code. Patch 2 cleans it up ;-) -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f71.google.com (mail-pg0-f71.google.com [74.125.83.71]) by kanga.kvack.org (Postfix) with ESMTP id 42DC96B0033 for ; Thu, 21 Dec 2017 16:29:45 -0500 (EST) Received: by mail-pg0-f71.google.com with SMTP id a5so15000808pgu.1 for ; Thu, 21 Dec 2017 13:29:45 -0800 (PST) Received: from mga03.intel.com (mga03.intel.com. [134.134.136.65]) by mx.google.com with ESMTPS id 61si15365346plz.285.2017.12.21.13.29.44 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 21 Dec 2017 13:29:44 -0800 (PST) Date: Thu, 21 Dec 2017 14:29:43 -0700 From: Ross Zwisler Subject: Re: [PATCH 1/2] mm: Make follow_pte_pmd an inline Message-ID: <20171221212943.GB9087@linux.intel.com> References: <20171219165823.24243-1-willy@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171219165823.24243-1-willy@infradead.org> Sender: owner-linux-mm@kvack.org List-ID: To: Matthew Wilcox Cc: linux-kernel@vger.kernel.org, Ross Zwisler , Dave Hansen , linux-mm@kvack.org, Josh Triplett , Matthew Wilcox On Tue, Dec 19, 2017 at 08:58:22AM -0800, Matthew Wilcox wrote: > From: Matthew Wilcox > > The one user of follow_pte_pmd (dax) emits a sparse warning because > it doesn't know that follow_pte_pmd conditionally returns with the > pte/pmd locked. The required annotation is already there; it's just > in the wrong file. Can you help me find the required annotation that is already there but in the wrong file? This does seem to quiet a lockep warning in fs/dax.c, but I think we still have a related one in mm/memory.c: mm/memory.c:4204:5: warning: context imbalance in '__follow_pte_pmd' - different lock contexts for basic block Should we deal with this one as well? -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f197.google.com (mail-pf0-f197.google.com [209.85.192.197]) by kanga.kvack.org (Postfix) with ESMTP id 2A3646B0033 for ; Thu, 21 Dec 2017 16:48:13 -0500 (EST) Received: by mail-pf0-f197.google.com with SMTP id e26so18957186pfi.15 for ; Thu, 21 Dec 2017 13:48:13 -0800 (PST) Received: from mga04.intel.com (mga04.intel.com. [192.55.52.120]) by mx.google.com with ESMTPS id a30si6316093pgn.797.2017.12.21.13.48.11 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 21 Dec 2017 13:48:12 -0800 (PST) Date: Thu, 21 Dec 2017 14:48:10 -0700 From: Ross Zwisler Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171221214810.GC9087@linux.intel.com> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171219165823.24243-2-willy@infradead.org> Sender: owner-linux-mm@kvack.org List-ID: To: Matthew Wilcox Cc: linux-kernel@vger.kernel.org, Ross Zwisler , Dave Hansen , linux-mm@kvack.org, Josh Triplett , Matthew Wilcox On Tue, Dec 19, 2017 at 08:58:23AM -0800, Matthew Wilcox wrote: > From: Matthew Wilcox > > The __cond_lock macro expects the function to return 'true' if the lock > was acquired and 'false' if it wasn't. We have another common calling > convention in the kernel, which is returning 0 on success and an errno > on failure. It's hard to use the existing __cond_lock macro for those > kinds of functions, so introduce __cond_lock_err() and convert the > two existing users. This is much cleaner! One quick issue below. > Signed-off-by: Matthew Wilcox > --- > include/linux/compiler_types.h | 2 ++ > include/linux/mm.h | 9 ++------- > mm/memory.c | 9 ++------- > 3 files changed, 6 insertions(+), 14 deletions(-) > > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > index 6b79a9bba9a7..ff3c41c78efa 100644 > --- a/include/linux/compiler_types.h > +++ b/include/linux/compiler_types.h > @@ -16,6 +16,7 @@ > # define __acquire(x) __context__(x,1) > # define __release(x) __context__(x,-1) > # define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) > +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) ^ I think we actually want this to return c here ^ The old code saved off the actual return value from __follow_pte_pmd() (say, -EINVAL) in 'res', and that was what was returned on error from both follow_pte_pmd() and follow_pte(). The value of 1 returned by __cond_lock() was just discarded (after we cast it to void for some reason). With this new code we actually return the value from __cond_lock_err(), which means that instead of returning -EINVAL, we'll return 1 on error. > # define __percpu __attribute__((noderef, address_space(3))) > # define __rcu __attribute__((noderef, address_space(4))) > # define __private __attribute__((noderef)) > @@ -42,6 +43,7 @@ extern void __chk_io_ptr(const volatile void __iomem *); > # define __acquire(x) (void)0 > # define __release(x) (void)0 > # define __cond_lock(x,c) (c) > +# define __cond_lock_err(x,c) (c) > # define __percpu > # define __rcu > # define __private > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 94a9d2149bd6..2ccdc980296b 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1328,13 +1328,8 @@ static inline int follow_pte_pmd(struct mm_struct *mm, unsigned long address, > unsigned long *start, unsigned long *end, > pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp) > { > - int res; > - > - /* (void) is needed to make gcc happy */ > - (void) __cond_lock(*ptlp, > - !(res = __follow_pte_pmd(mm, address, start, end, > - ptepp, pmdpp, ptlp))); > - return res; > + return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, start, end, > + ptepp, pmdpp, ptlp)); > } > > static inline void unmap_shared_mapping_range(struct address_space *mapping, > diff --git a/mm/memory.c b/mm/memory.c > index cb433662af21..92d58309cf45 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -4269,13 +4269,8 @@ int __follow_pte_pmd(struct mm_struct *mm, unsigned long address, > static inline int follow_pte(struct mm_struct *mm, unsigned long address, > pte_t **ptepp, spinlock_t **ptlp) > { > - int res; > - > - /* (void) is needed to make gcc happy */ > - (void) __cond_lock(*ptlp, > - !(res = __follow_pte_pmd(mm, address, NULL, NULL, > - ptepp, NULL, ptlp))); > - return res; > + return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, NULL, NULL, > + ptepp, NULL, ptlp)); > } > > /** > -- > 2.15.1 > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f197.google.com (mail-wr0-f197.google.com [209.85.128.197]) by kanga.kvack.org (Postfix) with ESMTP id 27E866B0033 for ; Thu, 21 Dec 2017 17:00:25 -0500 (EST) Received: by mail-wr0-f197.google.com with SMTP id l33so15622900wrl.5 for ; Thu, 21 Dec 2017 14:00:25 -0800 (PST) Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net. [2001:4b98:c:538::197]) by mx.google.com with ESMTPS id r65si5104071wmf.10.2017.12.21.14.00.23 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 21 Dec 2017 14:00:23 -0800 (PST) Date: Thu, 21 Dec 2017 14:00:16 -0800 From: Josh Triplett Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171221220015.GA14919@localhost> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171221214810.GC9087@linux.intel.com> Sender: owner-linux-mm@kvack.org List-ID: To: Ross Zwisler Cc: Matthew Wilcox , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox On Thu, Dec 21, 2017 at 02:48:10PM -0700, Ross Zwisler wrote: > On Tue, Dec 19, 2017 at 08:58:23AM -0800, Matthew Wilcox wrote: > > From: Matthew Wilcox > > > > The __cond_lock macro expects the function to return 'true' if the lock > > was acquired and 'false' if it wasn't. We have another common calling > > convention in the kernel, which is returning 0 on success and an errno > > on failure. It's hard to use the existing __cond_lock macro for those > > kinds of functions, so introduce __cond_lock_err() and convert the > > two existing users. > > This is much cleaner! One quick issue below. > > > Signed-off-by: Matthew Wilcox > > --- > > include/linux/compiler_types.h | 2 ++ > > include/linux/mm.h | 9 ++------- > > mm/memory.c | 9 ++------- > > 3 files changed, 6 insertions(+), 14 deletions(-) > > > > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > > index 6b79a9bba9a7..ff3c41c78efa 100644 > > --- a/include/linux/compiler_types.h > > +++ b/include/linux/compiler_types.h > > @@ -16,6 +16,7 @@ > > # define __acquire(x) __context__(x,1) > > # define __release(x) __context__(x,-1) > > # define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) > > +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) > ^ > I think we actually want this to return c here ^ Then you want to use ((c) ?: ...), to avoid evaluating c twice. - Josh Triplett -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f69.google.com (mail-pg0-f69.google.com [74.125.83.69]) by kanga.kvack.org (Postfix) with ESMTP id 7ADD96B0033 for ; Thu, 21 Dec 2017 17:10:53 -0500 (EST) Received: by mail-pg0-f69.google.com with SMTP id z12so16465195pgv.6 for ; Thu, 21 Dec 2017 14:10:53 -0800 (PST) Received: from mga11.intel.com (mga11.intel.com. [192.55.52.93]) by mx.google.com with ESMTPS id t189si14096379pgt.317.2017.12.21.14.10.52 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 21 Dec 2017 14:10:52 -0800 (PST) Date: Thu, 21 Dec 2017 15:10:50 -0700 From: Ross Zwisler Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171221221050.GD9087@linux.intel.com> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171221220015.GA14919@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171221220015.GA14919@localhost> Sender: owner-linux-mm@kvack.org List-ID: To: Josh Triplett Cc: Ross Zwisler , Matthew Wilcox , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox On Thu, Dec 21, 2017 at 02:00:16PM -0800, Josh Triplett wrote: > On Thu, Dec 21, 2017 at 02:48:10PM -0700, Ross Zwisler wrote: > > On Tue, Dec 19, 2017 at 08:58:23AM -0800, Matthew Wilcox wrote: > > > From: Matthew Wilcox > > > > > > The __cond_lock macro expects the function to return 'true' if the lock > > > was acquired and 'false' if it wasn't. We have another common calling > > > convention in the kernel, which is returning 0 on success and an errno > > > on failure. It's hard to use the existing __cond_lock macro for those > > > kinds of functions, so introduce __cond_lock_err() and convert the > > > two existing users. > > > > This is much cleaner! One quick issue below. > > > > > Signed-off-by: Matthew Wilcox > > > --- > > > include/linux/compiler_types.h | 2 ++ > > > include/linux/mm.h | 9 ++------- > > > mm/memory.c | 9 ++------- > > > 3 files changed, 6 insertions(+), 14 deletions(-) > > > > > > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > > > index 6b79a9bba9a7..ff3c41c78efa 100644 > > > --- a/include/linux/compiler_types.h > > > +++ b/include/linux/compiler_types.h > > > @@ -16,6 +16,7 @@ > > > # define __acquire(x) __context__(x,1) > > > # define __release(x) __context__(x,-1) > > > # define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) > > > +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) > > ^ > > I think we actually want this to return c here ^ > > Then you want to use ((c) ?: ...), to avoid evaluating c twice. Oh, yep, great catch. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl0-f70.google.com (mail-pl0-f70.google.com [209.85.160.70]) by kanga.kvack.org (Postfix) with ESMTP id E986C6B0038 for ; Thu, 21 Dec 2017 20:08:04 -0500 (EST) Received: by mail-pl0-f70.google.com with SMTP id w15so12558926plp.14 for ; Thu, 21 Dec 2017 17:08:04 -0800 (PST) Received: from bombadil.infradead.org (bombadil.infradead.org. [65.50.211.133]) by mx.google.com with ESMTPS id 71si9341109pge.254.2017.12.21.17.08.03 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 21 Dec 2017 17:08:03 -0800 (PST) Date: Thu, 21 Dec 2017 17:07:59 -0800 From: Matthew Wilcox Subject: Re: [PATCH 1/2] mm: Make follow_pte_pmd an inline Message-ID: <20171222010759.GA23624@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <20171221212943.GB9087@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171221212943.GB9087@linux.intel.com> Sender: owner-linux-mm@kvack.org List-ID: To: Ross Zwisler Cc: linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Josh Triplett , Matthew Wilcox On Thu, Dec 21, 2017 at 02:29:43PM -0700, Ross Zwisler wrote: > On Tue, Dec 19, 2017 at 08:58:22AM -0800, Matthew Wilcox wrote: > > From: Matthew Wilcox > > > > The one user of follow_pte_pmd (dax) emits a sparse warning because > > it doesn't know that follow_pte_pmd conditionally returns with the > > pte/pmd locked. The required annotation is already there; it's just > > in the wrong file. > > Can you help me find the required annotation that is already there but in the > wrong file? You cut it out ... that was the entire contents of the patch! The cond_lock annotation is correct, but sparse doesn't look across compilation units, so it can't see the one that's in mm/memory.c when it's compiling fs/dax.c. That's why it needs to be in a header file. > This does seem to quiet a lockep warning in fs/dax.c, but I think we still > have a related one in mm/memory.c: > > mm/memory.c:4204:5: warning: context imbalance in '__follow_pte_pmd' - different lock contexts for basic block > > Should we deal with this one as well? I'm not sure how to deal with that one, to be honest. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl0-f71.google.com (mail-pl0-f71.google.com [209.85.160.71]) by kanga.kvack.org (Postfix) with ESMTP id 7D4076B0038 for ; Thu, 21 Dec 2017 20:10:02 -0500 (EST) Received: by mail-pl0-f71.google.com with SMTP id q12so12486980pli.12 for ; Thu, 21 Dec 2017 17:10:02 -0800 (PST) Received: from bombadil.infradead.org (bombadil.infradead.org. [65.50.211.133]) by mx.google.com with ESMTPS id m7si14212971pgp.669.2017.12.21.17.10.01 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 21 Dec 2017 17:10:01 -0800 (PST) Date: Thu, 21 Dec 2017 17:10:00 -0800 From: Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171222011000.GB23624@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171221214810.GC9087@linux.intel.com> Sender: owner-linux-mm@kvack.org List-ID: To: Ross Zwisler Cc: linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Josh Triplett , Matthew Wilcox On Thu, Dec 21, 2017 at 02:48:10PM -0700, Ross Zwisler wrote: > > +++ b/include/linux/compiler_types.h > > @@ -16,6 +16,7 @@ > > # define __acquire(x) __context__(x,1) > > # define __release(x) __context__(x,-1) > > # define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) > > +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) > ^ > I think we actually want this to return c here ^ > > The old code saved off the actual return value from __follow_pte_pmd() (say, > -EINVAL) in 'res', and that was what was returned on error from both > follow_pte_pmd() and follow_pte(). The value of 1 returned by __cond_lock() > was just discarded (after we cast it to void for some reason). > > With this new code we actually return the value from __cond_lock_err(), which > means that instead of returning -EINVAL, we'll return 1 on error. Yes, but this define is only #if __CHECKER__, so it doesn't matter what we return as this code will never run. That said, if sparse supports the GNU syntax of ?: then I have no objection to doing that. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f72.google.com (mail-wm0-f72.google.com [74.125.82.72]) by kanga.kvack.org (Postfix) with ESMTP id 895AE6B0038 for ; Thu, 21 Dec 2017 23:21:29 -0500 (EST) Received: by mail-wm0-f72.google.com with SMTP id n13so4858609wmc.3 for ; Thu, 21 Dec 2017 20:21:29 -0800 (PST) Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net. [217.70.183.196]) by mx.google.com with ESMTPS id n61si8384349wrb.189.2017.12.21.20.21.27 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 21 Dec 2017 20:21:28 -0800 (PST) Date: Thu, 21 Dec 2017 20:21:20 -0800 From: Josh Triplett Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171222042120.GA18036@localhost> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171222011000.GB23624@bombadil.infradead.org> Sender: owner-linux-mm@kvack.org List-ID: To: Matthew Wilcox Cc: Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote: > On Thu, Dec 21, 2017 at 02:48:10PM -0700, Ross Zwisler wrote: > > > +++ b/include/linux/compiler_types.h > > > @@ -16,6 +16,7 @@ > > > # define __acquire(x) __context__(x,1) > > > # define __release(x) __context__(x,-1) > > > # define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) > > > +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) > > ^ > > I think we actually want this to return c here ^ > > > > The old code saved off the actual return value from __follow_pte_pmd() (say, > > -EINVAL) in 'res', and that was what was returned on error from both > > follow_pte_pmd() and follow_pte(). The value of 1 returned by __cond_lock() > > was just discarded (after we cast it to void for some reason). > > > > With this new code we actually return the value from __cond_lock_err(), which > > means that instead of returning -EINVAL, we'll return 1 on error. > > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we > return as this code will never run. It does matter slightly, as Sparse does some (very limited) value-based analyses. Let's future-proof it. > That said, if sparse supports the GNU syntax of ?: then I have no > objection to doing that. Sparse does support that syntax. - Josh Triplett -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f200.google.com (mail-pf0-f200.google.com [209.85.192.200]) by kanga.kvack.org (Postfix) with ESMTP id CE05D6B0253 for ; Fri, 22 Dec 2017 07:31:27 -0500 (EST) Received: by mail-pf0-f200.google.com with SMTP id f64so20163582pfd.6 for ; Fri, 22 Dec 2017 04:31:27 -0800 (PST) Received: from bombadil.infradead.org (bombadil.infradead.org. [65.50.211.133]) by mx.google.com with ESMTPS id v3si15202762pgq.731.2017.12.22.04.31.26 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 22 Dec 2017 04:31:26 -0800 (PST) Date: Fri, 22 Dec 2017 04:31:12 -0800 From: Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171222123112.GA6401@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171222042120.GA18036@localhost> Sender: owner-linux-mm@kvack.org List-ID: To: Josh Triplett Cc: Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote: > On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote: > > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we > > return as this code will never run. > > It does matter slightly, as Sparse does some (very limited) value-based > analyses. Let's future-proof it. > > > That said, if sparse supports the GNU syntax of ?: then I have no > > objection to doing that. > > Sparse does support that syntax. Great, I'll fix that and resubmit. While I've got you, I've been looking at some other sparse warnings from this file. There are several caused by sparse being unable to handle the following construct: if (foo) x = NULL; else { x = bar; __acquire(bar); } if (!x) return -ENOMEM; Writing it as: if (foo) return -ENOMEM; else { x = bar; __acquire(bar); } works just fine. ie this removes the warning: @@ -1070,9 +1070,9 @@ static int copy_pte_range(struct mm_struct *dst_mm, struct mm_struct *src_mm, again: init_rss_vec(rss); - dst_pte = pte_alloc_map_lock(dst_mm, dst_pmd, addr, &dst_ptl); - if (!dst_pte) + if (pte_alloc(dst_mm, dst_pmd, addr)) return -ENOMEM; + dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, addr, &dst_ptl); src_pte = pte_offset_map(src_pmd, addr); src_ptl = pte_lockptr(src_mm, src_pmd); spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING); Is there any chance sparse's dataflow analysis will be improved in the near future? -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f198.google.com (mail-pf0-f198.google.com [209.85.192.198]) by kanga.kvack.org (Postfix) with ESMTP id A83046B0038 for ; Fri, 22 Dec 2017 08:36:41 -0500 (EST) Received: by mail-pf0-f198.google.com with SMTP id y62so20262861pfd.3 for ; Fri, 22 Dec 2017 05:36:41 -0800 (PST) Received: from bombadil.infradead.org (bombadil.infradead.org. [65.50.211.133]) by mx.google.com with ESMTPS id x4si16296541plw.539.2017.12.22.05.36.39 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 22 Dec 2017 05:36:39 -0800 (PST) Date: Fri, 22 Dec 2017 05:36:34 -0800 From: Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171222133634.GE6401@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> <20171222123112.GA6401@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171222123112.GA6401@bombadil.infradead.org> Sender: owner-linux-mm@kvack.org List-ID: To: Josh Triplett Cc: Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote: > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote: > > On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote: > > > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we > > > return as this code will never run. > > > > It does matter slightly, as Sparse does some (very limited) value-based > > analyses. Let's future-proof it. > > > > > That said, if sparse supports the GNU syntax of ?: then I have no > > > objection to doing that. > > > > Sparse does support that syntax. > > Great, I'll fix that and resubmit. Except the context imbalance warning comes back if I do. This is sparse 0.5.1 (Debian's 0.5.1-2 package). -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f200.google.com (mail-wr0-f200.google.com [209.85.128.200]) by kanga.kvack.org (Postfix) with ESMTP id EAB466B0033 for ; Wed, 27 Dec 2017 09:28:59 -0500 (EST) Received: by mail-wr0-f200.google.com with SMTP id s5so19014442wra.3 for ; Wed, 27 Dec 2017 06:28:59 -0800 (PST) Received: from mail-sor-f65.google.com (mail-sor-f65.google.com. [209.85.220.65]) by mx.google.com with SMTPS id t7sor17546387edc.45.2017.12.27.06.28.57 for (Google Transport Security); Wed, 27 Dec 2017 06:28:58 -0800 (PST) Date: Wed, 27 Dec 2017 15:28:54 +0100 From: Luc Van Oostenryck Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171227142853.b5agfi2kzo25g5ot@ltop.local> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> <20171222123112.GA6401@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171222123112.GA6401@bombadil.infradead.org> Sender: owner-linux-mm@kvack.org List-ID: To: Matthew Wilcox Cc: Josh Triplett , Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote: > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote: > > While I've got you, I've been looking at some other sparse warnings from > this file. There are several caused by sparse being unable to handle > the following construct: > > if (foo) > x = NULL; > else { > x = bar; > __acquire(bar); > } > if (!x) > return -ENOMEM; > > Writing it as: > > if (foo) > return -ENOMEM; > else { > x = bar; > __acquire(bar); > } > > works just fine. ie this removes the warning: It must be noted that these two versions are not equivalent (in the first version, it also returns with -ENOMEM if bar is NULL/zero). It must be noted that sparse's goal regarding the context imbalance is to give the warning if some point in the code can be reached via two paths (or more) and the lock state (the context) is not identical in each of these paths. > > Is there any chance sparse's dataflow analysis will be improved in the > near future? A lot of functions in the kernel have this context imbalance, really a lot. For example, any function doing conditional locking is a problem here. Happily when these functions are inlined, sparse, thanks to its optimizations, can remove some paths and merge some others. So yes, by adding some smartness to sparse, some of the false warnings will be removed, however: 1) some __must_hold()/__acquires()/__releases() annotations are missing, making sparse's job impossible. 2) a lot of the 'false warnings' are not so false because there is indeed two possible paths with different lock state 3) it has its limits (at the end, giving the correct warning is equivalent to the halting problem). Now, to answer to your question, I'm not aware of any effort that would make a significant differences (it would need, IMO, code hoisting & value range propagation). -- Luc Van Oostenryck -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f71.google.com (mail-pg0-f71.google.com [74.125.83.71]) by kanga.kvack.org (Postfix) with ESMTP id 7758C6B0033 for ; Sat, 30 Dec 2017 02:17:29 -0500 (EST) Received: by mail-pg0-f71.google.com with SMTP id r8so7522705pgp.7 for ; Fri, 29 Dec 2017 23:17:29 -0800 (PST) Received: from bombadil.infradead.org (bombadil.infradead.org. [65.50.211.133]) by mx.google.com with ESMTPS id a20si29558503pfg.38.2017.12.29.23.17.28 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 29 Dec 2017 23:17:28 -0800 (PST) Date: Fri, 29 Dec 2017 23:17:20 -0800 From: Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171230071720.GE27959@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> <20171222123112.GA6401@bombadil.infradead.org> <20171227142853.b5agfi2kzo25g5ot@ltop.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171227142853.b5agfi2kzo25g5ot@ltop.local> Sender: owner-linux-mm@kvack.org List-ID: To: Luc Van Oostenryck Cc: Josh Triplett , Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox On Wed, Dec 27, 2017 at 03:28:54PM +0100, Luc Van Oostenryck wrote: > On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote: > > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote: > > > > While I've got you, I've been looking at some other sparse warnings from > > this file. There are several caused by sparse being unable to handle > > the following construct: > > > > if (foo) > > x = NULL; > > else { > > x = bar; > > __acquire(bar); > > } > > if (!x) > > return -ENOMEM; > > > > Writing it as: > > > > if (foo) > > return -ENOMEM; > > else { > > x = bar; > > __acquire(bar); > > } > > > > works just fine. ie this removes the warning: > > It must be noted that these two versions are not equivalent > (in the first version, it also returns with -ENOMEM if bar > is NULL/zero). They happen to be equivalent in the original; I was providing a simplified version. Here's the construct sparse can't understand: dst_pte = pte_alloc_map_lock(dst_mm, dst_pmd, addr, &dst_ptl); if (!dst_pte) return -ENOMEM; with: #define pte_alloc(mm, pmd, address) \ (unlikely(pmd_none(*(pmd))) && __pte_alloc(mm, pmd, address)) #define pte_offset_map_lock(mm, pmd, address, ptlp) \ ({ \ spinlock_t *__ptl = pte_lockptr(mm, pmd); \ pte_t *__pte = pte_offset_map(pmd, address); \ *(ptlp) = __ptl; \ spin_lock(__ptl); \ __pte; \ }) #define pte_alloc_map_lock(mm, pmd, address, ptlp) \ (pte_alloc(mm, pmd, address) ? \ NULL : pte_offset_map_lock(mm, pmd, address, ptlp)) If pte_alloc() succeeds, pte_offset_map_lock() will return non-NULL. Manually inlining pte_alloc_map_lock() into the caller like so: if (pte_alloc(dst_mm, dst_pmd, addr) return -ENOMEM; dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, addr, ptlp); causes sparse to not warn. > > Is there any chance sparse's dataflow analysis will be improved in the > > near future? > > A lot of functions in the kernel have this context imbalance, > really a lot. For example, any function doing conditional locking > is a problem here. Happily when these functions are inlined, > sparse, thanks to its optimizations, can remove some paths and > merge some others. > So yes, by adding some smartness to sparse, some of the false > warnings will be removed, however: > 1) some __must_hold()/__acquires()/__releases() annotations are > missing, making sparse's job impossible. Partly there's a documentation problem here. I'd really like to see a document explaining how to add sparse annotations to a function which intentionally does conditional locking. For example, should we be annotating the function as __acquires, and then marking the exits which don't acquire the lock with __acquire(), or should we not annotate the function, and annotate the exits which _do_ acquire the lock as __release() with a comment like /* Caller will release */ > 2) a lot of the 'false warnings' are not so false because there is > indeed two possible paths with different lock state > 3) it has its limits (at the end, giving the correct warning is > equivalent to the halting problem). > > Now, to answer to your question, I'm not aware of any effort that would > make a significant differences (it would need, IMO, code hoisting & > value range propagation). That's fair. I wonder if we were starting from scratch whether we'd choose to make sparse a GCC plugin today. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752738AbdLSQ6i (ORCPT ); Tue, 19 Dec 2017 11:58:38 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:49518 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751128AbdLSQ6h (ORCPT ); Tue, 19 Dec 2017 11:58:37 -0500 From: Matthew Wilcox To: linux-kernel@vger.kernel.org, Ross Zwisler , Dave Hansen , linux-mm@kvack.org, Josh Triplett Cc: Matthew Wilcox Subject: [PATCH 2/2] Introduce __cond_lock_err Date: Tue, 19 Dec 2017 08:58:23 -0800 Message-Id: <20171219165823.24243-2-willy@infradead.org> X-Mailer: git-send-email 2.14.3 In-Reply-To: <20171219165823.24243-1-willy@infradead.org> References: <20171219165823.24243-1-willy@infradead.org> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Matthew Wilcox The __cond_lock macro expects the function to return 'true' if the lock was acquired and 'false' if it wasn't. We have another common calling convention in the kernel, which is returning 0 on success and an errno on failure. It's hard to use the existing __cond_lock macro for those kinds of functions, so introduce __cond_lock_err() and convert the two existing users. Signed-off-by: Matthew Wilcox --- include/linux/compiler_types.h | 2 ++ include/linux/mm.h | 9 ++------- mm/memory.c | 9 ++------- 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 6b79a9bba9a7..ff3c41c78efa 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -16,6 +16,7 @@ # define __acquire(x) __context__(x,1) # define __release(x) __context__(x,-1) # define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) # define __percpu __attribute__((noderef, address_space(3))) # define __rcu __attribute__((noderef, address_space(4))) # define __private __attribute__((noderef)) @@ -42,6 +43,7 @@ extern void __chk_io_ptr(const volatile void __iomem *); # define __acquire(x) (void)0 # define __release(x) (void)0 # define __cond_lock(x,c) (c) +# define __cond_lock_err(x,c) (c) # define __percpu # define __rcu # define __private diff --git a/include/linux/mm.h b/include/linux/mm.h index 94a9d2149bd6..2ccdc980296b 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1328,13 +1328,8 @@ static inline int follow_pte_pmd(struct mm_struct *mm, unsigned long address, unsigned long *start, unsigned long *end, pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp) { - int res; - - /* (void) is needed to make gcc happy */ - (void) __cond_lock(*ptlp, - !(res = __follow_pte_pmd(mm, address, start, end, - ptepp, pmdpp, ptlp))); - return res; + return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, start, end, + ptepp, pmdpp, ptlp)); } static inline void unmap_shared_mapping_range(struct address_space *mapping, diff --git a/mm/memory.c b/mm/memory.c index cb433662af21..92d58309cf45 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -4269,13 +4269,8 @@ int __follow_pte_pmd(struct mm_struct *mm, unsigned long address, static inline int follow_pte(struct mm_struct *mm, unsigned long address, pte_t **ptepp, spinlock_t **ptlp) { - int res; - - /* (void) is needed to make gcc happy */ - (void) __cond_lock(*ptlp, - !(res = __follow_pte_pmd(mm, address, NULL, NULL, - ptepp, NULL, ptlp))); - return res; + return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, NULL, NULL, + ptepp, NULL, ptlp)); } /** -- 2.15.1 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752808AbdLSQ6n (ORCPT ); Tue, 19 Dec 2017 11:58:43 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:57993 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751188AbdLSQ6h (ORCPT ); Tue, 19 Dec 2017 11:58:37 -0500 From: Matthew Wilcox To: linux-kernel@vger.kernel.org, Ross Zwisler , Dave Hansen , linux-mm@kvack.org, Josh Triplett Cc: Matthew Wilcox Subject: [PATCH 1/2] mm: Make follow_pte_pmd an inline Date: Tue, 19 Dec 2017 08:58:22 -0800 Message-Id: <20171219165823.24243-1-willy@infradead.org> X-Mailer: git-send-email 2.14.3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Matthew Wilcox The one user of follow_pte_pmd (dax) emits a sparse warning because it doesn't know that follow_pte_pmd conditionally returns with the pte/pmd locked. The required annotation is already there; it's just in the wrong file. --- include/linux/mm.h | 15 ++++++++++++++- mm/memory.c | 16 +--------------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/include/linux/mm.h b/include/linux/mm.h index ea818ff739cd..94a9d2149bd6 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1314,7 +1314,7 @@ int copy_page_range(struct mm_struct *dst, struct mm_struct *src, struct vm_area_struct *vma); void unmap_mapping_range(struct address_space *mapping, loff_t const holebegin, loff_t const holelen, int even_cows); -int follow_pte_pmd(struct mm_struct *mm, unsigned long address, +int __follow_pte_pmd(struct mm_struct *mm, unsigned long address, unsigned long *start, unsigned long *end, pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp); int follow_pfn(struct vm_area_struct *vma, unsigned long address, @@ -1324,6 +1324,19 @@ int follow_phys(struct vm_area_struct *vma, unsigned long address, int generic_access_phys(struct vm_area_struct *vma, unsigned long addr, void *buf, int len, int write); +static inline int follow_pte_pmd(struct mm_struct *mm, unsigned long address, + unsigned long *start, unsigned long *end, + pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp) +{ + int res; + + /* (void) is needed to make gcc happy */ + (void) __cond_lock(*ptlp, + !(res = __follow_pte_pmd(mm, address, start, end, + ptepp, pmdpp, ptlp))); + return res; +} + static inline void unmap_shared_mapping_range(struct address_space *mapping, loff_t const holebegin, loff_t const holelen) { diff --git a/mm/memory.c b/mm/memory.c index cfaba6287702..cb433662af21 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -4201,7 +4201,7 @@ int __pmd_alloc(struct mm_struct *mm, pud_t *pud, unsigned long address) } #endif /* __PAGETABLE_PMD_FOLDED */ -static int __follow_pte_pmd(struct mm_struct *mm, unsigned long address, +int __follow_pte_pmd(struct mm_struct *mm, unsigned long address, unsigned long *start, unsigned long *end, pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp) { @@ -4278,20 +4278,6 @@ static inline int follow_pte(struct mm_struct *mm, unsigned long address, return res; } -int follow_pte_pmd(struct mm_struct *mm, unsigned long address, - unsigned long *start, unsigned long *end, - pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp) -{ - int res; - - /* (void) is needed to make gcc happy */ - (void) __cond_lock(*ptlp, - !(res = __follow_pte_pmd(mm, address, start, end, - ptepp, pmdpp, ptlp))); - return res; -} -EXPORT_SYMBOL(follow_pte_pmd); - /** * follow_pfn - look up PFN at a user virtual address * @vma: memory mapping -- 2.15.1 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753481AbdLSRFz (ORCPT ); Tue, 19 Dec 2017 12:05:55 -0500 Received: from smtprelay0057.hostedemail.com ([216.40.44.57]:44235 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751949AbdLSRFq (ORCPT ); Tue, 19 Dec 2017 12:05:46 -0500 X-Session-Marker: 6A6F6540706572636865732E636F6D X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,joe@perches.com,:::::::::::::,RULES_HIT:41:355:379:541:599:800:960:973:988:989:1260:1277:1311:1313:1314:1345:1359:1373:1437:1515:1516:1518:1534:1541:1593:1594:1711:1730:1747:1777:1792:2194:2199:2393:2559:2562:2828:3138:3139:3140:3141:3142:3352:3622:3865:3866:3867:3868:3870:3871:3872:3874:4321:5007:6119:7576:7688:7903:10004:10400:10848:11026:11232:11658:11914:12048:12296:12740:12760:12895:13069:13141:13230:13311:13357:13439:14096:14097:14181:14659:14721:21080:21451:21627:30003:30051:30054:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:1,LUA_SUMMARY:none X-HE-Tag: unit51_14fc627cac80f X-Filterd-Recvd-Size: 2097 Message-ID: <1513703142.1234.53.camel@perches.com> Subject: Re: [PATCH 1/2] mm: Make follow_pte_pmd an inline From: Joe Perches To: Matthew Wilcox , linux-kernel@vger.kernel.org, Ross Zwisler , Dave Hansen , linux-mm@kvack.org, Josh Triplett Cc: Matthew Wilcox Date: Tue, 19 Dec 2017 09:05:42 -0800 In-Reply-To: <20171219165823.24243-1-willy@infradead.org> References: <20171219165823.24243-1-willy@infradead.org> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.26.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2017-12-19 at 08:58 -0800, Matthew Wilcox wrote: > From: Matthew Wilcox > > The one user of follow_pte_pmd (dax) emits a sparse warning because > it doesn't know that follow_pte_pmd conditionally returns with the > pte/pmd locked. The required annotation is already there; it's just > in the wrong file. [] > diff --git a/include/linux/mm.h b/include/linux/mm.h [] > @@ -1324,6 +1324,19 @@ int follow_phys(struct vm_area_struct *vma, unsigned long address, > int generic_access_phys(struct vm_area_struct *vma, unsigned long addr, > void *buf, int len, int write); > > +static inline int follow_pte_pmd(struct mm_struct *mm, unsigned long address, > + unsigned long *start, unsigned long *end, > + pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp) > +{ > + int res; > + > + /* (void) is needed to make gcc happy */ > + (void) __cond_lock(*ptlp, > + !(res = __follow_pte_pmd(mm, address, start, end, > + ptepp, pmdpp, ptlp))); This seems obscure and difficult to read. Perhaps: res = __follow_pte_pmd(mm, address, start, end, ptepp, pmdpp, ptlp); (void)__cond_lock(*ptlp, !res); > + return res; > +} From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753168AbdLSRNJ (ORCPT ); Tue, 19 Dec 2017 12:13:09 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:44984 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753114AbdLSRND (ORCPT ); Tue, 19 Dec 2017 12:13:03 -0500 Date: Tue, 19 Dec 2017 09:12:54 -0800 From: Matthew Wilcox To: Joe Perches Cc: linux-kernel@vger.kernel.org, Ross Zwisler , Dave Hansen , linux-mm@kvack.org, Josh Triplett , Matthew Wilcox Subject: Re: [PATCH 1/2] mm: Make follow_pte_pmd an inline Message-ID: <20171219171254.GD30842@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <1513703142.1234.53.camel@perches.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1513703142.1234.53.camel@perches.com> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Dec 19, 2017 at 09:05:42AM -0800, Joe Perches wrote: > On Tue, 2017-12-19 at 08:58 -0800, Matthew Wilcox wrote: > > + /* (void) is needed to make gcc happy */ > > + (void) __cond_lock(*ptlp, > > + !(res = __follow_pte_pmd(mm, address, start, end, > > + ptepp, pmdpp, ptlp))); > > This seems obscure and difficult to read. Perhaps: > > res = __follow_pte_pmd(mm, address, start, end, ptepp, pmdpp, ptlp); > (void)__cond_lock(*ptlp, !res); Patch 1 moves the code. Patch 2 cleans it up ;-) From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755767AbdLUV3r (ORCPT ); Thu, 21 Dec 2017 16:29:47 -0500 Received: from mga05.intel.com ([192.55.52.43]:55169 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755738AbdLUV3n (ORCPT ); Thu, 21 Dec 2017 16:29:43 -0500 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,437,1508828400"; d="scan'208";a="3860843" Date: Thu, 21 Dec 2017 14:29:43 -0700 From: Ross Zwisler To: Matthew Wilcox Cc: linux-kernel@vger.kernel.org, Ross Zwisler , Dave Hansen , linux-mm@kvack.org, Josh Triplett , Matthew Wilcox Subject: Re: [PATCH 1/2] mm: Make follow_pte_pmd an inline Message-ID: <20171221212943.GB9087@linux.intel.com> References: <20171219165823.24243-1-willy@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171219165823.24243-1-willy@infradead.org> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Dec 19, 2017 at 08:58:22AM -0800, Matthew Wilcox wrote: > From: Matthew Wilcox > > The one user of follow_pte_pmd (dax) emits a sparse warning because > it doesn't know that follow_pte_pmd conditionally returns with the > pte/pmd locked. The required annotation is already there; it's just > in the wrong file. Can you help me find the required annotation that is already there but in the wrong file? This does seem to quiet a lockep warning in fs/dax.c, but I think we still have a related one in mm/memory.c: mm/memory.c:4204:5: warning: context imbalance in '__follow_pte_pmd' - different lock contexts for basic block Should we deal with this one as well? From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754431AbdLUVsN (ORCPT ); Thu, 21 Dec 2017 16:48:13 -0500 Received: from mga14.intel.com ([192.55.52.115]:30102 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752678AbdLUVsL (ORCPT ); Thu, 21 Dec 2017 16:48:11 -0500 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,437,1508828400"; d="scan'208";a="14285072" Date: Thu, 21 Dec 2017 14:48:10 -0700 From: Ross Zwisler To: Matthew Wilcox Cc: linux-kernel@vger.kernel.org, Ross Zwisler , Dave Hansen , linux-mm@kvack.org, Josh Triplett , Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171221214810.GC9087@linux.intel.com> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171219165823.24243-2-willy@infradead.org> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Dec 19, 2017 at 08:58:23AM -0800, Matthew Wilcox wrote: > From: Matthew Wilcox > > The __cond_lock macro expects the function to return 'true' if the lock > was acquired and 'false' if it wasn't. We have another common calling > convention in the kernel, which is returning 0 on success and an errno > on failure. It's hard to use the existing __cond_lock macro for those > kinds of functions, so introduce __cond_lock_err() and convert the > two existing users. This is much cleaner! One quick issue below. > Signed-off-by: Matthew Wilcox > --- > include/linux/compiler_types.h | 2 ++ > include/linux/mm.h | 9 ++------- > mm/memory.c | 9 ++------- > 3 files changed, 6 insertions(+), 14 deletions(-) > > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > index 6b79a9bba9a7..ff3c41c78efa 100644 > --- a/include/linux/compiler_types.h > +++ b/include/linux/compiler_types.h > @@ -16,6 +16,7 @@ > # define __acquire(x) __context__(x,1) > # define __release(x) __context__(x,-1) > # define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) > +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) ^ I think we actually want this to return c here ^ The old code saved off the actual return value from __follow_pte_pmd() (say, -EINVAL) in 'res', and that was what was returned on error from both follow_pte_pmd() and follow_pte(). The value of 1 returned by __cond_lock() was just discarded (after we cast it to void for some reason). With this new code we actually return the value from __cond_lock_err(), which means that instead of returning -EINVAL, we'll return 1 on error. > # define __percpu __attribute__((noderef, address_space(3))) > # define __rcu __attribute__((noderef, address_space(4))) > # define __private __attribute__((noderef)) > @@ -42,6 +43,7 @@ extern void __chk_io_ptr(const volatile void __iomem *); > # define __acquire(x) (void)0 > # define __release(x) (void)0 > # define __cond_lock(x,c) (c) > +# define __cond_lock_err(x,c) (c) > # define __percpu > # define __rcu > # define __private > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 94a9d2149bd6..2ccdc980296b 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1328,13 +1328,8 @@ static inline int follow_pte_pmd(struct mm_struct *mm, unsigned long address, > unsigned long *start, unsigned long *end, > pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp) > { > - int res; > - > - /* (void) is needed to make gcc happy */ > - (void) __cond_lock(*ptlp, > - !(res = __follow_pte_pmd(mm, address, start, end, > - ptepp, pmdpp, ptlp))); > - return res; > + return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, start, end, > + ptepp, pmdpp, ptlp)); > } > > static inline void unmap_shared_mapping_range(struct address_space *mapping, > diff --git a/mm/memory.c b/mm/memory.c > index cb433662af21..92d58309cf45 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -4269,13 +4269,8 @@ int __follow_pte_pmd(struct mm_struct *mm, unsigned long address, > static inline int follow_pte(struct mm_struct *mm, unsigned long address, > pte_t **ptepp, spinlock_t **ptlp) > { > - int res; > - > - /* (void) is needed to make gcc happy */ > - (void) __cond_lock(*ptlp, > - !(res = __follow_pte_pmd(mm, address, NULL, NULL, > - ptepp, NULL, ptlp))); > - return res; > + return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, NULL, NULL, > + ptepp, NULL, ptlp)); > } > > /** > -- > 2.15.1 > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753483AbdLUWA0 (ORCPT ); Thu, 21 Dec 2017 17:00:26 -0500 Received: from relay5-d.mail.gandi.net ([217.70.183.197]:46580 "EHLO relay5-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750933AbdLUWAZ (ORCPT ); Thu, 21 Dec 2017 17:00:25 -0500 X-Originating-IP: 50.39.166.153 Date: Thu, 21 Dec 2017 14:00:16 -0800 From: Josh Triplett To: Ross Zwisler Cc: Matthew Wilcox , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171221220015.GA14919@localhost> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171221214810.GC9087@linux.intel.com> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 21, 2017 at 02:48:10PM -0700, Ross Zwisler wrote: > On Tue, Dec 19, 2017 at 08:58:23AM -0800, Matthew Wilcox wrote: > > From: Matthew Wilcox > > > > The __cond_lock macro expects the function to return 'true' if the lock > > was acquired and 'false' if it wasn't. We have another common calling > > convention in the kernel, which is returning 0 on success and an errno > > on failure. It's hard to use the existing __cond_lock macro for those > > kinds of functions, so introduce __cond_lock_err() and convert the > > two existing users. > > This is much cleaner! One quick issue below. > > > Signed-off-by: Matthew Wilcox > > --- > > include/linux/compiler_types.h | 2 ++ > > include/linux/mm.h | 9 ++------- > > mm/memory.c | 9 ++------- > > 3 files changed, 6 insertions(+), 14 deletions(-) > > > > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > > index 6b79a9bba9a7..ff3c41c78efa 100644 > > --- a/include/linux/compiler_types.h > > +++ b/include/linux/compiler_types.h > > @@ -16,6 +16,7 @@ > > # define __acquire(x) __context__(x,1) > > # define __release(x) __context__(x,-1) > > # define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) > > +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) > ^ > I think we actually want this to return c here ^ Then you want to use ((c) ?: ...), to avoid evaluating c twice. - Josh Triplett From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753674AbdLUWK4 (ORCPT ); Thu, 21 Dec 2017 17:10:56 -0500 Received: from mga04.intel.com ([192.55.52.120]:65061 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750881AbdLUWKw (ORCPT ); Thu, 21 Dec 2017 17:10:52 -0500 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,437,1508828400"; d="scan'208";a="18013940" Date: Thu, 21 Dec 2017 15:10:50 -0700 From: Ross Zwisler To: Josh Triplett Cc: Ross Zwisler , Matthew Wilcox , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171221221050.GD9087@linux.intel.com> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171221220015.GA14919@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171221220015.GA14919@localhost> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 21, 2017 at 02:00:16PM -0800, Josh Triplett wrote: > On Thu, Dec 21, 2017 at 02:48:10PM -0700, Ross Zwisler wrote: > > On Tue, Dec 19, 2017 at 08:58:23AM -0800, Matthew Wilcox wrote: > > > From: Matthew Wilcox > > > > > > The __cond_lock macro expects the function to return 'true' if the lock > > > was acquired and 'false' if it wasn't. We have another common calling > > > convention in the kernel, which is returning 0 on success and an errno > > > on failure. It's hard to use the existing __cond_lock macro for those > > > kinds of functions, so introduce __cond_lock_err() and convert the > > > two existing users. > > > > This is much cleaner! One quick issue below. > > > > > Signed-off-by: Matthew Wilcox > > > --- > > > include/linux/compiler_types.h | 2 ++ > > > include/linux/mm.h | 9 ++------- > > > mm/memory.c | 9 ++------- > > > 3 files changed, 6 insertions(+), 14 deletions(-) > > > > > > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > > > index 6b79a9bba9a7..ff3c41c78efa 100644 > > > --- a/include/linux/compiler_types.h > > > +++ b/include/linux/compiler_types.h > > > @@ -16,6 +16,7 @@ > > > # define __acquire(x) __context__(x,1) > > > # define __release(x) __context__(x,-1) > > > # define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) > > > +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) > > ^ > > I think we actually want this to return c here ^ > > Then you want to use ((c) ?: ...), to avoid evaluating c twice. Oh, yep, great catch. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755772AbdLVBIF (ORCPT ); Thu, 21 Dec 2017 20:08:05 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:35681 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755562AbdLVBIC (ORCPT ); Thu, 21 Dec 2017 20:08:02 -0500 Date: Thu, 21 Dec 2017 17:07:59 -0800 From: Matthew Wilcox To: Ross Zwisler Cc: linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Josh Triplett , Matthew Wilcox Subject: Re: [PATCH 1/2] mm: Make follow_pte_pmd an inline Message-ID: <20171222010759.GA23624@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <20171221212943.GB9087@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171221212943.GB9087@linux.intel.com> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 21, 2017 at 02:29:43PM -0700, Ross Zwisler wrote: > On Tue, Dec 19, 2017 at 08:58:22AM -0800, Matthew Wilcox wrote: > > From: Matthew Wilcox > > > > The one user of follow_pte_pmd (dax) emits a sparse warning because > > it doesn't know that follow_pte_pmd conditionally returns with the > > pte/pmd locked. The required annotation is already there; it's just > > in the wrong file. > > Can you help me find the required annotation that is already there but in the > wrong file? You cut it out ... that was the entire contents of the patch! The cond_lock annotation is correct, but sparse doesn't look across compilation units, so it can't see the one that's in mm/memory.c when it's compiling fs/dax.c. That's why it needs to be in a header file. > This does seem to quiet a lockep warning in fs/dax.c, but I think we still > have a related one in mm/memory.c: > > mm/memory.c:4204:5: warning: context imbalance in '__follow_pte_pmd' - different lock contexts for basic block > > Should we deal with this one as well? I'm not sure how to deal with that one, to be honest. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755507AbdLVBKC (ORCPT ); Thu, 21 Dec 2017 20:10:02 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:48728 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753797AbdLVBKB (ORCPT ); Thu, 21 Dec 2017 20:10:01 -0500 Date: Thu, 21 Dec 2017 17:10:00 -0800 From: Matthew Wilcox To: Ross Zwisler Cc: linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Josh Triplett , Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171222011000.GB23624@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171221214810.GC9087@linux.intel.com> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 21, 2017 at 02:48:10PM -0700, Ross Zwisler wrote: > > +++ b/include/linux/compiler_types.h > > @@ -16,6 +16,7 @@ > > # define __acquire(x) __context__(x,1) > > # define __release(x) __context__(x,-1) > > # define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) > > +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) > ^ > I think we actually want this to return c here ^ > > The old code saved off the actual return value from __follow_pte_pmd() (say, > -EINVAL) in 'res', and that was what was returned on error from both > follow_pte_pmd() and follow_pte(). The value of 1 returned by __cond_lock() > was just discarded (after we cast it to void for some reason). > > With this new code we actually return the value from __cond_lock_err(), which > means that instead of returning -EINVAL, we'll return 1 on error. Yes, but this define is only #if __CHECKER__, so it doesn't matter what we return as this code will never run. That said, if sparse supports the GNU syntax of ?: then I have no objection to doing that. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755831AbdLVEVa (ORCPT ); Thu, 21 Dec 2017 23:21:30 -0500 Received: from relay4-d.mail.gandi.net ([217.70.183.196]:56875 "EHLO relay4-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755152AbdLVEV3 (ORCPT ); Thu, 21 Dec 2017 23:21:29 -0500 X-Originating-IP: 50.39.166.153 Date: Thu, 21 Dec 2017 20:21:20 -0800 From: Josh Triplett To: Matthew Wilcox Cc: Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171222042120.GA18036@localhost> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171222011000.GB23624@bombadil.infradead.org> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote: > On Thu, Dec 21, 2017 at 02:48:10PM -0700, Ross Zwisler wrote: > > > +++ b/include/linux/compiler_types.h > > > @@ -16,6 +16,7 @@ > > > # define __acquire(x) __context__(x,1) > > > # define __release(x) __context__(x,-1) > > > # define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) > > > +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) > > ^ > > I think we actually want this to return c here ^ > > > > The old code saved off the actual return value from __follow_pte_pmd() (say, > > -EINVAL) in 'res', and that was what was returned on error from both > > follow_pte_pmd() and follow_pte(). The value of 1 returned by __cond_lock() > > was just discarded (after we cast it to void for some reason). > > > > With this new code we actually return the value from __cond_lock_err(), which > > means that instead of returning -EINVAL, we'll return 1 on error. > > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we > return as this code will never run. It does matter slightly, as Sparse does some (very limited) value-based analyses. Let's future-proof it. > That said, if sparse supports the GNU syntax of ?: then I have no > objection to doing that. Sparse does support that syntax. - Josh Triplett From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755940AbdLVMb2 (ORCPT ); Fri, 22 Dec 2017 07:31:28 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:42198 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750995AbdLVMbY (ORCPT ); Fri, 22 Dec 2017 07:31:24 -0500 Date: Fri, 22 Dec 2017 04:31:12 -0800 From: Matthew Wilcox To: Josh Triplett Cc: Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171222123112.GA6401@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171222042120.GA18036@localhost> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote: > On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote: > > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we > > return as this code will never run. > > It does matter slightly, as Sparse does some (very limited) value-based > analyses. Let's future-proof it. > > > That said, if sparse supports the GNU syntax of ?: then I have no > > objection to doing that. > > Sparse does support that syntax. Great, I'll fix that and resubmit. While I've got you, I've been looking at some other sparse warnings from this file. There are several caused by sparse being unable to handle the following construct: if (foo) x = NULL; else { x = bar; __acquire(bar); } if (!x) return -ENOMEM; Writing it as: if (foo) return -ENOMEM; else { x = bar; __acquire(bar); } works just fine. ie this removes the warning: @@ -1070,9 +1070,9 @@ static int copy_pte_range(struct mm_struct *dst_mm, struct mm_struct *src_mm, again: init_rss_vec(rss); - dst_pte = pte_alloc_map_lock(dst_mm, dst_pmd, addr, &dst_ptl); - if (!dst_pte) + if (pte_alloc(dst_mm, dst_pmd, addr)) return -ENOMEM; + dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, addr, &dst_ptl); src_pte = pte_offset_map(src_pmd, addr); src_ptl = pte_lockptr(src_mm, src_pmd); spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING); Is there any chance sparse's dataflow analysis will be improved in the near future? From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755690AbdLVNgj (ORCPT ); Fri, 22 Dec 2017 08:36:39 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:53789 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752063AbdLVNgi (ORCPT ); Fri, 22 Dec 2017 08:36:38 -0500 Date: Fri, 22 Dec 2017 05:36:34 -0800 From: Matthew Wilcox To: Josh Triplett Cc: Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171222133634.GE6401@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> <20171222123112.GA6401@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171222123112.GA6401@bombadil.infradead.org> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote: > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote: > > On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote: > > > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we > > > return as this code will never run. > > > > It does matter slightly, as Sparse does some (very limited) value-based > > analyses. Let's future-proof it. > > > > > That said, if sparse supports the GNU syntax of ?: then I have no > > > objection to doing that. > > > > Sparse does support that syntax. > > Great, I'll fix that and resubmit. Except the context imbalance warning comes back if I do. This is sparse 0.5.1 (Debian's 0.5.1-2 package). From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751139AbdLWJj0 (ORCPT ); Sat, 23 Dec 2017 04:39:26 -0500 Received: from relay2-d.mail.gandi.net ([217.70.183.194]:51742 "EHLO relay2-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750713AbdLWJjU (ORCPT ); Sat, 23 Dec 2017 04:39:20 -0500 X-Originating-IP: 50.39.166.153 Date: Sat, 23 Dec 2017 01:39:11 -0800 From: Josh Triplett To: Matthew Wilcox Cc: Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox , linux-sparse@vger.kernel.org Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171223093910.GB6160@localhost> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> <20171222123112.GA6401@bombadil.infradead.org> <20171222133634.GE6401@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171222133634.GE6401@bombadil.infradead.org> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +linux-sparse On Fri, Dec 22, 2017 at 05:36:34AM -0800, Matthew Wilcox wrote: > On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote: > > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote: > > > On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote: > > > > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we > > > > return as this code will never run. > > > > > > It does matter slightly, as Sparse does some (very limited) value-based > > > analyses. Let's future-proof it. > > > > > > > That said, if sparse supports the GNU syntax of ?: then I have no > > > > objection to doing that. > > > > > > Sparse does support that syntax. > > > > Great, I'll fix that and resubmit. > > Except the context imbalance warning comes back if I do. This is sparse > 0.5.1 (Debian's 0.5.1-2 package). From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757014AbdLWNG2 (ORCPT ); Sat, 23 Dec 2017 08:06:28 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:43315 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756971AbdLWNGY (ORCPT ); Sat, 23 Dec 2017 08:06:24 -0500 Date: Sat, 23 Dec 2017 05:06:21 -0800 From: Matthew Wilcox To: Josh Triplett Cc: Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox , linux-sparse@vger.kernel.org Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171223130621.GA3994@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> <20171222123112.GA6401@bombadil.infradead.org> <20171222133634.GE6401@bombadil.infradead.org> <20171223093910.GB6160@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171223093910.GB6160@localhost> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Dec 23, 2017 at 01:39:11AM -0800, Josh Triplett wrote: > +linux-sparse Ehh ... we've probably trimmed too much to give linux-sparse a good summary. Here're the important lines from my patch: +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) + return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, start, end, + ptepp, pmdpp, ptlp)); This is supposed to be "If "c" is an error value, we don't have a lock, otherwise we have a lock". And to translate from linux-speak into sparse-speak: # define __acquire(x) __context__(x,1) Josh & Ross pointed out (quite correctly) that code which does something like if (foo()) return; will work with this, but code that does if (foo() < 0) return; will not because we're now returning 1 instead of -ENOMEM (for example). So they made the very sensible suggestion that I change the definition of __cond_lock to: # define __cond_lock_err(x,c) ((c) ?: ({ __acquire(x); 0; })) Unfortunately, when I do that, the context imbalance warning returns. As I said below, this is with sparse 0.5.1. > On Fri, Dec 22, 2017 at 05:36:34AM -0800, Matthew Wilcox wrote: > > On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote: > > > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote: > > > > On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote: > > > > > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we > > > > > return as this code will never run. > > > > > > > > It does matter slightly, as Sparse does some (very limited) value-based > > > > analyses. Let's future-proof it. > > > > > > > > > That said, if sparse supports the GNU syntax of ?: then I have no > > > > > objection to doing that. > > > > > > > > Sparse does support that syntax. > > > > > > Great, I'll fix that and resubmit. > > > > Except the context imbalance warning comes back if I do. This is sparse > > 0.5.1 (Debian's 0.5.1-2 package). > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751975AbdL0O3A (ORCPT ); Wed, 27 Dec 2017 09:29:00 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:46184 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751002AbdL0O26 (ORCPT ); Wed, 27 Dec 2017 09:28:58 -0500 X-Google-Smtp-Source: ACJfBovJNGVY9w9ez04ADD5hMXw6ba5muWkAC1gdplB4pWRwMtehvANa5s7B4qhEgBoO+3pY7M6tgQ== Date: Wed, 27 Dec 2017 15:28:54 +0100 From: Luc Van Oostenryck To: Matthew Wilcox Cc: Josh Triplett , Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171227142853.b5agfi2kzo25g5ot@ltop.local> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> <20171222123112.GA6401@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171222123112.GA6401@bombadil.infradead.org> User-Agent: NeoMutt/20171027 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote: > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote: > > While I've got you, I've been looking at some other sparse warnings from > this file. There are several caused by sparse being unable to handle > the following construct: > > if (foo) > x = NULL; > else { > x = bar; > __acquire(bar); > } > if (!x) > return -ENOMEM; > > Writing it as: > > if (foo) > return -ENOMEM; > else { > x = bar; > __acquire(bar); > } > > works just fine. ie this removes the warning: It must be noted that these two versions are not equivalent (in the first version, it also returns with -ENOMEM if bar is NULL/zero). It must be noted that sparse's goal regarding the context imbalance is to give the warning if some point in the code can be reached via two paths (or more) and the lock state (the context) is not identical in each of these paths. > > Is there any chance sparse's dataflow analysis will be improved in the > near future? A lot of functions in the kernel have this context imbalance, really a lot. For example, any function doing conditional locking is a problem here. Happily when these functions are inlined, sparse, thanks to its optimizations, can remove some paths and merge some others. So yes, by adding some smartness to sparse, some of the false warnings will be removed, however: 1) some __must_hold()/__acquires()/__releases() annotations are missing, making sparse's job impossible. 2) a lot of the 'false warnings' are not so false because there is indeed two possible paths with different lock state 3) it has its limits (at the end, giving the correct warning is equivalent to the halting problem). Now, to answer to your question, I'm not aware of any effort that would make a significant differences (it would need, IMO, code hoisting & value range propagation). -- Luc Van Oostenryck From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752043AbdL0Oi5 (ORCPT ); Wed, 27 Dec 2017 09:38:57 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:35664 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751002AbdL0Oiy (ORCPT ); Wed, 27 Dec 2017 09:38:54 -0500 X-Google-Smtp-Source: ACJfBosok4plS/TxfgrygElrl6l4FhiArhH/+bg17J15WIxOcjxnO2aRJDu6oYmuM05QF6YUuPMpXQ== Date: Wed, 27 Dec 2017 15:38:50 +0100 From: Luc Van Oostenryck To: Matthew Wilcox Cc: Josh Triplett , Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox , linux-sparse@vger.kernel.org Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171227143850.nnuatshhezurbu7r@ltop.local> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> <20171222123112.GA6401@bombadil.infradead.org> <20171222133634.GE6401@bombadil.infradead.org> <20171223093910.GB6160@localhost> <20171223130621.GA3994@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171223130621.GA3994@bombadil.infradead.org> User-Agent: NeoMutt/20171027 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Dec 23, 2017 at 05:06:21AM -0800, Matthew Wilcox wrote: > On Sat, Dec 23, 2017 at 01:39:11AM -0800, Josh Triplett wrote: > > +linux-sparse > > Ehh ... we've probably trimmed too much to give linux-sparse a good summary. > > Here're the important lines from my patch: > > +# define __cond_lock_err(x,c) ((c) ? 1 : ({ __acquire(x); 0; })) > > + return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, start, end, > + ptepp, pmdpp, ptlp)); > > This is supposed to be "If "c" is an error value, we don't have a lock, > otherwise we have a lock". And to translate from linux-speak into > sparse-speak: > > # define __acquire(x) __context__(x,1) > > Josh & Ross pointed out (quite correctly) that code which does something like > > if (foo()) > return; > > will work with this, but code that does > > if (foo() < 0) > return; > > will not because we're now returning 1 instead of -ENOMEM (for example). > > So they made the very sensible suggestion that I change the definition > of __cond_lock to: > > # define __cond_lock_err(x,c) ((c) ?: ({ __acquire(x); 0; })) > > Unfortunately, when I do that, the context imbalance warning returns. > As I said below, this is with sparse 0.5.1. I think this __cond_lock_err() is now OK (but some comment about how its use is different from __cond_lock() would be welcome). For the context imbalance, I would really need a concrete example to be able to help more because it depends heavily on what the test is and what code is before and after. If you can point me to a tree, a .config and a specific warning, I'll be glad to take a look. -- Luc Van Oostenryck From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750915AbdL3HR2 (ORCPT ); Sat, 30 Dec 2017 02:17:28 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:36819 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750815AbdL3HR1 (ORCPT ); Sat, 30 Dec 2017 02:17:27 -0500 Date: Fri, 29 Dec 2017 23:17:20 -0800 From: Matthew Wilcox To: Luc Van Oostenryck Cc: Josh Triplett , Ross Zwisler , linux-kernel@vger.kernel.org, Dave Hansen , linux-mm@kvack.org, Matthew Wilcox Subject: Re: [PATCH 2/2] Introduce __cond_lock_err Message-ID: <20171230071720.GE27959@bombadil.infradead.org> References: <20171219165823.24243-1-willy@infradead.org> <20171219165823.24243-2-willy@infradead.org> <20171221214810.GC9087@linux.intel.com> <20171222011000.GB23624@bombadil.infradead.org> <20171222042120.GA18036@localhost> <20171222123112.GA6401@bombadil.infradead.org> <20171227142853.b5agfi2kzo25g5ot@ltop.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171227142853.b5agfi2kzo25g5ot@ltop.local> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 27, 2017 at 03:28:54PM +0100, Luc Van Oostenryck wrote: > On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote: > > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote: > > > > While I've got you, I've been looking at some other sparse warnings from > > this file. There are several caused by sparse being unable to handle > > the following construct: > > > > if (foo) > > x = NULL; > > else { > > x = bar; > > __acquire(bar); > > } > > if (!x) > > return -ENOMEM; > > > > Writing it as: > > > > if (foo) > > return -ENOMEM; > > else { > > x = bar; > > __acquire(bar); > > } > > > > works just fine. ie this removes the warning: > > It must be noted that these two versions are not equivalent > (in the first version, it also returns with -ENOMEM if bar > is NULL/zero). They happen to be equivalent in the original; I was providing a simplified version. Here's the construct sparse can't understand: dst_pte = pte_alloc_map_lock(dst_mm, dst_pmd, addr, &dst_ptl); if (!dst_pte) return -ENOMEM; with: #define pte_alloc(mm, pmd, address) \ (unlikely(pmd_none(*(pmd))) && __pte_alloc(mm, pmd, address)) #define pte_offset_map_lock(mm, pmd, address, ptlp) \ ({ \ spinlock_t *__ptl = pte_lockptr(mm, pmd); \ pte_t *__pte = pte_offset_map(pmd, address); \ *(ptlp) = __ptl; \ spin_lock(__ptl); \ __pte; \ }) #define pte_alloc_map_lock(mm, pmd, address, ptlp) \ (pte_alloc(mm, pmd, address) ? \ NULL : pte_offset_map_lock(mm, pmd, address, ptlp)) If pte_alloc() succeeds, pte_offset_map_lock() will return non-NULL. Manually inlining pte_alloc_map_lock() into the caller like so: if (pte_alloc(dst_mm, dst_pmd, addr) return -ENOMEM; dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, addr, ptlp); causes sparse to not warn. > > Is there any chance sparse's dataflow analysis will be improved in the > > near future? > > A lot of functions in the kernel have this context imbalance, > really a lot. For example, any function doing conditional locking > is a problem here. Happily when these functions are inlined, > sparse, thanks to its optimizations, can remove some paths and > merge some others. > So yes, by adding some smartness to sparse, some of the false > warnings will be removed, however: > 1) some __must_hold()/__acquires()/__releases() annotations are > missing, making sparse's job impossible. Partly there's a documentation problem here. I'd really like to see a document explaining how to add sparse annotations to a function which intentionally does conditional locking. For example, should we be annotating the function as __acquires, and then marking the exits which don't acquire the lock with __acquire(), or should we not annotate the function, and annotate the exits which _do_ acquire the lock as __release() with a comment like /* Caller will release */ > 2) a lot of the 'false warnings' are not so false because there is > indeed two possible paths with different lock state > 3) it has its limits (at the end, giving the correct warning is > equivalent to the halting problem). > > Now, to answer to your question, I'm not aware of any effort that would > make a significant differences (it would need, IMO, code hoisting & > value range propagation). That's fair. I wonder if we were starting from scratch whether we'd choose to make sparse a GCC plugin today.