[PATCH 0/2] mm: force enable thp for dax

[PATCH 0/2] mm: force enable thp for dax

[Dan Williams]

Hi Andrew,

Please consider taking these 2 patches for 4.13. I spent some time
debugging why a user's device-dax configuration was always failing and
it turned out that their thp policy was set to 'never'. DAX should be
exempt from the policy since it is statically allocated and does not
suffer from any of the potentially negative side effects of thp. More
details in patch 2.

---

Dan Williams (2):
      mm: improve readability of transparent_hugepage_enabled()
      mm: always enable thp for dax mappings


 include/linux/dax.h     |    5 -----
 include/linux/fs.h      |    6 ++++++
 include/linux/huge_mm.h |   40 +++++++++++++++++++++++++++++-----------
 3 files changed, 35 insertions(+), 16 deletions(-)
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 1/2] mm: improve readability of transparent_hugepage_enabled()

[Dan Williams]

Turn the macro into a static inline and rewrite the condition checks for
better readability in preparation for adding another condition.

Cc: Jan Kara <jack@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/huge_mm.h |   35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index a3762d49ba39..c4706e2c3358 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -85,14 +85,26 @@ extern struct kobj_attribute shmem_enabled_attr;
 
 extern bool is_vma_temporary_stack(struct vm_area_struct *vma);
 
-#define transparent_hugepage_enabled(__vma)				\
-	((transparent_hugepage_flags &					\
-	  (1<<TRANSPARENT_HUGEPAGE_FLAG) ||				\
-	  (transparent_hugepage_flags &					\
-	   (1<<TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG) &&			\
-	   ((__vma)->vm_flags & VM_HUGEPAGE))) &&			\
-	 !((__vma)->vm_flags & VM_NOHUGEPAGE) &&			\
-	 !is_vma_temporary_stack(__vma))
+extern unsigned long transparent_hugepage_flags;
+
+static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
+{
+	if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
+		return true;
+
+	if (transparent_hugepage_flags
+			& (1 << TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG))
+		/* check vma flags */;
+	else
+		return false;
+
+	if ((vma->vm_flags & (VM_HUGEPAGE | VM_NOHUGEPAGE)) == VM_HUGEPAGE
+			&& !is_vma_temporary_stack(vma))
+		return true;
+
+	return false;
+}
+
 #define transparent_hugepage_use_zero_page()				\
 	(transparent_hugepage_flags &					\
 	 (1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG))
@@ -104,8 +116,6 @@ extern bool is_vma_temporary_stack(struct vm_area_struct *vma);
 #define transparent_hugepage_debug_cow() 0
 #endif /* CONFIG_DEBUG_VM */
 
-extern unsigned long transparent_hugepage_flags;
-
 extern unsigned long thp_get_unmapped_area(struct file *filp,
 		unsigned long addr, unsigned long len, unsigned long pgoff,
 		unsigned long flags);
@@ -223,7 +233,10 @@ void mm_put_huge_zero_page(struct mm_struct *mm);
 
 #define hpage_nr_pages(x) 1
 
-#define transparent_hugepage_enabled(__vma) 0
+static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
+{
+	return false;
+}
 
 static inline void prep_transhuge_page(struct page *page) {}
 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 2/2] mm: always enable thp for dax mappings

[Dan Williams]

The madvise policy for transparent huge pages is meant to avoid unwanted
allocations of transparent huge pages. It allows a policy of disabling
the extra memory pressure and effort to arrange for a huge page when it
is not needed.

DAX by definition never incurs this overhead since it is statically
allocated. The policy choice makes even less sense for device-dax which
tries to guarantee a given tlb-fault size. Specifically, the following
setting:

	echo never > /sys/kernel/mm/transparent_hugepage/enabled

...violates that guarantee and silently disables all device-dax
instances with a 2M or 1G alignment. So, let's avoid that non-obvious
side effect by force enabling thp for dax mappings in all cases.

It is worth noting that the reason this uses vma_is_dax(), and the
resulting header include changes, is that previous attempts to add a
VM_DAX flag were NAKd.

Cc: Jan Kara <jack@suse.cz>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/dax.h     |    5 -----
 include/linux/fs.h      |    6 ++++++
 include/linux/huge_mm.h |    5 +++++
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/linux/dax.h b/include/linux/dax.h
index 1f6b6072af64..cbaf3d53d66b 100644
--- a/include/linux/dax.h
+++ b/include/linux/dax.h
@@ -151,11 +151,6 @@ static inline unsigned int dax_radix_order(void *entry)
 #endif
 int dax_pfn_mkwrite(struct vm_fault *vmf);
 
