On Tue, Jun 13, 2017 at 2:06 PM, Ross Zwisler
<[email protected]> 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 <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: Ross Zwisler <[email protected]>
>> Cc: "Kirill A. Shutemov" <[email protected]>
>> Signed-off-by: Dan Williams <[email protected]>
>> ---
>> 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
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
