linux-kernel - Re: [v3 PATCH 1/2] mm: thp: make transhuge_vma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 17 Jul 2019 12:43:16 -0700 (PDT)
From:   Hugh Dickins <hughd@...gle.com>
To:     Yang Shi <yang.shi@...ux.alibaba.com>
cc:     hughd@...gle.com, kirill.shutemov@...ux.intel.com, mhocko@...e.com,
        vbabka@...e.cz, rientjes@...gle.com, akpm@...ux-foundation.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [v3 PATCH 1/2] mm: thp: make transhuge_vma_suitable available
 for anonymous THP

On Thu, 13 Jun 2019, Yang Shi wrote:

> The transhuge_vma_suitable() was only available for shmem THP, but
> anonymous THP has the same check except pgoff check.  And, it will be
> used for THP eligible check in the later patch, so make it available for
> all kind of THPs.  This also helps reduce code duplication slightly.
> 
> Since anonymous THP doesn't have to check pgoff, so make pgoff check
> shmem vma only.

Yes, I think you are right to avoid the pgoff check on anonymous.
I had originally thought that it would work out okay even with the
pgoff check on anonymous, and usually it would: but could give the
wrong answer on an mremap-moved anonymous area.

> 
> Cc: Hugh Dickins <hughd@...gle.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> Cc: Michal Hocko <mhocko@...e.com>
> Cc: Vlastimil Babka <vbabka@...e.cz>
> Cc: David Rientjes <rientjes@...gle.com>
> Signed-off-by: Yang Shi <yang.shi@...ux.alibaba.com>

Almost Acked-by me, but there's one nit I'd much prefer to change:
sorry for being such a late nuisance...

> ---
>  mm/huge_memory.c |  2 +-
>  mm/internal.h    | 25 +++++++++++++++++++++++++
>  mm/memory.c      | 13 -------------
>  3 files changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 9f8bce9..4bc2552 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -691,7 +691,7 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
>  	struct page *page;
>  	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
>  
> -	if (haddr < vma->vm_start || haddr + HPAGE_PMD_SIZE > vma->vm_end)
> +	if (!transhuge_vma_suitable(vma, haddr))
>  		return VM_FAULT_FALLBACK;
>  	if (unlikely(anon_vma_prepare(vma)))
>  		return VM_FAULT_OOM;
> diff --git a/mm/internal.h b/mm/internal.h
> index 9eeaf2b..7f096ba 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -555,4 +555,29 @@ static inline bool is_migrate_highatomic_page(struct page *page)
>  
>  void setup_zone_pageset(struct zone *zone);
>  extern struct page *alloc_new_node_page(struct page *page, unsigned long node);
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +#define HPAGE_CACHE_INDEX_MASK (HPAGE_PMD_NR - 1)
> +static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
> +		unsigned long haddr)
> +{
> +	/* Don't have to check pgoff for anonymous vma */
> +	if (!vma_is_anonymous(vma)) {
> +		if (((vma->vm_start >> PAGE_SHIFT) & HPAGE_CACHE_INDEX_MASK) !=
> +			(vma->vm_pgoff & HPAGE_CACHE_INDEX_MASK))
> +			return false;
> +	}
> +
> +	if (haddr < vma->vm_start || haddr + HPAGE_PMD_SIZE > vma->vm_end)
> +		return false;
> +	return true;
> +}
> +#else
> +static inline bool transhuge_vma_suitable(struct vma_area_struct *vma,
> +		unsigned long haddr)
> +{
> +	return false;
> +}
> +#endif
> +
>  #endif	/* __MM_INTERNAL_H */

... maybe I'm just not much of a fan of mm/internal.h (where at last you
find odd bits and pieces which you had expected to find elsewhere), and
maybe others will disagree: but I'd say transhuge_vma_suitable() surely
belongs in include/linux/huge_mm.h, near __transparent_hugepage_enabled().

But then your correct use of vma_is_anonymous() gets more complicated:
because that declaration is over in include/linux/mm.h; and although
linux/mm.h includes linux/huge_mm.h, vma_is_anonymous() comes lower down.

However... linux/mm.h's definition of vma_set_anonymous() comes higher
up, and it would make perfect sense to move vma_is_anonymous up to just
after vma_set_anonymous(), wouldn't it?  Should vma_is_shmem() and
vma_is_stack_for_current() declarations move with it? Probably yes:
they make more sense near vma_is_anonymous() than where they were.

Hugh

> diff --git a/mm/memory.c b/mm/memory.c
> index 96f1d47..2286424 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3205,19 +3205,6 @@ static vm_fault_t pte_alloc_one_map(struct vm_fault *vmf)
>  }
>  
>  #ifdef CONFIG_TRANSPARENT_HUGE_PAGECACHE
> -
> -#define HPAGE_CACHE_INDEX_MASK (HPAGE_PMD_NR - 1)
> -static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
> -		unsigned long haddr)
> -{
> -	if (((vma->vm_start >> PAGE_SHIFT) & HPAGE_CACHE_INDEX_MASK) !=
> -			(vma->vm_pgoff & HPAGE_CACHE_INDEX_MASK))
> -		return false;
> -	if (haddr < vma->vm_start || haddr + HPAGE_PMD_SIZE > vma->vm_end)
> -		return false;
> -	return true;
> -}
> -
>  static void deposit_prealloc_pte(struct vm_fault *vmf)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
> -- 
> 1.8.3.1