Rename the previously static folio_expected_refs() to clarify its
purpose and scope, making it an inline function
folio_migration_expected_refs() to calculate expected folio references
during migration. The function is only suitable for folios unmapped from
page tables.
Signed-off-by: Shivank Garg <shivankg@amd.com>
---
include/linux/migrate.h | 26 ++++++++++++++++++++++++++
mm/migrate.c | 22 ++++------------------
2 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index aaa2114498d6..083293a6d261 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -60,6 +60,32 @@ struct movable_operations {
/* Defined in mm/debug.c: */
extern const char *migrate_reason_names[MR_TYPES];
+/**
+ * folio_migrate_expected_refs - Count expected references for an unmapped folio.
+ * @mapping: The address space the folio belongs to.
+ * @folio: The folio to check.
+ *
+ * Calculate the expected reference count for a folio during migration.
+ * This function is only suitable for folios that are unmapped from page tables
+ * (i.e., no references from page table mappings: !folio_mapped()).
+ *
+ * Return: The expected reference count
+ */
+static inline int folio_migration_expected_refs(struct address_space *mapping,
+ struct folio *folio)
+{
+ int refs = 1;
+
+ if (!mapping)
+ return refs;
+
+ refs += folio_nr_pages(folio);
+ if (folio_test_private(folio))
+ refs++;
+
+ return refs;
+}
+
#ifdef CONFIG_MIGRATION
void putback_movable_pages(struct list_head *l);
diff --git a/mm/migrate.c b/mm/migrate.c
index 6e2488e5dbe4..6c785abce90e 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -445,20 +445,6 @@ void pmd_migration_entry_wait(struct mm_struct *mm, pmd_t *pmd)
}
#endif
-static int folio_expected_refs(struct address_space *mapping,
- struct folio *folio)
-{
- int refs = 1;
- if (!mapping)
- return refs;
-
- refs += folio_nr_pages(folio);
- if (folio_test_private(folio))
- refs++;
-
- return refs;
-}
-
/*
* Replace the folio in the mapping.
*
@@ -601,7 +587,7 @@ static int __folio_migrate_mapping(struct address_space *mapping,
int folio_migrate_mapping(struct address_space *mapping,
struct folio *newfolio, struct folio *folio, int extra_count)
{
- int expected_count = folio_expected_refs(mapping, folio) + extra_count;
+ int expected_count = folio_migration_expected_refs(mapping, folio) + extra_count;
if (folio_ref_count(folio) != expected_count)
return -EAGAIN;
@@ -618,7 +604,7 @@ int migrate_huge_page_move_mapping(struct address_space *mapping,
struct folio *dst, struct folio *src)
{
XA_STATE(xas, &mapping->i_pages, folio_index(src));
- int rc, expected_count = folio_expected_refs(mapping, src);
+ int rc, expected_count = folio_migration_expected_refs(mapping, src);
if (folio_ref_count(src) != expected_count)
return -EAGAIN;
@@ -749,7 +735,7 @@ static int __migrate_folio(struct address_space *mapping, struct folio *dst,
struct folio *src, void *src_private,
enum migrate_mode mode)
{
- int rc, expected_count = folio_expected_refs(mapping, src);
+ int rc, expected_count = folio_migration_expected_refs(mapping, src);
/* Check whether src does not have extra refs before we do more work */
if (folio_ref_count(src) != expected_count)
@@ -837,7 +823,7 @@ static int __buffer_migrate_folio(struct address_space *mapping,
return migrate_folio(mapping, dst, src, mode);
/* Check whether page does not have extra refs before we do more work */
- expected_count = folio_expected_refs(mapping, src);
+ expected_count = folio_migration_expected_refs(mapping, src);
if (folio_ref_count(src) != expected_count)
return -EAGAIN;
--
2.34.1On Tue, 22 Apr 2025 11:40:03 +0000 Shivank Garg <shivankg@amd.com> wrote: > Rename the previously static folio_expected_refs() to clarify its > purpose and scope, making it an inline function > folio_migration_expected_refs() to calculate expected folio references > during migration. The function is only suitable for folios unmapped from > page tables. > > ... > > +/** > + * folio_migrate_expected_refs - Count expected references for an unmapped folio. "folio_migration_expected_refs" It's concerning that one particular filesystem needs this - one suspects that it is doing something wrong, or that the present API offerings were misdesigned. It would be helpful if the changelogs were to explain what is special about JFS.
On Tue, Apr 22, 2025 at 04:41:11PM -0700, Andrew Morton wrote: > > +/** > > + * folio_migrate_expected_refs - Count expected references for an unmapped folio. > > "folio_migration_expected_refs" Please run make W=1 fs/jfs/ in order to run kernel-doc on this file. It'll flag this kind of error. > It's concerning that one particular filesystem needs this - one > suspects that it is doing something wrong, or that the present API > offerings were misdesigned. It would be helpful if the changelogs were > to explain what is special about JFS. It doesn't surprise me at all. Almost no filesystem implements its own migrate_folio operation. Without going into too much detail, almost all filesystems can use filemap_migrate_folio(), buffer_migrate_folio() or buffer_migrate_folio_norefs(). So this is not an indication that jfs is doing anything wrong (except maybe it's misdesigned in that the per-folio metadata caches the address of the folio, but changing that seems very much too much work to ask someone to do). What I do wonder is whether we want to have such a specialised function existing. We have can_split_folio() in huge_memory.c which is somewhat more comprehensive and doesn't require the folio to be unmapped first. I currently lack the capacity to write pseudo-code illustrating what I mean, but I'll have a try tomorrow.
On 23.04.25 02:36, Matthew Wilcox wrote: > On Tue, Apr 22, 2025 at 04:41:11PM -0700, Andrew Morton wrote: >>> +/** >>> + * folio_migrate_expected_refs - Count expected references for an unmapped folio. >> >> "folio_migration_expected_refs" > > Please run make W=1 fs/jfs/ in order to run kernel-doc on this file. > It'll flag this kind of error. > >> It's concerning that one particular filesystem needs this - one >> suspects that it is doing something wrong, or that the present API >> offerings were misdesigned. It would be helpful if the changelogs were >> to explain what is special about JFS. > > It doesn't surprise me at all. Almost no filesystem implements its own > migrate_folio operation. Without going into too much detail, almost > all filesystems can use filemap_migrate_folio(), buffer_migrate_folio() > or buffer_migrate_folio_norefs(). So this is not an indication that > jfs is doing anything wrong (except maybe it's misdesigned in that the > per-folio metadata caches the address of the folio, but changing that > seems very much too much work to ask someone to do). > > What I do wonder is whether we want to have such a specialised > function existing. We have can_split_folio() in huge_memory.c > which is somewhat more comprehensive and doesn't require the folio to be > unmapped first. I was debating with myself whether we should do the usual "refs from ->private, refs from page table mappings" .. dance, and look up the mapping from the folio instead of passing it in. I concluded that for this (migration) purpose the function is good enough as it is: if abused in wrong context (e.g., still ->private, still page table mappings), it would not fake that there are no unexpected references. Because references from ->private and page tables would be unexpected at this point. So I'm fine with this. A more generic function might be helpful, but in general it is more prone to races (e.g., page table mappings concurrently going away), so it gets trickier to document that properly. -- Cheers, David / dhildenb
On 23.04.25 09:22, David Hildenbrand wrote: > On 23.04.25 02:36, Matthew Wilcox wrote: >> On Tue, Apr 22, 2025 at 04:41:11PM -0700, Andrew Morton wrote: >>>> +/** >>>> + * folio_migrate_expected_refs - Count expected references for an unmapped folio. >>> >>> "folio_migration_expected_refs" >> >> Please run make W=1 fs/jfs/ in order to run kernel-doc on this file. >> It'll flag this kind of error. >> >>> It's concerning that one particular filesystem needs this - one >>> suspects that it is doing something wrong, or that the present API >>> offerings were misdesigned. It would be helpful if the changelogs were >>> to explain what is special about JFS. >> >> It doesn't surprise me at all. Almost no filesystem implements its own >> migrate_folio operation. Without going into too much detail, almost >> all filesystems can use filemap_migrate_folio(), buffer_migrate_folio() >> or buffer_migrate_folio_norefs(). So this is not an indication that >> jfs is doing anything wrong (except maybe it's misdesigned in that the >> per-folio metadata caches the address of the folio, but changing that >> seems very much too much work to ask someone to do). >> >> What I do wonder is whether we want to have such a specialised >> function existing. We have can_split_folio() in huge_memory.c >> which is somewhat more comprehensive and doesn't require the folio to be >> unmapped first. > > I was debating with myself whether we should do the usual "refs from > ->private, refs from page table mappings" .. dance, and look up the > mapping from the folio instead of passing it in. > > I concluded that for this (migration) purpose the function is good > enough as it is: if abused in wrong context (e.g., still ->private, > still page table mappings), it would not fake that there are no > unexpected references. Sorry, I forgot that we still care about the reference from ->private here. We expect the folio to be unmapped. -- Cheers, David / dhildenb
On Wed, Apr 23, 2025 at 09:25:05AM +0200, David Hildenbrand wrote: > On 23.04.25 09:22, David Hildenbrand wrote: > > On 23.04.25 02:36, Matthew Wilcox wrote: > > > On Tue, Apr 22, 2025 at 04:41:11PM -0700, Andrew Morton wrote: > > > > > +/** > > > > > + * folio_migrate_expected_refs - Count expected references for an unmapped folio. > > > > > > > > "folio_migration_expected_refs" > > > > > > What I do wonder is whether we want to have such a specialised > > > function existing. We have can_split_folio() in huge_memory.c > > > which is somewhat more comprehensive and doesn't require the folio to be > > > unmapped first. > > > > I was debating with myself whether we should do the usual "refs from > > ->private, refs from page table mappings" .. dance, and look up the > > mapping from the folio instead of passing it in. > > > > I concluded that for this (migration) purpose the function is good > > enough as it is: if abused in wrong context (e.g., still ->private, > > still page table mappings), it would not fake that there are no > > unexpected references. > > Sorry, I forgot that we still care about the reference from ->private here. > We expect the folio to be unmapped. Right, so just adding in folio_mapcount() will be a no-op for migration, but enable its reuse by can_split_folio(). Maybe. Anyway, the way I explain page refocunts to people (and I need to put this in a document somewhere): There are three types of contribution to the refcount: - Expected. These are deducible from the folio itself, and they're all findable. You need to figure out what the expected number of references are to a folio if you're going to try to freeze it. These can be references from the mapcount, the page cache, the swap cache, the private data, your call chain. - Temporary. Someone else has found the folio somehow; perhaps through the page cache, or by calling GUP or something. They mean you can't freeze the folio because you don't know who has the reference or how long they might hold it for. - Spurious. This is like a temporary reference, but worse because if you read the code, there should be no way for there to be any temporary references to the folio. Someone's found a stale pointer to this folio and has bumped the reference count while they check that the folio they have is the one they expected to find. They're going to find out that the pointer they followed is stale and put their refcount soon, but in the meantime you still can't freeze the folio. So I don't love the idea of having a function with the word "expected" in the name that returns a value which doesn't take into account all the potential contributors to the expected value. And sure we can keep adding qualifiers to the function name to indicate how it is to be used, but at some point I think we should say "It's OK for this to be a little less efficient so we can understand what it means".
Hi All,
Thank you for reviewing my patch and providing feedback.
On 4/24/2025 8:49 AM, Matthew Wilcox wrote:
> On Wed, Apr 23, 2025 at 09:25:05AM +0200, David Hildenbrand wrote:
>> On 23.04.25 09:22, David Hildenbrand wrote:
>>> On 23.04.25 02:36, Matthew Wilcox wrote:
>>>> On Tue, Apr 22, 2025 at 04:41:11PM -0700, Andrew Morton wrote:
>>>>>> +/**
>>>>>> + * folio_migrate_expected_refs - Count expected references for an unmapped folio.
>>>>>
>>>>> "folio_migration_expected_refs"
Thank you for catching this, I'll fix it.
I wasn't previously aware of using make W=1 to build kernel-docs and
check for warnings - this is very useful information for me.
I'll add to changelog to better explain why this is needed for JFS.
>>>>
>>>> What I do wonder is whether we want to have such a specialised
>>>> function existing. We have can_split_folio() in huge_memory.c
>>>> which is somewhat more comprehensive and doesn't require the folio to be
>>>> unmapped first.
>>>
>>> I was debating with myself whether we should do the usual "refs from
>>> ->private, refs from page table mappings" .. dance, and look up the
>>> mapping from the folio instead of passing it in.
>>>
>>> I concluded that for this (migration) purpose the function is good
>>> enough as it is: if abused in wrong context (e.g., still ->private,
>>> still page table mappings), it would not fake that there are no
>>> unexpected references.
>>
>> Sorry, I forgot that we still care about the reference from ->private here.
>> We expect the folio to be unmapped.
>
> Right, so just adding in folio_mapcount() will be a no-op for migration,
> but enable its reuse by can_split_folio(). Maybe. Anyway, the way I
> explain page refocunts to people (and I need to put this in a document
> somewhere):
>
> There are three types of contribution to the refcount:
>
> - Expected. These are deducible from the folio itself, and they're all
> findable. You need to figure out what the expected number of
> references are to a folio if you're going to try to freeze it.
> These can be references from the mapcount, the page cache, the swap
> cache, the private data, your call chain.
> - Temporary. Someone else has found the folio somehow; perhaps through
> the page cache, or by calling GUP or something. They mean you can't
> freeze the folio because you don't know who has the reference or how
> long they might hold it for.
> - Spurious. This is like a temporary reference, but worse because if
> you read the code, there should be no way for there to be any temporary
> references to the folio. Someone's found a stale pointer to this
> folio and has bumped the reference count while they check that the
> folio they have is the one they expected to find. They're going
> to find out that the pointer they followed is stale and put their
> refcount soon, but in the meantime you still can't freeze the folio.
>
> So I don't love the idea of having a function with the word "expected"
> in the name that returns a value which doesn't take into account all
> the potential contributors to the expected value. And sure we can keep
> adding qualifiers to the function name to indicate how it is to be used,
> but at some point I think we should say "It's OK for this to be a little
> less efficient so we can understand what it means".
Thank you, Willy, for the detailed explanation about page reference counting.
This has helped me understand the concept much better.
Based on your explanation and the discussion, I'm summarizing the 2 approaches:
1. Rename folio_migration_expected_refs to folio_migration_expected_base_refs, to
to clarify it does not account for other potential contributors.
or folio_unmapped_base_refs?
2. Accounting all possible contributors to expected refs:
folio_expected_refs(mapping, folio)
{
int refs = 1;
if (mapping) {
if (folio_test_anon(folio))
refs += folio_test_swapcache(folio) ?
folio_nr_pages(folio) : 0;
else
refs += folio_nr_pages(folio);
if (folio_test_private(folio))
refs++;
}
refs += folio_mapcount(folio); // takes mapped folio into account and evaluate as no-op for unmapped folios during migration
return refs;
}
Please let me know if this approach is acceptable or if you have
other suggestions for improvement.
Best Regards,
Shivank
On 24.04.25 13:57, Shivank Garg wrote:
> Hi All,
>
> Thank you for reviewing my patch and providing feedback.
>
> On 4/24/2025 8:49 AM, Matthew Wilcox wrote:
>> On Wed, Apr 23, 2025 at 09:25:05AM +0200, David Hildenbrand wrote:
>>> On 23.04.25 09:22, David Hildenbrand wrote:
>>>> On 23.04.25 02:36, Matthew Wilcox wrote:
>>>>> On Tue, Apr 22, 2025 at 04:41:11PM -0700, Andrew Morton wrote:
>>>>>>> +/**
>>>>>>> + * folio_migrate_expected_refs - Count expected references for an unmapped folio.
>>>>>>
>>>>>> "folio_migration_expected_refs"
>
> Thank you for catching this, I'll fix it.
>
> I wasn't previously aware of using make W=1 to build kernel-docs and
> check for warnings - this is very useful information for me.
>
> I'll add to changelog to better explain why this is needed for JFS.
>
>>>>>
>>>>> What I do wonder is whether we want to have such a specialised
>>>>> function existing. We have can_split_folio() in huge_memory.c
>>>>> which is somewhat more comprehensive and doesn't require the folio to be
>>>>> unmapped first.
>>>>
>>>> I was debating with myself whether we should do the usual "refs from
>>>> ->private, refs from page table mappings" .. dance, and look up the
>>>> mapping from the folio instead of passing it in.
>>>>
>>>> I concluded that for this (migration) purpose the function is good
>>>> enough as it is: if abused in wrong context (e.g., still ->private,
>>>> still page table mappings), it would not fake that there are no
>>>> unexpected references.
>>>
>>> Sorry, I forgot that we still care about the reference from ->private here.
>>> We expect the folio to be unmapped.
>>
>> Right, so just adding in folio_mapcount() will be a no-op for migration,
>> but enable its reuse by can_split_folio(). Maybe. Anyway, the way I
>> explain page refocunts to people (and I need to put this in a document
>> somewhere):
>>
>> There are three types of contribution to the refcount:
>>
>> - Expected. These are deducible from the folio itself, and they're all
>> findable. You need to figure out what the expected number of
>> references are to a folio if you're going to try to freeze it.
>> These can be references from the mapcount, the page cache, the swap
>> cache, the private data, your call chain.
>> - Temporary. Someone else has found the folio somehow; perhaps through
>> the page cache, or by calling GUP or something. They mean you can't
>> freeze the folio because you don't know who has the reference or how
>> long they might hold it for.
>> - Spurious. This is like a temporary reference, but worse because if
>> you read the code, there should be no way for there to be any temporary
>> references to the folio. Someone's found a stale pointer to this
>> folio and has bumped the reference count while they check that the
>> folio they have is the one they expected to find. They're going
>> to find out that the pointer they followed is stale and put their
>> refcount soon, but in the meantime you still can't freeze the folio.
>>
>> So I don't love the idea of having a function with the word "expected"
>> in the name that returns a value which doesn't take into account all
>> the potential contributors to the expected value. And sure we can keep
>> adding qualifiers to the function name to indicate how it is to be used,
>> but at some point I think we should say "It's OK for this to be a little
>> less efficient so we can understand what it means".
>
> Thank you, Willy, for the detailed explanation about page reference counting.
> This has helped me understand the concept much better.
>
> Based on your explanation and the discussion, I'm summarizing the 2 approaches:
>
> 1. Rename folio_migration_expected_refs to folio_migration_expected_base_refs, to
> to clarify it does not account for other potential contributors.
> or folio_unmapped_base_refs?
> 2. Accounting all possible contributors to expected refs:
> folio_expected_refs(mapping, folio)
> {
> int refs = 1;
>
> if (mapping) {
> if (folio_test_anon(folio))
> refs += folio_test_swapcache(folio) ?
> folio_nr_pages(folio) : 0;
> else
> refs += folio_nr_pages(folio);
>
> if (folio_test_private(folio))
> refs++;
> }
> refs += folio_mapcount(folio); // takes mapped folio into account and evaluate as no-op for unmapped folios during migration
> return refs;
> }
>
> Please let me know if this approach is acceptable or if you have
> other suggestions for improvement.
A couple of points:
1) Can we name it folio_expected_ref_count()
2) Can we avoid passing in the mapping? Might not be expensive to look it
up again. Below I avoid calling folio_mapping().
3) Can we delegate adding the additional reference to the caller? Will make it
easier to use elsewhere (e.g., not additional reference because we are holding
the page table lock).
4) Can we add kerneldoc, and in particular document the semantics?
Not sure if we should inline this function or put it into mm/utils.c
I'm thinking of something like (completely untested):
diff --git a/include/linux/mm.h b/include/linux/mm.h
index a205020e2a58b..a0ad4ed9a75ff 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2112,6 +2112,61 @@ static inline bool folio_maybe_mapped_shared(struct folio *folio)
return folio_test_large_maybe_mapped_shared(folio);
}
+/**
+ * folio_expected_ref_count - calculate the expected folio refcount
+ * @folio: the folio
+ *
+ * Calculate the expected folio refcount, taking references from the pagecache,
+ * swapcache, PG_private and page table mappings into account. Useful in
+ * combination with folio_ref_count() to detect unexpected references (e.g.,
+ * GUP or other temporary references).
+ *
+ * Does currently not consider references from the LRU cache. If the folio
+ * was isolated from the LRU (which is the case during migration or split),
+ * the folio was already isolated from the LRU and the LRU cache does not apply.
+ *
+ * Calling this function on an unmapped folio -- !folio_mapped() -- that is
+ * locked will return a stable result.
+ *
+ * Calling this function on a mapped folio will not result in a stable result,
+ * because nothing stops additional page table mappings from coming (e.g.,
+ * fork()) or going (e.g., munmap()).
+ *
+ * Calling this function without the folio lock will also not result in a
+ * stable result: for example, the folio might get dropped from the swapcache
+ * concurrently.
+ *
+ * However, even when called without the folio lock or on a mapped folio,
+ * this function can be used to detect unexpected references early (for example.
+ * if it makes sense to even lock the folio and unmap it).
+ *
+ * The caller must add any reference (e.g., from folio_try_get()) it might be
+ * holding itself to the result.
+ *
+ * Returns the expected folio refcount.
+ */
+static inline int folio_expected_ref_count(const struct folio *folio)
+{
+ const int order = folio_order(folio);
+ int ref_count = 0;
+
+ if (WARN_ON_ONCE(folio_test_slab(folio)))
+ return 0;
+
+ if (folio_test_anon(folio)) {
+ /* One reference per page from the swapcache. */
+ ref_count += folio_test_swapcache(folio) << order;
+ } else if (!((unsigned long)folio->mapping & PAGE_MAPPING_FLAGS)) {
+ /* One reference per page from the pagecache. */
+ ref_count += !!folio->mapping << order;
+ /* One reference from PG_private. */
+ ref_count += folio_test_private(folio);
+ }
+
+ /* One reference per page table mapping. */
+ return ref_count + folio_mapcount(folio);;
+}
+
#ifndef HAVE_ARCH_MAKE_FOLIO_ACCESSIBLE
static inline int arch_make_folio_accessible(struct folio *folio)
{
--
2.49.0
The PAGE_MAPPING_FLAGS can likely go away soon (I have patches for that),
then we only have to test for folio->mapping.
--
Cheers,
David / dhildenb
On 4/25/2025 1:17 PM, David Hildenbrand wrote:
> On 24.04.25 13:57, Shivank Garg wrote:
>> Hi All,
>>
>> Thank you for reviewing my patch and providing feedback.
>>
>> On 4/24/2025 8:49 AM, Matthew Wilcox wrote:
>>> On Wed, Apr 23, 2025 at 09:25:05AM +0200, David Hildenbrand wrote:
>>>> On 23.04.25 09:22, David Hildenbrand wrote:
>>>>> On 23.04.25 02:36, Matthew Wilcox wrote:
>>>>>> On Tue, Apr 22, 2025 at 04:41:11PM -0700, Andrew Morton wrote:
>>>>>>>> +/**
>>>>>>>> + * folio_migrate_expected_refs - Count expected references for an unmapped folio.
>>>>>>>
>>>>>>> "folio_migration_expected_refs"
>>
>> Thank you for catching this, I'll fix it.
>>
>> I wasn't previously aware of using make W=1 to build kernel-docs and
>> check for warnings - this is very useful information for me.
>>
>> I'll add to changelog to better explain why this is needed for JFS.
>>
>>>>>>
>>>>>> What I do wonder is whether we want to have such a specialised
>>>>>> function existing. We have can_split_folio() in huge_memory.c
>>>>>> which is somewhat more comprehensive and doesn't require the folio to be
>>>>>> unmapped first.
>>>>>
>>>>> I was debating with myself whether we should do the usual "refs from
>>>>> ->private, refs from page table mappings" .. dance, and look up the
>>>>> mapping from the folio instead of passing it in.
>>>>>
>>>>> I concluded that for this (migration) purpose the function is good
>>>>> enough as it is: if abused in wrong context (e.g., still ->private,
>>>>> still page table mappings), it would not fake that there are no
>>>>> unexpected references.
>>>>
>>>> Sorry, I forgot that we still care about the reference from ->private here.
>>>> We expect the folio to be unmapped.
>>>
>>> Right, so just adding in folio_mapcount() will be a no-op for migration,
>>> but enable its reuse by can_split_folio(). Maybe. Anyway, the way I
>>> explain page refocunts to people (and I need to put this in a document
>>> somewhere):
>>>
>>> There are three types of contribution to the refcount:
>>>
>>> - Expected. These are deducible from the folio itself, and they're all
>>> findable. You need to figure out what the expected number of
>>> references are to a folio if you're going to try to freeze it.
>>> These can be references from the mapcount, the page cache, the swap
>>> cache, the private data, your call chain.
>>> - Temporary. Someone else has found the folio somehow; perhaps through
>>> the page cache, or by calling GUP or something. They mean you can't
>>> freeze the folio because you don't know who has the reference or how
>>> long they might hold it for.
>>> - Spurious. This is like a temporary reference, but worse because if
>>> you read the code, there should be no way for there to be any temporary
>>> references to the folio. Someone's found a stale pointer to this
>>> folio and has bumped the reference count while they check that the
>>> folio they have is the one they expected to find. They're going
>>> to find out that the pointer they followed is stale and put their
>>> refcount soon, but in the meantime you still can't freeze the folio.
>>>
>>> So I don't love the idea of having a function with the word "expected"
>>> in the name that returns a value which doesn't take into account all
>>> the potential contributors to the expected value. And sure we can keep
>>> adding qualifiers to the function name to indicate how it is to be used,
>>> but at some point I think we should say "It's OK for this to be a little
>>> less efficient so we can understand what it means".
>>
>> Thank you, Willy, for the detailed explanation about page reference counting.
>> This has helped me understand the concept much better.
>>
>> Based on your explanation and the discussion, I'm summarizing the 2 approaches:
>>
>> 1. Rename folio_migration_expected_refs to folio_migration_expected_base_refs, to
>> to clarify it does not account for other potential contributors.
>> or folio_unmapped_base_refs?
>> 2. Accounting all possible contributors to expected refs:
>> folio_expected_refs(mapping, folio)
>> {
>> int refs = 1;
>>
>> if (mapping) {
>> if (folio_test_anon(folio))
>> refs += folio_test_swapcache(folio) ?
>> folio_nr_pages(folio) : 0;
>> else
>> refs += folio_nr_pages(folio);
>>
>> if (folio_test_private(folio))
>> refs++;
>> }
>> refs += folio_mapcount(folio); // takes mapped folio into account and evaluate as no-op for unmapped folios during migration
>> return refs;
>> }
>>
>> Please let me know if this approach is acceptable or if you have
>> other suggestions for improvement.
>
> A couple of points:
>
> 1) Can we name it folio_expected_ref_count()
>
> 2) Can we avoid passing in the mapping? Might not be expensive to look it
> up again. Below I avoid calling folio_mapping().
>
> 3) Can we delegate adding the additional reference to the caller? Will make it
> easier to use elsewhere (e.g., not additional reference because we are holding
> the page table lock).
>
> 4) Can we add kerneldoc, and in particular document the semantics?
>
> Not sure if we should inline this function or put it into mm/utils.c
>
Hi David,
Thank you for the detailed suggestions. They all make sense to me.
I did not understand a few changes in your patch below:
>
> I'm thinking of something like (completely untested):
>
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a205020e2a58b..a0ad4ed9a75ff 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2112,6 +2112,61 @@ static inline bool folio_maybe_mapped_shared(struct folio *folio)
> return folio_test_large_maybe_mapped_shared(folio);
> }
>
> +/**
> + * folio_expected_ref_count - calculate the expected folio refcount
> + * @folio: the folio
> + *
> + * Calculate the expected folio refcount, taking references from the pagecache,
> + * swapcache, PG_private and page table mappings into account. Useful in
> + * combination with folio_ref_count() to detect unexpected references (e.g.,
> + * GUP or other temporary references).
> + *
> + * Does currently not consider references from the LRU cache. If the folio
> + * was isolated from the LRU (which is the case during migration or split),
> + * the folio was already isolated from the LRU and the LRU cache does not apply.
> + *
> + * Calling this function on an unmapped folio -- !folio_mapped() -- that is
> + * locked will return a stable result.
> + *
> + * Calling this function on a mapped folio will not result in a stable result,
> + * because nothing stops additional page table mappings from coming (e.g.,
> + * fork()) or going (e.g., munmap()).
> + *
> + * Calling this function without the folio lock will also not result in a
> + * stable result: for example, the folio might get dropped from the swapcache
> + * concurrently.
> + *
> + * However, even when called without the folio lock or on a mapped folio,
> + * this function can be used to detect unexpected references early (for example.
> + * if it makes sense to even lock the folio and unmap it).
> + *
> + * The caller must add any reference (e.g., from folio_try_get()) it might be
> + * holding itself to the result.
> + *
> + * Returns the expected folio refcount.
> + */
> +static inline int folio_expected_ref_count(const struct folio *folio)
> +{
> + const int order = folio_order(folio);
> + int ref_count = 0;
Why are we not taking base ref_count as 1 like it's done in original folio_expected_refs
implementation?
> +
> + if (WARN_ON_ONCE(folio_test_slab(folio)))
> + return 0;
> +
> + if (folio_test_anon(folio)) {
> + /* One reference per page from the swapcache. */
> + ref_count += folio_test_swapcache(folio) << order;
why not use folio_nr_pages() here instead 1 << order?
something like folio_test_swapcache(folio) * folio_nr_pages(folio).
> + } else if (!((unsigned long)folio->mapping & PAGE_MAPPING_FLAGS)) {
> + /* One reference per page from the pagecache. */
> + ref_count += !!folio->mapping << order;
> + /* One reference from PG_private. */
> + ref_count += folio_test_private(folio);
> + }
> +
> + /* One reference per page table mapping. */
> + return ref_count + folio_mapcount(folio);;
> +}
> +
> #ifndef HAVE_ARCH_MAKE_FOLIO_ACCESSIBLE
> static inline int arch_make_folio_accessible(struct folio *folio)
> {
I tested your patch with stress-ng and my move-pages test code. I did not see
any bugs/errors.
Thanks,
Shivank
On 29.04.25 12:57, Shivank Garg wrote:
>
>
> On 4/25/2025 1:17 PM, David Hildenbrand wrote:
>> On 24.04.25 13:57, Shivank Garg wrote:
>>> Hi All,
>>>
>>> Thank you for reviewing my patch and providing feedback.
>>>
>>> On 4/24/2025 8:49 AM, Matthew Wilcox wrote:
>>>> On Wed, Apr 23, 2025 at 09:25:05AM +0200, David Hildenbrand wrote:
>>>>> On 23.04.25 09:22, David Hildenbrand wrote:
>>>>>> On 23.04.25 02:36, Matthew Wilcox wrote:
>>>>>>> On Tue, Apr 22, 2025 at 04:41:11PM -0700, Andrew Morton wrote:
>>>>>>>>> +/**
>>>>>>>>> + * folio_migrate_expected_refs - Count expected references for an unmapped folio.
>>>>>>>>
>>>>>>>> "folio_migration_expected_refs"
>>>
>>> Thank you for catching this, I'll fix it.
>>>
>>> I wasn't previously aware of using make W=1 to build kernel-docs and
>>> check for warnings - this is very useful information for me.
>>>
>>> I'll add to changelog to better explain why this is needed for JFS.
>>>
>>>>>>>
>>>>>>> What I do wonder is whether we want to have such a specialised
>>>>>>> function existing. We have can_split_folio() in huge_memory.c
>>>>>>> which is somewhat more comprehensive and doesn't require the folio to be
>>>>>>> unmapped first.
>>>>>>
>>>>>> I was debating with myself whether we should do the usual "refs from
>>>>>> ->private, refs from page table mappings" .. dance, and look up the
>>>>>> mapping from the folio instead of passing it in.
>>>>>>
>>>>>> I concluded that for this (migration) purpose the function is good
>>>>>> enough as it is: if abused in wrong context (e.g., still ->private,
>>>>>> still page table mappings), it would not fake that there are no
>>>>>> unexpected references.
>>>>>
>>>>> Sorry, I forgot that we still care about the reference from ->private here.
>>>>> We expect the folio to be unmapped.
>>>>
>>>> Right, so just adding in folio_mapcount() will be a no-op for migration,
>>>> but enable its reuse by can_split_folio(). Maybe. Anyway, the way I
>>>> explain page refocunts to people (and I need to put this in a document
>>>> somewhere):
>>>>
>>>> There are three types of contribution to the refcount:
>>>>
>>>> - Expected. These are deducible from the folio itself, and they're all
>>>> findable. You need to figure out what the expected number of
>>>> references are to a folio if you're going to try to freeze it.
>>>> These can be references from the mapcount, the page cache, the swap
>>>> cache, the private data, your call chain.
>>>> - Temporary. Someone else has found the folio somehow; perhaps through
>>>> the page cache, or by calling GUP or something. They mean you can't
>>>> freeze the folio because you don't know who has the reference or how
>>>> long they might hold it for.
>>>> - Spurious. This is like a temporary reference, but worse because if
>>>> you read the code, there should be no way for there to be any temporary
>>>> references to the folio. Someone's found a stale pointer to this
>>>> folio and has bumped the reference count while they check that the
>>>> folio they have is the one they expected to find. They're going
>>>> to find out that the pointer they followed is stale and put their
>>>> refcount soon, but in the meantime you still can't freeze the folio.
>>>>
>>>> So I don't love the idea of having a function with the word "expected"
>>>> in the name that returns a value which doesn't take into account all
>>>> the potential contributors to the expected value. And sure we can keep
>>>> adding qualifiers to the function name to indicate how it is to be used,
>>>> but at some point I think we should say "It's OK for this to be a little
>>>> less efficient so we can understand what it means".
>>>
>>> Thank you, Willy, for the detailed explanation about page reference counting.
>>> This has helped me understand the concept much better.
>>>
>>> Based on your explanation and the discussion, I'm summarizing the 2 approaches:
>>>
>>> 1. Rename folio_migration_expected_refs to folio_migration_expected_base_refs, to
>>> to clarify it does not account for other potential contributors.
>>> or folio_unmapped_base_refs?
>>> 2. Accounting all possible contributors to expected refs:
>>> folio_expected_refs(mapping, folio)
>>> {
>>> int refs = 1;
>>>
>>> if (mapping) {
>>> if (folio_test_anon(folio))
>>> refs += folio_test_swapcache(folio) ?
>>> folio_nr_pages(folio) : 0;
>>> else
>>> refs += folio_nr_pages(folio);
>>>
>>> if (folio_test_private(folio))
>>> refs++;
>>> }
>>> refs += folio_mapcount(folio); // takes mapped folio into account and evaluate as no-op for unmapped folios during migration
>>> return refs;
>>> }
>>>
>>> Please let me know if this approach is acceptable or if you have
>>> other suggestions for improvement.
>>
>> A couple of points:
>>
>> 1) Can we name it folio_expected_ref_count()
>>
>> 2) Can we avoid passing in the mapping? Might not be expensive to look it
>> up again. Below I avoid calling folio_mapping().
>>
>> 3) Can we delegate adding the additional reference to the caller? Will make it
>> easier to use elsewhere (e.g., not additional reference because we are holding
>> the page table lock).
>>
>> 4) Can we add kerneldoc, and in particular document the semantics?
>>
>> Not sure if we should inline this function or put it into mm/utils.c
>>
>
> Hi David,
>
> Thank you for the detailed suggestions. They all make sense to me.
>
> I did not understand a few changes in your patch below:
>>
>> I'm thinking of something like (completely untested):
>>
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index a205020e2a58b..a0ad4ed9a75ff 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -2112,6 +2112,61 @@ static inline bool folio_maybe_mapped_shared(struct folio *folio)
>> return folio_test_large_maybe_mapped_shared(folio);
>> }
>>
>> +/**
>> + * folio_expected_ref_count - calculate the expected folio refcount
>> + * @folio: the folio
>> + *
>> + * Calculate the expected folio refcount, taking references from the pagecache,
>> + * swapcache, PG_private and page table mappings into account. Useful in
>> + * combination with folio_ref_count() to detect unexpected references (e.g.,
>> + * GUP or other temporary references).
>> + *
>> + * Does currently not consider references from the LRU cache. If the folio
>> + * was isolated from the LRU (which is the case during migration or split),
>> + * the folio was already isolated from the LRU and the LRU cache does not apply.
>> + *
>> + * Calling this function on an unmapped folio -- !folio_mapped() -- that is
>> + * locked will return a stable result.
>> + *
>> + * Calling this function on a mapped folio will not result in a stable result,
>> + * because nothing stops additional page table mappings from coming (e.g.,
>> + * fork()) or going (e.g., munmap()).
>> + *
>> + * Calling this function without the folio lock will also not result in a
>> + * stable result: for example, the folio might get dropped from the swapcache
>> + * concurrently.
>> + *
>> + * However, even when called without the folio lock or on a mapped folio,
>> + * this function can be used to detect unexpected references early (for example.
>> + * if it makes sense to even lock the folio and unmap it).
>> + *
>> + * The caller must add any reference (e.g., from folio_try_get()) it might be
>> + * holding itself to the result.
>> + *
>> + * Returns the expected folio refcount.
>> + */
>> +static inline int folio_expected_ref_count(const struct folio *folio)
>> +{
>> + const int order = folio_order(folio);
>> + int ref_count = 0;
>
> Why are we not taking base ref_count as 1 like it's done in original folio_expected_refs
> implementation?
The idea is that this is the responsibility of the caller, which will
make this function more versatile.
For example, when we're holding the page table lock and want to check
for unexpected references, we wouldn't be holding any additional
reference from a folio_try_get() like migration code would.
>
>> +
>> + if (WARN_ON_ONCE(folio_test_slab(folio)))
>> + return 0;
>> +
>> + if (folio_test_anon(folio)) {
>> + /* One reference per page from the swapcache. */
>> + ref_count += folio_test_swapcache(folio) << order;
>
> why not use folio_nr_pages() here instead 1 << order?
> something like folio_test_swapcache(folio) * folio_nr_pages(folio).
A shift is typically cheaper than a multiplication, so it looked like a
low hanging fruit to use a shift here.
>
>> + } else if (!((unsigned long)folio->mapping & PAGE_MAPPING_FLAGS)) {
>> + /* One reference per page from the pagecache. */
>> + ref_count += !!folio->mapping << order;
>> + /* One reference from PG_private. */
>> + ref_count += folio_test_private(folio);
>> + }
>> +
>> + /* One reference per page table mapping. */
>> + return ref_count + folio_mapcount(folio);;
>
>> +}
>> +
>> #ifndef HAVE_ARCH_MAKE_FOLIO_ACCESSIBLE
>> static inline int arch_make_folio_accessible(struct folio *folio)
>> {
>
> I tested your patch with stress-ng and my move-pages test code. I did not see
> any bugs/errors.
Cool! It would be good to get some feedback from Willy on the kerneldoc,
if he's aware of other constraints etc.
--
Cheers,
David / dhildenb
On 22 Apr 2025, at 7:40, Shivank Garg wrote: > Rename the previously static folio_expected_refs() to clarify its > purpose and scope, making it an inline function > folio_migration_expected_refs() to calculate expected folio references > during migration. The function is only suitable for folios unmapped from > page tables. > > Signed-off-by: Shivank Garg <shivankg@amd.com> > --- > include/linux/migrate.h | 26 ++++++++++++++++++++++++++ > mm/migrate.c | 22 ++++------------------ > 2 files changed, 30 insertions(+), 18 deletions(-) > Acked-by: Zi Yan <ziy@nvidia.com> Best Regards, Yan, Zi
On 22.04.25 13:40, Shivank Garg wrote: > Rename the previously static folio_expected_refs() to clarify its > purpose and scope, making it an inline function > folio_migration_expected_refs() to calculate expected folio references > during migration. The function is only suitable for folios unmapped from > page tables. > > Signed-off-by: Shivank Garg <shivankg@amd.com> > --- Thanks! Acked-by: David Hildenbrand <david@redhat.com> -- Cheers, David / dhildenb
© 2016 - 2026 Red Hat, Inc.