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

[Dan Williams <dan.j.williams@intel.com>]

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.

--
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>