On Tue, Jun 13, 2017 at 2:06 PM, Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx> 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@xxxxxxx> >> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> >> Cc: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx> >> Cc: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx> >> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> >> --- >> 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. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx"> email@xxxxxxxxx </a>