| Message ID | 149739530612.20686.14760671150202647861.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue 13-06-17 16:08:26, 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> > Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> > [ross: fix logic to make conversion equivalent] > Acked-by: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Looks good. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > include/linux/huge_mm.h | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > index a3762d49ba39..c8119e856eb1 100644 > --- a/include/linux/huge_mm.h > +++ b/include/linux/huge_mm.h > @@ -85,14 +85,23 @@ 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 ((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)) > + return !!(vma->vm_flags & VM_HUGEPAGE); > + > + return false; > +} > + > #define transparent_hugepage_use_zero_page() \ > (transparent_hugepage_flags & \ > (1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG)) > @@ -104,8 +113,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 +230,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) {} > >
On Tue 13-06-17 16:08:26, 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> > Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> > [ross: fix logic to make conversion equivalent] > Acked-by: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> This is really a nice deobfuscation! Please note this will conflict with http://lkml.kernel.org/r/1496415802-30944-1-git-send-email-rppt@linux.vnet.ibm.com Trivial to resolve but I thought I should give you a heads up. Acked-by: Michal Hocko <mhocko@suse.com> > --- > include/linux/huge_mm.h | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > index a3762d49ba39..c8119e856eb1 100644 > --- a/include/linux/huge_mm.h > +++ b/include/linux/huge_mm.h > @@ -85,14 +85,23 @@ 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 ((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)) > + return !!(vma->vm_flags & VM_HUGEPAGE); > + > + return false; > +} > + > #define transparent_hugepage_use_zero_page() \ > (transparent_hugepage_flags & \ > (1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG)) > @@ -104,8 +113,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 +230,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) {} > > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
On 06/14/2017 01:08 AM, 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> > Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> > [ross: fix logic to make conversion equivalent] > Acked-by: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> vbabka@gusiac:~/wrk/cbmc> cbmc test-thp.c CBMC version 5.3 64-bit x86_64 linux Parsing test-thp.c file <command-line> line 0: <command-line>:0:0: warning: "__STDC_VERSION__" redefined file <command-line> line 0: <built-in>: note: this is the location of the previous definition Converting Type-checking test-thp file test-thp.c line 75 function main: function `assert' is not declared Generating GOTO Program Adding CPROVER library Function Pointer Removal Partial Inlining Generic Property Instrumentation Starting Bounded Model Checking size of program expression: 171 steps simple slicing removed 3 assignments Generated 1 VCC(s), 1 remaining after simplification Passing problem to propositional reduction converting SSA Running propositional reduction Post-processing Solving with MiniSAT 2.2.0 with simplifier 4899 variables, 13228 clauses SAT checker: negated claim is UNSATISFIABLE, i.e., holds Runtime decision procedure: 0.008s VERIFICATION SUCCESSFUL (and yeah, the v1 version fails :) > --- > include/linux/huge_mm.h | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > index a3762d49ba39..c8119e856eb1 100644 > --- a/include/linux/huge_mm.h > +++ b/include/linux/huge_mm.h > @@ -85,14 +85,23 @@ 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 ((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)) > + return !!(vma->vm_flags & VM_HUGEPAGE); > + > + return false; > +} > + > #define transparent_hugepage_use_zero_page() \ > (transparent_hugepage_flags & \ > (1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG)) > @@ -104,8 +113,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 +230,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) {} > > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> >
On Wed, Jun 14, 2017 at 9:53 AM, Vlastimil Babka <vbabka@suse.cz> wrote: > On 06/14/2017 01:08 AM, 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> >> Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> >> [ross: fix logic to make conversion equivalent] >> Acked-by: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > > vbabka@gusiac:~/wrk/cbmc> cbmc test-thp.c > CBMC version 5.3 64-bit x86_64 linux > Parsing test-thp.c > file <command-line> line 0: <command-line>:0:0: warning: > "__STDC_VERSION__" redefined > file <command-line> line 0: <built-in>: note: this is the location of > the previous definition > Converting > Type-checking test-thp > file test-thp.c line 75 function main: function `assert' is not declared > Generating GOTO Program > Adding CPROVER library > Function Pointer Removal > Partial Inlining > Generic Property Instrumentation > Starting Bounded Model Checking > size of program expression: 171 steps > simple slicing removed 3 assignments > Generated 1 VCC(s), 1 remaining after simplification > Passing problem to propositional reduction > converting SSA > Running propositional reduction > Post-processing > Solving with MiniSAT 2.2.0 with simplifier > 4899 variables, 13228 clauses > SAT checker: negated claim is UNSATISFIABLE, i.e., holds > Runtime decision procedure: 0.008s > VERIFICATION SUCCESSFUL > > (and yeah, the v1 version fails :) Can you share the test-thp.c so I can add this to my test collection? I'm assuming cbmc is "Bounded Model Checker for C/C++"?
On 06/14/2017 07:02 PM, Dan Williams wrote: > On Wed, Jun 14, 2017 at 9:53 AM, Vlastimil Babka <vbabka@suse.cz> wrote: > > Can you share the test-thp.c so I can add this to my test collection? Attached. > I'm assuming cbmc is "Bounded Model Checker for C/C++"? Yes. This blog from Paul inspired me: http://paulmck.livejournal.com/38997.html Works nicely, just if it finds a bug, the counterexamples are a bit of PITA to decipher :) Vlastimil
On Wed, Jun 14, 2017 at 5:45 AM, Michal Hocko <mhocko@kernel.org> wrote: > On Tue 13-06-17 16:08:26, 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> >> Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> >> [ross: fix logic to make conversion equivalent] >> Acked-by: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > This is really a nice deobfuscation! Please note this will conflict with > http://lkml.kernel.org/r/1496415802-30944-1-git-send-email-rppt@linux.vnet.ibm.com > > > Trivial to resolve but I thought I should give you a heads up. Hmm, I'm assuming that vma_is_dax() should override PRCTL_THP_DISABLE? ...and while we're there should vma_is_dax() also override VM_NOHUGEPAGE? This is with the assumption that the reason to turn off huge pages is to avoid mm pressure, dax exerts no such pressure. > Acked-by: Michal Hocko <mhocko@suse.com> Thanks for the heads up.
On Wed 14-06-17 12:26:46, Dan Williams wrote: > On Wed, Jun 14, 2017 at 5:45 AM, Michal Hocko <mhocko@kernel.org> wrote: > > On Tue 13-06-17 16:08:26, 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> > >> Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> > >> [ross: fix logic to make conversion equivalent] > >> Acked-by: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > > This is really a nice deobfuscation! Please note this will conflict with > > http://lkml.kernel.org/r/1496415802-30944-1-git-send-email-rppt@linux.vnet.ibm.com > > > > > > Trivial to resolve but I thought I should give you a heads up. > > Hmm, I'm assuming that vma_is_dax() should override PRCTL_THP_DISABLE? > ...and while we're there should vma_is_dax() also override > VM_NOHUGEPAGE? This is with the assumption that the reason to turn off > huge pages is to avoid mm pressure, dax exerts no such pressure. As the changelog of the referenced patch says another reason is to stop khugepaged from interfering and collapsing smaller pages into THP. If DAX mappings are subject to khugepaged then we really need to exclude it. Why would you want to override user's decision to disable THP anyway? I can see why the global knob should be ignored but if the disable is targeted for the specific VMA or the process then we should obey that, no? > > Acked-by: Michal Hocko <mhocko@suse.com> > > Thanks for the heads up.
On Thu, 15 Jun 2017 10:07:39 +0200 Michal Hocko <mhocko@kernel.org> wrote: > On Wed 14-06-17 12:26:46, Dan Williams wrote: > > On Wed, Jun 14, 2017 at 5:45 AM, Michal Hocko <mhocko@kernel.org> wrote: > > > On Tue 13-06-17 16:08:26, 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> > > >> Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > >> [ross: fix logic to make conversion equivalent] > > >> Acked-by: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > > >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > > > > This is really a nice deobfuscation! Please note this will conflict with > > > http://lkml.kernel.org/r/1496415802-30944-1-git-send-email-rppt@linux.vnet.ibm.com > > > > > > > > > Trivial to resolve but I thought I should give you a heads up. > > > > Hmm, I'm assuming that vma_is_dax() should override PRCTL_THP_DISABLE? > > ...and while we're there should vma_is_dax() also override > > VM_NOHUGEPAGE? This is with the assumption that the reason to turn off > > huge pages is to avoid mm pressure, dax exerts no such pressure. > > As the changelog of the referenced patch says another reason is to stop > khugepaged from interfering and collapsing smaller pages into THP. If > DAX mappings are subject to khugepaged then we really need to exclude > it. Why would you want to override user's decision to disable THP > anyway? I can see why the global knob should be ignored but if the > disable is targeted for the specific VMA or the process then we should > obey that, no? So... Like this? static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma) { if (vma->vm_flags & VM_NOHUGEPAGE)) return false; if (is_vma_temporary_stack(vma)) return false; if (test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags)) return false; if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG)) return true; if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG)) return !!(vma->vm_flags & VM_HUGEPAGE); return false; }
On Thu, Jun 15, 2017 at 1:07 AM, Michal Hocko <mhocko@kernel.org> wrote: > On Wed 14-06-17 12:26:46, Dan Williams wrote: >> On Wed, Jun 14, 2017 at 5:45 AM, Michal Hocko <mhocko@kernel.org> wrote: >> > On Tue 13-06-17 16:08:26, 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> >> >> Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> >> >> [ross: fix logic to make conversion equivalent] >> >> Acked-by: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> >> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> > >> > This is really a nice deobfuscation! Please note this will conflict with >> > http://lkml.kernel.org/r/1496415802-30944-1-git-send-email-rppt@linux.vnet.ibm.com >> > >> > >> > Trivial to resolve but I thought I should give you a heads up. >> >> Hmm, I'm assuming that vma_is_dax() should override PRCTL_THP_DISABLE? >> ...and while we're there should vma_is_dax() also override >> VM_NOHUGEPAGE? This is with the assumption that the reason to turn off >> huge pages is to avoid mm pressure, dax exerts no such pressure. > > As the changelog of the referenced patch says another reason is to stop > khugepaged from interfering and collapsing smaller pages into THP. If > DAX mappings are subject to khugepaged then we really need to exclude > it. Why would you want to override user's decision to disable THP > anyway? I can see why the global knob should be ignored but if the > disable is targeted for the specific VMA or the process then we should > obey that, no? I don't think DAX mappings have any interaction with THP machinery outside of piggybacking on some of the paths in the fault handling and the helpers to manage huge page table entries. Since DAX disables the page cache, and all DAX mappings are file-backed I don't see a need for a user to disable THP... does anybody else? I think DAX != THP for any of the cases that transparent_hugepage_enabled() cares about.
On Thu 15-06-17 13:21:46, Dan Williams wrote: > On Thu, Jun 15, 2017 at 1:07 AM, Michal Hocko <mhocko@kernel.org> wrote: > > On Wed 14-06-17 12:26:46, Dan Williams wrote: > >> On Wed, Jun 14, 2017 at 5:45 AM, Michal Hocko <mhocko@kernel.org> wrote: > >> > On Tue 13-06-17 16:08:26, 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> > >> >> Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> > >> >> [ross: fix logic to make conversion equivalent] > >> >> Acked-by: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > >> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> > >> > > >> > This is really a nice deobfuscation! Please note this will conflict with > >> > http://lkml.kernel.org/r/1496415802-30944-1-git-send-email-rppt@linux.vnet.ibm.com > >> > > >> > > >> > Trivial to resolve but I thought I should give you a heads up. > >> > >> Hmm, I'm assuming that vma_is_dax() should override PRCTL_THP_DISABLE? > >> ...and while we're there should vma_is_dax() also override > >> VM_NOHUGEPAGE? This is with the assumption that the reason to turn off > >> huge pages is to avoid mm pressure, dax exerts no such pressure. > > > > As the changelog of the referenced patch says another reason is to stop > > khugepaged from interfering and collapsing smaller pages into THP. If > > DAX mappings are subject to khugepaged then we really need to exclude > > it. Why would you want to override user's decision to disable THP > > anyway? I can see why the global knob should be ignored but if the > > disable is targeted for the specific VMA or the process then we should > > obey that, no? > > I don't think DAX mappings have any interaction with THP machinery > outside of piggybacking on some of the paths in the fault handling and > the helpers to manage huge page table entries. Since DAX disables the > page cache, and all DAX mappings are file-backed I don't see a need > for a user to disable THP... does anybody else? So let me ask differently. If the VMA is explicitly marked to not use THP resp. the process explicitly asked to be opted out from THP why should we make any exception for DAX? What makes DAX so special to ignore what an user asked for?
On Thu 15-06-17 13:06:58, Andrew Morton wrote: > On Thu, 15 Jun 2017 10:07:39 +0200 Michal Hocko <mhocko@kernel.org> wrote: > > > On Wed 14-06-17 12:26:46, Dan Williams wrote: > > > On Wed, Jun 14, 2017 at 5:45 AM, Michal Hocko <mhocko@kernel.org> wrote: > > > > On Tue 13-06-17 16:08:26, 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> > > > >> Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > > >> [ross: fix logic to make conversion equivalent] > > > >> Acked-by: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > > > >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > > > > > > This is really a nice deobfuscation! Please note this will conflict with > > > > http://lkml.kernel.org/r/1496415802-30944-1-git-send-email-rppt@linux.vnet.ibm.com > > > > > > > > > > > > Trivial to resolve but I thought I should give you a heads up. > > > > > > Hmm, I'm assuming that vma_is_dax() should override PRCTL_THP_DISABLE? > > > ...and while we're there should vma_is_dax() also override > > > VM_NOHUGEPAGE? This is with the assumption that the reason to turn off > > > huge pages is to avoid mm pressure, dax exerts no such pressure. > > > > As the changelog of the referenced patch says another reason is to stop > > khugepaged from interfering and collapsing smaller pages into THP. If > > DAX mappings are subject to khugepaged then we really need to exclude > > it. Why would you want to override user's decision to disable THP > > anyway? I can see why the global knob should be ignored but if the > > disable is targeted for the specific VMA or the process then we should > > obey that, no? > > So... Like this? > > static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma) > { > if (vma->vm_flags & VM_NOHUGEPAGE)) > return false; > > if (is_vma_temporary_stack(vma)) > return false; > > if (test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags)) > return false; > > if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG)) > return true; > > if (transparent_hugepage_flags & > (1 << TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG)) > return !!(vma->vm_flags & VM_HUGEPAGE); > > return false; > } yes
On Thu, Jun 15, 2017 at 3:22 PM, Michal Hocko <mhocko@kernel.org> wrote: > On Thu 15-06-17 13:21:46, Dan Williams wrote: >> On Thu, Jun 15, 2017 at 1:07 AM, Michal Hocko <mhocko@kernel.org> wrote: >> > On Wed 14-06-17 12:26:46, Dan Williams wrote: >> >> On Wed, Jun 14, 2017 at 5:45 AM, Michal Hocko <mhocko@kernel.org> wrote: >> >> > On Tue 13-06-17 16:08:26, 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> >> >> >> Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> >> >> >> [ross: fix logic to make conversion equivalent] >> >> >> Acked-by: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> >> >> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> >> > >> >> > This is really a nice deobfuscation! Please note this will conflict with >> >> > http://lkml.kernel.org/r/1496415802-30944-1-git-send-email-rppt@linux.vnet.ibm.com >> >> > >> >> > >> >> > Trivial to resolve but I thought I should give you a heads up. >> >> >> >> Hmm, I'm assuming that vma_is_dax() should override PRCTL_THP_DISABLE? >> >> ...and while we're there should vma_is_dax() also override >> >> VM_NOHUGEPAGE? This is with the assumption that the reason to turn off >> >> huge pages is to avoid mm pressure, dax exerts no such pressure. >> > >> > As the changelog of the referenced patch says another reason is to stop >> > khugepaged from interfering and collapsing smaller pages into THP. If >> > DAX mappings are subject to khugepaged then we really need to exclude >> > it. Why would you want to override user's decision to disable THP >> > anyway? I can see why the global knob should be ignored but if the >> > disable is targeted for the specific VMA or the process then we should >> > obey that, no? >> >> I don't think DAX mappings have any interaction with THP machinery >> outside of piggybacking on some of the paths in the fault handling and >> the helpers to manage huge page table entries. Since DAX disables the >> page cache, and all DAX mappings are file-backed I don't see a need >> for a user to disable THP... does anybody else? > > So let me ask differently. If the VMA is explicitly marked to not use > THP resp. the process explicitly asked to be opted out from THP why > should we make any exception for DAX? What makes DAX so special to > ignore what an user asked for? Hmm, the only case where this is a problem is in the device-dax case where it tries to guarantee a given fault granularity. In the filesystem-dax case it will fall back to 4K mappings which is expected, device-dax will just fail which is not. However, I think I can solve this just with better device-dax documentation that highlights this side-effect of disabling THP when the device is configured to support a minimum TLB size that is greater than PAGE_SIZE.
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h index a3762d49ba39..c8119e856eb1 100644 --- a/include/linux/huge_mm.h +++ b/include/linux/huge_mm.h @@ -85,14 +85,23 @@ 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 ((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)) + return !!(vma->vm_flags & VM_HUGEPAGE); + + return false; +} + #define transparent_hugepage_use_zero_page() \ (transparent_hugepage_flags & \ (1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG)) @@ -104,8 +113,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 +230,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) {}