From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941261AbcLVSho (ORCPT ); Thu, 22 Dec 2016 13:37:44 -0500 Received: from mail-oi0-f45.google.com ([209.85.218.45]:34124 "EHLO mail-oi0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1765821AbcLVShn (ORCPT ); Thu, 22 Dec 2016 13:37:43 -0500 MIME-Version: 1.0 In-Reply-To: <20161222164042.GA18407@linux.intel.com> References: <148228426023.2477.2662330241414304847.stgit@dwillia2-desk3.amr.corp.intel.com> <20161221085347.GB4756@quack2.suse.cz> <20161221175118.GA4716@linux.intel.com> <20161222100055.GB28510@quack2.suse.cz> <20161222164042.GA18407@linux.intel.com> From: Dan Williams Date: Thu, 22 Dec 2016 10:37:41 -0800 Message-ID: Subject: Re: [PATCH] dax: kill uml support To: Ross Zwisler , Jan Kara , Dan Williams , Andrew Morton , Matthew Wilcox , Dave Chinner , "linux-kernel@vger.kernel.org" , Dave Hansen , Alexander Viro , linux-fsdevel , Christoph Hellwig Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 22, 2016 at 8:40 AM, Ross Zwisler wrote: > On Thu, Dec 22, 2016 at 11:00:55AM +0100, Jan Kara wrote: >> 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. > > Yea, making an empty stub is cleaner. That has the added bonus that we don't > have to rely on the assumption that 'pmdp' will always be NULL in if > CONFIG_TRANSPARENT_HUGEPAGE isn't set. > > I'll reflow & send out v2. If you're going deeper might as well look at killing the other ifdef CONFIG_FS_DAX_PMD in that file as well.