-static inline bool vma_is_dax(struct vm_area_struct *vma)
-{
-	return vma->vm_file && IS_DAX(vma->vm_file->f_mapping->host);
-}
-
 static inline bool dax_mapping(struct address_space *mapping)
 {
 	return mapping->host && IS_DAX(mapping->host);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 803e5a9b2654..5916ab3a12d5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -18,6 +18,7 @@
 #include <linux/bug.h>
 #include <linux/mutex.h>
 #include <linux/rwsem.h>
+#include <linux/mm_types.h>
 #include <linux/capability.h>
 #include <linux/semaphore.h>
 #include <linux/fiemap.h>
@@ -3042,6 +3043,11 @@ static inline bool io_is_direct(struct file *filp)
 	return (filp->f_flags & O_DIRECT) || IS_DAX(filp->f_mapping->host);
 }
 
+static inline bool vma_is_dax(struct vm_area_struct *vma)
+{
+	return vma->vm_file && IS_DAX(vma->vm_file->f_mapping->host);
+}
+
 static inline int iocb_flags(struct file *file)
 {
 	int res = 0;
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index c4706e2c3358..901ed3767d1b 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -1,6 +1,8 @@
 #ifndef _LINUX_HUGE_MM_H
 #define _LINUX_HUGE_MM_H
 
+#include <linux/fs.h>
+
 extern int do_huge_pmd_anonymous_page(struct vm_fault *vmf);
 extern int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 			 pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,
@@ -92,6 +94,9 @@ static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
 	if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
 		return true;
 
+	if (vma_is_dax(vma))
+		return true;
+
 	if (transparent_hugepage_flags
 			& (1 << TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG))
 		/* check vma flags */;

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] mm: always enable thp for dax mappings

[Kirill A. Shutemov]

On Sat, Jun 10, 2017 at 02:49:37PM -0700, Dan Williams wrote:
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index c4706e2c3358..901ed3767d1b 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -1,6 +1,8 @@
>  #ifndef _LINUX_HUGE_MM_H
>  #define _LINUX_HUGE_MM_H
>  
> +#include <linux/fs.h>
> +

It means <linux/mm.h> now depends on <linux/fs.h>. I don't think it's a
good idea.

-- 
 Kirill A. Shutemov
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] mm: improve readability of transparent_hugepage_enabled()

[Kirill A. Shutemov]

On Sat, Jun 10, 2017 at 02:49:31PM -0700, Dan Williams wrote:
> Turn the macro into a static inline and rewrite the condition checks for
> better readability in preparation for adding another condition.
> 
> Cc: Jan Kara <jack@suse.cz>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Looks good (unless it breaks build somewhere).

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
 Kirill A. Shutemov
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] mm: always enable thp for dax mappings

[Dan Williams]

On Mon, Jun 12, 2017 at 5:07 AM, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
> On Sat, Jun 10, 2017 at 02:49:37PM -0700, Dan Williams wrote:
>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>> index c4706e2c3358..901ed3767d1b 100644
>> --- a/include/linux/huge_mm.h
>> +++ b/include/linux/huge_mm.h
>> @@ -1,6 +1,8 @@
>>  #ifndef _LINUX_HUGE_MM_H
>>  #define _LINUX_HUGE_MM_H
>>
>> +#include <linux/fs.h>
>> +
>
> It means <linux/mm.h> now depends on <linux/fs.h>. I don't think it's a
> good idea.

Seems to be ok as far as 0day-kbuild-robot is concerned. The
alternative is to move vma_is_dax() out of line. I think
transparent_hugepage_enabled() is called frequently enough to make it
worth it to keep it inline.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] mm: improve readability of transparent_hugepage_enabled()

[Ross Zwisler]

