| Message ID | 148228426023.2477.2662330241414304847.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? Honza
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. (If any of this sounds incorrect or I'm using the wrong terminology for anything, please correct me!) You're right that 'pmdp' should always be NULL if CONFIG_FS_DAX_PMD isn't defined. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 20, 2016 at 05:37:40PM -0800, 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. > > Cc: Jan Kara <jack@suse.cz> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Dave Chinner <david@fromorbit.com> > Cc: Dave Hansen <dave.hansen@intel.com> > Cc: Matthew Wilcox <mawilcox@microsoft.com> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Sure, this is fine, assuming UML was the only problem architecture. I've pushed this code to my kernel.org tree and should hopefully hear back from 0day shortly. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 21, 2016 at 10:53:20AM -0700, Ross Zwisler wrote: > On Tue, Dec 20, 2016 at 05:37:40PM -0800, 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. > > > > Cc: Jan Kara <jack@suse.cz> > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Dave Chinner <david@fromorbit.com> > > Cc: Dave Hansen <dave.hansen@intel.com> > > Cc: Matthew Wilcox <mawilcox@microsoft.com> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > > Cc: Ross Zwisler <ross.zwisler@linux.intel.com> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > Sure, this is fine, assuming UML was the only problem architecture. I've > pushed this code to my kernel.org tree and should hopefully hear back from > 0day shortly. 0day has given the thumbs up: Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 21, 2016 at 9:53 AM, Ross Zwisler <ross.zwisler@linux.intel.com> wrote: > On Tue, Dec 20, 2016 at 05:37:40PM -0800, 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. >> >> Cc: Jan Kara <jack@suse.cz> >> Cc: Christoph Hellwig <hch@lst.de> >> Cc: Dave Chinner <david@fromorbit.com> >> Cc: Dave Hansen <dave.hansen@intel.com> >> Cc: Matthew Wilcox <mawilcox@microsoft.com> >> Cc: Alexander Viro <viro@zeniv.linux.org.uk> >> Cc: Ross Zwisler <ross.zwisler@linux.intel.com> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > Sure, this is fine, assuming UML was the only problem architecture. I've > pushed this code to my kernel.org tree and should hopefully hear back from > 0day shortly. I got a success notification from 0day before I sent the patch. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 22, 2016 at 8:40 AM, Ross Zwisler <ross.zwisler@linux.intel.com> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/Kconfig b/fs/Kconfig index c2a377cdda2b..661931fb0ce0 100644 --- a/fs/Kconfig +++ b/fs/Kconfig @@ -37,7 +37,7 @@ source "fs/f2fs/Kconfig" config FS_DAX bool "Direct Access (DAX) support" depends on MMU - depends on !(ARM || MIPS || SPARC) + depends on !(ARM || MIPS || SPARC || UML) help Direct Access (DAX) can be used on memory-backed block devices. If the block device supports DAX and the filesystem supports DAX, 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; if (pfn != pmd_pfn(*pmdp)) @@ -727,7 +726,6 @@ static void dax_mapping_entry_mkclean(struct address_space *mapping, changed = true; unlock_pmd: spin_unlock(ptl); -#endif } else { if (pfn != pte_pfn(*ptep)) goto unlock_pte;
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. Cc: Jan Kara <jack@suse.cz> Cc: Christoph Hellwig <hch@lst.de> Cc: Dave Chinner <david@fromorbit.com> Cc: Dave Hansen <dave.hansen@intel.com> Cc: Matthew Wilcox <mawilcox@microsoft.com> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Ross Zwisler <ross.zwisler@linux.intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- fs/Kconfig | 2 +- fs/dax.c | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html