From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751334AbaJRVQd (ORCPT ); Sat, 18 Oct 2014 17:16:33 -0400 Received: from mail.efficios.com ([78.47.125.74]:57875 "EHLO mail.efficios.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751210AbaJRVQb (ORCPT ); Sat, 18 Oct 2014 17:16:31 -0400 Date: Sat, 18 Oct 2014 21:16:23 +0000 (UTC) From: Mathieu Desnoyers To: Matthew Wilcox Cc: Matthew Wilcox , linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Ross Zwisler Message-ID: <1585806495.11375.1413666983187.JavaMail.zimbra@efficios.com> In-Reply-To: <20141018174100.GO11522@wil.cx> References: <1411677218-29146-1-git-send-email-matthew.r.wilcox@intel.com> <1411677218-29146-20-git-send-email-matthew.r.wilcox@intel.com> <20141016123824.GQ19075@thinkos.etherlink> <20141016220126.GK11522@wil.cx> <1868658383.10922.1413560979310.JavaMail.zimbra@efficios.com> <20141018174100.GO11522@wil.cx> Subject: Re: [PATCH v11 19/21] dax: Add dax_zero_page_range MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [212.144.255.202] X-Mailer: Zimbra 8.0.7_GA_6021 (ZimbraWebClient - FF32 (Linux)/8.0.7_GA_6021) Thread-Topic: Add dax_zero_page_range Thread-Index: f5gxMclXRMMhEff6vJWpxRU8G6Nrow== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ----- Original Message ----- > From: "Matthew Wilcox" > To: "Mathieu Desnoyers" > Cc: "Matthew Wilcox" , "Matthew Wilcox" , > linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, "Ross Zwisler" > > Sent: Saturday, October 18, 2014 7:41:00 PM > Subject: Re: [PATCH v11 19/21] dax: Add dax_zero_page_range > > On Fri, Oct 17, 2014 at 03:49:39PM +0000, Mathieu Desnoyers wrote: > > > I kind of wonder if we shouldn't just declare the function. It's called > > > like this: > > > > > > if (IS_DAX(inode)) > > > return dax_zero_page_range(inode, from, length, > > > ext4_get_block); > > > return __ext4_block_zero_page_range(handle, mapping, from, > > > length); > > > > > > and if CONFIG_DAX is not set, IS_DAX evaluates to 0 at compile time, so > > > the compiler will optimise out the call to dax_zero_page_range() anyway. > > > > I strongly prefer to implement "unimplemented stub" as static inlines > > rather than defining to 0, because the compiler can check that the types > > passed to the function are valid, even in the #else configuration which > > uses the stubs. > > I think my explanation was unclear. This is what I meant: > > +++ b/include/linux/fs.h > @@ -2473,7 +2473,6 @@ extern loff_t fixed_size_llseek(struct file *file, > loff_t > offset, > extern int generic_file_open(struct inode * inode, struct file * filp); > extern int nonseekable_open(struct inode * inode, struct file * filp); > > -#ifdef CONFIG_FS_DAX > int dax_clear_blocks(struct inode *, sector_t block, long size); > int dax_zero_page_range(struct inode *, loff_t from, unsigned len, > get_block_t) > ; > int dax_truncate_page(struct inode *, loff_t from, get_block_t); > #define dax_mkwrite(vma, vmf, gb) dax_fault(vma, vmf, gb) > -#else > -static inline int dax_clear_blocks(struct inode *i, sector_t blk, long sz) > -{ > - return 0; > -} > - > -static inline int dax_truncate_page(struct inode *i, loff_t frm, get_block_t > gb) > -{ > - return 0; > -} > - > -static inline int dax_zero_page_range(struct inode *i, loff_t frm, > - unsigned len, get_block_t gb) > -{ > - return 0; > -} > - > -static inline ssize_t dax_do_io(int rw, struct kiocb *iocb, > - struct inode *inode, struct iov_iter *iter, loff_t pos, > - get_block_t get_block, dio_iodone_t end_io, int flags) > -{ > - return -ENOTTY; > -} > -#endif > > #ifdef CONFIG_BLOCK > typedef void (dio_submit_t)(int rw, struct bio *bio, struct inode *inode, > > > So after the preprocessor has run, the compiler will see: > > if (0) > return dax_zero_page_range(inode, from, length, ext4_get_block); > > and it will still do type checking on the call, even though it will eliminate > the call. > Indeed, since Linux is always compiled in O2 or Os, it will work. > I think what you're really complaining about is that the argument to > IS_DAX() is not checked for being an inode. > > We could solve that this way: > > #ifdef CONFIG_FS_DAX > #define S_DAX 8192 > #else > #define S_DAX 0 > #endif > ... > #define IS_DAX(inode) ((inode)->i_flags & S_DAX) > > After preprocessing, the compiler than sees: > > if (((inode)->i_flags & 0)) > return dax_zero_page_range(inode, from, length, ext4_get_block); > > and successfully deduces that the condition evaluates to 0, and still > elide the reference to dax_zero_page_range (checked with 'nm'). Sounds good, Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mathieu Desnoyers Subject: Re: [PATCH v11 19/21] dax: Add dax_zero_page_range Date: Sat, 18 Oct 2014 21:16:23 +0000 (UTC) Message-ID: <1585806495.11375.1413666983187.JavaMail.zimbra@efficios.com> References: <1411677218-29146-1-git-send-email-matthew.r.wilcox@intel.com> <1411677218-29146-20-git-send-email-matthew.r.wilcox@intel.com> <20141016123824.GQ19075@thinkos.etherlink> <20141016220126.GK11522@wil.cx> <1868658383.10922.1413560979310.JavaMail.zimbra@efficios.com> <20141018174100.GO11522@wil.cx> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Matthew Wilcox , linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Ross Zwisler To: Matthew Wilcox Return-path: In-Reply-To: <20141018174100.GO11522@wil.cx> Sender: owner-linux-mm@kvack.org List-Id: linux-fsdevel.vger.kernel.org ----- Original Message ----- > From: "Matthew Wilcox" > To: "Mathieu Desnoyers" > Cc: "Matthew Wilcox" , "Matthew Wilcox" , > linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, "Ross Zwisler" > > Sent: Saturday, October 18, 2014 7:41:00 PM > Subject: Re: [PATCH v11 19/21] dax: Add dax_zero_page_range > > On Fri, Oct 17, 2014 at 03:49:39PM +0000, Mathieu Desnoyers wrote: > > > I kind of wonder if we shouldn't just declare the function. It's called > > > like this: > > > > > > if (IS_DAX(inode)) > > > return dax_zero_page_range(inode, from, length, > > > ext4_get_block); > > > return __ext4_block_zero_page_range(handle, mapping, from, > > > length); > > > > > > and if CONFIG_DAX is not set, IS_DAX evaluates to 0 at compile time, so > > > the compiler will optimise out the call to dax_zero_page_range() anyway. > > > > I strongly prefer to implement "unimplemented stub" as static inlines > > rather than defining to 0, because the compiler can check that the types > > passed to the function are valid, even in the #else configuration which > > uses the stubs. > > I think my explanation was unclear. This is what I meant: > > +++ b/include/linux/fs.h > @@ -2473,7 +2473,6 @@ extern loff_t fixed_size_llseek(struct file *file, > loff_t > offset, > extern int generic_file_open(struct inode * inode, struct file * filp); > extern int nonseekable_open(struct inode * inode, struct file * filp); > > -#ifdef CONFIG_FS_DAX > int dax_clear_blocks(struct inode *, sector_t block, long size); > int dax_zero_page_range(struct inode *, loff_t from, unsigned len, > get_block_t) > ; > int dax_truncate_page(struct inode *, loff_t from, get_block_t); > #define dax_mkwrite(vma, vmf, gb) dax_fault(vma, vmf, gb) > -#else > -static inline int dax_clear_blocks(struct inode *i, sector_t blk, long sz) > -{ > - return 0; > -} > - > -static inline int dax_truncate_page(struct inode *i, loff_t frm, get_block_t > gb) > -{ > - return 0; > -} > - > -static inline int dax_zero_page_range(struct inode *i, loff_t frm, > - unsigned len, get_block_t gb) > -{ > - return 0; > -} > - > -static inline ssize_t dax_do_io(int rw, struct kiocb *iocb, > - struct inode *inode, struct iov_iter *iter, loff_t pos, > - get_block_t get_block, dio_iodone_t end_io, int flags) > -{ > - return -ENOTTY; > -} > -#endif > > #ifdef CONFIG_BLOCK > typedef void (dio_submit_t)(int rw, struct bio *bio, struct inode *inode, > > > So after the preprocessor has run, the compiler will see: > > if (0) > return dax_zero_page_range(inode, from, length, ext4_get_block); > > and it will still do type checking on the call, even though it will eliminate > the call. > Indeed, since Linux is always compiled in O2 or Os, it will work. > I think what you're really complaining about is that the argument to > IS_DAX() is not checked for being an inode. > > We could solve that this way: > > #ifdef CONFIG_FS_DAX > #define S_DAX 8192 > #else > #define S_DAX 0 > #endif > ... > #define IS_DAX(inode) ((inode)->i_flags & S_DAX) > > After preprocessing, the compiler than sees: > > if (((inode)->i_flags & 0)) > return dax_zero_page_range(inode, from, length, ext4_get_block); > > and successfully deduces that the condition evaluates to 0, and still > elide the reference to dax_zero_page_range (checked with 'nm'). Sounds good, Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com -- 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