diff mbox series

[v2,2/2] mm: collect the number of anon large folios on split_deferred list

Message ID 20240811224940.39876-3-21cnbao@gmail.com (mailing list archive)
State New
Headers show
Series mm: collect the number of anon mTHP | expand

Commit Message

Barry Song Aug. 11, 2024, 10:49 p.m. UTC 
From: Barry Song <v-songbaohua@oppo.com>

When an mTHP is added to the deferred_list, its partial pages
are unused, leading to wasted memory and potentially increasing
memory reclamation pressure.

Detailing the specifics of how unmapping occurs is quite difficult
and not that useful, so we adopt a simple approach: each time an
mTHP enters the deferred_list, we increment the count by 1; whenever
it leaves for any reason, we decrement the count by 1.

Signed-off-by: Barry Song <v-songbaohua@oppo.com>
---
 Documentation/admin-guide/mm/transhuge.rst | 5 +++++
 include/linux/huge_mm.h                    | 1 +
 mm/huge_memory.c                           | 6 ++++++
 3 files changed, 12 insertions(+)

Comments

David Hildenbrand Aug. 21, 2024, 9:39 p.m. UTC | #1 
On 12.08.24 00:49, Barry Song wrote:
> From: Barry Song <v-songbaohua@oppo.com>
> 
> When an mTHP is added to the deferred_list, its partial pages
> are unused, leading to wasted memory and potentially increasing
> memory reclamation pressure.
> 
> Detailing the specifics of how unmapping occurs is quite difficult
> and not that useful, so we adopt a simple approach: each time an
> mTHP enters the deferred_list, we increment the count by 1; whenever
> it leaves for any reason, we decrement the count by 1.
> 
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> ---
>   Documentation/admin-guide/mm/transhuge.rst | 5 +++++
>   include/linux/huge_mm.h                    | 1 +
>   mm/huge_memory.c                           | 6 ++++++
>   3 files changed, 12 insertions(+)
> 
> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> index 9fdfb46e4560..7072469de8a8 100644
> --- a/Documentation/admin-guide/mm/transhuge.rst
> +++ b/Documentation/admin-guide/mm/transhuge.rst
> @@ -532,6 +532,11 @@ nr_anon
>          These huge pages could be entirely mapped or have partially
>          unmapped/unused subpages.
>   
> +nr_split_deferred
> +       the number of anon huge pages which have been partially unmapped
> +       and put onto split queue. Those unmapped subpages are also unused
> +       and temporarily wasting memory.

The name suggests something else ... like a counter of how many have 
been deferred split :)

Would "nr_anon_partially_mapped" "nr_anon_split_pending" (or something 
less mouthful) be clearer?

(likely "anon" really should be part of the name in any case)

The name we chose (and the implied semantics) will likely have 
implications on the handling of Usamas series.
Barry Song Aug. 21, 2024, 10:01 p.m. UTC | #2 
On Thu, Aug 22, 2024 at 5:39 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 12.08.24 00:49, Barry Song wrote:
> > From: Barry Song <v-songbaohua@oppo.com>
> >
> > When an mTHP is added to the deferred_list, its partial pages
> > are unused, leading to wasted memory and potentially increasing
> > memory reclamation pressure.
> >
> > Detailing the specifics of how unmapping occurs is quite difficult
> > and not that useful, so we adopt a simple approach: each time an
> > mTHP enters the deferred_list, we increment the count by 1; whenever
> > it leaves for any reason, we decrement the count by 1.
> >
> > Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> > ---
> >   Documentation/admin-guide/mm/transhuge.rst | 5 +++++
> >   include/linux/huge_mm.h                    | 1 +
> >   mm/huge_memory.c                           | 6 ++++++
> >   3 files changed, 12 insertions(+)
> >
> > diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> > index 9fdfb46e4560..7072469de8a8 100644
> > --- a/Documentation/admin-guide/mm/transhuge.rst
> > +++ b/Documentation/admin-guide/mm/transhuge.rst
> > @@ -532,6 +532,11 @@ nr_anon
> >          These huge pages could be entirely mapped or have partially
> >          unmapped/unused subpages.
> >
> > +nr_split_deferred
> > +       the number of anon huge pages which have been partially unmapped
> > +       and put onto split queue. Those unmapped subpages are also unused
> > +       and temporarily wasting memory.
>
> The name suggests something else ... like a counter of how many have
> been deferred split :)
>
> Would "nr_anon_partially_mapped" "nr_anon_split_pending" (or something
> less mouthful) be clearer?
>
> (likely "anon" really should be part of the name in any case)
>
> The name we chose (and the implied semantics) will likely have
> implications on the handling of Usamas series.
>

