| Message ID | 0e2a6f232e7579a2e4407ecf075531980d97f286.1716367360.git.baolin.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: drop the 'anon_' prefix for swap-out mTHP counters | expand |
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" <ying.huang@intel.com> >> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >> --- > > 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 <baolin.wang@linux.alibaba.com> 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" <ying.huang@intel.com> > >> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> > >> --- > > > > 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@redhat.com/ 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 > <baolin.wang@linux.alibaba.com> 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" <ying.huang@intel.com> >>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >>>> --- >>> >>> 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@redhat.com/ > > 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/ce6be451-7c5a-402f-8340-be40699829c2@redhat.com/ > > 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 <baolin.wang@linux.alibaba.com> wrote: > > > > On 2024/5/22 18:40, Barry Song wrote: > > On Wed, May 22, 2024 at 9:38 PM Baolin Wang > > <baolin.wang@linux.alibaba.com> 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" <ying.huang@intel.com> > >>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> > >>>> --- > >>> > >>> 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@redhat.com/ > > > > 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/ce6be451-7c5a-402f-8340-be40699829c2@redhat.com/ > > > > 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 <baolin.wang@linux.alibaba.com> writes: > On 2024/5/22 18:40, Barry Song wrote: >> On Wed, May 22, 2024 at 9:38 PM Baolin Wang >> <baolin.wang@linux.alibaba.com> 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" <ying.huang@intel.com> >>>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >>>>> --- >>>> >>>> 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@redhat.com/ >> 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/ce6be451-7c5a-402f-8340-be40699829c2@redhat.com/ 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 <21cnbao@gmail.com> writes: > On Wed, May 22, 2024 at 9:38 PM Baolin Wang > <baolin.wang@linux.alibaba.com> 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" <ying.huang@intel.com> >> >> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >> >> --- >> > >> > 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@redhat.com/ > > 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 <21cnbao@gmail.com> writes: > >> On Wed, May 22, 2024 at 9:38 PM Baolin Wang >> <baolin.wang@linux.alibaba.com> 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" <ying.huang@intel.com> >>>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >>>>> --- >>>> >>>> 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@redhat.com/ >> >> 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 <baolin.wang@linux.alibaba.com> wrote: > > > > On 2024/5/23 09:14, Huang, Ying wrote: > > Barry Song <21cnbao@gmail.com> writes: > > > >> On Wed, May 22, 2024 at 9:38 PM Baolin Wang > >> <baolin.wang@linux.alibaba.com> 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" <ying.huang@intel.com> > >>>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> > >>>>> --- > >>>> > >>>> 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@redhat.com/ > >> > >> 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 > <baolin.wang@linux.alibaba.com> wrote: >> >> >> >> On 2024/5/23 09:14, Huang, Ying wrote: >>> Barry Song <21cnbao@gmail.com> writes: >>> >>>> On Wed, May 22, 2024 at 9:38 PM Baolin Wang >>>> <baolin.wang@linux.alibaba.com> 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" <ying.huang@intel.com> >>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >>>>>>> --- >>>>>> >>>>>> 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@redhat.com/ >>>> >>>> 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/cover.1716285099.git.baolin.wang@linux.alibaba.com/ > 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.
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;
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" <ying.huang@intel.com> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> --- 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(-)