linux-kernel - Re: [patch v2 for-4.20] Revert "mm, thp: consolidate THP gfp handling into alloc_hugepage_direct

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] [day] [month] [year] [list]

Message-ID: <4ffce3ba-2a80-4d57-876e-32a6b7eae57f@suse.cz>
Date:   Sat, 8 Dec 2018 11:02:36 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     David Rientjes <rientjes@...gle.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andrea Arcangeli <aarcange@...hat.com>,
        mgorman@...hsingularity.net, Michal Hocko <mhocko@...nel.org>,
        ying.huang@...el.com, s.priebe@...fihost.ag,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        alex.williamson@...hat.com, lkp@...org, kirill@...temov.name,
        Andrew Morton <akpm@...ux-foundation.org>,
        zi.yan@...rutgers.edu
Subject: Re: [patch v2 for-4.20] Revert "mm, thp: consolidate THP gfp handling
 into alloc_hugepage_direct_gfpmask"

On 12/7/18 11:50 PM, David Rientjes wrote:
> This reverts commit 89c83fb539f95491be80cdd5158e6f0ce329e317.
> 
> This should have been done as part of 2f0799a0ffc0 ("mm, thp: restore
> node-local hugepage allocations").  The movement of the thp allocation
> policy from alloc_pages_vma() to alloc_hugepage_direct_gfpmask() was
> intended to only set __GFP_THISNODE for mempolicies that are not
> MPOL_BIND whereas the revert could set this regardless of mempolicy.
> 
> While the check for MPOL_BIND between alloc_hugepage_direct_gfpmask()
> and alloc_pages_vma() was racy, that has since been removed since the

I would have expected mmap_sem to prevent the race, as faults have it
locked for read and updating mempolicies for write, IIRC? But didn't
check in detail.

> revert.  What is left is the possibility to use __GFP_THISNODE in
> policy_node() when it is unexpected because the special handling for
> hugepages in alloc_pages_vma()  was removed as part of the consolidation.

Yeah that was a bug.

> Secondly, prior to 89c83fb539f9, alloc_pages_vma() implemented a somewhat 
> different policy for hugepage allocations, which were allocated through 
> alloc_hugepage_vma().  For hugepage allocations, if the allocating 
> process's node is in the set of allowed nodes, allocate with 
> __GFP_THISNODE for that node (for MPOL_PREFERRED, use that node with 
> __GFP_THISNODE instead).  This was changed for shmem_alloc_hugepage() to 
> allow fallback to other nodes in 89c83fb539f9 as it did for new_page() in 
> mm/mempolicy.c which is functionally different behavior and removes the 
> requirement to only allocate hugepages locally.

TBH this slight difference was known and stated in the changelog of
89c83fb539f9 so you could have objected.

> So this commit does a full revert of 89c83fb539f9 instead of the partial
> revert that was done in 2f0799a0ffc0.  The result is the same thp
> allocation policy for 4.20 that was in 4.19.
> 
> Fixes: 89c83fb539f9 ("mm, thp: consolidate THP gfp handling into alloc_hugepage_direct_gfpmask")
> Fixes: 2f0799a0ffc0 ("mm, thp: restore node-local hugepage allocations")
> Signed-off-by: David Rientjes <rientjes@...gle.com>
> ---
>  This indeed restores the thp allocation policy fully to what it was in
>  4.19 since there is obivously more discussion to be had about how the
>  NUMA aspects of thp allocations should be addressed.  We can do this
>  with a stable 4.20 tree in the background that has the same allocation
>  policy that was in 4.0.

I agree that this is probably the safest option for now so that the next
rc doesn't contain the warning introduced in 2f0799a0ffc0.