Re: [PATCH v2 4/4] mm: split underutilized THPs

[David Hildenbrand]

On 09.08.24 12:31, Usama Arif wrote:
> On 08/08/2024 16:55, David Hildenbrand wrote:
> > On 07.08.24 15:46, Usama Arif wrote:
> > > This 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
> > > (__do_huge_pmd_anonymous_page) or collapsed by khugepaged
> > > (collapse_huge_page), the THP is added to  _deferred_list. Whenever memory
> > > reclaim happens in linux, the kernel runs the deferred_split
> > > shrinker which goes through the _deferred_list.
> > >
> > > If the folio was partially mapped, the shrinker attempts to split it.
> > > A new boolean is added to be able to distinguish between partially
> > > mapped folios and others in the deferred_list at split time in
> > > deferred_split_scan. Its needed as __folio_remove_rmap decrements
> > > the folio mapcount elements, hence it won't be possible to distinguish
> > > between partially mapped folios and others in deferred_split_scan
> > > without the boolean.
> >
> > Just so I get this right: Are you saying that we might now add fully mapped folios to the deferred split queue and that's what you want to distinguish?
>
> Yes
>
> > If that's the case, then could we use a bit in folio->_flags_1 instead?
> Yes, thats a good idea. Will create the below flag for the next revision
>
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 5769fe6e4950..5825bd1cf6db 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -189,6 +189,11 @@ enum pageflags {
>   
>   #define PAGEFLAGS_MASK         ((1UL << NR_PAGEFLAGS) - 1)
>   
> +enum folioflags_1 {
> +       /* The first 8 bits of folio->_flags_1 are used to keep track of folio order */
> +       FOLIO_PARTIALLY_MAPPED = 8,     /* folio is partially mapped */
> +}

This might be what you want to achieve:

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index a0a29bd092f8..d4722ed60ef8 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -182,6 +182,7 @@ enum pageflags {
        /* At least one page in this folio has the hwpoison flag set */
        PG_has_hwpoisoned = PG_active,
        PG_large_rmappable = PG_workingset, /* anon or file-backed */
+       PG_partially_mapped, /* was identified to be partially mapped */
 };
 
 #define PAGEFLAGS_MASK         ((1UL << NR_PAGEFLAGS) - 1)
@@ -861,8 +862,9 @@ static inline void ClearPageCompound(struct page *page)
        ClearPageHead(page);
 }
 FOLIO_FLAG(large_rmappable, FOLIO_SECOND_PAGE)
+FOLIO_FLAG(partially_mapped, FOLIO_SECOND_PAGE)
 #else
-FOLIO_FLAG_FALSE(large_rmappable)
+FOLIO_FLAG_FALSE(partially_mapped)
 #endif
 
 #define PG_head_mask ((1UL << PG_head))

The downside is an atomic op to set/clear, but it should likely not really matter
(initially, the flag will be clear, and we should only ever set it once when
partially unmapping). If it hurts, we can reconsider.

[...]

> > I would actually suggest to split decoupling of "_deferred_list" and "partially mapped" into a separate preparation patch.
> Yes, will do. I will split it into 3 patches, 1st one that introduces FOLIO_PARTIALLY_MAPPED and sets/clear it in the right place without introducing any functional change, 2nd to split underutilized THPs and 3rd to add sysfs entry to enable/disable the shrinker. Should make the patches quite small and easy to review.


Great! As always, please shout if you disagree with something I propose :)

--
Cheers,

David / dhildenb