On Sat, Jun 10, 2017 at 02:49:31PM -0700, Dan Williams wrote:
> Turn the macro into a static inline and rewrite the condition checks for
> better readability in preparation for adding another condition.
> 
> Cc: Jan Kara <jack@suse.cz>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  include/linux/huge_mm.h |   35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index a3762d49ba39..c4706e2c3358 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -85,14 +85,26 @@ extern struct kobj_attribute shmem_enabled_attr;
>  
>  extern bool is_vma_temporary_stack(struct vm_area_struct *vma);
>  
> -#define transparent_hugepage_enabled(__vma)				\
> -	((transparent_hugepage_flags &					\
> -	  (1<<TRANSPARENT_HUGEPAGE_FLAG) ||				\
> -	  (transparent_hugepage_flags &					\
> -	   (1<<TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG) &&			\
> -	   ((__vma)->vm_flags & VM_HUGEPAGE))) &&			\
> -	 !((__vma)->vm_flags & VM_NOHUGEPAGE) &&			\
> -	 !is_vma_temporary_stack(__vma))
> +extern unsigned long transparent_hugepage_flags;
> +
> +static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
> +{
> +	if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
> +		return true;
> +
> +	if (transparent_hugepage_flags
> +			& (1 << TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG))
> +		/* check vma flags */;
> +	else
> +		return false;
> +
> +	if ((vma->vm_flags & (VM_HUGEPAGE | VM_NOHUGEPAGE)) == VM_HUGEPAGE
> +			&& !is_vma_temporary_stack(vma))
> +		return true;
> +
> +	return false;
> +}

I don't think that these are equivalent.  Here is the logic from the macro,
with whitespace added so things are more readable:

#define transparent_hugepage_enabled(__vma)
(
	(
	  transparent_hugepage_flags & (1<<TRANSPARENT_HUGEPAGE_FLAG)

	  ||

	  (
	    transparent_hugepage_flags & (1<<TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG)

	    &&

	    ((__vma)->vm_flags & VM_HUGEPAGE)
	  )
	)
	 
	 &&

	 !((__vma)->vm_flags & VM_NOHUGEPAGE)
	 
	 &&

	 !is_vma_temporary_stack(__vma)
)

So, if the VM_NOHUGEPAGE flag is set or if the vma is for a temporary stack,
we always bail.  Also, we only care about the VM_HUGEPAGE flag in the presence
of TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG.

I think this static inline is logically equivalent (untested):

static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
{
	if ((vma->vm_flags & VM_NOHUGEPAGE) || is_vma_temporary_stack(vma))
		return false;

	if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
		return true;

	if ((transparent_hugepage_flags &
				(1 << TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG))
			&& vma->vm_flags & VM_HUGEPAGE)
		return true;

	return false;
}

The ordering of the checks is different, but we're not using && or || to
short-circuit checks with side effects, so I think it is more readable and
should be fine.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] mm: improve readability of transparent_hugepage_enabled()

[Dan Williams]