Hi David,

Your point is quite similar to my V1, though not exactly the same. I aimed to
make the name more meaningful for users.
https://lore.kernel.org/all/20240808010457.228753-3-21cnbao@gmail.com/

Ryan felt that the name should be consistent with the existing split_deferred.
https://lore.kernel.org/all/36e8f1be-868d-4bce-8f32-e2d96b8b7af3@arm.com/#t

It seems that the existing split_deferred may now be less optimal with
Usama's series, as entirely_mapped folios might also be on the list.

Ryan is out right now, but I suppose he will be convinced that
"nr_anon_partially_mapped" is probably a better name once
he returns and reviews Usama's series. :-)

> --
> Cheers,
>
> David / dhildenb
>

Thanks
Barry
David Hildenbrand Aug. 21, 2024, 10:10 p.m. UTC | #3 
On 22.08.24 00:01, Barry Song wrote:
> On Thu, Aug 22, 2024 at 5:39 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 12.08.24 00:49, Barry Song wrote:
>>> From: Barry Song <v-songbaohua@oppo.com>
>>>
>>> When an mTHP is added to the deferred_list, its partial pages
>>> are unused, leading to wasted memory and potentially increasing
>>> memory reclamation pressure.
>>>
>>> Detailing the specifics of how unmapping occurs is quite difficult
>>> and not that useful, so we adopt a simple approach: each time an
>>> mTHP enters the deferred_list, we increment the count by 1; whenever
>>> it leaves for any reason, we decrement the count by 1.
>>>
>>> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
>>> ---
>>>    Documentation/admin-guide/mm/transhuge.rst | 5 +++++
>>>    include/linux/huge_mm.h                    | 1 +
>>>    mm/huge_memory.c                           | 6 ++++++
>>>    3 files changed, 12 insertions(+)
>>>
>>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
>>> index 9fdfb46e4560..7072469de8a8 100644
>>> --- a/Documentation/admin-guide/mm/transhuge.rst
>>> +++ b/Documentation/admin-guide/mm/transhuge.rst
>>> @@ -532,6 +532,11 @@ nr_anon
>>>           These huge pages could be entirely mapped or have partially
>>>           unmapped/unused subpages.
>>>
>>> +nr_split_deferred
>>> +       the number of anon huge pages which have been partially unmapped
>>> +       and put onto split queue. Those unmapped subpages are also unused
>>> +       and temporarily wasting memory.
>>
>> The name suggests something else ... like a counter of how many have
>> been deferred split :)
>>
>> Would "nr_anon_partially_mapped" "nr_anon_split_pending" (or something
>> less mouthful) be clearer?
>>
>> (likely "anon" really should be part of the name in any case)
>>
>> The name we chose (and the implied semantics) will likely have
>> implications on the handling of Usamas series.
>>
> 
> Hi David,
> 
> Your point is quite similar to my V1, though not exactly the same. I aimed to
> make the name more meaningful for users.
> https://lore.kernel.org/all/20240808010457.228753-3-21cnbao@gmail.com/
> 
> Ryan felt that the name should be consistent with the existing split_deferred.
> https://lore.kernel.org/all/36e8f1be-868d-4bce-8f32-e2d96b8b7af3@arm.com/#t

Right, it's an increasing counter of how often we added something to the 
deferred split queue. It's more like a low-level primitive, although 
currently only used for one reason (anon + partial mapping).

> 
> It seems that the existing split_deferred may now be less optimal with
> Usama's series, as entirely_mapped folios might also be on the list.

Yes.

> 
> Ryan is out right now, but I suppose he will be convinced that
> "nr_anon_partially_mapped" is probably a better name once
> he returns and reviews Usama's series. :-)

Maybe ;) At least it's clear from the name what we are talking about.
diff mbox series

