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

[Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx>]

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

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.