On Tue, 13 Aug 2024, Usama Arif wrote:
> The current upstream default policy for THP is always. However, Meta
> uses madvise in production as the current THP=always policy vastly
> overprovisions THPs in sparsely accessed memory areas, resulting in
> excessive memory pressure and premature OOM killing.
> Using madvise + relying on khugepaged has certain drawbacks over
> THP=always. Using madvise hints mean THPs aren't "transparent" and
> require userspace changes. Waiting for khugepaged to scan memory and
> collapse pages into THP can be slow and unpredictable in terms of performance
> (i.e. you dont know when the collapse will happen), while production
> environments require predictable performance. If there is enough memory
> available, its better for both performance and predictability to have
> a THP from fault time, i.e. THP=always rather than wait for khugepaged
> to collapse it, and deal with sparsely populated THPs when the system is
> running out of memory.
>
> This patch-series is an attempt to mitigate the issue of running out of
> memory when THP is always enabled. During runtime whenever a THP is being
> faulted in or collapsed by khugepaged, the THP is added to a list.
> Whenever memory reclaim happens, the kernel runs the deferred_split
> shrinker which goes through the list and checks if the THP was underutilized,
> i.e. how many of the base 4K pages of the entire THP were zero-filled.
> If this number goes above a certain threshold, the shrinker will attempt
> to split that THP. Then at remap time, the pages that were zero-filled are
> mapped to the shared zeropage, hence saving memory. This method avoids the
> downside of wasting memory in areas where THP is sparsely filled when THP
> is always enabled, while still providing the upside THPs like reduced TLB
> misses without having to use madvise.
>
> Meta production workloads that were CPU bound (>99% CPU utilzation) were
> tested with THP shrinker. The results after 2 hours are as follows:
>
> | THP=madvise | THP=always | THP=always
> | | | + shrinker series
> | | | + max_ptes_none=409
> -----------------------------------------------------------------------------
> Performance improvement | - | +1.8% | +1.7%
> (over THP=madvise) | | |
> -----------------------------------------------------------------------------
> Memory usage | 54.6G | 58.8G (+7.7%) | 55.9G (+2.4%)
> -----------------------------------------------------------------------------
> max_ptes_none=409 means that any THP that has more than 409 out of 512
> (80%) zero filled filled pages will be split.
>
> To test out the patches, the below commands without the shrinker will
> invoke OOM killer immediately and kill stress, but will not fail with
> the shrinker:
>
> echo 450 > /sys/kernel/mm/transparent_hugepage/khugepaged/max_ptes_none
> mkdir /sys/fs/cgroup/test
> echo $$ > /sys/fs/cgroup/test/cgroup.procs
> echo 20M > /sys/fs/cgroup/test/memory.max
> echo 0 > /sys/fs/cgroup/test/memory.swap.max
> # allocate twice memory.max for each stress worker and touch 40/512 of
> # each THP, i.e. vm-stride 50K.
> # With the shrinker, max_ptes_none of 470 and below won't invoke OOM
> # killer.
> # Without the shrinker, OOM killer is invoked immediately irrespective
> # of max_ptes_none value and kills stress.
> stress --vm 1 --vm-bytes 40M --vm-stride 50K
>
> v2 -> v3:
> - Use my_zero_pfn instead of page_to_pfn(ZERO_PAGE(..)) (Johannes)
> - Use flags argument instead of bools in remove_migration_ptes (Johannes)
> - Use a new flag in folio->_flags_1 instead of folio->_partially_mapped
> (David Hildenbrand).
> - Split out the last patch of v2 into 3, one for introducing the flag,
> one for splitting underutilized THPs on _deferred_list and one for adding
> sysfs entry to disable splitting (David Hildenbrand).
>
> v1 -> v2:
> - Turn page checks and operations to folio versions in __split_huge_page.
> This means patches 1 and 2 from v1 are no longer needed.
> (David Hildenbrand)
> - Map to shared zeropage in all cases if the base page is zero-filled.
> The uffd selftest was removed.
> (David Hildenbrand).
> - rename 'dirty' to 'contains_data' in try_to_map_unused_to_zeropage
> (Rik van Riel).
> - Use unsigned long instead of uint64_t (kernel test robot).
>
> Alexander Zhu (1):
> mm: selftest to verify zero-filled pages are mapped to zeropage
>
> Usama Arif (3):
> mm: Introduce a pageflag for partially mapped folios
> mm: split underutilized THPs
> mm: add sysfs entry to disable splitting underutilized THPs
>
> Yu Zhao (2):
> mm: free zapped tail pages when splitting isolated thp
> mm: remap unused subpages to shared zeropage when splitting isolated
> thp
Sorry, I don't have time to review this, but notice you're intending
a v4 of the series, so want to bring up four points quickly before that.
1. Even with the two fixes to 1/6 in __split_huge_page(), under load
this series causes system lockup, with interrupts disabled on most CPUs.
The error is in deferred_split_scan(), where the old code just did
a list_splice_tail() under split_queue_lock, but this series ends up
doing more there, including a folio_put(): deadlock when racing, and
that is the final folio_put() which brings refcount down to 0, which
then wants to take split_queue_lock.
The patch I've been using successfully on 6.11-rc3-next-20240816 below:
I do have other problems with current mm commits, so have not been able
to sustain a load for very long, but suspect those problems unrelated
to this series. Please fold this fix, or your own equivalent, into
your next version.
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3270,7 +3270,8 @@ static void __split_huge_page(struct pag
folio_clear_active(new_folio);
folio_clear_unevictable(new_folio);
- if (!folio_batch_add(&free_folios, folio)) {
+ list_del(&new_folio->lru);
+ if (!folio_batch_add(&free_folios, new_folio)) {
mem_cgroup_uncharge_folios(&free_folios);
free_unref_folios(&free_folios);
}
@@ -3706,42 +3707,37 @@ static unsigned long deferred_split_scan
bool did_split = false;
bool underutilized = false;
- if (folio_test_partially_mapped(folio))
- goto split;
- underutilized = thp_underutilized(folio);
- if (underutilized)
- goto split;
- continue;
-split:
+ if (!folio_test_partially_mapped(folio)) {
+ underutilized = thp_underutilized(folio);
+ if (!underutilized)
+ goto next;
+ }
if (!folio_trylock(folio))
- continue;
- did_split = !split_folio(folio);
- folio_unlock(folio);
- if (did_split) {
- /* Splitting removed folio from the list, drop reference here */
- folio_put(folio);
+ goto next;
+ if (!split_folio(folio)) {
+ did_split = true;
if (underutilized)
count_vm_event(THP_UNDERUTILIZED_SPLIT_PAGE);
split++;
}
- }
-
- spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
- /*
- * Only add back to the queue if folio is partially mapped.
- * If thp_underutilized returns false, or if split_folio fails in
- * the case it was underutilized, then consider it used and don't
- * add it back to split_queue.
- */
- list_for_each_entry_safe(folio, next, &list, _deferred_list) {
- if (folio_test_partially_mapped(folio))
- list_move(&folio->_deferred_list, &ds_queue->split_queue);
- else {
+ folio_unlock(folio);
+next:
+ /*
+ * split_folio() removes folio from list on success.
+ * Only add back to the queue if folio is partially mapped.
+ * If thp_underutilized returns false, or if split_folio fails
+ * in the case it was underutilized, then consider it used and
+ * don't add it back to split_queue.
+ */
+ if (!did_split && !folio_test_partially_mapped(folio)) {
list_del_init(&folio->_deferred_list);
ds_queue->split_queue_len--;
}
folio_put(folio);
}
+
+ spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
+ list_splice_tail(&list, &ds_queue->split_queue);
spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
/*
2. I don't understand why there needs to be a new PG_partially_mapped
flag, with all its attendant sets and tests and clears all over. Why
can't deferred_split_scan() detect that case for itself, using the
criteria from __folio_remove_rmap()? I see folio->_nr_pages_mapped
is commented "Do not use outside of rmap and debug code", and
folio_nr_pages_mapped() is currently only used from mm/debug.c; but
using the info already maintained is preferable to adding a PG_flag
(and perhaps more efficient - skips splitting when _nr_pages_mapped
already fell to 0 and folio will soon be freed).
3. Everything in /sys/kernel/mm/transparent_hugepage/ is about THPs,
so please remove the "thp_" from "thp_low_util_shrinker" -
"shrink_underused" perhaps. And it needs a brief description in
Documentation/admin-guide/mm/transhuge.rst.
4. Wouldn't "underused" be better than "underutilized" throughout?
Hugh