On Tue, Jun 13, 2017 at 2:06 PM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Sat, Jun 10, 2017 at 02:49:31PM -0700, Dan Williams wrote:
>> Turn the macro into a static inline and rewrite the condition checks for
>> better readability in preparation for adding another condition.
>>
>> Cc: Jan Kara <jack@suse.cz>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  include/linux/huge_mm.h |   35 ++++++++++++++++++++++++-----------
>>  1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>> index a3762d49ba39..c4706e2c3358 100644
>> --- a/include/linux/huge_mm.h
>> +++ b/include/linux/huge_mm.h
>> @@ -85,14 +85,26 @@ extern struct kobj_attribute shmem_enabled_attr;
>>
>>  extern bool is_vma_temporary_stack(struct vm_area_struct *vma);
>>
>> -#define transparent_hugepage_enabled(__vma)                          \
>> -     ((transparent_hugepage_flags &                                  \
>> -       (1<<TRANSPARENT_HUGEPAGE_FLAG) ||                             \
>> -       (transparent_hugepage_flags &                                 \
>> -        (1<<TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG) &&                   \
>> -        ((__vma)->vm_flags & VM_HUGEPAGE))) &&                       \
>> -      !((__vma)->vm_flags & VM_NOHUGEPAGE) &&                        \
>> -      !is_vma_temporary_stack(__vma))
>> +extern unsigned long transparent_hugepage_flags;
>> +
>> +static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
>> +{
>> +     if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
>> +             return true;
>> +
>> +     if (transparent_hugepage_flags
>> +                     & (1 << TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG))
>> +             /* check vma flags */;
>> +     else
>> +             return false;
>> +
>> +     if ((vma->vm_flags & (VM_HUGEPAGE | VM_NOHUGEPAGE)) == VM_HUGEPAGE
>> +                     && !is_vma_temporary_stack(vma))
>> +             return true;
>> +
>> +     return false;
>> +}
>
> I don't think that these are equivalent.  Here is the logic from the macro,
> with whitespace added so things are more readable:
>
> #define transparent_hugepage_enabled(__vma)
> (
>         (
>           transparent_hugepage_flags & (1<<TRANSPARENT_HUGEPAGE_FLAG)
>
>           ||
>
>           (
>             transparent_hugepage_flags & (1<<TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG)
>
>             &&
>
>             ((__vma)->vm_flags & VM_HUGEPAGE)
>           )
>         )
>
>          &&
>
>          !((__vma)->vm_flags & VM_NOHUGEPAGE)
>
>          &&
>
>          !is_vma_temporary_stack(__vma)

Yeah, good catch I had read the VM_NOHUGEPAGE flag as being relative
to the REQ_MADV_FLAG.

> )
>
> So, if the VM_NOHUGEPAGE flag is set or if the vma is for a temporary stack,
> we always bail.  Also, we only care about the VM_HUGEPAGE flag in the presence
> of TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG.
>
> I think this static inline is logically equivalent (untested):
>
> static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
> {
>         if ((vma->vm_flags & VM_NOHUGEPAGE) || is_vma_temporary_stack(vma))
>                 return false;
>
>         if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
>                 return true;
>
>         if ((transparent_hugepage_flags &
>                                 (1 << TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG))
>                         && vma->vm_flags & VM_HUGEPAGE)
>                 return true;

We can clean this up a bit and do:

   return !!(vma->vm_flags & VM_HUGEPAGE)

...to drop the &&


>         return false;
> }
>
> The ordering of the checks is different, but we're not using && or || to
> short-circuit checks with side effects, so I think it is more readable and
> should be fine.

Agreed.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] mm: improve readability of transparent_hugepage_enabled()

[Ross Zwisler]

On Tue, Jun 13, 2017 at 02:16:49PM -0700, Dan Williams wrote:
> On Tue, Jun 13, 2017 at 2:06 PM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > So, if the VM_NOHUGEPAGE flag is set or if the vma is for a temporary stack,
> > we always bail.  Also, we only care about the VM_HUGEPAGE flag in the presence
> > of TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG.
> >
> > I think this static inline is logically equivalent (untested):
> >
> > static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
> > {
> >         if ((vma->vm_flags & VM_NOHUGEPAGE) || is_vma_temporary_stack(vma))
> >                 return false;
> >
> >         if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
> >                 return true;
> >
> >         if ((transparent_hugepage_flags &
> >                                 (1 << TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG))
> >                         && vma->vm_flags & VM_HUGEPAGE)
> >                 return true;
> 
> We can clean this up a bit and do:
> 
>    return !!(vma->vm_flags & VM_HUGEPAGE)
> 
> ...to drop the &&

Sure, that'll read better.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] mm: always enable thp for dax mappings

[Ross Zwisler]

On Mon, Jun 12, 2017 at 07:47:19AM -0700, Dan Williams wrote:
> On Mon, Jun 12, 2017 at 5:07 AM, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> > On Sat, Jun 10, 2017 at 02:49:37PM -0700, Dan Williams wrote:
> >> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >> index c4706e2c3358..901ed3767d1b 100644
> >> --- a/include/linux/huge_mm.h
> >> +++ b/include/linux/huge_mm.h
> >> @@ -1,6 +1,8 @@
> >>  #ifndef _LINUX_HUGE_MM_H
> >>  #define _LINUX_HUGE_MM_H
> >>
> >> +#include <linux/fs.h>
> >> +
> >
> > It means <linux/mm.h> now depends on <linux/fs.h>. I don't think it's a
> > good idea.
> 
> Seems to be ok as far as 0day-kbuild-robot is concerned. The
> alternative is to move vma_is_dax() out of line. I think
> transparent_hugepage_enabled() is called frequently enough to make it
> worth it to keep it inline.

Yea, I played with moving vma_is_dax() to include/linux/mm.h instead, but ran
into the issue where IS_DAX() is defined in include/linux/fs.h.  So, any way
we slice it we end up requiring both MM and FS includes for this to work.

Since the way you have it here apparently works and passes zero-day, my vote
is to just go with it.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm