* [PATCH v1] mm: shmem: Rename mTHP shmem counters
@ 2024-07-08 11:24 Ryan Roberts
2024-07-08 11:36 ` Barry Song
2024-07-09 1:26 ` Lance Yang
0 siblings, 2 replies; 15+ messages in thread
From: Ryan Roberts @ 2024-07-08 11:24 UTC (permalink / raw)
To: Andrew Morton, Hugh Dickins, Jonathan Corbet, Barry Song,
David Hildenbrand, Baolin Wang, Lance Yang, Matthew Wilcox,
Zi Yan, Daniel Gomez
Cc: Ryan Roberts, linux-kernel, linux-mm
The legacy PMD-sized THP counters at /proc/vmstat include
thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
rather confusingly refer to shmem THP and do not include any other types
of file pages. This is inconsistent since in most other places in the
kernel, THP counters are explicitly separated for anon, shmem and file
flavours. However, we are stuck with it since it constitutes a user ABI.
Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
anonymous shmem") added equivalent mTHP stats for shmem, keeping the
same "file_" prefix in the names. But in future, we may want to add
extra stats to cover actual file pages, at which point, it would all
become very confusing.
So let's take the opportunity to rename these new counters "shmem_"
before the change makes it upstream and the ABI becomes immutable.
Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
Hi All,
Applies on top of today's mm-unstable (2073cda629a4) and tested with mm
selftests; no regressions observed.
The backstory here is that I'd like to introduce some counters for regular file
folio allocations to observe how often large folio allocation succeeds, but
these shmem counters are named "file" which is going to make things confusing.
So hoping to solve that before commit 66f44583f9b6 ("mm: shmem: add mTHP
counters for anonymous shmem") goes upstream (it is currently in mm-stable).
Admittedly, this change means the mTHP stat names are not the same as the legacy
PMD-size THP names, but I think that's a smaller issue than having "file_" mTHP
stats that only count shmem, then having to introduce "file2_" or "pgcache_"
stats for the regular file memory, which is even more inconsistent IMHO. I guess
the alternative is to count both shmem and file in these mTHP stats (that's how
they were documented anyway) but I think it's better to be able to consider them
separately like we do for all the other counters.
Thanks,
Ryan
Documentation/admin-guide/mm/transhuge.rst | 12 ++++++------
include/linux/huge_mm.h | 6 +++---
mm/huge_memory.c | 12 ++++++------
mm/shmem.c | 8 ++++----
4 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
index 747c811ee8f1..8b891689fc13 100644
--- a/Documentation/admin-guide/mm/transhuge.rst
+++ b/Documentation/admin-guide/mm/transhuge.rst
@@ -496,16 +496,16 @@ swpout_fallback
Usually because failed to allocate some continuous swap space
for the huge page.
-file_alloc
- is incremented every time a file huge page is successfully
+shmem_alloc
+ is incremented every time a shmem huge page is successfully
allocated.
-file_fallback
- is incremented if a file huge page is attempted to be allocated
+shmem_fallback
+ is incremented if a shmem huge page is attempted to be allocated
but fails and instead falls back to using small pages.
-file_fallback_charge
- is incremented if a file huge page cannot be charged and instead
+shmem_fallback_charge
+ is incremented if a shmem huge page cannot be charged and instead
falls back to using small pages even though the allocation was
successful.
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index acb6ac24a07e..cff002be83eb 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -269,9 +269,9 @@ enum mthp_stat_item {
MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE,
MTHP_STAT_SWPOUT,
MTHP_STAT_SWPOUT_FALLBACK,
- MTHP_STAT_FILE_ALLOC,
- MTHP_STAT_FILE_FALLBACK,
- MTHP_STAT_FILE_FALLBACK_CHARGE,
+ MTHP_STAT_SHMEM_ALLOC,
+ MTHP_STAT_SHMEM_FALLBACK,
+ MTHP_STAT_SHMEM_FALLBACK_CHARGE,
MTHP_STAT_SPLIT,
MTHP_STAT_SPLIT_FAILED,
MTHP_STAT_SPLIT_DEFERRED,
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 9ec64aa2be94..f9696c94e211 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -568,9 +568,9 @@ 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(swpout, MTHP_STAT_SWPOUT);
DEFINE_MTHP_STAT_ATTR(swpout_fallback, MTHP_STAT_SWPOUT_FALLBACK);
-DEFINE_MTHP_STAT_ATTR(file_alloc, MTHP_STAT_FILE_ALLOC);
-DEFINE_MTHP_STAT_ATTR(file_fallback, MTHP_STAT_FILE_FALLBACK);
-DEFINE_MTHP_STAT_ATTR(file_fallback_charge, MTHP_STAT_FILE_FALLBACK_CHARGE);
+DEFINE_MTHP_STAT_ATTR(shmem_alloc, MTHP_STAT_SHMEM_ALLOC);
+DEFINE_MTHP_STAT_ATTR(shmem_fallback, MTHP_STAT_SHMEM_FALLBACK);
+DEFINE_MTHP_STAT_ATTR(shmem_fallback_charge, MTHP_STAT_SHMEM_FALLBACK_CHARGE);
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);
@@ -581,9 +581,9 @@ static struct attribute *stats_attrs[] = {
&anon_fault_fallback_charge_attr.attr,
&swpout_attr.attr,
&swpout_fallback_attr.attr,
- &file_alloc_attr.attr,
- &file_fallback_attr.attr,
- &file_fallback_charge_attr.attr,
+ &shmem_alloc_attr.attr,
+ &shmem_fallback_attr.attr,
+ &shmem_fallback_charge_attr.attr,
&split_attr.attr,
&split_failed_attr.attr,
&split_deferred_attr.attr,
diff --git a/mm/shmem.c b/mm/shmem.c
index 921d59c3d669..f24dfbd387ba 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1777,7 +1777,7 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
if (pages == HPAGE_PMD_NR)
count_vm_event(THP_FILE_FALLBACK);
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
- count_mthp_stat(order, MTHP_STAT_FILE_FALLBACK);
+ count_mthp_stat(order, MTHP_STAT_SHMEM_FALLBACK);
#endif
order = next_order(&suitable_orders, order);
}
@@ -1804,8 +1804,8 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
count_vm_event(THP_FILE_FALLBACK_CHARGE);
}
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
- count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK);
- count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK_CHARGE);
+ count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK);
+ count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK_CHARGE);
#endif
}
goto unlock;
@@ -2181,7 +2181,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
if (folio_test_pmd_mappable(folio))
count_vm_event(THP_FILE_ALLOC);
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
- count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_ALLOC);
+ count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_ALLOC);
#endif
goto alloced;
}
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-08 11:24 [PATCH v1] mm: shmem: Rename mTHP shmem counters Ryan Roberts
@ 2024-07-08 11:36 ` Barry Song
2024-07-08 12:29 ` Ryan Roberts
2024-07-09 1:26 ` Lance Yang
1 sibling, 1 reply; 15+ messages in thread
From: Barry Song @ 2024-07-08 11:36 UTC (permalink / raw)
To: Ryan Roberts
Cc: Andrew Morton, Hugh Dickins, Jonathan Corbet, David Hildenbrand,
Baolin Wang, Lance Yang, Matthew Wilcox, Zi Yan, Daniel Gomez,
linux-kernel, linux-mm
On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>
> The legacy PMD-sized THP counters at /proc/vmstat include
> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
> rather confusingly refer to shmem THP and do not include any other types
> of file pages. This is inconsistent since in most other places in the
> kernel, THP counters are explicitly separated for anon, shmem and file
> flavours. However, we are stuck with it since it constitutes a user ABI.
>
> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
> same "file_" prefix in the names. But in future, we may want to add
> extra stats to cover actual file pages, at which point, it would all
> become very confusing.
>
> So let's take the opportunity to rename these new counters "shmem_"
> before the change makes it upstream and the ABI becomes immutable.
Personally, I think this approach is much clearer. However, I recall
we discussed this
before [1], and it seems that inconsistency is a concern?
[1] https://lore.kernel.org/linux-mm/05d0096e4ec3e572d1d52d33a31a661321ac1551.1713755580.git.baolin.wang@linux.alibaba.com/
>
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>
> Hi All,
>
> Applies on top of today's mm-unstable (2073cda629a4) and tested with mm
> selftests; no regressions observed.
>
> The backstory here is that I'd like to introduce some counters for regular file
> folio allocations to observe how often large folio allocation succeeds, but
> these shmem counters are named "file" which is going to make things confusing.
> So hoping to solve that before commit 66f44583f9b6 ("mm: shmem: add mTHP
> counters for anonymous shmem") goes upstream (it is currently in mm-stable).
>
> Admittedly, this change means the mTHP stat names are not the same as the legacy
> PMD-size THP names, but I think that's a smaller issue than having "file_" mTHP
> stats that only count shmem, then having to introduce "file2_" or "pgcache_"
> stats for the regular file memory, which is even more inconsistent IMHO. I guess
> the alternative is to count both shmem and file in these mTHP stats (that's how
> they were documented anyway) but I think it's better to be able to consider them
> separately like we do for all the other counters.
>
> Thanks,
> Ryan
>
> Documentation/admin-guide/mm/transhuge.rst | 12 ++++++------
> include/linux/huge_mm.h | 6 +++---
> mm/huge_memory.c | 12 ++++++------
> mm/shmem.c | 8 ++++----
> 4 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> index 747c811ee8f1..8b891689fc13 100644
> --- a/Documentation/admin-guide/mm/transhuge.rst
> +++ b/Documentation/admin-guide/mm/transhuge.rst
> @@ -496,16 +496,16 @@ swpout_fallback
> Usually because failed to allocate some continuous swap space
> for the huge page.
>
> -file_alloc
> - is incremented every time a file huge page is successfully
> +shmem_alloc
> + is incremented every time a shmem huge page is successfully
> allocated.
>
> -file_fallback
> - is incremented if a file huge page is attempted to be allocated
> +shmem_fallback
> + is incremented if a shmem huge page is attempted to be allocated
> but fails and instead falls back to using small pages.
>
> -file_fallback_charge
> - is incremented if a file huge page cannot be charged and instead
> +shmem_fallback_charge
> + is incremented if a shmem huge page cannot be charged and instead
> falls back to using small pages even though the allocation was
> successful.
>
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index acb6ac24a07e..cff002be83eb 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -269,9 +269,9 @@ enum mthp_stat_item {
> MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE,
> MTHP_STAT_SWPOUT,
> MTHP_STAT_SWPOUT_FALLBACK,
> - MTHP_STAT_FILE_ALLOC,
> - MTHP_STAT_FILE_FALLBACK,
> - MTHP_STAT_FILE_FALLBACK_CHARGE,
> + MTHP_STAT_SHMEM_ALLOC,
> + MTHP_STAT_SHMEM_FALLBACK,
> + MTHP_STAT_SHMEM_FALLBACK_CHARGE,
> MTHP_STAT_SPLIT,
> MTHP_STAT_SPLIT_FAILED,
> MTHP_STAT_SPLIT_DEFERRED,
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 9ec64aa2be94..f9696c94e211 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -568,9 +568,9 @@ 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(swpout, MTHP_STAT_SWPOUT);
> DEFINE_MTHP_STAT_ATTR(swpout_fallback, MTHP_STAT_SWPOUT_FALLBACK);
> -DEFINE_MTHP_STAT_ATTR(file_alloc, MTHP_STAT_FILE_ALLOC);
> -DEFINE_MTHP_STAT_ATTR(file_fallback, MTHP_STAT_FILE_FALLBACK);
> -DEFINE_MTHP_STAT_ATTR(file_fallback_charge, MTHP_STAT_FILE_FALLBACK_CHARGE);
> +DEFINE_MTHP_STAT_ATTR(shmem_alloc, MTHP_STAT_SHMEM_ALLOC);
> +DEFINE_MTHP_STAT_ATTR(shmem_fallback, MTHP_STAT_SHMEM_FALLBACK);
> +DEFINE_MTHP_STAT_ATTR(shmem_fallback_charge, MTHP_STAT_SHMEM_FALLBACK_CHARGE);
> 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);
> @@ -581,9 +581,9 @@ static struct attribute *stats_attrs[] = {
> &anon_fault_fallback_charge_attr.attr,
> &swpout_attr.attr,
> &swpout_fallback_attr.attr,
> - &file_alloc_attr.attr,
> - &file_fallback_attr.attr,
> - &file_fallback_charge_attr.attr,
> + &shmem_alloc_attr.attr,
> + &shmem_fallback_attr.attr,
> + &shmem_fallback_charge_attr.attr,
> &split_attr.attr,
> &split_failed_attr.attr,
> &split_deferred_attr.attr,
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 921d59c3d669..f24dfbd387ba 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1777,7 +1777,7 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
> if (pages == HPAGE_PMD_NR)
> count_vm_event(THP_FILE_FALLBACK);
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> - count_mthp_stat(order, MTHP_STAT_FILE_FALLBACK);
> + count_mthp_stat(order, MTHP_STAT_SHMEM_FALLBACK);
> #endif
> order = next_order(&suitable_orders, order);
> }
> @@ -1804,8 +1804,8 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
> count_vm_event(THP_FILE_FALLBACK_CHARGE);
> }
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK);
> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK_CHARGE);
> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK);
> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK_CHARGE);
> #endif
> }
> goto unlock;
> @@ -2181,7 +2181,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> if (folio_test_pmd_mappable(folio))
> count_vm_event(THP_FILE_ALLOC);
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_ALLOC);
> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_ALLOC);
> #endif
> goto alloced;
> }
> --
> 2.43.0
>
Thanks
Barry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-08 11:36 ` Barry Song
@ 2024-07-08 12:29 ` Ryan Roberts
2024-07-08 20:50 ` David Hildenbrand
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Ryan Roberts @ 2024-07-08 12:29 UTC (permalink / raw)
To: Barry Song
Cc: Andrew Morton, Hugh Dickins, Jonathan Corbet, David Hildenbrand,
Baolin Wang, Lance Yang, Matthew Wilcox, Zi Yan, Daniel Gomez,
linux-kernel, linux-mm
On 08/07/2024 12:36, Barry Song wrote:
> On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>
>> The legacy PMD-sized THP counters at /proc/vmstat include
>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
>> rather confusingly refer to shmem THP and do not include any other types
>> of file pages. This is inconsistent since in most other places in the
>> kernel, THP counters are explicitly separated for anon, shmem and file
>> flavours. However, we are stuck with it since it constitutes a user ABI.
>>
>> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
>> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
>> same "file_" prefix in the names. But in future, we may want to add
>> extra stats to cover actual file pages, at which point, it would all
>> become very confusing.
>>
>> So let's take the opportunity to rename these new counters "shmem_"
>> before the change makes it upstream and the ABI becomes immutable.
>
> Personally, I think this approach is much clearer. However, I recall
> we discussed this
> before [1], and it seems that inconsistency is a concern?
Embarrassingly, I don't recall that converstation at all :-| but at least what I
said then is consistent with what I've done in this patch.
I think David's conclusion from that thread was to call them FILE_, and add both
shmem and pagecache counts to those counters, meaning we can keep the same name
as legacy THP counters. But those legacy THP counters only count shmem, and I
don't think we would get away with adding pagecache counts to those at this
point? (argument: they have been around for long time and there is a risk that
user space relies on them and if they were to dramatically increase due to
pagecache addition now that could break things). In that case, there is still
inconsistency, but its worse; the names are consistent but the semantics are
inconsistent.
So my vote is to change to SHMEM_ as per this patch :)
>
> [1] https://lore.kernel.org/linux-mm/05d0096e4ec3e572d1d52d33a31a661321ac1551.1713755580.git.baolin.wang@linux.alibaba.com/
>
>
>>
>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>> ---
>>
>> Hi All,
>>
>> Applies on top of today's mm-unstable (2073cda629a4) and tested with mm
>> selftests; no regressions observed.
>>
>> The backstory here is that I'd like to introduce some counters for regular file
>> folio allocations to observe how often large folio allocation succeeds, but
>> these shmem counters are named "file" which is going to make things confusing.
>> So hoping to solve that before commit 66f44583f9b6 ("mm: shmem: add mTHP
>> counters for anonymous shmem") goes upstream (it is currently in mm-stable).
>>
>> Admittedly, this change means the mTHP stat names are not the same as the legacy
>> PMD-size THP names, but I think that's a smaller issue than having "file_" mTHP
>> stats that only count shmem, then having to introduce "file2_" or "pgcache_"
>> stats for the regular file memory, which is even more inconsistent IMHO. I guess
>> the alternative is to count both shmem and file in these mTHP stats (that's how
>> they were documented anyway) but I think it's better to be able to consider them
>> separately like we do for all the other counters.
>>
>> Thanks,
>> Ryan
>>
>> Documentation/admin-guide/mm/transhuge.rst | 12 ++++++------
>> include/linux/huge_mm.h | 6 +++---
>> mm/huge_memory.c | 12 ++++++------
>> mm/shmem.c | 8 ++++----
>> 4 files changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
>> index 747c811ee8f1..8b891689fc13 100644
>> --- a/Documentation/admin-guide/mm/transhuge.rst
>> +++ b/Documentation/admin-guide/mm/transhuge.rst
>> @@ -496,16 +496,16 @@ swpout_fallback
>> Usually because failed to allocate some continuous swap space
>> for the huge page.
>>
>> -file_alloc
>> - is incremented every time a file huge page is successfully
>> +shmem_alloc
>> + is incremented every time a shmem huge page is successfully
>> allocated.
>>
>> -file_fallback
>> - is incremented if a file huge page is attempted to be allocated
>> +shmem_fallback
>> + is incremented if a shmem huge page is attempted to be allocated
>> but fails and instead falls back to using small pages.
>>
>> -file_fallback_charge
>> - is incremented if a file huge page cannot be charged and instead
>> +shmem_fallback_charge
>> + is incremented if a shmem huge page cannot be charged and instead
>> falls back to using small pages even though the allocation was
>> successful.
>>
>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>> index acb6ac24a07e..cff002be83eb 100644
>> --- a/include/linux/huge_mm.h
>> +++ b/include/linux/huge_mm.h
>> @@ -269,9 +269,9 @@ enum mthp_stat_item {
>> MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE,
>> MTHP_STAT_SWPOUT,
>> MTHP_STAT_SWPOUT_FALLBACK,
>> - MTHP_STAT_FILE_ALLOC,
>> - MTHP_STAT_FILE_FALLBACK,
>> - MTHP_STAT_FILE_FALLBACK_CHARGE,
>> + MTHP_STAT_SHMEM_ALLOC,
>> + MTHP_STAT_SHMEM_FALLBACK,
>> + MTHP_STAT_SHMEM_FALLBACK_CHARGE,
>> MTHP_STAT_SPLIT,
>> MTHP_STAT_SPLIT_FAILED,
>> MTHP_STAT_SPLIT_DEFERRED,
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 9ec64aa2be94..f9696c94e211 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -568,9 +568,9 @@ 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(swpout, MTHP_STAT_SWPOUT);
>> DEFINE_MTHP_STAT_ATTR(swpout_fallback, MTHP_STAT_SWPOUT_FALLBACK);
>> -DEFINE_MTHP_STAT_ATTR(file_alloc, MTHP_STAT_FILE_ALLOC);
>> -DEFINE_MTHP_STAT_ATTR(file_fallback, MTHP_STAT_FILE_FALLBACK);
>> -DEFINE_MTHP_STAT_ATTR(file_fallback_charge, MTHP_STAT_FILE_FALLBACK_CHARGE);
>> +DEFINE_MTHP_STAT_ATTR(shmem_alloc, MTHP_STAT_SHMEM_ALLOC);
>> +DEFINE_MTHP_STAT_ATTR(shmem_fallback, MTHP_STAT_SHMEM_FALLBACK);
>> +DEFINE_MTHP_STAT_ATTR(shmem_fallback_charge, MTHP_STAT_SHMEM_FALLBACK_CHARGE);
>> 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);
>> @@ -581,9 +581,9 @@ static struct attribute *stats_attrs[] = {
>> &anon_fault_fallback_charge_attr.attr,
>> &swpout_attr.attr,
>> &swpout_fallback_attr.attr,
>> - &file_alloc_attr.attr,
>> - &file_fallback_attr.attr,
>> - &file_fallback_charge_attr.attr,
>> + &shmem_alloc_attr.attr,
>> + &shmem_fallback_attr.attr,
>> + &shmem_fallback_charge_attr.attr,
>> &split_attr.attr,
>> &split_failed_attr.attr,
>> &split_deferred_attr.attr,
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 921d59c3d669..f24dfbd387ba 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -1777,7 +1777,7 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
>> if (pages == HPAGE_PMD_NR)
>> count_vm_event(THP_FILE_FALLBACK);
>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> - count_mthp_stat(order, MTHP_STAT_FILE_FALLBACK);
>> + count_mthp_stat(order, MTHP_STAT_SHMEM_FALLBACK);
>> #endif
>> order = next_order(&suitable_orders, order);
>> }
>> @@ -1804,8 +1804,8 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
>> count_vm_event(THP_FILE_FALLBACK_CHARGE);
>> }
>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK);
>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK_CHARGE);
>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK);
>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK_CHARGE);
>> #endif
>> }
>> goto unlock;
>> @@ -2181,7 +2181,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
>> if (folio_test_pmd_mappable(folio))
>> count_vm_event(THP_FILE_ALLOC);
>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_ALLOC);
>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_ALLOC);
>> #endif
>> goto alloced;
>> }
>> --
>> 2.43.0
>>
>
> Thanks
> Barry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-08 12:29 ` Ryan Roberts
@ 2024-07-08 20:50 ` David Hildenbrand
2024-07-09 1:21 ` Lance Yang
2024-07-09 7:47 ` Ryan Roberts
2024-07-09 1:07 ` Baolin Wang
2024-07-09 1:44 ` Barry Song
2 siblings, 2 replies; 15+ messages in thread
From: David Hildenbrand @ 2024-07-08 20:50 UTC (permalink / raw)
To: Ryan Roberts, Barry Song
Cc: Andrew Morton, Hugh Dickins, Jonathan Corbet, Baolin Wang,
Lance Yang, Matthew Wilcox, Zi Yan, Daniel Gomez, linux-kernel,
linux-mm
On 08.07.24 14:29, Ryan Roberts wrote:
> On 08/07/2024 12:36, Barry Song wrote:
>> On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>>
>>> The legacy PMD-sized THP counters at /proc/vmstat include
>>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
>>> rather confusingly refer to shmem THP and do not include any other types
>>> of file pages. This is inconsistent since in most other places in the
>>> kernel, THP counters are explicitly separated for anon, shmem and file
>>> flavours. However, we are stuck with it since it constitutes a user ABI.
>>>
>>> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
>>> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
>>> same "file_" prefix in the names. But in future, we may want to add
>>> extra stats to cover actual file pages, at which point, it would all
>>> become very confusing.
>>>
>>> So let's take the opportunity to rename these new counters "shmem_"
>>> before the change makes it upstream and the ABI becomes immutable.
>>
>> Personally, I think this approach is much clearer. However, I recall
>> we discussed this
>> before [1], and it seems that inconsistency is a concern?
>
> Embarrassingly, I don't recall that converstation at all :-| but at least what I
> said then is consistent with what I've done in this patch.
>
> I think David's conclusion from that thread was to call them FILE_, and add both
> shmem and pagecache counts to those counters, meaning we can keep the same name
> as legacy THP counters. But those legacy THP counters only count shmem, and I
> don't think we would get away with adding pagecache counts to those at this
> point? (argument: they have been around for long time and there is a risk that
> user space relies on them and if they were to dramatically increase due to
> pagecache addition now that could break things). In that case, there is still
> inconsistency, but its worse; the names are consistent but the semantics are
> inconsistent.
>
> So my vote is to change to SHMEM_ as per this patch :)
I also forgot most of the discussion, but these 3 legacy counters are
really only (currently) incremented for shmem. I think my idea was to
keep everything as FILE_ for now, maybe at some point make the pagecache
also use them, and then maybe have separate FILE_ + SHMEM_.
But yeah, likely it's best to only have "shmem" here for now, because
who knows what we can actually change about the legacy counters. But
it's always though messing with legacy stuff that is clearly suboptimal ...
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-08 12:29 ` Ryan Roberts
2024-07-08 20:50 ` David Hildenbrand
@ 2024-07-09 1:07 ` Baolin Wang
2024-07-09 1:44 ` Barry Song
2 siblings, 0 replies; 15+ messages in thread
From: Baolin Wang @ 2024-07-09 1:07 UTC (permalink / raw)
To: Ryan Roberts, Barry Song
Cc: Andrew Morton, Hugh Dickins, Jonathan Corbet, David Hildenbrand,
Lance Yang, Matthew Wilcox, Zi Yan, Daniel Gomez, linux-kernel,
linux-mm
On 2024/7/8 20:29, Ryan Roberts wrote:
> On 08/07/2024 12:36, Barry Song wrote:
>> On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>>
>>> The legacy PMD-sized THP counters at /proc/vmstat include
>>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
>>> rather confusingly refer to shmem THP and do not include any other types
>>> of file pages. This is inconsistent since in most other places in the
>>> kernel, THP counters are explicitly separated for anon, shmem and file
>>> flavours. However, we are stuck with it since it constitutes a user ABI.
>>>
>>> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
>>> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
>>> same "file_" prefix in the names. But in future, we may want to add
>>> extra stats to cover actual file pages, at which point, it would all
>>> become very confusing.
>>>
>>> So let's take the opportunity to rename these new counters "shmem_"
>>> before the change makes it upstream and the ABI becomes immutable.
>>
>> Personally, I think this approach is much clearer. However, I recall
>> we discussed this
>> before [1], and it seems that inconsistency is a concern?
>
> Embarrassingly, I don't recall that converstation at all :-| but at least what I
> said then is consistent with what I've done in this patch.
>
> I think David's conclusion from that thread was to call them FILE_, and add both
> shmem and pagecache counts to those counters, meaning we can keep the same name
> as legacy THP counters. But those legacy THP counters only count shmem, and I
> don't think we would get away with adding pagecache counts to those at this
> point? (argument: they have been around for long time and there is a risk that
> user space relies on them and if they were to dramatically increase due to
> pagecache addition now that could break things). In that case, there is still
> inconsistency, but its worse; the names are consistent but the semantics are
> inconsistent.
>
> So my vote is to change to SHMEM_ as per this patch :)
>
>>
>> [1] https://lore.kernel.org/linux-mm/05d0096e4ec3e572d1d52d33a31a661321ac1551.1713755580.git.baolin.wang@linux.alibaba.com/
My original preference was also for SHMEM-specific counters :)
So feel free to add:
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-08 20:50 ` David Hildenbrand
@ 2024-07-09 1:21 ` Lance Yang
2024-07-09 7:47 ` Ryan Roberts
1 sibling, 0 replies; 15+ messages in thread
From: Lance Yang @ 2024-07-09 1:21 UTC (permalink / raw)
To: David Hildenbrand
Cc: Ryan Roberts, Barry Song, Andrew Morton, Hugh Dickins,
Jonathan Corbet, Baolin Wang, Matthew Wilcox, Zi Yan,
Daniel Gomez, linux-kernel, linux-mm
On Tue, Jul 9, 2024 at 4:50 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 08.07.24 14:29, Ryan Roberts wrote:
> > On 08/07/2024 12:36, Barry Song wrote:
> >> On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
> >>>
> >>> The legacy PMD-sized THP counters at /proc/vmstat include
> >>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
> >>> rather confusingly refer to shmem THP and do not include any other types
> >>> of file pages. This is inconsistent since in most other places in the
> >>> kernel, THP counters are explicitly separated for anon, shmem and file
> >>> flavours. However, we are stuck with it since it constitutes a user ABI.
> >>>
> >>> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
> >>> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
> >>> same "file_" prefix in the names. But in future, we may want to add
> >>> extra stats to cover actual file pages, at which point, it would all
> >>> become very confusing.
> >>>
> >>> So let's take the opportunity to rename these new counters "shmem_"
> >>> before the change makes it upstream and the ABI becomes immutable.
> >>
> >> Personally, I think this approach is much clearer. However, I recall
> >> we discussed this
> >> before [1], and it seems that inconsistency is a concern?
> >
> > Embarrassingly, I don't recall that converstation at all :-| but at least what I
> > said then is consistent with what I've done in this patch.
> >
> > I think David's conclusion from that thread was to call them FILE_, and add both
> > shmem and pagecache counts to those counters, meaning we can keep the same name
> > as legacy THP counters. But those legacy THP counters only count shmem, and I
> > don't think we would get away with adding pagecache counts to those at this
> > point? (argument: they have been around for long time and there is a risk that
> > user space relies on them and if they were to dramatically increase due to
> > pagecache addition now that could break things). In that case, there is still
> > inconsistency, but its worse; the names are consistent but the semantics are
> > inconsistent.
> >
> > So my vote is to change to SHMEM_ as per this patch :)
>
> I also forgot most of the discussion, but these 3 legacy counters are
> really only (currently) incremented for shmem. I think my idea was to
> keep everything as FILE_ for now, maybe at some point make the pagecache
> also use them, and then maybe have separate FILE_ + SHMEM_.
>
> But yeah, likely it's best to only have "shmem" here for now, because
> who knows what we can actually change about the legacy counters. But
> it's always though messing with legacy stuff that is clearly suboptimal ...
Couldn't agree more! It's never an easy task to handle such matters :)
Perhaps, the time has come for us to separate FILE_ and SHMEM_.
Thanks,
Lance
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-08 11:24 [PATCH v1] mm: shmem: Rename mTHP shmem counters Ryan Roberts
2024-07-08 11:36 ` Barry Song
@ 2024-07-09 1:26 ` Lance Yang
1 sibling, 0 replies; 15+ messages in thread
From: Lance Yang @ 2024-07-09 1:26 UTC (permalink / raw)
To: Ryan Roberts
Cc: Andrew Morton, Hugh Dickins, Jonathan Corbet, Barry Song,
David Hildenbrand, Baolin Wang, Matthew Wilcox, Zi Yan,
Daniel Gomez, linux-kernel, linux-mm
On Mon, Jul 8, 2024 at 7:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>
> The legacy PMD-sized THP counters at /proc/vmstat include
> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
> rather confusingly refer to shmem THP and do not include any other types
> of file pages. This is inconsistent since in most other places in the
> kernel, THP counters are explicitly separated for anon, shmem and file
> flavours. However, we are stuck with it since it constitutes a user ABI.
>
> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
> same "file_" prefix in the names. But in future, we may want to add
> extra stats to cover actual file pages, at which point, it would all
> become very confusing.
>
> So let's take the opportunity to rename these new counters "shmem_"
> before the change makes it upstream and the ABI becomes immutable.
>
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
LGTM. Feel free to add:
Reviewed-by: Lance Yang <ioworker0@gmail.com>
Thanks,
Lance
> ---
>
> Hi All,
>
> Applies on top of today's mm-unstable (2073cda629a4) and tested with mm
> selftests; no regressions observed.
>
> The backstory here is that I'd like to introduce some counters for regular file
> folio allocations to observe how often large folio allocation succeeds, but
> these shmem counters are named "file" which is going to make things confusing.
> So hoping to solve that before commit 66f44583f9b6 ("mm: shmem: add mTHP
> counters for anonymous shmem") goes upstream (it is currently in mm-stable).
>
> Admittedly, this change means the mTHP stat names are not the same as the legacy
> PMD-size THP names, but I think that's a smaller issue than having "file_" mTHP
> stats that only count shmem, then having to introduce "file2_" or "pgcache_"
> stats for the regular file memory, which is even more inconsistent IMHO. I guess
> the alternative is to count both shmem and file in these mTHP stats (that's how
> they were documented anyway) but I think it's better to be able to consider them
> separately like we do for all the other counters.
>
> Thanks,
> Ryan
>
> Documentation/admin-guide/mm/transhuge.rst | 12 ++++++------
> include/linux/huge_mm.h | 6 +++---
> mm/huge_memory.c | 12 ++++++------
> mm/shmem.c | 8 ++++----
> 4 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> index 747c811ee8f1..8b891689fc13 100644
> --- a/Documentation/admin-guide/mm/transhuge.rst
> +++ b/Documentation/admin-guide/mm/transhuge.rst
> @@ -496,16 +496,16 @@ swpout_fallback
> Usually because failed to allocate some continuous swap space
> for the huge page.
>
> -file_alloc
> - is incremented every time a file huge page is successfully
> +shmem_alloc
> + is incremented every time a shmem huge page is successfully
> allocated.
>
> -file_fallback
> - is incremented if a file huge page is attempted to be allocated
> +shmem_fallback
> + is incremented if a shmem huge page is attempted to be allocated
> but fails and instead falls back to using small pages.
>
> -file_fallback_charge
> - is incremented if a file huge page cannot be charged and instead
> +shmem_fallback_charge
> + is incremented if a shmem huge page cannot be charged and instead
> falls back to using small pages even though the allocation was
> successful.
>
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index acb6ac24a07e..cff002be83eb 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -269,9 +269,9 @@ enum mthp_stat_item {
> MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE,
> MTHP_STAT_SWPOUT,
> MTHP_STAT_SWPOUT_FALLBACK,
> - MTHP_STAT_FILE_ALLOC,
> - MTHP_STAT_FILE_FALLBACK,
> - MTHP_STAT_FILE_FALLBACK_CHARGE,
> + MTHP_STAT_SHMEM_ALLOC,
> + MTHP_STAT_SHMEM_FALLBACK,
> + MTHP_STAT_SHMEM_FALLBACK_CHARGE,
> MTHP_STAT_SPLIT,
> MTHP_STAT_SPLIT_FAILED,
> MTHP_STAT_SPLIT_DEFERRED,
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 9ec64aa2be94..f9696c94e211 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -568,9 +568,9 @@ 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(swpout, MTHP_STAT_SWPOUT);
> DEFINE_MTHP_STAT_ATTR(swpout_fallback, MTHP_STAT_SWPOUT_FALLBACK);
> -DEFINE_MTHP_STAT_ATTR(file_alloc, MTHP_STAT_FILE_ALLOC);
> -DEFINE_MTHP_STAT_ATTR(file_fallback, MTHP_STAT_FILE_FALLBACK);
> -DEFINE_MTHP_STAT_ATTR(file_fallback_charge, MTHP_STAT_FILE_FALLBACK_CHARGE);
> +DEFINE_MTHP_STAT_ATTR(shmem_alloc, MTHP_STAT_SHMEM_ALLOC);
> +DEFINE_MTHP_STAT_ATTR(shmem_fallback, MTHP_STAT_SHMEM_FALLBACK);
> +DEFINE_MTHP_STAT_ATTR(shmem_fallback_charge, MTHP_STAT_SHMEM_FALLBACK_CHARGE);
> 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);
> @@ -581,9 +581,9 @@ static struct attribute *stats_attrs[] = {
> &anon_fault_fallback_charge_attr.attr,
> &swpout_attr.attr,
> &swpout_fallback_attr.attr,
> - &file_alloc_attr.attr,
> - &file_fallback_attr.attr,
> - &file_fallback_charge_attr.attr,
> + &shmem_alloc_attr.attr,
> + &shmem_fallback_attr.attr,
> + &shmem_fallback_charge_attr.attr,
> &split_attr.attr,
> &split_failed_attr.attr,
> &split_deferred_attr.attr,
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 921d59c3d669..f24dfbd387ba 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1777,7 +1777,7 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
> if (pages == HPAGE_PMD_NR)
> count_vm_event(THP_FILE_FALLBACK);
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> - count_mthp_stat(order, MTHP_STAT_FILE_FALLBACK);
> + count_mthp_stat(order, MTHP_STAT_SHMEM_FALLBACK);
> #endif
> order = next_order(&suitable_orders, order);
> }
> @@ -1804,8 +1804,8 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
> count_vm_event(THP_FILE_FALLBACK_CHARGE);
> }
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK);
> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK_CHARGE);
> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK);
> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK_CHARGE);
> #endif
> }
> goto unlock;
> @@ -2181,7 +2181,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> if (folio_test_pmd_mappable(folio))
> count_vm_event(THP_FILE_ALLOC);
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_ALLOC);
> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_ALLOC);
> #endif
> goto alloced;
> }
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-08 12:29 ` Ryan Roberts
2024-07-08 20:50 ` David Hildenbrand
2024-07-09 1:07 ` Baolin Wang
@ 2024-07-09 1:44 ` Barry Song
2024-07-09 7:55 ` Ryan Roberts
2 siblings, 1 reply; 15+ messages in thread
From: Barry Song @ 2024-07-09 1:44 UTC (permalink / raw)
To: ryan.roberts
Cc: akpm, baohua, baolin.wang, corbet, da.gomez, david, hughd,
ioworker0, linux-kernel, linux-mm, willy, ziy
On Tue, Jul 9, 2024 at 12:30 AM Ryan Roberts <ryan.roberts@arm.com> wrote:
>
> On 08/07/2024 12:36, Barry Song wrote:
> > On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
> >>
> >> The legacy PMD-sized THP counters at /proc/vmstat include
> >> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
> >> rather confusingly refer to shmem THP and do not include any other types
> >> of file pages. This is inconsistent since in most other places in the
> >> kernel, THP counters are explicitly separated for anon, shmem and file
> >> flavours. However, we are stuck with it since it constitutes a user ABI.
> >>
> >> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
> >> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
> >> same "file_" prefix in the names. But in future, we may want to add
> >> extra stats to cover actual file pages, at which point, it would all
> >> become very confusing.
> >>
> >> So let's take the opportunity to rename these new counters "shmem_"
> >> before the change makes it upstream and the ABI becomes immutable.
> >
> > Personally, I think this approach is much clearer. However, I recall
> > we discussed this
> > before [1], and it seems that inconsistency is a concern?
>
> Embarrassingly, I don't recall that converstation at all :-| but at least what I
> said then is consistent with what I've done in this patch.
>
> I think David's conclusion from that thread was to call them FILE_, and add both
> shmem and pagecache counts to those counters, meaning we can keep the same name
> as legacy THP counters. But those legacy THP counters only count shmem, and I
> don't think we would get away with adding pagecache counts to those at this
> point? (argument: they have been around for long time and there is a risk that
> user space relies on them and if they were to dramatically increase due to
> pagecache addition now that could break things). In that case, there is still
> inconsistency, but its worse; the names are consistent but the semantics are
> inconsistent.
>
> So my vote is to change to SHMEM_ as per this patch :)
I have no objections. However, I dislike the documentation for
thp_file_*. Perhaps we can clean it all up together ?
diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
index 709fe10b60f4..65df48cb3bbb 100644
--- a/Documentation/admin-guide/mm/transhuge.rst
+++ b/Documentation/admin-guide/mm/transhuge.rst
@@ -417,21 +417,22 @@ thp_collapse_alloc_failed
the allocation.
thp_file_alloc
- is incremented every time a file huge page is successfully
- allocated.
+ is incremented every time a file (including shmem) huge page is
+ successfully allocated.
thp_file_fallback
- is incremented if a file huge page is attempted to be allocated
- but fails and instead falls back to using small pages.
+ is incremented if a file (including shmem) huge page is attempted
+ to be allocated but fails and instead falls back to using small
+ pages.
thp_file_fallback_charge
- is incremented if a file huge page cannot be charged and instead
- falls back to using small pages even though the allocation was
- successful.
+ is incremented if a file (including shmem) huge page cannot be
+ charged and instead falls back to using small pages even though
+ the allocation was successful.
thp_file_mapped
- is incremented every time a file huge page is mapped into
- user address space.
+ is incremented every time a file (including shmem) huge page is
+ mapped into user address space.
thp_split_page
is incremented every time a huge page is split into base
>
> >
> > [1] https://lore.kernel.org/linux-mm/05d0096e4ec3e572d1d52d33a31a661321ac1551.1713755580.git.baolin.wang@linux.alibaba.com/
> >
> >
> >>
> >> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> >> ---
> >>
> >> Hi All,
> >>
> >> Applies on top of today's mm-unstable (2073cda629a4) and tested with mm
> >> selftests; no regressions observed.
> >>
> >> The backstory here is that I'd like to introduce some counters for regular file
> >> folio allocations to observe how often large folio allocation succeeds, but
> >> these shmem counters are named "file" which is going to make things confusing.
> >> So hoping to solve that before commit 66f44583f9b6 ("mm: shmem: add mTHP
> >> counters for anonymous shmem") goes upstream (it is currently in mm-stable).
> >>
> >> Admittedly, this change means the mTHP stat names are not the same as the legacy
> >> PMD-size THP names, but I think that's a smaller issue than having "file_" mTHP
> >> stats that only count shmem, then having to introduce "file2_" or "pgcache_"
> >> stats for the regular file memory, which is even more inconsistent IMHO. I guess
> >> the alternative is to count both shmem and file in these mTHP stats (that's how
> >> they were documented anyway) but I think it's better to be able to consider them
> >> separately like we do for all the other counters.
> >>
> >> Thanks,
> >> Ryan
> >>
> >> Documentation/admin-guide/mm/transhuge.rst | 12 ++++++------
> >> include/linux/huge_mm.h | 6 +++---
> >> mm/huge_memory.c | 12 ++++++------
> >> mm/shmem.c | 8 ++++----
> >> 4 files changed, 19 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> >> index 747c811ee8f1..8b891689fc13 100644
> >> --- a/Documentation/admin-guide/mm/transhuge.rst
> >> +++ b/Documentation/admin-guide/mm/transhuge.rst
> >> @@ -496,16 +496,16 @@ swpout_fallback
> >> Usually because failed to allocate some continuous swap space
> >> for the huge page.
> >>
> >> -file_alloc
> >> - is incremented every time a file huge page is successfully
> >> +shmem_alloc
> >> + is incremented every time a shmem huge page is successfully
> >> allocated.
> >>
> >> -file_fallback
> >> - is incremented if a file huge page is attempted to be allocated
> >> +shmem_fallback
> >> + is incremented if a shmem huge page is attempted to be allocated
> >> but fails and instead falls back to using small pages.
> >>
> >> -file_fallback_charge
> >> - is incremented if a file huge page cannot be charged and instead
> >> +shmem_fallback_charge
> >> + is incremented if a shmem huge page cannot be charged and instead
> >> falls back to using small pages even though the allocation was
> >> successful.
> >>
> >> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >> index acb6ac24a07e..cff002be83eb 100644
> >> --- a/include/linux/huge_mm.h
> >> +++ b/include/linux/huge_mm.h
> >> @@ -269,9 +269,9 @@ enum mthp_stat_item {
> >> MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE,
> >> MTHP_STAT_SWPOUT,
> >> MTHP_STAT_SWPOUT_FALLBACK,
> >> - MTHP_STAT_FILE_ALLOC,
> >> - MTHP_STAT_FILE_FALLBACK,
> >> - MTHP_STAT_FILE_FALLBACK_CHARGE,
> >> + MTHP_STAT_SHMEM_ALLOC,
> >> + MTHP_STAT_SHMEM_FALLBACK,
> >> + MTHP_STAT_SHMEM_FALLBACK_CHARGE,
> >> MTHP_STAT_SPLIT,
> >> MTHP_STAT_SPLIT_FAILED,
> >> MTHP_STAT_SPLIT_DEFERRED,
> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >> index 9ec64aa2be94..f9696c94e211 100644
> >> --- a/mm/huge_memory.c
> >> +++ b/mm/huge_memory.c
> >> @@ -568,9 +568,9 @@ 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(swpout, MTHP_STAT_SWPOUT);
> >> DEFINE_MTHP_STAT_ATTR(swpout_fallback, MTHP_STAT_SWPOUT_FALLBACK);
> >> -DEFINE_MTHP_STAT_ATTR(file_alloc, MTHP_STAT_FILE_ALLOC);
> >> -DEFINE_MTHP_STAT_ATTR(file_fallback, MTHP_STAT_FILE_FALLBACK);
> >> -DEFINE_MTHP_STAT_ATTR(file_fallback_charge, MTHP_STAT_FILE_FALLBACK_CHARGE);
> >> +DEFINE_MTHP_STAT_ATTR(shmem_alloc, MTHP_STAT_SHMEM_ALLOC);
> >> +DEFINE_MTHP_STAT_ATTR(shmem_fallback, MTHP_STAT_SHMEM_FALLBACK);
> >> +DEFINE_MTHP_STAT_ATTR(shmem_fallback_charge, MTHP_STAT_SHMEM_FALLBACK_CHARGE);
> >> 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);
> >> @@ -581,9 +581,9 @@ static struct attribute *stats_attrs[] = {
> >> &anon_fault_fallback_charge_attr.attr,
> >> &swpout_attr.attr,
> >> &swpout_fallback_attr.attr,
> >> - &file_alloc_attr.attr,
> >> - &file_fallback_attr.attr,
> >> - &file_fallback_charge_attr.attr,
> >> + &shmem_alloc_attr.attr,
> >> + &shmem_fallback_attr.attr,
> >> + &shmem_fallback_charge_attr.attr,
> >> &split_attr.attr,
> >> &split_failed_attr.attr,
> >> &split_deferred_attr.attr,
> >> diff --git a/mm/shmem.c b/mm/shmem.c
> >> index 921d59c3d669..f24dfbd387ba 100644
> >> --- a/mm/shmem.c
> >> +++ b/mm/shmem.c
> >> @@ -1777,7 +1777,7 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
> >> if (pages == HPAGE_PMD_NR)
> >> count_vm_event(THP_FILE_FALLBACK);
> >> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >> - count_mthp_stat(order, MTHP_STAT_FILE_FALLBACK);
> >> + count_mthp_stat(order, MTHP_STAT_SHMEM_FALLBACK);
> >> #endif
> >> order = next_order(&suitable_orders, order);
> >> }
> >> @@ -1804,8 +1804,8 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
> >> count_vm_event(THP_FILE_FALLBACK_CHARGE);
> >> }
> >> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK);
> >> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK_CHARGE);
> >> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK);
> >> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK_CHARGE);
> >> #endif
> >> }
> >> goto unlock;
> >> @@ -2181,7 +2181,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> >> if (folio_test_pmd_mappable(folio))
> >> count_vm_event(THP_FILE_ALLOC);
> >> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_ALLOC);
> >> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_ALLOC);
> >> #endif
> >> goto alloced;
> >> }
> >> --
> >> 2.43.0
> >>
> >
Thanks
Barry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-08 20:50 ` David Hildenbrand
2024-07-09 1:21 ` Lance Yang
@ 2024-07-09 7:47 ` Ryan Roberts
2024-07-09 7:54 ` David Hildenbrand
1 sibling, 1 reply; 15+ messages in thread
From: Ryan Roberts @ 2024-07-09 7:47 UTC (permalink / raw)
To: David Hildenbrand, Barry Song
Cc: Andrew Morton, Hugh Dickins, Jonathan Corbet, Baolin Wang,
Lance Yang, Matthew Wilcox, Zi Yan, Daniel Gomez, linux-kernel,
linux-mm
On 08/07/2024 21:50, David Hildenbrand wrote:
> On 08.07.24 14:29, Ryan Roberts wrote:
>> On 08/07/2024 12:36, Barry Song wrote:
>>> On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>>>
>>>> The legacy PMD-sized THP counters at /proc/vmstat include
>>>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
>>>> rather confusingly refer to shmem THP and do not include any other types
>>>> of file pages. This is inconsistent since in most other places in the
>>>> kernel, THP counters are explicitly separated for anon, shmem and file
>>>> flavours. However, we are stuck with it since it constitutes a user ABI.
>>>>
>>>> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
>>>> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
>>>> same "file_" prefix in the names. But in future, we may want to add
>>>> extra stats to cover actual file pages, at which point, it would all
>>>> become very confusing.
>>>>
>>>> So let's take the opportunity to rename these new counters "shmem_"
>>>> before the change makes it upstream and the ABI becomes immutable.
>>>
>>> Personally, I think this approach is much clearer. However, I recall
>>> we discussed this
>>> before [1], and it seems that inconsistency is a concern?
>>
>> Embarrassingly, I don't recall that converstation at all :-| but at least what I
>> said then is consistent with what I've done in this patch.
>>
>> I think David's conclusion from that thread was to call them FILE_, and add both
>> shmem and pagecache counts to those counters, meaning we can keep the same name
>> as legacy THP counters. But those legacy THP counters only count shmem, and I
>> don't think we would get away with adding pagecache counts to those at this
>> point? (argument: they have been around for long time and there is a risk that
>> user space relies on them and if they were to dramatically increase due to
>> pagecache addition now that could break things). In that case, there is still
>> inconsistency, but its worse; the names are consistent but the semantics are
>> inconsistent.
>>
>> So my vote is to change to SHMEM_ as per this patch :)
>
> I also forgot most of the discussion, but these 3 legacy counters are really
> only (currently) incremented for shmem. I think my idea was to keep everything
> as FILE_ for now, maybe at some point make the pagecache also use them, and then
> maybe have separate FILE_ + SHMEM_.
>
> But yeah, likely it's best to only have "shmem" here for now, because who knows
> what we can actually change about the legacy counters. But it's always though
> messing with legacy stuff that is clearly suboptimal ...
Sorry David, I've read your response a bunch of times and am still not
completely sure what you are advocating for.
To make my proposal crystal clear, I think we should leave the legacy counters
alone - neither change their name nor what they count (which is _only_ shmem). I
think we should rename the new mTHP counters to "shmem" and have them continue
to only count shmem. Then additionally, I think we should introduce new "file"
mTHP counters that count the page cache allocations (that's a future set of
patches, which I'm working on together with user controls to determine which
mTHP sizes can be used by page cache).
As suggested by Barry, I propose to also improve the documentation for the
legacy counters to make it clear that dispite their name being "file" they are
actually counting "shmem". I'll do this for v2.
David, would you support this approach? If so, I'd like to push this forwards
asap so that it gets into v6.11 to avoid ever exposing the mthp counters with
the "file" name.
Thanks,
Ryan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-09 7:47 ` Ryan Roberts
@ 2024-07-09 7:54 ` David Hildenbrand
2024-07-09 7:59 ` Ryan Roberts
0 siblings, 1 reply; 15+ messages in thread
From: David Hildenbrand @ 2024-07-09 7:54 UTC (permalink / raw)
To: Ryan Roberts, Barry Song
Cc: Andrew Morton, Hugh Dickins, Jonathan Corbet, Baolin Wang,
Lance Yang, Matthew Wilcox, Zi Yan, Daniel Gomez, linux-kernel,
linux-mm
On 09.07.24 09:47, Ryan Roberts wrote:
> On 08/07/2024 21:50, David Hildenbrand wrote:
>> On 08.07.24 14:29, Ryan Roberts wrote:
>>> On 08/07/2024 12:36, Barry Song wrote:
>>>> On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>>>>
>>>>> The legacy PMD-sized THP counters at /proc/vmstat include
>>>>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
>>>>> rather confusingly refer to shmem THP and do not include any other types
>>>>> of file pages. This is inconsistent since in most other places in the
>>>>> kernel, THP counters are explicitly separated for anon, shmem and file
>>>>> flavours. However, we are stuck with it since it constitutes a user ABI.
>>>>>
>>>>> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
>>>>> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
>>>>> same "file_" prefix in the names. But in future, we may want to add
>>>>> extra stats to cover actual file pages, at which point, it would all
>>>>> become very confusing.
>>>>>
>>>>> So let's take the opportunity to rename these new counters "shmem_"
>>>>> before the change makes it upstream and the ABI becomes immutable.
>>>>
>>>> Personally, I think this approach is much clearer. However, I recall
>>>> we discussed this
>>>> before [1], and it seems that inconsistency is a concern?
>>>
>>> Embarrassingly, I don't recall that converstation at all :-| but at least what I
>>> said then is consistent with what I've done in this patch.
>>>
>>> I think David's conclusion from that thread was to call them FILE_, and add both
>>> shmem and pagecache counts to those counters, meaning we can keep the same name
>>> as legacy THP counters. But those legacy THP counters only count shmem, and I
>>> don't think we would get away with adding pagecache counts to those at this
>>> point? (argument: they have been around for long time and there is a risk that
>>> user space relies on them and if they were to dramatically increase due to
>>> pagecache addition now that could break things). In that case, there is still
>>> inconsistency, but its worse; the names are consistent but the semantics are
>>> inconsistent.
>>>
>>> So my vote is to change to SHMEM_ as per this patch :)
>>
>> I also forgot most of the discussion, but these 3 legacy counters are really
>> only (currently) incremented for shmem. I think my idea was to keep everything
>> as FILE_ for now, maybe at some point make the pagecache also use them, and then
>> maybe have separate FILE_ + SHMEM_.
>>
>> But yeah, likely it's best to only have "shmem" here for now, because who knows
>> what we can actually change about the legacy counters. But it's always though
>> messing with legacy stuff that is clearly suboptimal ...
>
> Sorry David, I've read your response a bunch of times and am still not
> completely sure what you are advocating for.
>
> To make my proposal crystal clear, I think we should leave the legacy counters
> alone - neither change their name nor what they count (which is _only_ shmem). I
> think we should rename the new mTHP counters to "shmem" and have them continue
> to only count shmem. Then additionally, I think we should introduce new "file"
> mTHP counters that count the page cache allocations (that's a future set of
> patches, which I'm working on together with user controls to determine which
> mTHP sizes can be used by page cache).
>
> As suggested by Barry, I propose to also improve the documentation for the
> legacy counters to make it clear that dispite their name being "file" they are
> actually counting "shmem". I'll do this for v2.
Yes, and please. Likely we should document for the legacy ones (if not
already done) that they only track PMD-sized THPs.
>
> David, would you support this approach? If so, I'd like to push this forwards
> asap so that it gets into v6.11 to avoid ever exposing the mthp counters with
> the "file" name.
Yes, sorry for not being clear.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-09 1:44 ` Barry Song
@ 2024-07-09 7:55 ` Ryan Roberts
2024-07-09 8:13 ` Barry Song
0 siblings, 1 reply; 15+ messages in thread
From: Ryan Roberts @ 2024-07-09 7:55 UTC (permalink / raw)
To: Barry Song
Cc: akpm, baohua, baolin.wang, corbet, da.gomez, david, hughd,
ioworker0, linux-kernel, linux-mm, willy, ziy
On 09/07/2024 02:44, Barry Song wrote:
> On Tue, Jul 9, 2024 at 12:30 AM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>
>> On 08/07/2024 12:36, Barry Song wrote:
>>> On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>>>
>>>> The legacy PMD-sized THP counters at /proc/vmstat include
>>>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
>>>> rather confusingly refer to shmem THP and do not include any other types
>>>> of file pages. This is inconsistent since in most other places in the
>>>> kernel, THP counters are explicitly separated for anon, shmem and file
>>>> flavours. However, we are stuck with it since it constitutes a user ABI.
>>>>
>>>> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
>>>> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
>>>> same "file_" prefix in the names. But in future, we may want to add
>>>> extra stats to cover actual file pages, at which point, it would all
>>>> become very confusing.
>>>>
>>>> So let's take the opportunity to rename these new counters "shmem_"
>>>> before the change makes it upstream and the ABI becomes immutable.
>>>
>>> Personally, I think this approach is much clearer. However, I recall
>>> we discussed this
>>> before [1], and it seems that inconsistency is a concern?
>>
>> Embarrassingly, I don't recall that converstation at all :-| but at least what I
>> said then is consistent with what I've done in this patch.
>>
>> I think David's conclusion from that thread was to call them FILE_, and add both
>> shmem and pagecache counts to those counters, meaning we can keep the same name
>> as legacy THP counters. But those legacy THP counters only count shmem, and I
>> don't think we would get away with adding pagecache counts to those at this
>> point? (argument: they have been around for long time and there is a risk that
>> user space relies on them and if they were to dramatically increase due to
>> pagecache addition now that could break things). In that case, there is still
>> inconsistency, but its worse; the names are consistent but the semantics are
>> inconsistent.
>>
>> So my vote is to change to SHMEM_ as per this patch :)
>
> I have no objections. However, I dislike the documentation for
> thp_file_*. Perhaps we can clean it all up together ?
I agree that we should clean this documentation up and I'm happy to roll it into
v2. However, I don't think what you have suggested is quite right.
thp_file_alloc, thp_file_fallback and thp_file_fallback_charge *only* count
shmem. They don't count pagecache. So perhaps the change should be "...every
time a shmem huge page (dispite being named after "file", the counter measures
only shmem) is..."?
thp_file_mapped includes both file and shmem, so agree with your change there.
What do you think?
>
> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> index 709fe10b60f4..65df48cb3bbb 100644
> --- a/Documentation/admin-guide/mm/transhuge.rst
> +++ b/Documentation/admin-guide/mm/transhuge.rst
> @@ -417,21 +417,22 @@ thp_collapse_alloc_failed
> the allocation.
>
> thp_file_alloc
> - is incremented every time a file huge page is successfully
> - allocated.
> + is incremented every time a file (including shmem) huge page is
> + successfully allocated.
>
> thp_file_fallback
> - is incremented if a file huge page is attempted to be allocated
> - but fails and instead falls back to using small pages.
> + is incremented if a file (including shmem) huge page is attempted
> + to be allocated but fails and instead falls back to using small
> + pages.
>
> thp_file_fallback_charge
> - is incremented if a file huge page cannot be charged and instead
> - falls back to using small pages even though the allocation was
> - successful.
> + is incremented if a file (including shmem) huge page cannot be
> + charged and instead falls back to using small pages even though
> + the allocation was successful.
>
> thp_file_mapped
> - is incremented every time a file huge page is mapped into
> - user address space.
> + is incremented every time a file (including shmem) huge page is
> + mapped into user address space.
>
> thp_split_page
> is incremented every time a huge page is split into base
>
>>
>>>
>>> [1] https://lore.kernel.org/linux-mm/05d0096e4ec3e572d1d52d33a31a661321ac1551.1713755580.git.baolin.wang@linux.alibaba.com/
>>>
>>>
>>>>
>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>> ---
>>>>
>>>> Hi All,
>>>>
>>>> Applies on top of today's mm-unstable (2073cda629a4) and tested with mm
>>>> selftests; no regressions observed.
>>>>
>>>> The backstory here is that I'd like to introduce some counters for regular file
>>>> folio allocations to observe how often large folio allocation succeeds, but
>>>> these shmem counters are named "file" which is going to make things confusing.
>>>> So hoping to solve that before commit 66f44583f9b6 ("mm: shmem: add mTHP
>>>> counters for anonymous shmem") goes upstream (it is currently in mm-stable).
>>>>
>>>> Admittedly, this change means the mTHP stat names are not the same as the legacy
>>>> PMD-size THP names, but I think that's a smaller issue than having "file_" mTHP
>>>> stats that only count shmem, then having to introduce "file2_" or "pgcache_"
>>>> stats for the regular file memory, which is even more inconsistent IMHO. I guess
>>>> the alternative is to count both shmem and file in these mTHP stats (that's how
>>>> they were documented anyway) but I think it's better to be able to consider them
>>>> separately like we do for all the other counters.
>>>>
>>>> Thanks,
>>>> Ryan
>>>>
>>>> Documentation/admin-guide/mm/transhuge.rst | 12 ++++++------
>>>> include/linux/huge_mm.h | 6 +++---
>>>> mm/huge_memory.c | 12 ++++++------
>>>> mm/shmem.c | 8 ++++----
>>>> 4 files changed, 19 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
>>>> index 747c811ee8f1..8b891689fc13 100644
>>>> --- a/Documentation/admin-guide/mm/transhuge.rst
>>>> +++ b/Documentation/admin-guide/mm/transhuge.rst
>>>> @@ -496,16 +496,16 @@ swpout_fallback
>>>> Usually because failed to allocate some continuous swap space
>>>> for the huge page.
>>>>
>>>> -file_alloc
>>>> - is incremented every time a file huge page is successfully
>>>> +shmem_alloc
>>>> + is incremented every time a shmem huge page is successfully
>>>> allocated.
>>>>
>>>> -file_fallback
>>>> - is incremented if a file huge page is attempted to be allocated
>>>> +shmem_fallback
>>>> + is incremented if a shmem huge page is attempted to be allocated
>>>> but fails and instead falls back to using small pages.
>>>>
>>>> -file_fallback_charge
>>>> - is incremented if a file huge page cannot be charged and instead
>>>> +shmem_fallback_charge
>>>> + is incremented if a shmem huge page cannot be charged and instead
>>>> falls back to using small pages even though the allocation was
>>>> successful.
>>>>
>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>> index acb6ac24a07e..cff002be83eb 100644
>>>> --- a/include/linux/huge_mm.h
>>>> +++ b/include/linux/huge_mm.h
>>>> @@ -269,9 +269,9 @@ enum mthp_stat_item {
>>>> MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE,
>>>> MTHP_STAT_SWPOUT,
>>>> MTHP_STAT_SWPOUT_FALLBACK,
>>>> - MTHP_STAT_FILE_ALLOC,
>>>> - MTHP_STAT_FILE_FALLBACK,
>>>> - MTHP_STAT_FILE_FALLBACK_CHARGE,
>>>> + MTHP_STAT_SHMEM_ALLOC,
>>>> + MTHP_STAT_SHMEM_FALLBACK,
>>>> + MTHP_STAT_SHMEM_FALLBACK_CHARGE,
>>>> MTHP_STAT_SPLIT,
>>>> MTHP_STAT_SPLIT_FAILED,
>>>> MTHP_STAT_SPLIT_DEFERRED,
>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>> index 9ec64aa2be94..f9696c94e211 100644
>>>> --- a/mm/huge_memory.c
>>>> +++ b/mm/huge_memory.c
>>>> @@ -568,9 +568,9 @@ 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(swpout, MTHP_STAT_SWPOUT);
>>>> DEFINE_MTHP_STAT_ATTR(swpout_fallback, MTHP_STAT_SWPOUT_FALLBACK);
>>>> -DEFINE_MTHP_STAT_ATTR(file_alloc, MTHP_STAT_FILE_ALLOC);
>>>> -DEFINE_MTHP_STAT_ATTR(file_fallback, MTHP_STAT_FILE_FALLBACK);
>>>> -DEFINE_MTHP_STAT_ATTR(file_fallback_charge, MTHP_STAT_FILE_FALLBACK_CHARGE);
>>>> +DEFINE_MTHP_STAT_ATTR(shmem_alloc, MTHP_STAT_SHMEM_ALLOC);
>>>> +DEFINE_MTHP_STAT_ATTR(shmem_fallback, MTHP_STAT_SHMEM_FALLBACK);
>>>> +DEFINE_MTHP_STAT_ATTR(shmem_fallback_charge, MTHP_STAT_SHMEM_FALLBACK_CHARGE);
>>>> 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);
>>>> @@ -581,9 +581,9 @@ static struct attribute *stats_attrs[] = {
>>>> &anon_fault_fallback_charge_attr.attr,
>>>> &swpout_attr.attr,
>>>> &swpout_fallback_attr.attr,
>>>> - &file_alloc_attr.attr,
>>>> - &file_fallback_attr.attr,
>>>> - &file_fallback_charge_attr.attr,
>>>> + &shmem_alloc_attr.attr,
>>>> + &shmem_fallback_attr.attr,
>>>> + &shmem_fallback_charge_attr.attr,
>>>> &split_attr.attr,
>>>> &split_failed_attr.attr,
>>>> &split_deferred_attr.attr,
>>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>>> index 921d59c3d669..f24dfbd387ba 100644
>>>> --- a/mm/shmem.c
>>>> +++ b/mm/shmem.c
>>>> @@ -1777,7 +1777,7 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
>>>> if (pages == HPAGE_PMD_NR)
>>>> count_vm_event(THP_FILE_FALLBACK);
>>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>> - count_mthp_stat(order, MTHP_STAT_FILE_FALLBACK);
>>>> + count_mthp_stat(order, MTHP_STAT_SHMEM_FALLBACK);
>>>> #endif
>>>> order = next_order(&suitable_orders, order);
>>>> }
>>>> @@ -1804,8 +1804,8 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
>>>> count_vm_event(THP_FILE_FALLBACK_CHARGE);
>>>> }
>>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK);
>>>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK_CHARGE);
>>>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK);
>>>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK_CHARGE);
>>>> #endif
>>>> }
>>>> goto unlock;
>>>> @@ -2181,7 +2181,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
>>>> if (folio_test_pmd_mappable(folio))
>>>> count_vm_event(THP_FILE_ALLOC);
>>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_ALLOC);
>>>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_ALLOC);
>>>> #endif
>>>> goto alloced;
>>>> }
>>>> --
>>>> 2.43.0
>>>>
>>>
>
> Thanks
> Barry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-09 7:54 ` David Hildenbrand
@ 2024-07-09 7:59 ` Ryan Roberts
0 siblings, 0 replies; 15+ messages in thread
From: Ryan Roberts @ 2024-07-09 7:59 UTC (permalink / raw)
To: David Hildenbrand, Barry Song
Cc: Andrew Morton, Hugh Dickins, Jonathan Corbet, Baolin Wang,
Lance Yang, Matthew Wilcox, Zi Yan, Daniel Gomez, linux-kernel,
linux-mm
On 09/07/2024 08:54, David Hildenbrand wrote:
> On 09.07.24 09:47, Ryan Roberts wrote:
>> On 08/07/2024 21:50, David Hildenbrand wrote:
>>> On 08.07.24 14:29, Ryan Roberts wrote:
>>>> On 08/07/2024 12:36, Barry Song wrote:
>>>>> On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>>>>>
>>>>>> The legacy PMD-sized THP counters at /proc/vmstat include
>>>>>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
>>>>>> rather confusingly refer to shmem THP and do not include any other types
>>>>>> of file pages. This is inconsistent since in most other places in the
>>>>>> kernel, THP counters are explicitly separated for anon, shmem and file
>>>>>> flavours. However, we are stuck with it since it constitutes a user ABI.
>>>>>>
>>>>>> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
>>>>>> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
>>>>>> same "file_" prefix in the names. But in future, we may want to add
>>>>>> extra stats to cover actual file pages, at which point, it would all
>>>>>> become very confusing.
>>>>>>
>>>>>> So let's take the opportunity to rename these new counters "shmem_"
>>>>>> before the change makes it upstream and the ABI becomes immutable.
>>>>>
>>>>> Personally, I think this approach is much clearer. However, I recall
>>>>> we discussed this
>>>>> before [1], and it seems that inconsistency is a concern?
>>>>
>>>> Embarrassingly, I don't recall that converstation at all :-| but at least
>>>> what I
>>>> said then is consistent with what I've done in this patch.
>>>>
>>>> I think David's conclusion from that thread was to call them FILE_, and add
>>>> both
>>>> shmem and pagecache counts to those counters, meaning we can keep the same name
>>>> as legacy THP counters. But those legacy THP counters only count shmem, and I
>>>> don't think we would get away with adding pagecache counts to those at this
>>>> point? (argument: they have been around for long time and there is a risk that
>>>> user space relies on them and if they were to dramatically increase due to
>>>> pagecache addition now that could break things). In that case, there is still
>>>> inconsistency, but its worse; the names are consistent but the semantics are
>>>> inconsistent.
>>>>
>>>> So my vote is to change to SHMEM_ as per this patch :)
>>>
>>> I also forgot most of the discussion, but these 3 legacy counters are really
>>> only (currently) incremented for shmem. I think my idea was to keep everything
>>> as FILE_ for now, maybe at some point make the pagecache also use them, and then
>>> maybe have separate FILE_ + SHMEM_.
>>>
>>> But yeah, likely it's best to only have "shmem" here for now, because who knows
>>> what we can actually change about the legacy counters. But it's always though
>>> messing with legacy stuff that is clearly suboptimal ...
>>
>> Sorry David, I've read your response a bunch of times and am still not
>> completely sure what you are advocating for.
>>
>> To make my proposal crystal clear, I think we should leave the legacy counters
>> alone - neither change their name nor what they count (which is _only_ shmem). I
>> think we should rename the new mTHP counters to "shmem" and have them continue
>> to only count shmem. Then additionally, I think we should introduce new "file"
>> mTHP counters that count the page cache allocations (that's a future set of
>> patches, which I'm working on together with user controls to determine which
>> mTHP sizes can be used by page cache).
>>
>> As suggested by Barry, I propose to also improve the documentation for the
>> legacy counters to make it clear that dispite their name being "file" they are
>> actually counting "shmem". I'll do this for v2.
>
> Yes, and please. Likely we should document for the legacy ones (if not already
> done) that they only track PMD-sized THPs.
The PMD-ness is already documented for the legacy counters IIRC, but I'll double
check.
>
>>
>> David, would you support this approach? If so, I'd like to push this forwards
>> asap so that it gets into v6.11 to avoid ever exposing the mthp counters with
>> the "file" name.
>
> Yes, sorry for not being clear.
No worries, it sounds like at least Lance understood exectly what you were
saying, so likely I'm just being obtuse.
Anyway, I'll respin with the docs improvements and get v2 out shortly.
Thanks,
Ryan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-09 7:55 ` Ryan Roberts
@ 2024-07-09 8:13 ` Barry Song
2024-07-09 8:35 ` Ryan Roberts
0 siblings, 1 reply; 15+ messages in thread
From: Barry Song @ 2024-07-09 8:13 UTC (permalink / raw)
To: Ryan Roberts
Cc: akpm, baolin.wang, corbet, da.gomez, david, hughd, ioworker0,
linux-kernel, linux-mm, willy, ziy
On Tue, Jul 9, 2024 at 7:55 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>
> On 09/07/2024 02:44, Barry Song wrote:
> > On Tue, Jul 9, 2024 at 12:30 AM Ryan Roberts <ryan.roberts@arm.com> wrote:
> >>
> >> On 08/07/2024 12:36, Barry Song wrote:
> >>> On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
> >>>>
> >>>> The legacy PMD-sized THP counters at /proc/vmstat include
> >>>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
> >>>> rather confusingly refer to shmem THP and do not include any other types
> >>>> of file pages. This is inconsistent since in most other places in the
> >>>> kernel, THP counters are explicitly separated for anon, shmem and file
> >>>> flavours. However, we are stuck with it since it constitutes a user ABI.
> >>>>
> >>>> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
> >>>> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
> >>>> same "file_" prefix in the names. But in future, we may want to add
> >>>> extra stats to cover actual file pages, at which point, it would all
> >>>> become very confusing.
> >>>>
> >>>> So let's take the opportunity to rename these new counters "shmem_"
> >>>> before the change makes it upstream and the ABI becomes immutable.
> >>>
> >>> Personally, I think this approach is much clearer. However, I recall
> >>> we discussed this
> >>> before [1], and it seems that inconsistency is a concern?
> >>
> >> Embarrassingly, I don't recall that converstation at all :-| but at least what I
> >> said then is consistent with what I've done in this patch.
> >>
> >> I think David's conclusion from that thread was to call them FILE_, and add both
> >> shmem and pagecache counts to those counters, meaning we can keep the same name
> >> as legacy THP counters. But those legacy THP counters only count shmem, and I
> >> don't think we would get away with adding pagecache counts to those at this
> >> point? (argument: they have been around for long time and there is a risk that
> >> user space relies on them and if they were to dramatically increase due to
> >> pagecache addition now that could break things). In that case, there is still
> >> inconsistency, but its worse; the names are consistent but the semantics are
> >> inconsistent.
> >>
> >> So my vote is to change to SHMEM_ as per this patch :)
> >
> > I have no objections. However, I dislike the documentation for
> > thp_file_*. Perhaps we can clean it all up together ?
>
> I agree that we should clean this documentation up and I'm happy to roll it into
> v2. However, I don't think what you have suggested is quite right.
>
> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge *only* count
> shmem. They don't count pagecache. So perhaps the change should be "...every
> time a shmem huge page (dispite being named after "file", the counter measures
> only shmem) is..."?
I understand what you are saying, and I know that thp_file_* has only
included shmem so far. My question is whether it will include regular
files in the future? If not, I am perfectly fine with your approach.
READ_ONLY_THP_FOR_FS isn't applicable in this path as it is created
by khugepaged collapse.
>
> thp_file_mapped includes both file and shmem, so agree with your change there.
>
> What do you think?
>
>
> >
> > diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> > index 709fe10b60f4..65df48cb3bbb 100644
> > --- a/Documentation/admin-guide/mm/transhuge.rst
> > +++ b/Documentation/admin-guide/mm/transhuge.rst
> > @@ -417,21 +417,22 @@ thp_collapse_alloc_failed
> > the allocation.
> >
> > thp_file_alloc
> > - is incremented every time a file huge page is successfully
> > - allocated.
> > + is incremented every time a file (including shmem) huge page is
> > + successfully allocated.
> >
> > thp_file_fallback
> > - is incremented if a file huge page is attempted to be allocated
> > - but fails and instead falls back to using small pages.
> > + is incremented if a file (including shmem) huge page is attempted
> > + to be allocated but fails and instead falls back to using small
> > + pages.
> >
> > thp_file_fallback_charge
> > - is incremented if a file huge page cannot be charged and instead
> > - falls back to using small pages even though the allocation was
> > - successful.
> > + is incremented if a file (including shmem) huge page cannot be
> > + charged and instead falls back to using small pages even though
> > + the allocation was successful.
> >
> > thp_file_mapped
> > - is incremented every time a file huge page is mapped into
> > - user address space.
> > + is incremented every time a file (including shmem) huge page is
> > + mapped into user address space.
> >
> > thp_split_page
> > is incremented every time a huge page is split into base
> >
> >>
> >>>
> >>> [1] https://lore.kernel.org/linux-mm/05d0096e4ec3e572d1d52d33a31a661321ac1551.1713755580.git.baolin.wang@linux.alibaba.com/
> >>>
> >>>
> >>>>
> >>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> >>>> ---
> >>>>
> >>>> Hi All,
> >>>>
> >>>> Applies on top of today's mm-unstable (2073cda629a4) and tested with mm
> >>>> selftests; no regressions observed.
> >>>>
> >>>> The backstory here is that I'd like to introduce some counters for regular file
> >>>> folio allocations to observe how often large folio allocation succeeds, but
> >>>> these shmem counters are named "file" which is going to make things confusing.
> >>>> So hoping to solve that before commit 66f44583f9b6 ("mm: shmem: add mTHP
> >>>> counters for anonymous shmem") goes upstream (it is currently in mm-stable).
> >>>>
> >>>> Admittedly, this change means the mTHP stat names are not the same as the legacy
> >>>> PMD-size THP names, but I think that's a smaller issue than having "file_" mTHP
> >>>> stats that only count shmem, then having to introduce "file2_" or "pgcache_"
> >>>> stats for the regular file memory, which is even more inconsistent IMHO. I guess
> >>>> the alternative is to count both shmem and file in these mTHP stats (that's how
> >>>> they were documented anyway) but I think it's better to be able to consider them
> >>>> separately like we do for all the other counters.
> >>>>
> >>>> Thanks,
> >>>> Ryan
> >>>>
> >>>> Documentation/admin-guide/mm/transhuge.rst | 12 ++++++------
> >>>> include/linux/huge_mm.h | 6 +++---
> >>>> mm/huge_memory.c | 12 ++++++------
> >>>> mm/shmem.c | 8 ++++----
> >>>> 4 files changed, 19 insertions(+), 19 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> >>>> index 747c811ee8f1..8b891689fc13 100644
> >>>> --- a/Documentation/admin-guide/mm/transhuge.rst
> >>>> +++ b/Documentation/admin-guide/mm/transhuge.rst
> >>>> @@ -496,16 +496,16 @@ swpout_fallback
> >>>> Usually because failed to allocate some continuous swap space
> >>>> for the huge page.
> >>>>
> >>>> -file_alloc
> >>>> - is incremented every time a file huge page is successfully
> >>>> +shmem_alloc
> >>>> + is incremented every time a shmem huge page is successfully
> >>>> allocated.
> >>>>
> >>>> -file_fallback
> >>>> - is incremented if a file huge page is attempted to be allocated
> >>>> +shmem_fallback
> >>>> + is incremented if a shmem huge page is attempted to be allocated
> >>>> but fails and instead falls back to using small pages.
> >>>>
> >>>> -file_fallback_charge
> >>>> - is incremented if a file huge page cannot be charged and instead
> >>>> +shmem_fallback_charge
> >>>> + is incremented if a shmem huge page cannot be charged and instead
> >>>> falls back to using small pages even though the allocation was
> >>>> successful.
> >>>>
> >>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >>>> index acb6ac24a07e..cff002be83eb 100644
> >>>> --- a/include/linux/huge_mm.h
> >>>> +++ b/include/linux/huge_mm.h
> >>>> @@ -269,9 +269,9 @@ enum mthp_stat_item {
> >>>> MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE,
> >>>> MTHP_STAT_SWPOUT,
> >>>> MTHP_STAT_SWPOUT_FALLBACK,
> >>>> - MTHP_STAT_FILE_ALLOC,
> >>>> - MTHP_STAT_FILE_FALLBACK,
> >>>> - MTHP_STAT_FILE_FALLBACK_CHARGE,
> >>>> + MTHP_STAT_SHMEM_ALLOC,
> >>>> + MTHP_STAT_SHMEM_FALLBACK,
> >>>> + MTHP_STAT_SHMEM_FALLBACK_CHARGE,
> >>>> MTHP_STAT_SPLIT,
> >>>> MTHP_STAT_SPLIT_FAILED,
> >>>> MTHP_STAT_SPLIT_DEFERRED,
> >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >>>> index 9ec64aa2be94..f9696c94e211 100644
> >>>> --- a/mm/huge_memory.c
> >>>> +++ b/mm/huge_memory.c
> >>>> @@ -568,9 +568,9 @@ 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(swpout, MTHP_STAT_SWPOUT);
> >>>> DEFINE_MTHP_STAT_ATTR(swpout_fallback, MTHP_STAT_SWPOUT_FALLBACK);
> >>>> -DEFINE_MTHP_STAT_ATTR(file_alloc, MTHP_STAT_FILE_ALLOC);
> >>>> -DEFINE_MTHP_STAT_ATTR(file_fallback, MTHP_STAT_FILE_FALLBACK);
> >>>> -DEFINE_MTHP_STAT_ATTR(file_fallback_charge, MTHP_STAT_FILE_FALLBACK_CHARGE);
> >>>> +DEFINE_MTHP_STAT_ATTR(shmem_alloc, MTHP_STAT_SHMEM_ALLOC);
> >>>> +DEFINE_MTHP_STAT_ATTR(shmem_fallback, MTHP_STAT_SHMEM_FALLBACK);
> >>>> +DEFINE_MTHP_STAT_ATTR(shmem_fallback_charge, MTHP_STAT_SHMEM_FALLBACK_CHARGE);
> >>>> 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);
> >>>> @@ -581,9 +581,9 @@ static struct attribute *stats_attrs[] = {
> >>>> &anon_fault_fallback_charge_attr.attr,
> >>>> &swpout_attr.attr,
> >>>> &swpout_fallback_attr.attr,
> >>>> - &file_alloc_attr.attr,
> >>>> - &file_fallback_attr.attr,
> >>>> - &file_fallback_charge_attr.attr,
> >>>> + &shmem_alloc_attr.attr,
> >>>> + &shmem_fallback_attr.attr,
> >>>> + &shmem_fallback_charge_attr.attr,
> >>>> &split_attr.attr,
> >>>> &split_failed_attr.attr,
> >>>> &split_deferred_attr.attr,
> >>>> diff --git a/mm/shmem.c b/mm/shmem.c
> >>>> index 921d59c3d669..f24dfbd387ba 100644
> >>>> --- a/mm/shmem.c
> >>>> +++ b/mm/shmem.c
> >>>> @@ -1777,7 +1777,7 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
> >>>> if (pages == HPAGE_PMD_NR)
> >>>> count_vm_event(THP_FILE_FALLBACK);
> >>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >>>> - count_mthp_stat(order, MTHP_STAT_FILE_FALLBACK);
> >>>> + count_mthp_stat(order, MTHP_STAT_SHMEM_FALLBACK);
> >>>> #endif
> >>>> order = next_order(&suitable_orders, order);
> >>>> }
> >>>> @@ -1804,8 +1804,8 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
> >>>> count_vm_event(THP_FILE_FALLBACK_CHARGE);
> >>>> }
> >>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >>>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK);
> >>>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK_CHARGE);
> >>>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK);
> >>>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK_CHARGE);
> >>>> #endif
> >>>> }
> >>>> goto unlock;
> >>>> @@ -2181,7 +2181,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> >>>> if (folio_test_pmd_mappable(folio))
> >>>> count_vm_event(THP_FILE_ALLOC);
> >>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >>>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_ALLOC);
> >>>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_ALLOC);
> >>>> #endif
> >>>> goto alloced;
> >>>> }
> >>>> --
> >>>> 2.43.0
> >>>>
> >>>
Thanks
Barry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-09 8:13 ` Barry Song
@ 2024-07-09 8:35 ` Ryan Roberts
2024-07-09 8:40 ` Barry Song
0 siblings, 1 reply; 15+ messages in thread
From: Ryan Roberts @ 2024-07-09 8:35 UTC (permalink / raw)
To: Barry Song
Cc: akpm, baolin.wang, corbet, da.gomez, david, hughd, ioworker0,
linux-kernel, linux-mm, willy, ziy
On 09/07/2024 09:13, Barry Song wrote:
> On Tue, Jul 9, 2024 at 7:55 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>
>> On 09/07/2024 02:44, Barry Song wrote:
>>> On Tue, Jul 9, 2024 at 12:30 AM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>>>
>>>> On 08/07/2024 12:36, Barry Song wrote:
>>>>> On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>>>>>
>>>>>> The legacy PMD-sized THP counters at /proc/vmstat include
>>>>>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
>>>>>> rather confusingly refer to shmem THP and do not include any other types
>>>>>> of file pages. This is inconsistent since in most other places in the
>>>>>> kernel, THP counters are explicitly separated for anon, shmem and file
>>>>>> flavours. However, we are stuck with it since it constitutes a user ABI.
>>>>>>
>>>>>> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
>>>>>> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
>>>>>> same "file_" prefix in the names. But in future, we may want to add
>>>>>> extra stats to cover actual file pages, at which point, it would all
>>>>>> become very confusing.
>>>>>>
>>>>>> So let's take the opportunity to rename these new counters "shmem_"
>>>>>> before the change makes it upstream and the ABI becomes immutable.
>>>>>
>>>>> Personally, I think this approach is much clearer. However, I recall
>>>>> we discussed this
>>>>> before [1], and it seems that inconsistency is a concern?
>>>>
>>>> Embarrassingly, I don't recall that converstation at all :-| but at least what I
>>>> said then is consistent with what I've done in this patch.
>>>>
>>>> I think David's conclusion from that thread was to call them FILE_, and add both
>>>> shmem and pagecache counts to those counters, meaning we can keep the same name
>>>> as legacy THP counters. But those legacy THP counters only count shmem, and I
>>>> don't think we would get away with adding pagecache counts to those at this
>>>> point? (argument: they have been around for long time and there is a risk that
>>>> user space relies on them and if they were to dramatically increase due to
>>>> pagecache addition now that could break things). In that case, there is still
>>>> inconsistency, but its worse; the names are consistent but the semantics are
>>>> inconsistent.
>>>>
>>>> So my vote is to change to SHMEM_ as per this patch :)
>>>
>>> I have no objections. However, I dislike the documentation for
>>> thp_file_*. Perhaps we can clean it all up together ?
>>
>> I agree that we should clean this documentation up and I'm happy to roll it into
>> v2. However, I don't think what you have suggested is quite right.
>>
>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge *only* count
>> shmem. They don't count pagecache. So perhaps the change should be "...every
>> time a shmem huge page (dispite being named after "file", the counter measures
>> only shmem) is..."?
>
> I understand what you are saying, and I know that thp_file_* has only
> included shmem so far. My question is whether it will include regular
> files in the future? If not, I am perfectly fine with your approach.
My whole reasoning for this patch is based on the assertion that since
THP_FILE_ALLOC has been there for 8 years and in all that time has only counted
shmem, then its highly likely that someone is depending on that semantic and we
can't change it. I don't have any actual evidence of code that relies on it though.
I propose I change the docs to reflect what's actually happening today (i.e.
shmem *only*). If we later decide we want to also report page cache numbers
through that same counter, then we can change the docs at that point. But if I
get my way, we'll soon have mTHP counters for FILE, which is solely for page
cache. So You'll be able to get all the fine-grained info out of those and there
will be no need to mess with the legacy counters.
>
> READ_ONLY_THP_FOR_FS isn't applicable in this path as it is created
> by khugepaged collapse.
>
>>
>> thp_file_mapped includes both file and shmem, so agree with your change there.
>>
>> What do you think?
>>
>>
>>>
>>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
>>> index 709fe10b60f4..65df48cb3bbb 100644
>>> --- a/Documentation/admin-guide/mm/transhuge.rst
>>> +++ b/Documentation/admin-guide/mm/transhuge.rst
>>> @@ -417,21 +417,22 @@ thp_collapse_alloc_failed
>>> the allocation.
>>>
>>> thp_file_alloc
>>> - is incremented every time a file huge page is successfully
>>> - allocated.
>>> + is incremented every time a file (including shmem) huge page is
>>> + successfully allocated.
>>>
>>> thp_file_fallback
>>> - is incremented if a file huge page is attempted to be allocated
>>> - but fails and instead falls back to using small pages.
>>> + is incremented if a file (including shmem) huge page is attempted
>>> + to be allocated but fails and instead falls back to using small
>>> + pages.
>>>
>>> thp_file_fallback_charge
>>> - is incremented if a file huge page cannot be charged and instead
>>> - falls back to using small pages even though the allocation was
>>> - successful.
>>> + is incremented if a file (including shmem) huge page cannot be
>>> + charged and instead falls back to using small pages even though
>>> + the allocation was successful.
>>>
>>> thp_file_mapped
>>> - is incremented every time a file huge page is mapped into
>>> - user address space.
>>> + is incremented every time a file (including shmem) huge page is
>>> + mapped into user address space.
>>>
>>> thp_split_page
>>> is incremented every time a huge page is split into base
>>>
>>>>
>>>>>
>>>>> [1] https://lore.kernel.org/linux-mm/05d0096e4ec3e572d1d52d33a31a661321ac1551.1713755580.git.baolin.wang@linux.alibaba.com/
>>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>>>> ---
>>>>>>
>>>>>> Hi All,
>>>>>>
>>>>>> Applies on top of today's mm-unstable (2073cda629a4) and tested with mm
>>>>>> selftests; no regressions observed.
>>>>>>
>>>>>> The backstory here is that I'd like to introduce some counters for regular file
>>>>>> folio allocations to observe how often large folio allocation succeeds, but
>>>>>> these shmem counters are named "file" which is going to make things confusing.
>>>>>> So hoping to solve that before commit 66f44583f9b6 ("mm: shmem: add mTHP
>>>>>> counters for anonymous shmem") goes upstream (it is currently in mm-stable).
>>>>>>
>>>>>> Admittedly, this change means the mTHP stat names are not the same as the legacy
>>>>>> PMD-size THP names, but I think that's a smaller issue than having "file_" mTHP
>>>>>> stats that only count shmem, then having to introduce "file2_" or "pgcache_"
>>>>>> stats for the regular file memory, which is even more inconsistent IMHO. I guess
>>>>>> the alternative is to count both shmem and file in these mTHP stats (that's how
>>>>>> they were documented anyway) but I think it's better to be able to consider them
>>>>>> separately like we do for all the other counters.
>>>>>>
>>>>>> Thanks,
>>>>>> Ryan
>>>>>>
>>>>>> Documentation/admin-guide/mm/transhuge.rst | 12 ++++++------
>>>>>> include/linux/huge_mm.h | 6 +++---
>>>>>> mm/huge_memory.c | 12 ++++++------
>>>>>> mm/shmem.c | 8 ++++----
>>>>>> 4 files changed, 19 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
>>>>>> index 747c811ee8f1..8b891689fc13 100644
>>>>>> --- a/Documentation/admin-guide/mm/transhuge.rst
>>>>>> +++ b/Documentation/admin-guide/mm/transhuge.rst
>>>>>> @@ -496,16 +496,16 @@ swpout_fallback
>>>>>> Usually because failed to allocate some continuous swap space
>>>>>> for the huge page.
>>>>>>
>>>>>> -file_alloc
>>>>>> - is incremented every time a file huge page is successfully
>>>>>> +shmem_alloc
>>>>>> + is incremented every time a shmem huge page is successfully
>>>>>> allocated.
>>>>>>
>>>>>> -file_fallback
>>>>>> - is incremented if a file huge page is attempted to be allocated
>>>>>> +shmem_fallback
>>>>>> + is incremented if a shmem huge page is attempted to be allocated
>>>>>> but fails and instead falls back to using small pages.
>>>>>>
>>>>>> -file_fallback_charge
>>>>>> - is incremented if a file huge page cannot be charged and instead
>>>>>> +shmem_fallback_charge
>>>>>> + is incremented if a shmem huge page cannot be charged and instead
>>>>>> falls back to using small pages even though the allocation was
>>>>>> successful.
>>>>>>
>>>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>>>> index acb6ac24a07e..cff002be83eb 100644
>>>>>> --- a/include/linux/huge_mm.h
>>>>>> +++ b/include/linux/huge_mm.h
>>>>>> @@ -269,9 +269,9 @@ enum mthp_stat_item {
>>>>>> MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE,
>>>>>> MTHP_STAT_SWPOUT,
>>>>>> MTHP_STAT_SWPOUT_FALLBACK,
>>>>>> - MTHP_STAT_FILE_ALLOC,
>>>>>> - MTHP_STAT_FILE_FALLBACK,
>>>>>> - MTHP_STAT_FILE_FALLBACK_CHARGE,
>>>>>> + MTHP_STAT_SHMEM_ALLOC,
>>>>>> + MTHP_STAT_SHMEM_FALLBACK,
>>>>>> + MTHP_STAT_SHMEM_FALLBACK_CHARGE,
>>>>>> MTHP_STAT_SPLIT,
>>>>>> MTHP_STAT_SPLIT_FAILED,
>>>>>> MTHP_STAT_SPLIT_DEFERRED,
>>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>>>> index 9ec64aa2be94..f9696c94e211 100644
>>>>>> --- a/mm/huge_memory.c
>>>>>> +++ b/mm/huge_memory.c
>>>>>> @@ -568,9 +568,9 @@ 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(swpout, MTHP_STAT_SWPOUT);
>>>>>> DEFINE_MTHP_STAT_ATTR(swpout_fallback, MTHP_STAT_SWPOUT_FALLBACK);
>>>>>> -DEFINE_MTHP_STAT_ATTR(file_alloc, MTHP_STAT_FILE_ALLOC);
>>>>>> -DEFINE_MTHP_STAT_ATTR(file_fallback, MTHP_STAT_FILE_FALLBACK);
>>>>>> -DEFINE_MTHP_STAT_ATTR(file_fallback_charge, MTHP_STAT_FILE_FALLBACK_CHARGE);
>>>>>> +DEFINE_MTHP_STAT_ATTR(shmem_alloc, MTHP_STAT_SHMEM_ALLOC);
>>>>>> +DEFINE_MTHP_STAT_ATTR(shmem_fallback, MTHP_STAT_SHMEM_FALLBACK);
>>>>>> +DEFINE_MTHP_STAT_ATTR(shmem_fallback_charge, MTHP_STAT_SHMEM_FALLBACK_CHARGE);
>>>>>> 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);
>>>>>> @@ -581,9 +581,9 @@ static struct attribute *stats_attrs[] = {
>>>>>> &anon_fault_fallback_charge_attr.attr,
>>>>>> &swpout_attr.attr,
>>>>>> &swpout_fallback_attr.attr,
>>>>>> - &file_alloc_attr.attr,
>>>>>> - &file_fallback_attr.attr,
>>>>>> - &file_fallback_charge_attr.attr,
>>>>>> + &shmem_alloc_attr.attr,
>>>>>> + &shmem_fallback_attr.attr,
>>>>>> + &shmem_fallback_charge_attr.attr,
>>>>>> &split_attr.attr,
>>>>>> &split_failed_attr.attr,
>>>>>> &split_deferred_attr.attr,
>>>>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>>>>> index 921d59c3d669..f24dfbd387ba 100644
>>>>>> --- a/mm/shmem.c
>>>>>> +++ b/mm/shmem.c
>>>>>> @@ -1777,7 +1777,7 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
>>>>>> if (pages == HPAGE_PMD_NR)
>>>>>> count_vm_event(THP_FILE_FALLBACK);
>>>>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>>>> - count_mthp_stat(order, MTHP_STAT_FILE_FALLBACK);
>>>>>> + count_mthp_stat(order, MTHP_STAT_SHMEM_FALLBACK);
>>>>>> #endif
>>>>>> order = next_order(&suitable_orders, order);
>>>>>> }
>>>>>> @@ -1804,8 +1804,8 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
>>>>>> count_vm_event(THP_FILE_FALLBACK_CHARGE);
>>>>>> }
>>>>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>>>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK);
>>>>>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK_CHARGE);
>>>>>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK);
>>>>>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK_CHARGE);
>>>>>> #endif
>>>>>> }
>>>>>> goto unlock;
>>>>>> @@ -2181,7 +2181,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
>>>>>> if (folio_test_pmd_mappable(folio))
>>>>>> count_vm_event(THP_FILE_ALLOC);
>>>>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>>>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_ALLOC);
>>>>>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_ALLOC);
>>>>>> #endif
>>>>>> goto alloced;
>>>>>> }
>>>>>> --
>>>>>> 2.43.0
>>>>>>
>>>>>
>
> Thanks
> Barry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters
2024-07-09 8:35 ` Ryan Roberts
@ 2024-07-09 8:40 ` Barry Song
0 siblings, 0 replies; 15+ messages in thread
From: Barry Song @ 2024-07-09 8:40 UTC (permalink / raw)
To: Ryan Roberts
Cc: akpm, baolin.wang, corbet, da.gomez, david, hughd, ioworker0,
linux-kernel, linux-mm, willy, ziy
On Tue, Jul 9, 2024 at 8:35 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>
> On 09/07/2024 09:13, Barry Song wrote:
> > On Tue, Jul 9, 2024 at 7:55 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
> >>
> >> On 09/07/2024 02:44, Barry Song wrote:
> >>> On Tue, Jul 9, 2024 at 12:30 AM Ryan Roberts <ryan.roberts@arm.com> wrote:
> >>>>
> >>>> On 08/07/2024 12:36, Barry Song wrote:
> >>>>> On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
> >>>>>>
> >>>>>> The legacy PMD-sized THP counters at /proc/vmstat include
> >>>>>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
> >>>>>> rather confusingly refer to shmem THP and do not include any other types
> >>>>>> of file pages. This is inconsistent since in most other places in the
> >>>>>> kernel, THP counters are explicitly separated for anon, shmem and file
> >>>>>> flavours. However, we are stuck with it since it constitutes a user ABI.
> >>>>>>
> >>>>>> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
> >>>>>> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
> >>>>>> same "file_" prefix in the names. But in future, we may want to add
> >>>>>> extra stats to cover actual file pages, at which point, it would all
> >>>>>> become very confusing.
> >>>>>>
> >>>>>> So let's take the opportunity to rename these new counters "shmem_"
> >>>>>> before the change makes it upstream and the ABI becomes immutable.
> >>>>>
> >>>>> Personally, I think this approach is much clearer. However, I recall
> >>>>> we discussed this
> >>>>> before [1], and it seems that inconsistency is a concern?
> >>>>
> >>>> Embarrassingly, I don't recall that converstation at all :-| but at least what I
> >>>> said then is consistent with what I've done in this patch.
> >>>>
> >>>> I think David's conclusion from that thread was to call them FILE_, and add both
> >>>> shmem and pagecache counts to those counters, meaning we can keep the same name
> >>>> as legacy THP counters. But those legacy THP counters only count shmem, and I
> >>>> don't think we would get away with adding pagecache counts to those at this
> >>>> point? (argument: they have been around for long time and there is a risk that
> >>>> user space relies on them and if they were to dramatically increase due to
> >>>> pagecache addition now that could break things). In that case, there is still
> >>>> inconsistency, but its worse; the names are consistent but the semantics are
> >>>> inconsistent.
> >>>>
> >>>> So my vote is to change to SHMEM_ as per this patch :)
> >>>
> >>> I have no objections. However, I dislike the documentation for
> >>> thp_file_*. Perhaps we can clean it all up together ?
> >>
> >> I agree that we should clean this documentation up and I'm happy to roll it into
> >> v2. However, I don't think what you have suggested is quite right.
> >>
> >> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge *only* count
> >> shmem. They don't count pagecache. So perhaps the change should be "...every
> >> time a shmem huge page (dispite being named after "file", the counter measures
> >> only shmem) is..."?
> >
> > I understand what you are saying, and I know that thp_file_* has only
> > included shmem so far. My question is whether it will include regular
> > files in the future? If not, I am perfectly fine with your approach.
>
> My whole reasoning for this patch is based on the assertion that since
> THP_FILE_ALLOC has been there for 8 years and in all that time has only counted
> shmem, then its highly likely that someone is depending on that semantic and we
> can't change it. I don't have any actual evidence of code that relies on it though.
>
> I propose I change the docs to reflect what's actually happening today (i.e.
> shmem *only*). If we later decide we want to also report page cache numbers
> through that same counter, then we can change the docs at that point. But if I
> get my way, we'll soon have mTHP counters for FILE, which is solely for page
> cache. So You'll be able to get all the fine-grained info out of those and there
> will be no need to mess with the legacy counters.
Make sense to me. I'd rather we go to
/sys/kernel/mm/transparent_hugepage/hugepages-2048kB/stats/file_*
/sys/kernel/mm/transparent_hugepage/hugepages-2048kB/stats/shmem_*
if we later have 2MiB file counters.
>
> >
> > READ_ONLY_THP_FOR_FS isn't applicable in this path as it is created
> > by khugepaged collapse.
> >
> >>
> >> thp_file_mapped includes both file and shmem, so agree with your change there.
> >>
> >> What do you think?
> >>
> >>
> >>>
> >>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> >>> index 709fe10b60f4..65df48cb3bbb 100644
> >>> --- a/Documentation/admin-guide/mm/transhuge.rst
> >>> +++ b/Documentation/admin-guide/mm/transhuge.rst
> >>> @@ -417,21 +417,22 @@ thp_collapse_alloc_failed
> >>> the allocation.
> >>>
> >>> thp_file_alloc
> >>> - is incremented every time a file huge page is successfully
> >>> - allocated.
> >>> + is incremented every time a file (including shmem) huge page is
> >>> + successfully allocated.
> >>>
> >>> thp_file_fallback
> >>> - is incremented if a file huge page is attempted to be allocated
> >>> - but fails and instead falls back to using small pages.
> >>> + is incremented if a file (including shmem) huge page is attempted
> >>> + to be allocated but fails and instead falls back to using small
> >>> + pages.
> >>>
> >>> thp_file_fallback_charge
> >>> - is incremented if a file huge page cannot be charged and instead
> >>> - falls back to using small pages even though the allocation was
> >>> - successful.
> >>> + is incremented if a file (including shmem) huge page cannot be
> >>> + charged and instead falls back to using small pages even though
> >>> + the allocation was successful.
> >>>
> >>> thp_file_mapped
> >>> - is incremented every time a file huge page is mapped into
> >>> - user address space.
> >>> + is incremented every time a file (including shmem) huge page is
> >>> + mapped into user address space.
> >>>
> >>> thp_split_page
> >>> is incremented every time a huge page is split into base
> >>>
> >>>>
> >>>>>
> >>>>> [1] https://lore.kernel.org/linux-mm/05d0096e4ec3e572d1d52d33a31a661321ac1551.1713755580.git.baolin.wang@linux.alibaba.com/
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> >>>>>> ---
> >>>>>>
> >>>>>> Hi All,
> >>>>>>
> >>>>>> Applies on top of today's mm-unstable (2073cda629a4) and tested with mm
> >>>>>> selftests; no regressions observed.
> >>>>>>
> >>>>>> The backstory here is that I'd like to introduce some counters for regular file
> >>>>>> folio allocations to observe how often large folio allocation succeeds, but
> >>>>>> these shmem counters are named "file" which is going to make things confusing.
> >>>>>> So hoping to solve that before commit 66f44583f9b6 ("mm: shmem: add mTHP
> >>>>>> counters for anonymous shmem") goes upstream (it is currently in mm-stable).
> >>>>>>
> >>>>>> Admittedly, this change means the mTHP stat names are not the same as the legacy
> >>>>>> PMD-size THP names, but I think that's a smaller issue than having "file_" mTHP
> >>>>>> stats that only count shmem, then having to introduce "file2_" or "pgcache_"
> >>>>>> stats for the regular file memory, which is even more inconsistent IMHO. I guess
> >>>>>> the alternative is to count both shmem and file in these mTHP stats (that's how
> >>>>>> they were documented anyway) but I think it's better to be able to consider them
> >>>>>> separately like we do for all the other counters.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Ryan
> >>>>>>
> >>>>>> Documentation/admin-guide/mm/transhuge.rst | 12 ++++++------
> >>>>>> include/linux/huge_mm.h | 6 +++---
> >>>>>> mm/huge_memory.c | 12 ++++++------
> >>>>>> mm/shmem.c | 8 ++++----
> >>>>>> 4 files changed, 19 insertions(+), 19 deletions(-)
> >>>>>>
> >>>>>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
> >>>>>> index 747c811ee8f1..8b891689fc13 100644
> >>>>>> --- a/Documentation/admin-guide/mm/transhuge.rst
> >>>>>> +++ b/Documentation/admin-guide/mm/transhuge.rst
> >>>>>> @@ -496,16 +496,16 @@ swpout_fallback
> >>>>>> Usually because failed to allocate some continuous swap space
> >>>>>> for the huge page.
> >>>>>>
> >>>>>> -file_alloc
> >>>>>> - is incremented every time a file huge page is successfully
> >>>>>> +shmem_alloc
> >>>>>> + is incremented every time a shmem huge page is successfully
> >>>>>> allocated.
> >>>>>>
> >>>>>> -file_fallback
> >>>>>> - is incremented if a file huge page is attempted to be allocated
> >>>>>> +shmem_fallback
> >>>>>> + is incremented if a shmem huge page is attempted to be allocated
> >>>>>> but fails and instead falls back to using small pages.
> >>>>>>
> >>>>>> -file_fallback_charge
> >>>>>> - is incremented if a file huge page cannot be charged and instead
> >>>>>> +shmem_fallback_charge
> >>>>>> + is incremented if a shmem huge page cannot be charged and instead
> >>>>>> falls back to using small pages even though the allocation was
> >>>>>> successful.
> >>>>>>
> >>>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >>>>>> index acb6ac24a07e..cff002be83eb 100644
> >>>>>> --- a/include/linux/huge_mm.h
> >>>>>> +++ b/include/linux/huge_mm.h
> >>>>>> @@ -269,9 +269,9 @@ enum mthp_stat_item {
> >>>>>> MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE,
> >>>>>> MTHP_STAT_SWPOUT,
> >>>>>> MTHP_STAT_SWPOUT_FALLBACK,
> >>>>>> - MTHP_STAT_FILE_ALLOC,
> >>>>>> - MTHP_STAT_FILE_FALLBACK,
> >>>>>> - MTHP_STAT_FILE_FALLBACK_CHARGE,
> >>>>>> + MTHP_STAT_SHMEM_ALLOC,
> >>>>>> + MTHP_STAT_SHMEM_FALLBACK,
> >>>>>> + MTHP_STAT_SHMEM_FALLBACK_CHARGE,
> >>>>>> MTHP_STAT_SPLIT,
> >>>>>> MTHP_STAT_SPLIT_FAILED,
> >>>>>> MTHP_STAT_SPLIT_DEFERRED,
> >>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >>>>>> index 9ec64aa2be94..f9696c94e211 100644
> >>>>>> --- a/mm/huge_memory.c
> >>>>>> +++ b/mm/huge_memory.c
> >>>>>> @@ -568,9 +568,9 @@ 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(swpout, MTHP_STAT_SWPOUT);
> >>>>>> DEFINE_MTHP_STAT_ATTR(swpout_fallback, MTHP_STAT_SWPOUT_FALLBACK);
> >>>>>> -DEFINE_MTHP_STAT_ATTR(file_alloc, MTHP_STAT_FILE_ALLOC);
> >>>>>> -DEFINE_MTHP_STAT_ATTR(file_fallback, MTHP_STAT_FILE_FALLBACK);
> >>>>>> -DEFINE_MTHP_STAT_ATTR(file_fallback_charge, MTHP_STAT_FILE_FALLBACK_CHARGE);
> >>>>>> +DEFINE_MTHP_STAT_ATTR(shmem_alloc, MTHP_STAT_SHMEM_ALLOC);
> >>>>>> +DEFINE_MTHP_STAT_ATTR(shmem_fallback, MTHP_STAT_SHMEM_FALLBACK);
> >>>>>> +DEFINE_MTHP_STAT_ATTR(shmem_fallback_charge, MTHP_STAT_SHMEM_FALLBACK_CHARGE);
> >>>>>> 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);
> >>>>>> @@ -581,9 +581,9 @@ static struct attribute *stats_attrs[] = {
> >>>>>> &anon_fault_fallback_charge_attr.attr,
> >>>>>> &swpout_attr.attr,
> >>>>>> &swpout_fallback_attr.attr,
> >>>>>> - &file_alloc_attr.attr,
> >>>>>> - &file_fallback_attr.attr,
> >>>>>> - &file_fallback_charge_attr.attr,
> >>>>>> + &shmem_alloc_attr.attr,
> >>>>>> + &shmem_fallback_attr.attr,
> >>>>>> + &shmem_fallback_charge_attr.attr,
> >>>>>> &split_attr.attr,
> >>>>>> &split_failed_attr.attr,
> >>>>>> &split_deferred_attr.attr,
> >>>>>> diff --git a/mm/shmem.c b/mm/shmem.c
> >>>>>> index 921d59c3d669..f24dfbd387ba 100644
> >>>>>> --- a/mm/shmem.c
> >>>>>> +++ b/mm/shmem.c
> >>>>>> @@ -1777,7 +1777,7 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
> >>>>>> if (pages == HPAGE_PMD_NR)
> >>>>>> count_vm_event(THP_FILE_FALLBACK);
> >>>>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >>>>>> - count_mthp_stat(order, MTHP_STAT_FILE_FALLBACK);
> >>>>>> + count_mthp_stat(order, MTHP_STAT_SHMEM_FALLBACK);
> >>>>>> #endif
> >>>>>> order = next_order(&suitable_orders, order);
> >>>>>> }
> >>>>>> @@ -1804,8 +1804,8 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
> >>>>>> count_vm_event(THP_FILE_FALLBACK_CHARGE);
> >>>>>> }
> >>>>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >>>>>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK);
> >>>>>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK_CHARGE);
> >>>>>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK);
> >>>>>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK_CHARGE);
> >>>>>> #endif
> >>>>>> }
> >>>>>> goto unlock;
> >>>>>> @@ -2181,7 +2181,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> >>>>>> if (folio_test_pmd_mappable(folio))
> >>>>>> count_vm_event(THP_FILE_ALLOC);
> >>>>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >>>>>> - count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_ALLOC);
> >>>>>> + count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_ALLOC);
> >>>>>> #endif
> >>>>>> goto alloced;
> >>>>>> }
> >>>>>> --
> >>>>>> 2.43.0
> >>>>>>
> >>>>>
> >
Thanks
Barry
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-07-09 8:40 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-07-08 11:24 [PATCH v1] mm: shmem: Rename mTHP shmem counters Ryan Roberts
2024-07-08 11:36 ` Barry Song
2024-07-08 12:29 ` Ryan Roberts
2024-07-08 20:50 ` David Hildenbrand
2024-07-09 1:21 ` Lance Yang
2024-07-09 7:47 ` Ryan Roberts
2024-07-09 7:54 ` David Hildenbrand
2024-07-09 7:59 ` Ryan Roberts
2024-07-09 1:07 ` Baolin Wang
2024-07-09 1:44 ` Barry Song
2024-07-09 7:55 ` Ryan Roberts
2024-07-09 8:13 ` Barry Song
2024-07-09 8:35 ` Ryan Roberts
2024-07-09 8:40 ` Barry Song
2024-07-09 1:26 ` Lance Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox