Re: [v3 PATCH 2/2] mm: thp: fix false negative of shmem vma's THP eligibility

[Vlastimil Babka <vbabka@suse.cz>]

On 6/13/19 6:44 AM, Yang Shi wrote:
> The commit 7635d9cbe832 ("mm, thp, proc: report THP eligibility for each
> vma") introduced THPeligible bit for processes' smaps. But, when checking
> the eligibility for shmem vma, __transparent_hugepage_enabled() is
> called to override the result from shmem_huge_enabled().  It may result
> in the anonymous vma's THP flag override shmem's.  For example, running a
> simple test which create THP for shmem, but with anonymous THP disabled,
> when reading the process's smaps, it may show:

...

> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 01d4eb0..6a13882 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -796,7 +796,8 @@ static int show_smap(struct seq_file *m, void *v)
>  
>  	__show_smap(m, &mss);
>  
> -	seq_printf(m, "THPeligible:    %d\n", transparent_hugepage_enabled(vma));
> +	seq_printf(m, "THPeligible:		%d\n",
> +		   transparent_hugepage_enabled(vma));
>  
>  	if (arch_pkeys_enabled())
>  		seq_printf(m, "ProtectionKey:  %8u\n", vma_pkey(vma));
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 4bc2552..36f0225 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -65,10 +65,15 @@
>  
>  bool transparent_hugepage_enabled(struct vm_area_struct *vma)
>  {
> +	/* The addr is used to check if the vma size fits */
> +	unsigned long addr = (vma->vm_end & HPAGE_PMD_MASK) - HPAGE_PMD_SIZE;
> +
> +	if (!transhuge_vma_suitable(vma, addr))
> +		return false;

Sorry for replying rather late, and not in the v2 thread, but unlike
Hugh I'm not convinced that we should include vma size/alignment in the
test for reporting THPeligible, which was supposed to reflect
administrative settings and madvise hints. I guess it's mostly a matter
of personal feeling. But one objective distinction is that the admin
settings and madvise do have an exact binary result for the whole VMA,
while this check is more fuzzy - only part of the VMA's span might be
properly sized+aligned, and THPeligible will be 1 for the whole VMA.

>  	if (vma_is_anonymous(vma))
>  		return __transparent_hugepage_enabled(vma);
> -	if (vma_is_shmem(vma) && shmem_huge_enabled(vma))
> -		return __transparent_hugepage_enabled(vma);
> +	if (vma_is_shmem(vma))
> +		return shmem_huge_enabled(vma);
>  
>  	return false;
>  }
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 1bb3b8d..a807712 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3872,6 +3872,9 @@ bool shmem_huge_enabled(struct vm_area_struct *vma)
>  	loff_t i_size;
>  	pgoff_t off;
>  
> +	if ((vma->vm_flags & VM_NOHUGEPAGE) ||
> +	    test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
> +		return false;
>  	if (shmem_huge == SHMEM_HUGE_FORCE)
>  		return true;
>  	if (shmem_huge == SHMEM_HUGE_DENY)
>