| Message ID | 149713137177.17377.6712234218256825718.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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.
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.
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.
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) {}
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(-)