The mTHP swap related counters: 'anon_swpout' and 'anon_swpout_fallback' are
confusing with an 'anon_' prefix, since the shmem can swap out non-anonymous
pages. So drop the 'anon_' prefix to keep consistent with the old swap counter
names.
Suggested-by: "Huang, Ying" <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
include/linux/huge_mm.h | 4 ++--
mm/huge_memory.c | 8 ++++----
mm/page_io.c | 2 +-
mm/vmscan.c | 2 +-
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index c8d3ec116e29..8c72d3786583 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -269,8 +269,8 @@ enum mthp_stat_item {
MTHP_STAT_ANON_FAULT_ALLOC,
MTHP_STAT_ANON_FAULT_FALLBACK,
MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE,
- MTHP_STAT_ANON_SWPOUT,
- MTHP_STAT_ANON_SWPOUT_FALLBACK,
+ MTHP_STAT_SWPOUT,
+ MTHP_STAT_SWPOUT_FALLBACK,
__MTHP_STAT_COUNT
};
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 317de2afd371..89932fd0f62e 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -558,15 +558,15 @@ static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
DEFINE_MTHP_STAT_ATTR(anon_fault_alloc, MTHP_STAT_ANON_FAULT_ALLOC);
DEFINE_MTHP_STAT_ATTR(anon_fault_fallback, MTHP_STAT_ANON_FAULT_FALLBACK);
DEFINE_MTHP_STAT_ATTR(anon_fault_fallback_charge, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
-DEFINE_MTHP_STAT_ATTR(anon_swpout, MTHP_STAT_ANON_SWPOUT);
-DEFINE_MTHP_STAT_ATTR(anon_swpout_fallback, MTHP_STAT_ANON_SWPOUT_FALLBACK);
+DEFINE_MTHP_STAT_ATTR(swpout, MTHP_STAT_SWPOUT);
+DEFINE_MTHP_STAT_ATTR(swpout_fallback, MTHP_STAT_SWPOUT_FALLBACK);
static struct attribute *stats_attrs[] = {
&anon_fault_alloc_attr.attr,
&anon_fault_fallback_attr.attr,
&anon_fault_fallback_charge_attr.attr,
- &anon_swpout_attr.attr,
- &anon_swpout_fallback_attr.attr,
+ &swpout_attr.attr,
+ &swpout_fallback_attr.attr,
NULL,
};
diff --git a/mm/page_io.c b/mm/page_io.c
index 46c603dddf04..0a150c240bf4 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -217,7 +217,7 @@ static inline void count_swpout_vm_event(struct folio *folio)
count_memcg_folio_events(folio, THP_SWPOUT, 1);
count_vm_event(THP_SWPOUT);
}
- count_mthp_stat(folio_order(folio), MTHP_STAT_ANON_SWPOUT);
+ count_mthp_stat(folio_order(folio), MTHP_STAT_SWPOUT);
#endif
count_vm_events(PSWPOUT, folio_nr_pages(folio));
}
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 6981a71c8ef0..18b796605aa5 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1244,7 +1244,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
THP_SWPOUT_FALLBACK, 1);
count_vm_event(THP_SWPOUT_FALLBACK);
}
- count_mthp_stat(order, MTHP_STAT_ANON_SWPOUT_FALLBACK);
+ count_mthp_stat(order, MTHP_STAT_SWPOUT_FALLBACK);
#endif
if (!add_to_swap(folio))
goto activate_locked_split;
--
2.39.3
On 22.05.24 10:51, Baolin Wang wrote:
> The mTHP swap related counters: 'anon_swpout' and 'anon_swpout_fallback' are
> confusing with an 'anon_' prefix, since the shmem can swap out non-anonymous
> pages. So drop the 'anon_' prefix to keep consistent with the old swap counter
> names.
>
> Suggested-by: "Huang, Ying" <[email protected]>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
Am I daydreaming or did we add the anon_ for a reason and discussed the
interaction with shmem? At least I remember some discussion around that.
--
Cheers,
David / dhildenb
On 2024/5/22 16:58, David Hildenbrand wrote:
> On 22.05.24 10:51, Baolin Wang wrote:
>> The mTHP swap related counters: 'anon_swpout' and
>> 'anon_swpout_fallback' are
>> confusing with an 'anon_' prefix, since the shmem can swap out
>> non-anonymous
>> pages. So drop the 'anon_' prefix to keep consistent with the old swap
>> counter
>> names.
>>
>> Suggested-by: "Huang, Ying" <[email protected]>
>> Signed-off-by: Baolin Wang <[email protected]>
>> ---
>
> Am I daydreaming or did we add the anon_ for a reason and discussed the
> interaction with shmem? At least I remember some discussion around that.
Do you mean the shmem mTHP allocation counters in previous
discussion[1]? But for 'anon_swpout' and 'anon_swpout_fallback', I can
not find previous discussions that provided a reason for adding the
‘anon_’ prefix. Barry, any comments? Thanks.
[1]
https://lore.kernel.org/all/05d0096e4ec3e572d1d52d33a31a661321ac1551.1713755580.git.baolin.wang@linux.alibaba.com/
On Wed, May 22, 2024 at 9:38 PM Baolin Wang
<[email protected]> wrote:
>
>
>
> On 2024/5/22 16:58, David Hildenbrand wrote:
> > On 22.05.24 10:51, Baolin Wang wrote:
> >> The mTHP swap related counters: 'anon_swpout' and
> >> 'anon_swpout_fallback' are
> >> confusing with an 'anon_' prefix, since the shmem can swap out
> >> non-anonymous
> >> pages. So drop the 'anon_' prefix to keep consistent with the old swap
> >> counter
> >> names.
> >>
> >> Suggested-by: "Huang, Ying" <[email protected]>
> >> Signed-off-by: Baolin Wang <[email protected]>
> >> ---
> >
> > Am I daydreaming or did we add the anon_ for a reason and discussed the
> > interaction with shmem? At least I remember some discussion around that.
>
> Do you mean the shmem mTHP allocation counters in previous
> discussion[1]? But for 'anon_swpout' and 'anon_swpout_fallback', I can
> not find previous discussions that provided a reason for adding the
> ‘anon_’ prefix. Barry, any comments? Thanks.
HI Baolin,
We had tons of emails discussing about namin and I found this email,
https://lore.kernel.org/all/[email protected]/
David had this comment,
"I'm wondering if these should be ANON specific for now. We might want to
add others (shmem, file) in the future."
This is likely how the 'anon_' prefix started being added, although it
wasn't specifically
targeting swapout.
I sense your patch slightly alters the behavior of thp_swpout_fallback
in /proc/vmstat.
Previously, we didn't classify them as THP_SWPOUT_FALLBACK, even though we
always split them.
if (folio_test_anon(folio) && folio_test_swapbacked(folio)) {
...
if (!add_to_swap(folio)) {
int __maybe_unused order =
folio_order(folio);
if (!folio_test_large(folio))
goto activate_locked_split;
/* Fallback to swap normal pages */
if (split_folio_to_list(folio,
folio_list))
goto activate_locked;
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
if (nr_pages >= HPAGE_PMD_NR) {
count_memcg_folio_events(folio,
THP_SWPOUT_FALLBACK, 1);
count_vm_event(THP_SWPOUT_FALLBACK);
}
count_mthp_stat(order,
MTHP_STAT_ANON_SWPOUT_FALLBACK);
#endif
if (!add_to_swap(folio))
goto activate_locked_split;
}
}
} else if (folio_test_swapbacked(folio) &&
folio_test_large(folio)) {
/* Split shmem folio */
if (split_folio_to_list(folio, folio_list))
goto keep_locked;
}
If the goal is to incorporate pmd-mapped shmem under thp_swpout* in
/proc/vmstat,
and if there is consistency between /proc/vmstat and sys regarding
their definitions,
then I have no objection to this patch. However, shmem_swpout and shmem_swpout_*
appear more intuitive, given that thp_swpout_* in /proc/vmstat has
never shown any
increments for shmem until now - we have been always splitting shmem in vmscan.
By the way, if this patch is accepted, it must be included in version
6.10 to maintain
ABI compatibility. Additionally, documentation must be updated accordingly.
>
> [1]
> https://lore.kernel.org/all/05d0096e4ec3e572d1d52d33a31a661321ac1551.1713755580.git.baolin.wang@linux.alibaba.com/
Thanks
Barry
On 2024/5/22 18:40, Barry Song wrote:
> On Wed, May 22, 2024 at 9:38 PM Baolin Wang
> <[email protected]> wrote:
>>
>>
>>
>> On 2024/5/22 16:58, David Hildenbrand wrote:
>>> On 22.05.24 10:51, Baolin Wang wrote:
>>>> The mTHP swap related counters: 'anon_swpout' and
>>>> 'anon_swpout_fallback' are
>>>> confusing with an 'anon_' prefix, since the shmem can swap out
>>>> non-anonymous
>>>> pages. So drop the 'anon_' prefix to keep consistent with the old swap
>>>> counter
>>>> names.
>>>>
>>>> Suggested-by: "Huang, Ying" <[email protected]>
>>>> Signed-off-by: Baolin Wang <[email protected]>
>>>> ---
>>>
>>> Am I daydreaming or did we add the anon_ for a reason and discussed the
>>> interaction with shmem? At least I remember some discussion around that.
>>
>> Do you mean the shmem mTHP allocation counters in previous
>> discussion[1]? But for 'anon_swpout' and 'anon_swpout_fallback', I can
>> not find previous discussions that provided a reason for adding the
>> ‘anon_’ prefix. Barry, any comments? Thanks.
>
> HI Baolin,
> We had tons of emails discussing about namin and I found this email,
>
> https://lore.kernel.org/all/[email protected]/
>
> David had this comment,
> "I'm wondering if these should be ANON specific for now. We might want to
> add others (shmem, file) in the future."
>
> This is likely how the 'anon_' prefix started being added, although it
> wasn't specifically
> targeting swapout.
That's what I missed before. Thanks Barry.
> I sense your patch slightly alters the behavior of thp_swpout_fallback
> in /proc/vmstat.
> Previously, we didn't classify them as THP_SWPOUT_FALLBACK, even though we
> always split them.
Sorry I did not get you here. I just re-name the mTHP swpout_fallback,
how can this patch change the THP_SWPOUT_FALLBACK statistic counted by
count_vm_event()?
> if (folio_test_anon(folio) && folio_test_swapbacked(folio)) {
> ...
> if (!add_to_swap(folio)) {
> int __maybe_unused order =
> folio_order(folio);
>
> if (!folio_test_large(folio))
> goto activate_locked_split;
> /* Fallback to swap normal pages */
> if (split_folio_to_list(folio,
> folio_list))
> goto activate_locked;
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> if (nr_pages >= HPAGE_PMD_NR) {
> count_memcg_folio_events(folio,
> THP_SWPOUT_FALLBACK, 1);
>
> count_vm_event(THP_SWPOUT_FALLBACK);
> }
> count_mthp_stat(order,
> MTHP_STAT_ANON_SWPOUT_FALLBACK);
> #endif
> if (!add_to_swap(folio))
> goto activate_locked_split;
> }
> }
> } else if (folio_test_swapbacked(folio) &&
> folio_test_large(folio)) {
> /* Split shmem folio */
> if (split_folio_to_list(folio, folio_list))
> goto keep_locked;
> }
>
>
>
> If the goal is to incorporate pmd-mapped shmem under thp_swpout* in
> /proc/vmstat,
> and if there is consistency between /proc/vmstat and sys regarding
> their definitions,
> then I have no objection to this patch.
I think this is the goal, moreover shmem will support large folio (not
only THP) in future, so swpout related counters should be defined as
clear as possible.
However, shmem_swpout and shmem_swpout_*
> appear more intuitive, given that thp_swpout_* in /proc/vmstat has
> never shown any
> increments for shmem until now - we have been always splitting shmem in vmscan.
This is somewhat similar to our previous discussion on the naming of the
shmem's mTHP counter[1], as David suggested, we should keep counter name
consistency for now and add more in the future as needed.
[1]
https://lore.kernel.org/all/[email protected]/
>
> By the way, if this patch is accepted, it must be included in version
> 6.10 to maintain
> ABI compatibility. Additionally, documentation must be updated accordingly.
Sure. I missed update the documentation, and will do in next version.
Hi Baolin,
On Wed, May 22, 2024 at 7:24 PM Baolin Wang
<[email protected]> wrote:
>
>
>
> On 2024/5/22 18:40, Barry Song wrote:
> > On Wed, May 22, 2024 at 9:38 PM Baolin Wang
> > <[email protected]> wrote:
> >>
> >>
> >>
> >> On 2024/5/22 16:58, David Hildenbrand wrote:
> >>> On 22.05.24 10:51, Baolin Wang wrote:
> >>>> The mTHP swap related counters: 'anon_swpout' and
> >>>> 'anon_swpout_fallback' are
> >>>> confusing with an 'anon_' prefix, since the shmem can swap out
> >>>> non-anonymous
> >>>> pages. So drop the 'anon_' prefix to keep consistent with the old swap
> >>>> counter
> >>>> names.
> >>>>
> >>>> Suggested-by: "Huang, Ying" <[email protected]>
> >>>> Signed-off-by: Baolin Wang <[email protected]>
> >>>> ---
> >>>
> >>> Am I daydreaming or did we add the anon_ for a reason and discussed the
> >>> interaction with shmem? At least I remember some discussion around that.
> >>
> >> Do you mean the shmem mTHP allocation counters in previous
> >> discussion[1]? But for 'anon_swpout' and 'anon_swpout_fallback', I can
> >> not find previous discussions that provided a reason for adding the
> >> ‘anon_’ prefix. Barry, any comments? Thanks.
> >
> > HI Baolin,
> > We had tons of emails discussing about namin and I found this email,
> >
> > https://lore.kernel.org/all/bca6d142-15fd-4af5-9f71-821f891e8305@redhatcom/
> >
> > David had this comment,
> > "I'm wondering if these should be ANON specific for now. We might want to
> > add others (shmem, file) in the future."
> >
> > This is likely how the 'anon_' prefix started being added, although it
> > wasn't specifically
> > targeting swapout.
>
> That's what I missed before. Thanks Barry.
>
> > I sense your patch slightly alters the behavior of thp_swpout_fallback
> > in /proc/vmstat.
> > Previously, we didn't classify them as THP_SWPOUT_FALLBACK, even though we
> > always split them.
>
> Sorry I did not get you here. I just re-name the mTHP swpout_fallback,
> how can this patch change the THP_SWPOUT_FALLBACK statistic counted by
> count_vm_event()?
Currently, PMD-mapped shmem folios are not accounted for in
THP_SWPOUT and related counters.
So, IMO, if we intend to account for them in those counters in the
future, removing
the 'anon_' prefix from the mTHP swap counters would be reasonable :)
Thanks,
Lance
>
> > if (folio_test_anon(folio) && folio_test_swapbacked(folio)) {
> > ...
> > if (!add_to_swap(folio)) {
> > int __maybe_unused order =
> > folio_order(folio);
> >
> > if (!folio_test_large(folio))
> > goto activate_locked_split;
> > /* Fallback to swap normal pages */
> > if (split_folio_to_list(folio,
> > folio_list))
> > goto activate_locked;
> > #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > if (nr_pages >= HPAGE_PMD_NR) {
> > count_memcg_folio_events(folio,
> > THP_SWPOUT_FALLBACK, 1);
> >
> > count_vm_event(THP_SWPOUT_FALLBACK);
> > }
> > count_mthp_stat(order,
> > MTHP_STAT_ANON_SWPOUT_FALLBACK);
> > #endif
> > if (!add_to_swap(folio))
> > goto activate_locked_split;
> > }
> > }
> > } else if (folio_test_swapbacked(folio) &&
> > folio_test_large(folio)) {
> > /* Split shmem folio */
> > if (split_folio_to_list(folio, folio_list))
> > goto keep_locked;
> > }
> >
> >
> >
> > If the goal is to incorporate pmd-mapped shmem under thp_swpout* in
> > /proc/vmstat,
> > and if there is consistency between /proc/vmstat and sys regarding
> > their definitions,
> > then I have no objection to this patch.
>
> I think this is the goal, moreover shmem will support large folio (not
> only THP) in future, so swpout related counters should be defined as
> clear as possible.
>
> However, shmem_swpout and shmem_swpout_*
> > appear more intuitive, given that thp_swpout_* in /proc/vmstat has
> > never shown any
> > increments for shmem until now - we have been always splitting shmem in vmscan.
>
> This is somewhat similar to our previous discussion on the naming of the
> shmem's mTHP counter[1], as David suggested, we should keep counter name
> consistency for now and add more in the future as needed.
>
> [1]
> https://lore.kernel.org/all/[email protected]/
> >
> > By the way, if this patch is accepted, it must be included in version
> > 6.10 to maintain
> > ABI compatibility. Additionally, documentation must be updated accordingly.
>
> Sure. I missed update the documentation, and will do in next version.
>
Baolin Wang <[email protected]> writes:
> On 2024/5/22 18:40, Barry Song wrote:
>> On Wed, May 22, 2024 at 9:38 PM Baolin Wang
>> <[email protected]> wrote:
>>>
>>>
>>>
>>> On 2024/5/22 16:58, David Hildenbrand wrote:
>>>> On 22.05.24 10:51, Baolin Wang wrote:
>>>>> The mTHP swap related counters: 'anon_swpout' and
>>>>> 'anon_swpout_fallback' are
>>>>> confusing with an 'anon_' prefix, since the shmem can swap out
>>>>> non-anonymous
>>>>> pages. So drop the 'anon_' prefix to keep consistent with the old swap
>>>>> counter
>>>>> names.
>>>>>
>>>>> Suggested-by: "Huang, Ying" <[email protected]>
>>>>> Signed-off-by: Baolin Wang <[email protected]>
>>>>> ---
>>>>
>>>> Am I daydreaming or did we add the anon_ for a reason and discussed the
>>>> interaction with shmem? At least I remember some discussion around that.
>>>
>>> Do you mean the shmem mTHP allocation counters in previous
>>> discussion[1]? But for 'anon_swpout' and 'anon_swpout_fallback', I can
>>> not find previous discussions that provided a reason for adding the
>>> ‘anon_’ prefix. Barry, any comments? Thanks.
>> HI Baolin,
>> We had tons of emails discussing about namin and I found this email,
>> https://lore.kernel.org/all/[email protected]/
>> David had this comment,
>> "I'm wondering if these should be ANON specific for now. We might want to
>> add others (shmem, file) in the future."
>> This is likely how the 'anon_' prefix started being added, although
>> it
>> wasn't specifically
>> targeting swapout.
>
> That's what I missed before. Thanks Barry.
>
>> I sense your patch slightly alters the behavior of thp_swpout_fallback
>> in /proc/vmstat.
>> Previously, we didn't classify them as THP_SWPOUT_FALLBACK, even though we
>> always split them.
>
> Sorry I did not get you here. I just re-name the mTHP swpout_fallback,
> how can this patch change the THP_SWPOUT_FALLBACK statistic counted by
> count_vm_event()?
>
>> if (folio_test_anon(folio) && folio_test_swapbacked(folio)) {
>> ...
>> if (!add_to_swap(folio)) {
>> int __maybe_unused order =
>> folio_order(folio);
>> if
>> (!folio_test_large(folio))
>> goto activate_locked_split;
>> /* Fallback to swap normal pages */
>> if (split_folio_to_list(folio,
>> folio_list))
>> goto activate_locked;
>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> if (nr_pages >= HPAGE_PMD_NR) {
>> count_memcg_folio_events(folio,
>> THP_SWPOUT_FALLBACK, 1);
>> count_vm_event(THP_SWPOUT_FALLBACK);
>> }
>> count_mthp_stat(order,
>> MTHP_STAT_ANON_SWPOUT_FALLBACK);
>> #endif
>> if (!add_to_swap(folio))
>> goto activate_locked_split;
>> }
>> }
>> } else if (folio_test_swapbacked(folio) &&
>> folio_test_large(folio)) {
>> /* Split shmem folio */
>> if (split_folio_to_list(folio, folio_list))
>> goto keep_locked;
>> }
>> If the goal is to incorporate pmd-mapped shmem under thp_swpout* in
>> /proc/vmstat,
>> and if there is consistency between /proc/vmstat and sys regarding
>> their definitions,
>> then I have no objection to this patch.
>
> I think this is the goal, moreover shmem will support large folio (not
> only THP) in future, so swpout related counters should be defined as
> clear as possible.
>
> However, shmem_swpout and shmem_swpout_*
>> appear more intuitive, given that thp_swpout_* in /proc/vmstat has
>> never shown any
>> increments for shmem until now - we have been always splitting shmem in vmscan.
>
> This is somewhat similar to our previous discussion on the naming of
> the shmem's mTHP counter[1], as David suggested, we should keep
> counter name consistency for now and add more in the future as needed.
>
> [1]
> https://lore.kernel.org/all/[email protected]/
Yes. I don't find that it's necessary to distinguish anonymous and
shmem mTHP swap-out now. If we need it in the future, we can add that
at that time.
>> By the way, if this patch is accepted, it must be included in
>> version
>> 6.10 to maintain
>> ABI compatibility. Additionally, documentation must be updated accordingly.
>
> Sure. I missed update the documentation, and will do in next version.
--
Best Regards,
Huang, Ying
Barry Song <[email protected]> writes:
> On Wed, May 22, 2024 at 9:38 PM Baolin Wang
> <[email protected]> wrote:
>>
>>
>>
>> On 2024/5/22 16:58, David Hildenbrand wrote:
>> > On 22.05.24 10:51, Baolin Wang wrote:
>> >> The mTHP swap related counters: 'anon_swpout' and
>> >> 'anon_swpout_fallback' are
>> >> confusing with an 'anon_' prefix, since the shmem can swap out
>> >> non-anonymous
>> >> pages. So drop the 'anon_' prefix to keep consistent with the old swap
>> >> counter
>> >> names.
>> >>
>> >> Suggested-by: "Huang, Ying" <[email protected]>
>> >> Signed-off-by: Baolin Wang <[email protected]>
>> >> ---
>> >
>> > Am I daydreaming or did we add the anon_ for a reason and discussed the
>> > interaction with shmem? At least I remember some discussion around that.
>>
>> Do you mean the shmem mTHP allocation counters in previous
>> discussion[1]? But for 'anon_swpout' and 'anon_swpout_fallback', I can
>> not find previous discussions that provided a reason for adding the
>> ‘anon_’ prefix. Barry, any comments? Thanks.
>
> HI Baolin,
> We had tons of emails discussing about namin and I found this email,
>
> https://lore.kernel.org/all/[email protected]/
>
> David had this comment,
> "I'm wondering if these should be ANON specific for now. We might want to
> add others (shmem, file) in the future."
>
> This is likely how the 'anon_' prefix started being added, although it
> wasn't specifically
> targeting swapout.
>
> I sense your patch slightly alters the behavior of thp_swpout_fallback
> in /proc/vmstat.
> Previously, we didn't classify them as THP_SWPOUT_FALLBACK, even though we
> always split them.
IIUC, "fallback" means you try to do something, but fail, so try
something else as fallback. If so, then we don't need to count
splitting shmem large folio as fallback.
For example, before commit 5ed890ce5147 ("mm: vmscan: avoid split during
shrink_folio_list()"), if folio_entire_mapcount() == 0, we will split
the THP. But we will not count it as "fallback" because we haven't
tried to swap it out as a whole.
>
> if (folio_test_anon(folio) && folio_test_swapbacked(folio)) {
> ...
> if (!add_to_swap(folio)) {
> int __maybe_unused order =
> folio_order(folio);
>
> if (!folio_test_large(folio))
> goto activate_locked_split;
> /* Fallback to swap normal pages */
> if (split_folio_to_list(folio,
> folio_list))
> goto activate_locked;
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> if (nr_pages >= HPAGE_PMD_NR) {
> count_memcg_folio_events(folio,
> THP_SWPOUT_FALLBACK, 1);
>
> count_vm_event(THP_SWPOUT_FALLBACK);
> }
> count_mthp_stat(order,
> MTHP_STAT_ANON_SWPOUT_FALLBACK);
> #endif
> if (!add_to_swap(folio))
> goto activate_locked_split;
> }
> }
> } else if (folio_test_swapbacked(folio) &&
> folio_test_large(folio)) {
> /* Split shmem folio */
> if (split_folio_to_list(folio, folio_list))
> goto keep_locked;
> }
>
>
>
> If the goal is to incorporate pmd-mapped shmem under thp_swpout* in
> /proc/vmstat,
> and if there is consistency between /proc/vmstat and sys regarding
> their definitions,
> then I have no objection to this patch. However, shmem_swpout and shmem_swpout_*
> appear more intuitive, given that thp_swpout_* in /proc/vmstat has
> never shown any
> increments for shmem until now - we have been always splitting shmem in vmscan.
>
> By the way, if this patch is accepted, it must be included in version
> 6.10 to maintain
> ABI compatibility. Additionally, documentation must be updated accordingly.
>
>>
>> [1]
>> https://lore.kernel.org/all/05d0096e4ec3e572d1d52d33a31a661321ac1551.1713755580.git.baolin.wang@linux.alibaba.com/
>
--
Best Regards,
Huang, Ying
On 2024/5/23 09:14, Huang, Ying wrote:
> Barry Song <[email protected]> writes:
>
>> On Wed, May 22, 2024 at 9:38 PM Baolin Wang
>> <[email protected]> wrote:
>>>
>>>
>>>
>>> On 2024/5/22 16:58, David Hildenbrand wrote:
>>>> On 22.05.24 10:51, Baolin Wang wrote:
>>>>> The mTHP swap related counters: 'anon_swpout' and
>>>>> 'anon_swpout_fallback' are
>>>>> confusing with an 'anon_' prefix, since the shmem can swap out
>>>>> non-anonymous
>>>>> pages. So drop the 'anon_' prefix to keep consistent with the old swap
>>>>> counter
>>>>> names.
>>>>>
>>>>> Suggested-by: "Huang, Ying" <[email protected]>
>>>>> Signed-off-by: Baolin Wang <[email protected]>
>>>>> ---
>>>>
>>>> Am I daydreaming or did we add the anon_ for a reason and discussed the
>>>> interaction with shmem? At least I remember some discussion around that.
>>>
>>> Do you mean the shmem mTHP allocation counters in previous
>>> discussion[1]? But for 'anon_swpout' and 'anon_swpout_fallback', I can
>>> not find previous discussions that provided a reason for adding the
>>> ‘anon_’ prefix. Barry, any comments? Thanks.
>>
>> HI Baolin,
>> We had tons of emails discussing about namin and I found this email,
>>
>> https://lore.kernel.org/all/[email protected]/
>>
>> David had this comment,
>> "I'm wondering if these should be ANON specific for now. We might want to
>> add others (shmem, file) in the future."
>>
>> This is likely how the 'anon_' prefix started being added, although it
>> wasn't specifically
>> targeting swapout.
>>
>> I sense your patch slightly alters the behavior of thp_swpout_fallback
>> in /proc/vmstat.
>> Previously, we didn't classify them as THP_SWPOUT_FALLBACK, even though we
>> always split them.
>
> IIUC, "fallback" means you try to do something, but fail, so try
> something else as fallback. If so, then we don't need to count
> splitting shmem large folio as fallback.
Agree. In additon, IIUC we have never counted splitting shmem large
folio as THP_SWPOUT_FALLBACK before or after this patch.
> For example, before commit 5ed890ce5147 ("mm: vmscan: avoid split during
> shrink_folio_list()"), if folio_entire_mapcount() == 0, we will split
> the THP. But we will not count it as "fallback" because we haven't
> tried to swap it out as a whole.
On Thu, May 23, 2024 at 1:38 PM Baolin Wang
<[email protected]> wrote:
>
>
>
> On 2024/5/23 09:14, Huang, Ying wrote:
> > Barry Song <[email protected]> writes:
> >
> >> On Wed, May 22, 2024 at 9:38 PM Baolin Wang
> >> <[email protected]> wrote:
> >>>
> >>>
> >>>
> >>> On 2024/5/22 16:58, David Hildenbrand wrote:
> >>>> On 22.05.24 10:51, Baolin Wang wrote:
> >>>>> The mTHP swap related counters: 'anon_swpout' and
> >>>>> 'anon_swpout_fallback' are
> >>>>> confusing with an 'anon_' prefix, since the shmem can swap out
> >>>>> non-anonymous
> >>>>> pages. So drop the 'anon_' prefix to keep consistent with the old swap
> >>>>> counter
> >>>>> names.
> >>>>>
> >>>>> Suggested-by: "Huang, Ying" <[email protected]>
> >>>>> Signed-off-by: Baolin Wang <[email protected]>
> >>>>> ---
> >>>>
> >>>> Am I daydreaming or did we add the anon_ for a reason and discussed the
> >>>> interaction with shmem? At least I remember some discussion around that.
> >>>
> >>> Do you mean the shmem mTHP allocation counters in previous
> >>> discussion[1]? But for 'anon_swpout' and 'anon_swpout_fallback', I can
> >>> not find previous discussions that provided a reason for adding the
> >>> ‘anon_’ prefix. Barry, any comments? Thanks.
> >>
> >> HI Baolin,
> >> We had tons of emails discussing about namin and I found this email,
> >>
> >> https://lore.kernel.org/all/[email protected]/
> >>
> >> David had this comment,
> >> "I'm wondering if these should be ANON specific for now. We might want to
> >> add others (shmem, file) in the future."
> >>
> >> This is likely how the 'anon_' prefix started being added, although it
> >> wasn't specifically
> >> targeting swapout.
> >>
> >> I sense your patch slightly alters the behavior of thp_swpout_fallback
> >> in /proc/vmstat.
> >> Previously, we didn't classify them as THP_SWPOUT_FALLBACK, even though we
> >> always split them.
> >
> > IIUC, "fallback" means you try to do something, but fail, so try
> > something else as fallback. If so, then we don't need to count
> > splitting shmem large folio as fallback.
>
> Agree. In additon, IIUC we have never counted splitting shmem large
> folio as THP_SWPOUT_FALLBACK before or after this patch.
Hi Baolin,
My point is that THP_SWPOUT* has been dedicated to anonymous memory for years
because we have not had the capability to perform THP_SWPOUT for shared memory
before. This is the historical context of thp_swpout* in /proc/vmstat,
even though it is
not ideal. Therefore, placing shmem sysfs entries in
/sys/kernel/mm/transparent_hugepage/hugepages-2048kB/stats
allows us to monitor SWPOUT and SWPOUT FALLBACK for shmem without altering
the tradition of /proc/vmstat.
But I am not firm on this because I don't see the necessity to
differentiate shmem's
swpout from anon's swpout. They basically seem the same while anon mTHP
faults might be significantly different from file mTHP faults, in which case we
must distinguish them. So please send version 2 with the updated documentation.
I believe it should target v6.10-rc rather than v6.11 to avoid ABI
conflicts if it is
accepted.
>
> > For example, before commit 5ed890ce5147 ("mm: vmscan: avoid split during
> > shrink_folio_list()"), if folio_entire_mapcount() == 0, we will split
> > the THP. But we will not count it as "fallback" because we haven't
> > tried to swap it out as a whole.
Thanks
Barry
On 2024/5/23 10:12, Barry Song wrote:
> On Thu, May 23, 2024 at 1:38 PM Baolin Wang
> <[email protected]> wrote:
>>
>>
>>
>> On 2024/5/23 09:14, Huang, Ying wrote:
>>> Barry Song <[email protected]> writes:
>>>
>>>> On Wed, May 22, 2024 at 9:38 PM Baolin Wang
>>>> <[email protected]> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2024/5/22 16:58, David Hildenbrand wrote:
>>>>>> On 22.05.24 10:51, Baolin Wang wrote:
>>>>>>> The mTHP swap related counters: 'anon_swpout' and
>>>>>>> 'anon_swpout_fallback' are
>>>>>>> confusing with an 'anon_' prefix, since the shmem can swap out
>>>>>>> non-anonymous
>>>>>>> pages. So drop the 'anon_' prefix to keep consistent with the old swap
>>>>>>> counter
>>>>>>> names.
>>>>>>>
>>>>>>> Suggested-by: "Huang, Ying" <[email protected]>
>>>>>>> Signed-off-by: Baolin Wang <[email protected]>
>>>>>>> ---
>>>>>>
>>>>>> Am I daydreaming or did we add the anon_ for a reason and discussed the
>>>>>> interaction with shmem? At least I remember some discussion around that.
>>>>>
>>>>> Do you mean the shmem mTHP allocation counters in previous
>>>>> discussion[1]? But for 'anon_swpout' and 'anon_swpout_fallback', I can
>>>>> not find previous discussions that provided a reason for adding the
>>>>> ‘anon_’ prefix. Barry, any comments? Thanks.
>>>>
>>>> HI Baolin,
>>>> We had tons of emails discussing about namin and I found this email,
>>>>
>>>> https://lore.kernel.org/all/[email protected]/
>>>>
>>>> David had this comment,
>>>> "I'm wondering if these should be ANON specific for now. We might want to
>>>> add others (shmem, file) in the future."
>>>>
>>>> This is likely how the 'anon_' prefix started being added, although it
>>>> wasn't specifically
>>>> targeting swapout.
>>>>
>>>> I sense your patch slightly alters the behavior of thp_swpout_fallback
>>>> in /proc/vmstat.
>>>> Previously, we didn't classify them as THP_SWPOUT_FALLBACK, even though we
>>>> always split them.
>>>
>>> IIUC, "fallback" means you try to do something, but fail, so try
>>> something else as fallback. If so, then we don't need to count
>>> splitting shmem large folio as fallback.
>>
>> Agree. In additon, IIUC we have never counted splitting shmem large
>> folio as THP_SWPOUT_FALLBACK before or after this patch.
>
> Hi Baolin,
>
> My point is that THP_SWPOUT* has been dedicated to anonymous memory for years
> because we have not had the capability to perform THP_SWPOUT for shared memory
> before. This is the historical context of thp_swpout* in /proc/vmstat,
> even though it is
> not ideal. Therefore, placing shmem sysfs entries in
> /sys/kernel/mm/transparent_hugepage/hugepages-2048kB/stats
> allows us to monitor SWPOUT and SWPOUT FALLBACK for shmem without altering
> the tradition of /proc/vmstat.
OK, I see your point. IMO this patch will not change the behaviors of
thp_swpout* in /proc/vmstat until adding support for large folio
swap-out for shmem[1]. Anyway we can talk about this in that thread.
[1]
https://lore.kernel.org/all/[email protected]/
> But I am not firm on this because I don't see the necessity to
> differentiate shmem's
> swpout from anon's swpout. They basically seem the same while anon mTHP
> faults might be significantly different from file mTHP faults, in which case we
> must distinguish them. So please send version 2 with the updated documentation.
> I believe it should target v6.10-rc rather than v6.11 to avoid ABI
> conflicts if it is
> accepted.
Sure. Will do. Thanks.