linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ross Zwisler <ross.zwisler@linux.intel.com>
To: Jan Kara <jack@suse.cz>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	akpm@linux-foundation.org,
	Matthew Wilcox <mawilcox@microsoft.com>,
	Dave Chinner <david@fromorbit.com>,
	linux-kernel@vger.kernel.org, Dave Hansen <dave.hansen@intel.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel@vger.kernel.org, Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH] dax: kill uml support
Date: Thu, 22 Dec 2016 09:40:42 -0700	[thread overview]
Message-ID: <20161222164042.GA18407@linux.intel.com> (raw)
In-Reply-To: <20161222100055.GB28510@quack2.suse.cz>

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.

  reply	other threads:[~2016-12-22 16:40 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-21  1:37 Dan Williams
2016-12-21  8:53 ` Jan Kara
2016-12-21 17:51   ` Ross Zwisler
2016-12-22 10:00     ` Jan Kara
2016-12-22 16:40       ` Ross Zwisler [this message]
2016-12-22 18:37         ` Dan Williams
2016-12-21 17:53 ` Ross Zwisler
2016-12-21 18:45   ` Ross Zwisler
2016-12-21 18:45   ` Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161222164042.GA18407@linux.intel.com \
    --to=ross.zwisler@linux.intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=dan.j.williams@intel.com \
    --cc=dave.hansen@intel.com \
    --cc=david@fromorbit.com \
    --cc=hch@lst.de \
    --cc=jack@suse.cz \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mawilcox@microsoft.com \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH] dax: kill uml support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).