From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:58348 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S938785AbcLVKA6 (ORCPT ); Thu, 22 Dec 2016 05:00:58 -0500 Date: Thu, 22 Dec 2016 11:00:55 +0100 From: Jan Kara To: Ross Zwisler Cc: Jan Kara , Dan Williams , akpm@linux-foundation.org, Matthew Wilcox , Dave Chinner , linux-kernel@vger.kernel.org, Dave Hansen , Alexander Viro , linux-fsdevel@vger.kernel.org, Christoph Hellwig Subject: Re: [PATCH] dax: kill uml support Message-ID: <20161222100055.GB28510@quack2.suse.cz> References: <148228426023.2477.2662330241414304847.stgit@dwillia2-desk3.amr.corp.intel.com> <20161221085347.GB4756@quack2.suse.cz> <20161221175118.GA4716@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161221175118.GA4716@linux.intel.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed 21-12-16 10:51:18, Ross Zwisler wrote: > On Wed, Dec 21, 2016 at 09:53:47AM +0100, Jan Kara wrote: > > On Tue 20-12-16 17:37:40, Dan Williams wrote: > > > The lack of common transparent-huge-page helpers for UML is becoming > > > increasingly painful for fs/dax.c now that it is growing more pmd > > > functionality. Add UML to the list of unsupported architectures, and > > > clean up no-longer-necessary ifdef as a result. > > ... > > > diff --git a/fs/dax.c b/fs/dax.c > > > index ddcddfeaa03b..86df835783ea 100644 > > > --- a/fs/dax.c > > > +++ b/fs/dax.c > > > @@ -710,8 +710,7 @@ static void dax_mapping_entry_mkclean(struct address_space *mapping, > > > if (follow_pte_pmd(vma->vm_mm, address, &ptep, &pmdp, &ptl)) > > > continue; > > > > > > - if (pmdp) { > > > -#ifdef CONFIG_FS_DAX_PMD > > > + if (pmdp && IS_ENABLED(CONFIG_FS_DAX_PMD)) { > > > pmd_t pmd; > > > > So I was under the impression that pmdp can never be != NULL when > > CONFIG_FS_DAX_PMD is disabled. Otherwise Ross' patch would leave ptl locked > > in that case... Did I miss something or we can just remove IS_ENABLED() > > check? > > We need the IS_ENABLED() check to prevent a different build error. > > The #ifdef was there to prevent compile time errors where pmd_pfn(), > pmd_write(), etc were all undefined symbols because they aren't defined in > the arch/um headers. > > For x86_64 we can get linker errors if CONFIG_TRANSPARENT_HUGEPAGE isn't set: > > fs/built-in.o: In function `dax_writeback_mapping_range.part.27': > dax.c:(.text+0x4ce28): undefined reference to `pmdp_huge_clear_flush' > > In this config we do have a prototype for pmdp_huge_clear_flush() in > include/asm-generic/pgtable.h so it doesn't show up as an undefined symbol, > but the implementation in mm/pgtable-generic.c is in a > #ifdef CONFIG_TRANSPARENT_HUGEPAGE block. > > The IS_ENABLED() lets the compiler optimize out the code that calls > pmdp_huge_clear_flush() so it doesn't try and resolve that symbol at link > time. Ah, OK. Won't it be cleaner to just have empty stub for pmdp_huge_clear_flush() if !CONFIG_TRANSPARENT_HUGEPAGE? I like that more but I can live with IS_ENABLED check as well but please add a comment that it is there only to make compiler happy. Honza -- Jan Kara SUSE Labs, CR