Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [patch v2 for-4.20] Revert "mm, thp: consolidate THP gfp handling into alloc_hugepage_direct_gfpmask" | From | Vlastimil Babka <> | Date | Sat, 8 Dec 2018 11:02:36 +0100 |
| |
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@google.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.
| ![\](/images/icornerr.gif) |