Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1545549 > unrolled thread

[PATCH] dax: kill uml support

Back to article view | Back to linux.kernel

Contents

  [PATCH] dax: kill uml support Dan Williams <dan.j.williams@intel.com> - 2016-12-21 02:50 +0100
    Re: [PATCH] dax: kill uml support Jan Kara <jack@suse.cz> - 2016-12-21 10:00 +0100
      Re: [PATCH] dax: kill uml support Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-12-21 19:00 +0100
        Re: [PATCH] dax: kill uml support Jan Kara <jack@suse.cz> - 2016-12-22 11:10 +0100
          Re: [PATCH] dax: kill uml support Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-12-22 17:50 +0100
            Re: [PATCH] dax: kill uml support Dan Williams <dan.j.williams@intel.com> - 2016-12-22 19:40 +0100
    Re: [PATCH] dax: kill uml support Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-12-21 19:00 +0100
      Re: [PATCH] dax: kill uml support Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-12-21 19:50 +0100
      Re: [PATCH] dax: kill uml support Dan Williams <dan.j.williams@intel.com> - 2016-12-21 19:50 +0100

#1545549 — [PATCH] dax: kill uml support

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(-)

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;

[toc] | [next] | [standalone]

#1545673

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1545915

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.

[toc] | [prev] | [next] | [standalone]

#1546252

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 <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1546486

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.

[toc] | [prev] | [next] | [standalone]

#1546534

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.

[toc] | [prev] | [next] | [standalone]

#1545916

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.

[toc] | [prev] | [next] | [standalone]

#1545940

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>

[toc] | [prev] | [next] | [standalone]

#1545955

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web