Patch

diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
index 9fdfb46e4560..7072469de8a8 100644
--- a/Documentation/admin-guide/mm/transhuge.rst
+++ b/Documentation/admin-guide/mm/transhuge.rst
@@ -532,6 +532,11 @@  nr_anon
        These huge pages could be entirely mapped or have partially
        unmapped/unused subpages.
 
+nr_split_deferred
+       the number of anon huge pages which have been partially unmapped
+       and put onto split queue. Those unmapped subpages are also unused
+       and temporarily wasting memory.
+
 As the system ages, allocating huge pages may be expensive as the
 system uses memory compaction to copy data around memory to free a
 huge page for use. There are some counters in ``/proc/vmstat`` to help
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index 2ee2971e4e10..1e2d5dbe82c5 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -127,6 +127,7 @@  enum mthp_stat_item {
 	MTHP_STAT_SPLIT_FAILED,
 	MTHP_STAT_SPLIT_DEFERRED,
 	MTHP_STAT_NR_ANON,
+	MTHP_STAT_NR_SPLIT_DEFERRED,
 	__MTHP_STAT_COUNT
 };
 
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index afb911789df8..1a12c011e2df 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -597,6 +597,7 @@  DEFINE_MTHP_STAT_ATTR(split, MTHP_STAT_SPLIT);
 DEFINE_MTHP_STAT_ATTR(split_failed, MTHP_STAT_SPLIT_FAILED);
 DEFINE_MTHP_STAT_ATTR(split_deferred, MTHP_STAT_SPLIT_DEFERRED);
 DEFINE_MTHP_STAT_ATTR(nr_anon, MTHP_STAT_NR_ANON);
+DEFINE_MTHP_STAT_ATTR(nr_split_deferred, MTHP_STAT_NR_SPLIT_DEFERRED);
 
 static struct attribute *anon_stats_attrs[] = {
 	&anon_fault_alloc_attr.attr,
@@ -610,6 +611,7 @@  static struct attribute *anon_stats_attrs[] = {
 	&split_failed_attr.attr,
 	&split_deferred_attr.attr,
 	&nr_anon_attr.attr,
+	&nr_split_deferred_attr.attr,
 	NULL,
 };
 
@@ -3359,6 +3361,7 @@  int split_huge_page_to_list_to_order(struct page *page, struct list_head *list,
 		if (folio_order(folio) > 1 &&
 		    !list_empty(&folio->_deferred_list)) {
 			ds_queue->split_queue_len--;
+			mod_mthp_stat(folio_order(folio), MTHP_STAT_NR_SPLIT_DEFERRED, -1);
 			/*
 			 * Reinitialize page_deferred_list after removing the
 			 * page from the split_queue, otherwise a subsequent
@@ -3425,6 +3428,7 @@  void __folio_undo_large_rmappable(struct folio *folio)
 	spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
 	if (!list_empty(&folio->_deferred_list)) {
 		ds_queue->split_queue_len--;
+		mod_mthp_stat(folio_order(folio), MTHP_STAT_NR_SPLIT_DEFERRED, -1);
 		list_del_init(&folio->_deferred_list);
 	}
 	spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
@@ -3466,6 +3470,7 @@  void deferred_split_folio(struct folio *folio)
 		if (folio_test_pmd_mappable(folio))
 			count_vm_event(THP_DEFERRED_SPLIT_PAGE);
 		count_mthp_stat(folio_order(folio), MTHP_STAT_SPLIT_DEFERRED);
+		mod_mthp_stat(folio_order(folio), MTHP_STAT_NR_SPLIT_DEFERRED, 1);
 		list_add_tail(&folio->_deferred_list, &ds_queue->split_queue);
 		ds_queue->split_queue_len++;
 #ifdef CONFIG_MEMCG
@@ -3513,6 +3518,7 @@  static unsigned long deferred_split_scan(struct shrinker *shrink,
 			list_move(&folio->_deferred_list, &list);
 		} else {
 			/* We lost race with folio_put() */
+			mod_mthp_stat(folio_order(folio), MTHP_STAT_NR_SPLIT_DEFERRED, -1);
 			list_del_init(&folio->_deferred_list);
 			ds_queue->split_queue_len--;
 		}