Hi everyone,
This series contains a few patches to remove unneeded lock page and
PageMovable check, reduce the rcu lock duration. Also we fix potential
pte_unmap on an not mapped pte. More details can be found in the
respective changelogs. Thanks!
---
v2:
collect Reviewed-by tag
make isolate_huge_page consistent with isolate_lru_page
add hugetlbfs variant of hugetlb_migration_entry_wait
v1:
rebase [1] on mainline.
[1] https://lore.kernel.org/lkml/[email protected]/T/
---
Miaohe Lin (4):
mm/migration: reduce the rcu lock duration
mm/migration: remove unneeded lock page and PageMovable check
mm/migration: return errno when isolate_huge_page failed
mm/migration: fix potential pte_unmap on an not mapped pte
include/linux/hugetlb.h | 6 +++---
include/linux/swapops.h | 12 ++++++++----
mm/gup.c | 2 +-
mm/hugetlb.c | 15 +++++++--------
mm/memory-failure.c | 2 +-
mm/mempolicy.c | 2 +-
mm/migrate.c | 39 +++++++++++++++++++++++++--------------
7 files changed, 46 insertions(+), 32 deletions(-)
--
2.23.0
When non-lru movable page was freed from under us, __ClearPageMovable must
have been done. Even if it's not done, ClearPageIsolated here won't hurt
as page will be freed anyway. So we can thus remove unneeded lock page and
PageMovable check here.
Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
mm/migrate.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index b779646665fe..0fc4651b3e39 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1093,12 +1093,8 @@ static int unmap_and_move(new_page_t get_new_page,
/* page was freed from under us. So we are done. */
ClearPageActive(page);
ClearPageUnevictable(page);
- if (unlikely(__PageMovable(page))) {
- lock_page(page);
- if (!PageMovable(page))
- ClearPageIsolated(page);
- unlock_page(page);
- }
+ if (unlikely(__PageMovable(page)))
+ ClearPageIsolated(page);
goto out;
}
--
2.23.0
__migration_entry_wait and migration_entry_wait_on_locked assume pte is
always mapped from caller. But this is not the case when it's called from
migration_entry_wait_huge and follow_huge_pmd. Add a hugetlbfs variant that
calls hugetlb_migration_entry_wait(ptep == NULL) to fix this issue.
Fixes: 30dad30922cc ("mm: migration: add migrate_entry_wait_huge()")
Suggested-by: David Hildenbrand <[email protected]>
Signed-off-by: Miaohe Lin <[email protected]>
---
include/linux/swapops.h | 12 ++++++++----
mm/hugetlb.c | 4 ++--
mm/migrate.c | 23 +++++++++++++++++++----
3 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/include/linux/swapops.h b/include/linux/swapops.h
index 30cded849ee4..862e5a2053b1 100644
--- a/include/linux/swapops.h
+++ b/include/linux/swapops.h
@@ -244,8 +244,10 @@ extern void __migration_entry_wait(struct mm_struct *mm, pte_t *ptep,
spinlock_t *ptl);
extern void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,
unsigned long address);
-extern void migration_entry_wait_huge(struct vm_area_struct *vma,
- struct mm_struct *mm, pte_t *pte);
+#ifdef CONFIG_HUGETLB_PAGE
+extern void __migration_entry_wait_huge(pte_t *ptep, spinlock_t *ptl);
+extern void migration_entry_wait_huge(struct vm_area_struct *vma, pte_t *pte);
+#endif
#else
static inline swp_entry_t make_readable_migration_entry(pgoff_t offset)
{
@@ -271,8 +273,10 @@ static inline void __migration_entry_wait(struct mm_struct *mm, pte_t *ptep,
spinlock_t *ptl) { }
static inline void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,
unsigned long address) { }
-static inline void migration_entry_wait_huge(struct vm_area_struct *vma,
- struct mm_struct *mm, pte_t *pte) { }
+#ifdef CONFIG_HUGETLB_PAGE
+static inline void __migration_entry_wait_huge(pte_t *ptep, spinlock_t *ptl) { }
+static inline void migration_entry_wait_huge(struct vm_area_struct *vma, pte_t *pte) { }
+#endif
static inline int is_writable_migration_entry(swp_entry_t entry)
{
return 0;
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 098f81e8550d..994361ec75e0 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5689,7 +5689,7 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
*/
entry = huge_ptep_get(ptep);
if (unlikely(is_hugetlb_entry_migration(entry))) {
- migration_entry_wait_huge(vma, mm, ptep);
+ migration_entry_wait_huge(vma, ptep);
return 0;
} else if (unlikely(is_hugetlb_entry_hwpoisoned(entry)))
return VM_FAULT_HWPOISON_LARGE |
@@ -6907,7 +6907,7 @@ follow_huge_pmd(struct mm_struct *mm, unsigned long address,
} else {
if (is_hugetlb_entry_migration(pte)) {
spin_unlock(ptl);
- __migration_entry_wait(mm, (pte_t *)pmd, ptl);
+ __migration_entry_wait_huge((pte_t *)pmd, ptl);
goto retry;
}
/*
diff --git a/mm/migrate.c b/mm/migrate.c
index c937a496239b..7b31d0b06977 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -315,13 +315,28 @@ void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,
__migration_entry_wait(mm, ptep, ptl);
}
-void migration_entry_wait_huge(struct vm_area_struct *vma,
- struct mm_struct *mm, pte_t *pte)
+#ifdef CONFIG_HUGETLB_PAGE
+void __migration_entry_wait_huge(pte_t *ptep, spinlock_t *ptl)
{
- spinlock_t *ptl = huge_pte_lockptr(hstate_vma(vma), mm, pte);
- __migration_entry_wait(mm, pte, ptl);
+ pte_t pte;
+
+ spin_lock(ptl);
+ pte = huge_ptep_get(ptep);
+
+ if (unlikely(!is_hugetlb_entry_migration(pte)))
+ spin_unlock(ptl);
+ else
+ migration_entry_wait_on_locked(pte_to_swp_entry(pte), NULL, ptl);
}
+void migration_entry_wait_huge(struct vm_area_struct *vma, pte_t *pte)
+{
+ spinlock_t *ptl = huge_pte_lockptr(hstate_vma(vma), vma->vm_mm, pte);
+
+ __migration_entry_wait_huge(pte, ptl);
+}
+#endif
+
#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
void pmd_migration_entry_wait(struct mm_struct *mm, pmd_t *pmd)
{
--
2.23.0
On 25.04.22 15:27, Miaohe Lin wrote:
> When non-lru movable page was freed from under us, __ClearPageMovable must
> have been done. Even if it's not done, ClearPageIsolated here won't hurt
> as page will be freed anyway. So we can thus remove unneeded lock page and
> PageMovable check here.
>
> Signed-off-by: Miaohe Lin <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> ---
> mm/migrate.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index b779646665fe..0fc4651b3e39 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1093,12 +1093,8 @@ static int unmap_and_move(new_page_t get_new_page,
> /* page was freed from under us. So we are done. */
> ClearPageActive(page);
> ClearPageUnevictable(page);
> - if (unlikely(__PageMovable(page))) {
> - lock_page(page);
> - if (!PageMovable(page))
> - ClearPageIsolated(page);
> - unlock_page(page);
> - }
> + if (unlikely(__PageMovable(page)))
> + ClearPageIsolated(page);
> goto out;
> }
Hm, that code+change raises a couple of questions.
We're doing here the same as in putback_movable_pages(). So I guess the
difference here is that the caller did release the reference while the
page was isolated, while we don't assume the same in
putback_movable_pages().
Shouldn't whoever owned the page have cleared that? IOW, is it even
valid that we see a movable or isolated page here (WARN/BUG?)?
At least for balloon compaction, I remember that __PageMovable() is
properly cleared before freeing it via balloon_page_delete().
Also, I am not sure how reliable that page count check is here: if we'd
have another speculative reference to the page, we might see
"page_count(page) > 1" and not take that path, although the previous
owner released the last reference.
--
Thanks,
David / dhildenb
On 25.04.22 15:27, Miaohe Lin wrote:
> __migration_entry_wait and migration_entry_wait_on_locked assume pte is
> always mapped from caller. But this is not the case when it's called from
> migration_entry_wait_huge and follow_huge_pmd. Add a hugetlbfs variant that
> calls hugetlb_migration_entry_wait(ptep == NULL) to fix this issue.
>
> Fixes: 30dad30922cc ("mm: migration: add migrate_entry_wait_huge()")
> Suggested-by: David Hildenbrand <[email protected]>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> include/linux/swapops.h | 12 ++++++++----
> mm/hugetlb.c | 4 ++--
> mm/migrate.c | 23 +++++++++++++++++++----
> 3 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> index 30cded849ee4..862e5a2053b1 100644
> --- a/include/linux/swapops.h
> +++ b/include/linux/swapops.h
> @@ -244,8 +244,10 @@ extern void __migration_entry_wait(struct mm_struct *mm, pte_t *ptep,
> spinlock_t *ptl);
> extern void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,
> unsigned long address);
> -extern void migration_entry_wait_huge(struct vm_area_struct *vma,
> - struct mm_struct *mm, pte_t *pte);
> +#ifdef CONFIG_HUGETLB_PAGE
> +extern void __migration_entry_wait_huge(pte_t *ptep, spinlock_t *ptl);
> +extern void migration_entry_wait_huge(struct vm_area_struct *vma, pte_t *pte);
> +#endif
> #else
> static inline swp_entry_t make_readable_migration_entry(pgoff_t offset)
> {
> @@ -271,8 +273,10 @@ static inline void __migration_entry_wait(struct mm_struct *mm, pte_t *ptep,
> spinlock_t *ptl) { }
> static inline void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,
> unsigned long address) { }
> -static inline void migration_entry_wait_huge(struct vm_area_struct *vma,
> - struct mm_struct *mm, pte_t *pte) { }
> +#ifdef CONFIG_HUGETLB_PAGE
> +static inline void __migration_entry_wait_huge(pte_t *ptep, spinlock_t *ptl) { }
> +static inline void migration_entry_wait_huge(struct vm_area_struct *vma, pte_t *pte) { }
> +#endif
> static inline int is_writable_migration_entry(swp_entry_t entry)
> {
> return 0;
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 098f81e8550d..994361ec75e0 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5689,7 +5689,7 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> */
> entry = huge_ptep_get(ptep);
> if (unlikely(is_hugetlb_entry_migration(entry))) {
> - migration_entry_wait_huge(vma, mm, ptep);
> + migration_entry_wait_huge(vma, ptep);
> return 0;
> } else if (unlikely(is_hugetlb_entry_hwpoisoned(entry)))
> return VM_FAULT_HWPOISON_LARGE |
> @@ -6907,7 +6907,7 @@ follow_huge_pmd(struct mm_struct *mm, unsigned long address,
> } else {
> if (is_hugetlb_entry_migration(pte)) {
> spin_unlock(ptl);
> - __migration_entry_wait(mm, (pte_t *)pmd, ptl);
> + __migration_entry_wait_huge((pte_t *)pmd, ptl);
The unlock+immediate relock looks a bit sub-optimal, but that's already
been that way before your change.
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 2022/4/29 18:07, David Hildenbrand wrote:
> On 25.04.22 15:27, Miaohe Lin wrote:
>> When non-lru movable page was freed from under us, __ClearPageMovable must
>> have been done. Even if it's not done, ClearPageIsolated here won't hurt
>> as page will be freed anyway. So we can thus remove unneeded lock page and
>> PageMovable check here.
>>
>> Signed-off-by: Miaohe Lin <[email protected]>
>> Reviewed-by: Christoph Hellwig <[email protected]>
>> ---
>> mm/migrate.c | 8 ++------
>> 1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index b779646665fe..0fc4651b3e39 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1093,12 +1093,8 @@ static int unmap_and_move(new_page_t get_new_page,
>> /* page was freed from under us. So we are done. */
>> ClearPageActive(page);
>> ClearPageUnevictable(page);
>> - if (unlikely(__PageMovable(page))) {
>> - lock_page(page);
>> - if (!PageMovable(page))
>> - ClearPageIsolated(page);
>> - unlock_page(page);
>> - }
>> + if (unlikely(__PageMovable(page)))
>> + ClearPageIsolated(page);
>> goto out;
>> }
>
> Hm, that code+change raises a couple of questions.
>
> We're doing here the same as in putback_movable_pages(). So I guess the
> difference here is that the caller did release the reference while the
> page was isolated, while we don't assume the same in
> putback_movable_pages().
Agree.
>
>
> Shouldn't whoever owned the page have cleared that? IOW, is it even
> valid that we see a movable or isolated page here (WARN/BUG?)?
>
> At least for balloon compaction, I remember that __PageMovable() is
> properly cleared before freeing it via balloon_page_delete().
z3fold, zsmalloc will do __ClearPageMovable when the page is going to be released.
So I think we shouldn't see a movable page here:
void __ClearPageMovable(struct page *page)
{
VM_BUG_ON_PAGE(!PageMovable(page), page);
/*
* Clear registered address_space val with keeping PAGE_MAPPING_MOVABLE
* flag so that VM can catch up released page by driver after isolation.
* With it, VM migration doesn't try to put it back.
*/
page->mapping = (void *)((unsigned long)page->mapping &
PAGE_MAPPING_MOVABLE);
}
But it seems there is no guarantee for PageIsolated flag. Or am I miss something?
>
>
> Also, I am not sure how reliable that page count check is here: if we'd
> have another speculative reference to the page, we might see
> "page_count(page) > 1" and not take that path, although the previous
> owner released the last reference.
IIUC, there should not be such speculative reference. The driver should have taken care
of it.
Thanks!
>
>
On 2022/5/11 23:23, David Hildenbrand wrote:
> On 09.05.22 10:51, Miaohe Lin wrote:
>> On 2022/4/29 18:07, David Hildenbrand wrote:
snip
>>
>> z3fold, zsmalloc will do __ClearPageMovable when the page is going to be released.
>> So I think we shouldn't see a movable page here:
>>
>> void __ClearPageMovable(struct page *page)
>> {
>> VM_BUG_ON_PAGE(!PageMovable(page), page);
>> /*
>> * Clear registered address_space val with keeping PAGE_MAPPING_MOVABLE
>> * flag so that VM can catch up released page by driver after isolation.
>> * With it, VM migration doesn't try to put it back.
>> */
>> page->mapping = (void *)((unsigned long)page->mapping &
>> PAGE_MAPPING_MOVABLE);
>> }
>>
>> But it seems there is no guarantee for PageIsolated flag. Or am I miss something?
>
> At least the code we have now:
>
> if (unlikely(__PageMovable(page)))
> ClearPageIsolated(page);
>
> Should be dead code. So PG_isolated could remain set.
>
> If PG_isolated is still set, it will get cleared in the buddy when
> freeing the page via
>
> page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
Yes, check_free_page only complains about flags belonging to PAGE_FLAGS_CHECK_AT_FREE and PG_isolated
will be cleared in the buddy when freeing the page. But it might not be a good idea to reply on this ?
IMHO, it should be better to clear the PG_isolated explicitly ourselves.
>
>>
>>>
>>>
>>> Also, I am not sure how reliable that page count check is here: if we'd
>>> have another speculative reference to the page, we might see
>>> "page_count(page) > 1" and not take that path, although the previous
>>> owner released the last reference.
>>
>> IIUC, there should not be such speculative reference. The driver should have taken care
>> of it.
>
> How can you prevent any kind of speculative references?
>
> See isolate_movable_page() as an example, which grabs a speculative
> reference to then find out that the page is already isolated by someone
> else, to then back off.
You're right. isolate_movable_page will be an speculative references case. But the page count check here
is just an optimization. If we encounter speculative references, it still works with useless effort of
migrating to be released page.
Thanks!
>
On 2022/5/12 15:10, David Hildenbrand wrote:
>>> If PG_isolated is still set, it will get cleared in the buddy when
>>> freeing the page via
>>>
>>> page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>
>> Yes, check_free_page only complains about flags belonging to PAGE_FLAGS_CHECK_AT_FREE and PG_isolated
>> will be cleared in the buddy when freeing the page. But it might not be a good idea to reply on this ?
>> IMHO, it should be better to clear the PG_isolated explicitly ourselves.
>
> I think we can pretty much rely on this handling in the buddy :)
So is the below code change what you're suggesting?
if (page_count(page) == 1) {
/* page was freed from under us. So we are done. */
ClearPageActive(page);
ClearPageUnevictable(page);
- if (unlikely(__PageMovable(page)))
- ClearPageIsolated(page);
goto out;
}
>
>>
>>>
>>>>
>>>>>
>>>>>
>>>>> Also, I am not sure how reliable that page count check is here: if we'd
>>>>> have another speculative reference to the page, we might see
>>>>> "page_count(page) > 1" and not take that path, although the previous
>>>>> owner released the last reference.
>>>>
>>>> IIUC, there should not be such speculative reference. The driver should have taken care
>>>> of it.
>>>
>>> How can you prevent any kind of speculative references?
>>>
>>> See isolate_movable_page() as an example, which grabs a speculative
>>> reference to then find out that the page is already isolated by someone
>>> else, to then back off.
>>
>> You're right. isolate_movable_page will be an speculative references case. But the page count check here
>> is just an optimization. If we encounter speculative references, it still works with useless effort of
>> migrating to be released page.
>
>
> Not really. The issue is that PAGE_FLAGS_CHECK_AT_FREE contains
> PG_active and PG_unevictable.
>
> We only clear those 2 flags if "page_count(page) == 1". Consequently,
> with a speculative reference, we'll run into the check_free_page_bad()
> when dropping the last reference.
It seems if a speculative reference happens after the "page_count(page) == 1" check,
it's ok because we cleared the PG_active and PG_unevictable. And if it happens before
the check, this code block is skipped and the page will be freed after migration. The
PG_active and PG_unevictable will be correctly cleared when page is actually freed via
__folio_clear_active. (Please see below comment)
>
> This is just shaky. Special casing on "page_count(page) == 1" for
> detecting "was this freed by the owner" is not 100% water proof.
>
> In an ideal world, we'd just get rid of that whole block of code and let
> the actual freeing code clear PG_active and PG_unevictable. But that
> would require changes to free_pages_prepare().
>
>
> Now I do wonder, if we ever even have PG_active or PG_unevictable still
> set when the page was freed by the owner in this code. IOW, maybe that
> is dead code as well and we can just remove the whole shaky
> "page_count(page) == 1" code block.
Think about below common scene: Anonymous page is actively used by the sole owner process, so it
will have PG_active set. Then process exited while vm tries to migrate that page. So the page
should have refcnt == 1 while PG_active is set? Note normally PG_active should be cleared when
the page is released:
__put_single_page
PageLRU
__clear_page_lru_flags
__folio_clear_active
__folio_clear_unevictable
But for isolated page, PageLRU is cleared. So when the isolated page is released, __clear_page_lru_flags
won't be called. So we have to clear the PG_active and PG_unevictable here manully. So I think
this code block works. Or am I miss something again?
Thanks!
>
> Ccing Minchan, who added clearing of the pageflags at that point.
>
>> If PG_isolated is still set, it will get cleared in the buddy when
>> freeing the page via
>>
>> page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
>
> Yes, check_free_page only complains about flags belonging to PAGE_FLAGS_CHECK_AT_FREE and PG_isolated
> will be cleared in the buddy when freeing the page. But it might not be a good idea to reply on this ?
> IMHO, it should be better to clear the PG_isolated explicitly ourselves.
I think we can pretty much rely on this handling in the buddy :)
>
>>
>>>
>>>>
>>>>
>>>> Also, I am not sure how reliable that page count check is here: if we'd
>>>> have another speculative reference to the page, we might see
>>>> "page_count(page) > 1" and not take that path, although the previous
>>>> owner released the last reference.
>>>
>>> IIUC, there should not be such speculative reference. The driver should have taken care
>>> of it.
>>
>> How can you prevent any kind of speculative references?
>>
>> See isolate_movable_page() as an example, which grabs a speculative
>> reference to then find out that the page is already isolated by someone
>> else, to then back off.
>
> You're right. isolate_movable_page will be an speculative references case. But the page count check here
> is just an optimization. If we encounter speculative references, it still works with useless effort of
> migrating to be released page.
Not really. The issue is that PAGE_FLAGS_CHECK_AT_FREE contains
PG_active and PG_unevictable.
We only clear those 2 flags if "page_count(page) == 1". Consequently,
with a speculative reference, we'll run into the check_free_page_bad()
when dropping the last reference.
This is just shaky. Special casing on "page_count(page) == 1" for
detecting "was this freed by the owner" is not 100% water proof.
In an ideal world, we'd just get rid of that whole block of code and let
the actual freeing code clear PG_active and PG_unevictable. But that
would require changes to free_pages_prepare().
Now I do wonder, if we ever even have PG_active or PG_unevictable still
set when the page was freed by the owner in this code. IOW, maybe that
is dead code as well and we can just remove the whole shaky
"page_count(page) == 1" code block.
Ccing Minchan, who added clearing of the pageflags at that point.
--
Thanks,
David / dhildenb
On 12.05.22 15:26, Miaohe Lin wrote:
> On 2022/5/12 15:10, David Hildenbrand wrote:
>>>> If PG_isolated is still set, it will get cleared in the buddy when
>>>> freeing the page via
>>>>
>>>> page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>>
>>> Yes, check_free_page only complains about flags belonging to PAGE_FLAGS_CHECK_AT_FREE and PG_isolated
>>> will be cleared in the buddy when freeing the page. But it might not be a good idea to reply on this ?
>>> IMHO, it should be better to clear the PG_isolated explicitly ourselves.
>>
>> I think we can pretty much rely on this handling in the buddy :)
>
> So is the below code change what you're suggesting?
>
> if (page_count(page) == 1) {
> /* page was freed from under us. So we are done. */
> ClearPageActive(page);
> ClearPageUnevictable(page);
> - if (unlikely(__PageMovable(page)))
> - ClearPageIsolated(page);
> goto out;
> }
Yeah, unless I am missing something important :)
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> Also, I am not sure how reliable that page count check is here: if we'd
>>>>>> have another speculative reference to the page, we might see
>>>>>> "page_count(page) > 1" and not take that path, although the previous
>>>>>> owner released the last reference.
>>>>>
>>>>> IIUC, there should not be such speculative reference. The driver should have taken care
>>>>> of it.
>>>>
>>>> How can you prevent any kind of speculative references?
>>>>
>>>> See isolate_movable_page() as an example, which grabs a speculative
>>>> reference to then find out that the page is already isolated by someone
>>>> else, to then back off.
>>>
>>> You're right. isolate_movable_page will be an speculative references case. But the page count check here
>>> is just an optimization. If we encounter speculative references, it still works with useless effort of
>>> migrating to be released page.
>>
>>
>> Not really. The issue is that PAGE_FLAGS_CHECK_AT_FREE contains
>> PG_active and PG_unevictable.
>>
>> We only clear those 2 flags if "page_count(page) == 1". Consequently,
>> with a speculative reference, we'll run into the check_free_page_bad()
>> when dropping the last reference.
>
> It seems if a speculative reference happens after the "page_count(page) == 1" check,
> it's ok because we cleared the PG_active and PG_unevictable. And if it happens before
> the check, this code block is skipped and the page will be freed after migration. The
> PG_active and PG_unevictable will be correctly cleared when page is actually freed via
> __folio_clear_active. (Please see below comment)
>
>>
>> This is just shaky. Special casing on "page_count(page) == 1" for
>> detecting "was this freed by the owner" is not 100% water proof.
>>
>> In an ideal world, we'd just get rid of that whole block of code and let
>> the actual freeing code clear PG_active and PG_unevictable. But that
>> would require changes to free_pages_prepare().
>>
>>
>> Now I do wonder, if we ever even have PG_active or PG_unevictable still
>> set when the page was freed by the owner in this code. IOW, maybe that
>> is dead code as well and we can just remove the whole shaky
>> "page_count(page) == 1" code block.
>
> Think about below common scene: Anonymous page is actively used by the sole owner process, so it
> will have PG_active set. Then process exited while vm tries to migrate that page. So the page
> should have refcnt == 1 while PG_active is set? Note normally PG_active should be cleared when
> the page is released:
>
> __put_single_page
> PageLRU
> __clear_page_lru_flags
> __folio_clear_active
> __folio_clear_unevictable
>
> But for isolated page, PageLRU is cleared. So when the isolated page is released, __clear_page_lru_flags
> won't be called. So we have to clear the PG_active and PG_unevictable here manully. So I think
> this code block works. Or am I miss something again?
Let's assume the following: page as freed by the owner and we enter
unmap_and_move().
#1: enter unmap_and_move() // page_count is 1
#2: enter isolate_movable_page() // page_count is 1
#2: get_page_unless_zero() // page_count is now 2
#1: if (page_count(page) == 1) { // does not trigger
#2: put_page(page); // page_count is now 1
#1: put_page(page); // page_count is now 0 -> freed
#1 will trigger __put_page() -> __put_single_page() ->
__page_cache_release() will not clear the flags because it's not an LRU
page at that point in time, right (-> isolated)?
We did not run that code block that would clear PG_active and
PG_unevictable.
Which still leaves the questions:
a) If PG_active and PG_unevictable was cleared, where?
b) Why is that code block that conditionally clears the flags of any
value and why can't we simply drop it?
--
Thanks,
David / dhildenb
On 09.05.22 10:51, Miaohe Lin wrote:
> On 2022/4/29 18:07, David Hildenbrand wrote:
>> On 25.04.22 15:27, Miaohe Lin wrote:
>>> When non-lru movable page was freed from under us, __ClearPageMovable must
>>> have been done. Even if it's not done, ClearPageIsolated here won't hurt
>>> as page will be freed anyway. So we can thus remove unneeded lock page and
>>> PageMovable check here.
>>>
>>> Signed-off-by: Miaohe Lin <[email protected]>
>>> Reviewed-by: Christoph Hellwig <[email protected]>
>>> ---
>>> mm/migrate.c | 8 ++------
>>> 1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index b779646665fe..0fc4651b3e39 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -1093,12 +1093,8 @@ static int unmap_and_move(new_page_t get_new_page,
>>> /* page was freed from under us. So we are done. */
>>> ClearPageActive(page);
>>> ClearPageUnevictable(page);
>>> - if (unlikely(__PageMovable(page))) {
>>> - lock_page(page);
>>> - if (!PageMovable(page))
>>> - ClearPageIsolated(page);
>>> - unlock_page(page);
>>> - }
>>> + if (unlikely(__PageMovable(page)))
>>> + ClearPageIsolated(page);
>>> goto out;
>>> }
>>
>> Hm, that code+change raises a couple of questions.
>>
>> We're doing here the same as in putback_movable_pages(). So I guess the
>> difference here is that the caller did release the reference while the
>> page was isolated, while we don't assume the same in
>> putback_movable_pages().
>
> Agree.
>
>>
>>
>> Shouldn't whoever owned the page have cleared that? IOW, is it even
>> valid that we see a movable or isolated page here (WARN/BUG?)?
>>
>> At least for balloon compaction, I remember that __PageMovable() is
>> properly cleared before freeing it via balloon_page_delete().
>
> z3fold, zsmalloc will do __ClearPageMovable when the page is going to be released.
> So I think we shouldn't see a movable page here:
>
> void __ClearPageMovable(struct page *page)
> {
> VM_BUG_ON_PAGE(!PageMovable(page), page);
> /*
> * Clear registered address_space val with keeping PAGE_MAPPING_MOVABLE
> * flag so that VM can catch up released page by driver after isolation.
> * With it, VM migration doesn't try to put it back.
> */
> page->mapping = (void *)((unsigned long)page->mapping &
> PAGE_MAPPING_MOVABLE);
> }
>
> But it seems there is no guarantee for PageIsolated flag. Or am I miss something?
At least the code we have now:
if (unlikely(__PageMovable(page)))
ClearPageIsolated(page);
Should be dead code. So PG_isolated could remain set.
If PG_isolated is still set, it will get cleared in the buddy when
freeing the page via
page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
>
>>
>>
>> Also, I am not sure how reliable that page count check is here: if we'd
>> have another speculative reference to the page, we might see
>> "page_count(page) > 1" and not take that path, although the previous
>> owner released the last reference.
>
> IIUC, there should not be such speculative reference. The driver should have taken care
> of it.
How can you prevent any kind of speculative references?
See isolate_movable_page() as an example, which grabs a speculative
reference to then find out that the page is already isolated by someone
else, to then back off.
--
Thanks,
David / dhildenb
On 2022/5/13 0:50, David Hildenbrand wrote:
> On 12.05.22 15:26, Miaohe Lin wrote:
>> On 2022/5/12 15:10, David Hildenbrand wrote:
>>>>> If PG_isolated is still set, it will get cleared in the buddy when
>>>>> freeing the page via
>>>>>
>>>>> page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>>>
>>>> Yes, check_free_page only complains about flags belonging to PAGE_FLAGS_CHECK_AT_FREE and PG_isolated
>>>> will be cleared in the buddy when freeing the page. But it might not be a good idea to reply on this ?
>>>> IMHO, it should be better to clear the PG_isolated explicitly ourselves.
>>>
>>> I think we can pretty much rely on this handling in the buddy :)
>>
>> So is the below code change what you're suggesting?
>>
>> if (page_count(page) == 1) {
>> /* page was freed from under us. So we are done. */
>> ClearPageActive(page);
>> ClearPageUnevictable(page);
>> - if (unlikely(__PageMovable(page)))
>> - ClearPageIsolated(page);
>> goto out;
>> }
>
> Yeah, unless I am missing something important :)
>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Also, I am not sure how reliable that page count check is here: if we'd
>>>>>>> have another speculative reference to the page, we might see
>>>>>>> "page_count(page) > 1" and not take that path, although the previous
>>>>>>> owner released the last reference.
>>>>>>
>>>>>> IIUC, there should not be such speculative reference. The driver should have taken care
>>>>>> of it.
>>>>>
>>>>> How can you prevent any kind of speculative references?
>>>>>
>>>>> See isolate_movable_page() as an example, which grabs a speculative
>>>>> reference to then find out that the page is already isolated by someone
>>>>> else, to then back off.
>>>>
>>>> You're right. isolate_movable_page will be an speculative references case. But the page count check here
>>>> is just an optimization. If we encounter speculative references, it still works with useless effort of
>>>> migrating to be released page.
>>>
>>>
>>> Not really. The issue is that PAGE_FLAGS_CHECK_AT_FREE contains
>>> PG_active and PG_unevictable.
>>>
>>> We only clear those 2 flags if "page_count(page) == 1". Consequently,
>>> with a speculative reference, we'll run into the check_free_page_bad()
>>> when dropping the last reference.
>>
>> It seems if a speculative reference happens after the "page_count(page) == 1" check,
>> it's ok because we cleared the PG_active and PG_unevictable. And if it happens before
>> the check, this code block is skipped and the page will be freed after migration. The
>> PG_active and PG_unevictable will be correctly cleared when page is actually freed via
>> __folio_clear_active. (Please see below comment)
>>
>>>
>>> This is just shaky. Special casing on "page_count(page) == 1" for
>>> detecting "was this freed by the owner" is not 100% water proof.
>>>
>>> In an ideal world, we'd just get rid of that whole block of code and let
>>> the actual freeing code clear PG_active and PG_unevictable. But that
>>> would require changes to free_pages_prepare().
>>>
>>>
>>> Now I do wonder, if we ever even have PG_active or PG_unevictable still
>>> set when the page was freed by the owner in this code. IOW, maybe that
>>> is dead code as well and we can just remove the whole shaky
>>> "page_count(page) == 1" code block.
>>
>> Think about below common scene: Anonymous page is actively used by the sole owner process, so it
>> will have PG_active set. Then process exited while vm tries to migrate that page. So the page
>> should have refcnt == 1 while PG_active is set? Note normally PG_active should be cleared when
>> the page is released:
>>
>> __put_single_page
>> PageLRU
>> __clear_page_lru_flags
>> __folio_clear_active
>> __folio_clear_unevictable
>>
>> But for isolated page, PageLRU is cleared. So when the isolated page is released, __clear_page_lru_flags
>> won't be called. So we have to clear the PG_active and PG_unevictable here manully. So I think
>> this code block works. Or am I miss something again?
>
> Let's assume the following: page as freed by the owner and we enter
> unmap_and_move().
>
>
> #1: enter unmap_and_move() // page_count is 1
> #2: enter isolate_movable_page() // page_count is 1
> #2: get_page_unless_zero() // page_count is now 2
> #1: if (page_count(page) == 1) { // does not trigger
> #2: put_page(page); // page_count is now 1
> #1: put_page(page); // page_count is now 0 -> freed
>
>
> #1 will trigger __put_page() -> __put_single_page() ->
> __page_cache_release() will not clear the flags because it's not an LRU
> page at that point in time, right (-> isolated)?
Sorry, you're right. I thought the old page will be freed via putback_lru_page which will
set PageLRU back instead of put_page directly. So if the above race occurs, PG_active and
PG_unevictable will remain set while page goes to the buddy and check_free_page will complain
about it. But it seems this is never witnessed?
>
> We did not run that code block that would clear PG_active and
> PG_unevictable.
>
> Which still leaves the questions:
>
> a) If PG_active and PG_unevictable was cleared, where?
For LRU pages, PG_active and PG_unevictable are cleared via __page_cache_release. And for isolated
(LRU) pages, PG_active and PG_unevictable should be cleared ourselves?
> b) Why is that code block that conditionally clears the flags of any
> value and why can't we simply drop it?
>
To fix the issue, should we clear PG_active and PG_unevictable unconditionally here?
Thanks a lot!
On 2022/5/13 0:50, David Hildenbrand wrote:
> On 12.05.22 15:26, Miaohe Lin wrote:
>> On 2022/5/12 15:10, David Hildenbrand wrote:
>>>>> If PG_isolated is still set, it will get cleared in the buddy when
>>>>> freeing the page via
>>>>>
>>>>> page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>>>
>>>> Yes, check_free_page only complains about flags belonging to PAGE_FLAGS_CHECK_AT_FREE and PG_isolated
>>>> will be cleared in the buddy when freeing the page. But it might not be a good idea to reply on this ?
>>>> IMHO, it should be better to clear the PG_isolated explicitly ourselves.
>>>
>>> I think we can pretty much rely on this handling in the buddy :)
>>
>> So is the below code change what you're suggesting?
>>
>> if (page_count(page) == 1) {
>> /* page was freed from under us. So we are done. */
>> ClearPageActive(page);
>> ClearPageUnevictable(page);
>> - if (unlikely(__PageMovable(page)))
>> - ClearPageIsolated(page);
>> goto out;
>> }
>
> Yeah, unless I am missing something important :)
>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Also, I am not sure how reliable that page count check is here: if we'd
>>>>>>> have another speculative reference to the page, we might see
>>>>>>> "page_count(page) > 1" and not take that path, although the previous
>>>>>>> owner released the last reference.
>>>>>>
>>>>>> IIUC, there should not be such speculative reference. The driver should have taken care
>>>>>> of it.
>>>>>
>>>>> How can you prevent any kind of speculative references?
>>>>>
>>>>> See isolate_movable_page() as an example, which grabs a speculative
>>>>> reference to then find out that the page is already isolated by someone
>>>>> else, to then back off.
>>>>
>>>> You're right. isolate_movable_page will be an speculative references case. But the page count check here
>>>> is just an optimization. If we encounter speculative references, it still works with useless effort of
>>>> migrating to be released page.
>>>
>>>
>>> Not really. The issue is that PAGE_FLAGS_CHECK_AT_FREE contains
>>> PG_active and PG_unevictable.
>>>
>>> We only clear those 2 flags if "page_count(page) == 1". Consequently,
>>> with a speculative reference, we'll run into the check_free_page_bad()
>>> when dropping the last reference.
>>
>> It seems if a speculative reference happens after the "page_count(page) == 1" check,
>> it's ok because we cleared the PG_active and PG_unevictable. And if it happens before
>> the check, this code block is skipped and the page will be freed after migration. The
>> PG_active and PG_unevictable will be correctly cleared when page is actually freed via
>> __folio_clear_active. (Please see below comment)
>>
>>>
>>> This is just shaky. Special casing on "page_count(page) == 1" for
>>> detecting "was this freed by the owner" is not 100% water proof.
>>>
>>> In an ideal world, we'd just get rid of that whole block of code and let
>>> the actual freeing code clear PG_active and PG_unevictable. But that
>>> would require changes to free_pages_prepare().
>>>
>>>
>>> Now I do wonder, if we ever even have PG_active or PG_unevictable still
>>> set when the page was freed by the owner in this code. IOW, maybe that
>>> is dead code as well and we can just remove the whole shaky
>>> "page_count(page) == 1" code block.
>>
>> Think about below common scene: Anonymous page is actively used by the sole owner process, so it
>> will have PG_active set. Then process exited while vm tries to migrate that page. So the page
>> should have refcnt == 1 while PG_active is set? Note normally PG_active should be cleared when
>> the page is released:
>>
>> __put_single_page
>> PageLRU
>> __clear_page_lru_flags
>> __folio_clear_active
>> __folio_clear_unevictable
>>
>> But for isolated page, PageLRU is cleared. So when the isolated page is released, __clear_page_lru_flags
>> won't be called. So we have to clear the PG_active and PG_unevictable here manully. So I think
>> this code block works. Or am I miss something again?
>
> Let's assume the following: page as freed by the owner and we enter
> unmap_and_move().
>
>
> #1: enter unmap_and_move() // page_count is 1
> #2: enter isolate_movable_page() // page_count is 1
> #2: get_page_unless_zero() // page_count is now 2
> #1: if (page_count(page) == 1) { // does not trigger
> #2: put_page(page); // page_count is now 1
> #1: put_page(page); // page_count is now 0 -> freed
>
>
> #1 will trigger __put_page() -> __put_single_page() ->
> __page_cache_release() will not clear the flags because it's not an LRU
> page at that point in time, right (-> isolated)?
>
> We did not run that code block that would clear PG_active and
> PG_unevictable.
>
> Which still leaves the questions:
>
> a) If PG_active and PG_unevictable was cleared, where?
> b) Why is that code block that conditionally clears the flags of any
> value and why can't we simply drop it?
>
I took a more close look of the code today. And I think the current code works:
There are 3 cases in unmap_and_move:
1.page is freed through "if (page_count(page) == 1)" code block. This works
as PG_active and PG_unevictable are cleared here.
2. Failed to migrate the page. The page won't be release so we don't care about it.
3. The page is migrated successfully. The PG_active and PG_unevictable are cleared
via folio_migrate_flags():
if (folio_test_clear_active(folio)) {
VM_BUG_ON_FOLIO(folio_test_unevictable(folio), folio);
folio_set_active(newfolio);
} else if (folio_test_clear_unevictable(folio))
folio_set_unevictable(newfolio);
For the above race case, the page won't be freed through "if (page_count(page) == 1)" code block.
It will just be migrated and freed via put_page() after folio_migrate_flags() having cleared PG_active
and PG_unevictable.
Am I miss something again ? ;)
Thanks a lot!
On 2022/5/31 19:59, David Hildenbrand wrote:
> Sorry for the late reply, was on vacation.
That's all right. Hope you have a great time. ;)
>
>>>>
>>>> But for isolated page, PageLRU is cleared. So when the isolated page is released, __clear_page_lru_flags
>>>> won't be called. So we have to clear the PG_active and PG_unevictable here manully. So I think
>>>> this code block works. Or am I miss something again?
>>>
>>> Let's assume the following: page as freed by the owner and we enter
>>> unmap_and_move().
>>>
>>>
>>> #1: enter unmap_and_move() // page_count is 1
>>> #2: enter isolate_movable_page() // page_count is 1
>>> #2: get_page_unless_zero() // page_count is now 2
>>> #1: if (page_count(page) == 1) { // does not trigger
>>> #2: put_page(page); // page_count is now 1
>>> #1: put_page(page); // page_count is now 0 -> freed
>>>
>>>
>>> #1 will trigger __put_page() -> __put_single_page() ->
>>> __page_cache_release() will not clear the flags because it's not an LRU
>>> page at that point in time, right (-> isolated)?
>>
>> Sorry, you're right. I thought the old page will be freed via putback_lru_page which will
>> set PageLRU back instead of put_page directly. So if the above race occurs, PG_active and
>> PG_unevictable will remain set while page goes to the buddy and check_free_page will complain
>> about it. But it seems this is never witnessed?
>
> Maybe
>
> a) we were lucky so far and didn't trigger it
> b) the whole code block is dead code because we are missing something
> c) we are missing something else :)
I think I found the things we missed in another email [1].
[1]: https://lore.kernel.org/all/[email protected]/
Paste the main content of [1] here:
"
There are 3 cases in unmap_and_move:
1.page is freed through "if (page_count(page) == 1)" code block. This works
as PG_active and PG_unevictable are cleared here.
2. Failed to migrate the page. The page won't be release so we don't care about it.
3. The page is migrated successfully. The PG_active and PG_unevictable are cleared
via folio_migrate_flags():
if (folio_test_clear_active(folio)) {
VM_BUG_ON_FOLIO(folio_test_unevictable(folio), folio);
folio_set_active(newfolio);
} else if (folio_test_clear_unevictable(folio))
folio_set_unevictable(newfolio);
For the above race case, the page won't be freed through "if (page_count(page) == 1)" code block.
It will just be migrated and freed via put_page() after folio_migrate_flags() having cleared PG_active
and PG_unevictable.
"
Or Am I miss something again? :)
>
>>
>>>
>>> We did not run that code block that would clear PG_active and
>>> PG_unevictable.
>>>
>>> Which still leaves the questions:
>>>
>>> a) If PG_active and PG_unevictable was cleared, where?
>>
>> For LRU pages, PG_active and PG_unevictable are cleared via __page_cache_release. And for isolated
>> (LRU) pages, PG_active and PG_unevictable should be cleared ourselves?
>>
>>> b) Why is that code block that conditionally clears the flags of any
>>> value and why can't we simply drop it?
>>>
>>
>> To fix the issue, should we clear PG_active and PG_unevictable unconditionally here?
>
> I wonder if we should simply teach actual freeing code to simply clear
> both flags when freeing an isolated page? IOW, to detect "isolated LRU"
> is getting freed and fixup?
IMHO, clearing both flags are done by the caller indeed. Another example I found when I
read the khugepaged code recently is pasted below:
collapse_file
...
page_ref_unfreeze(page, 1);
ClearPageActive(page);
ClearPageUnevictable(page);
unlock_page(page);
put_page(page);
index++;
...
Thanks!
>
On 31.05.22 14:37, Miaohe Lin wrote:
> On 2022/5/31 19:59, David Hildenbrand wrote:
>> Sorry for the late reply, was on vacation.
>
> That's all right. Hope you have a great time. ;)
>
>>
>>>>>
>>>>> But for isolated page, PageLRU is cleared. So when the isolated page is released, __clear_page_lru_flags
>>>>> won't be called. So we have to clear the PG_active and PG_unevictable here manully. So I think
>>>>> this code block works. Or am I miss something again?
>>>>
>>>> Let's assume the following: page as freed by the owner and we enter
>>>> unmap_and_move().
>>>>
>>>>
>>>> #1: enter unmap_and_move() // page_count is 1
>>>> #2: enter isolate_movable_page() // page_count is 1
>>>> #2: get_page_unless_zero() // page_count is now 2
>>>> #1: if (page_count(page) == 1) { // does not trigger
>>>> #2: put_page(page); // page_count is now 1
>>>> #1: put_page(page); // page_count is now 0 -> freed
>>>>
>>>>
>>>> #1 will trigger __put_page() -> __put_single_page() ->
>>>> __page_cache_release() will not clear the flags because it's not an LRU
>>>> page at that point in time, right (-> isolated)?
>>>
>>> Sorry, you're right. I thought the old page will be freed via putback_lru_page which will
>>> set PageLRU back instead of put_page directly. So if the above race occurs, PG_active and
>>> PG_unevictable will remain set while page goes to the buddy and check_free_page will complain
>>> about it. But it seems this is never witnessed?
>>
>> Maybe
>>
>> a) we were lucky so far and didn't trigger it
>> b) the whole code block is dead code because we are missing something
>> c) we are missing something else :)
>
> I think I found the things we missed in another email [1].
> [1]: https://lore.kernel.org/all/[email protected]/
>
> Paste the main content of [1] here:
>
> "
> There are 3 cases in unmap_and_move:
>
> 1.page is freed through "if (page_count(page) == 1)" code block. This works
> as PG_active and PG_unevictable are cleared here.
>
> 2. Failed to migrate the page. The page won't be release so we don't care about it.
Right, page is un-isolated.
>
> 3. The page is migrated successfully. The PG_active and PG_unevictable are cleared
> via folio_migrate_flags():
>
> if (folio_test_clear_active(folio)) {
> VM_BUG_ON_FOLIO(folio_test_unevictable(folio), folio);
> folio_set_active(newfolio);
> } else if (folio_test_clear_unevictable(folio))
> folio_set_unevictable(newfolio);
Right.
>
> For the above race case, the page won't be freed through "if (page_count(page) == 1)" code block.
> It will just be migrated and freed via put_page() after folio_migrate_flags() having cleared PG_active
> and PG_unevictable.
> "
> Or Am I miss something again? :)
For #1, I'm still not sure what would happen on a speculative reference.
It's worth summarizing that
a) free_pages_prepare() will clear both flags via page->flags &=
~PAGE_FLAGS_CHECK_AT_PREP;
b) free_pages_prepare() will bail out if any flag is set in
check_free_page().
As we've never seen b) in the wild, this certainly has low priority, and
maybe it really cannot happen right now.
However, maybe really allowing these flags to be set when freeing the
page and removing the "page_count(page) == 1" case from migration code
would be the clean thing to do.
--
Thanks,
David / dhildenb
Sorry for the late reply, was on vacation.
>>>
>>> But for isolated page, PageLRU is cleared. So when the isolated page is released, __clear_page_lru_flags
>>> won't be called. So we have to clear the PG_active and PG_unevictable here manully. So I think
>>> this code block works. Or am I miss something again?
>>
>> Let's assume the following: page as freed by the owner and we enter
>> unmap_and_move().
>>
>>
>> #1: enter unmap_and_move() // page_count is 1
>> #2: enter isolate_movable_page() // page_count is 1
>> #2: get_page_unless_zero() // page_count is now 2
>> #1: if (page_count(page) == 1) { // does not trigger
>> #2: put_page(page); // page_count is now 1
>> #1: put_page(page); // page_count is now 0 -> freed
>>
>>
>> #1 will trigger __put_page() -> __put_single_page() ->
>> __page_cache_release() will not clear the flags because it's not an LRU
>> page at that point in time, right (-> isolated)?
>
> Sorry, you're right. I thought the old page will be freed via putback_lru_page which will
> set PageLRU back instead of put_page directly. So if the above race occurs, PG_active and
> PG_unevictable will remain set while page goes to the buddy and check_free_page will complain
> about it. But it seems this is never witnessed?
Maybe
a) we were lucky so far and didn't trigger it
b) the whole code block is dead code because we are missing something
c) we are missing something else :)
>
>>
>> We did not run that code block that would clear PG_active and
>> PG_unevictable.
>>
>> Which still leaves the questions:
>>
>> a) If PG_active and PG_unevictable was cleared, where?
>
> For LRU pages, PG_active and PG_unevictable are cleared via __page_cache_release. And for isolated
> (LRU) pages, PG_active and PG_unevictable should be cleared ourselves?
>
>> b) Why is that code block that conditionally clears the flags of any
>> value and why can't we simply drop it?
>>
>
> To fix the issue, should we clear PG_active and PG_unevictable unconditionally here?
I wonder if we should simply teach actual freeing code to simply clear
both flags when freeing an isolated page? IOW, to detect "isolated LRU"
is getting freed and fixup?
--
Thanks,
David / dhildenb
On 02.06.22 09:40, Miaohe Lin wrote:
> On 2022/6/1 18:31, David Hildenbrand wrote:
>> On 31.05.22 14:37, Miaohe Lin wrote:
>>> On 2022/5/31 19:59, David Hildenbrand wrote:
>>>> Sorry for the late reply, was on vacation.
>>>
>>> That's all right. Hope you have a great time. ;)
>>>
>>>>
>>>>>>>
>>>>>>> But for isolated page, PageLRU is cleared. So when the isolated page is released, __clear_page_lru_flags
>>>>>>> won't be called. So we have to clear the PG_active and PG_unevictable here manully. So I think
>>>>>>> this code block works. Or am I miss something again?
>>>>>>
>>>>>> Let's assume the following: page as freed by the owner and we enter
>>>>>> unmap_and_move().
>>>>>>
>>>>>>
>>>>>> #1: enter unmap_and_move() // page_count is 1
>>>>>> #2: enter isolate_movable_page() // page_count is 1
>>>>>> #2: get_page_unless_zero() // page_count is now 2
>>>>>> #1: if (page_count(page) == 1) { // does not trigger
>>>>>> #2: put_page(page); // page_count is now 1
>>>>>> #1: put_page(page); // page_count is now 0 -> freed
>>>>>>
>>>>>>
>>>>>> #1 will trigger __put_page() -> __put_single_page() ->
>>>>>> __page_cache_release() will not clear the flags because it's not an LRU
>>>>>> page at that point in time, right (-> isolated)?
>>>>>
>>>>> Sorry, you're right. I thought the old page will be freed via putback_lru_page which will
>>>>> set PageLRU back instead of put_page directly. So if the above race occurs, PG_active and
>>>>> PG_unevictable will remain set while page goes to the buddy and check_free_page will complain
>>>>> about it. But it seems this is never witnessed?
>>>>
>>>> Maybe
>>>>
>>>> a) we were lucky so far and didn't trigger it
>>>> b) the whole code block is dead code because we are missing something
>>>> c) we are missing something else :)
>>>
>>> I think I found the things we missed in another email [1].
>>> [1]: https://lore.kernel.org/all/[email protected]/
>>>
>>> Paste the main content of [1] here:
>>>
>>> "
>>> There are 3 cases in unmap_and_move:
>>>
>>> 1.page is freed through "if (page_count(page) == 1)" code block. This works
>>> as PG_active and PG_unevictable are cleared here.
>>>
>>> 2. Failed to migrate the page. The page won't be release so we don't care about it.
>>
>> Right, page is un-isolated.
>>
>>>
>>> 3. The page is migrated successfully. The PG_active and PG_unevictable are cleared
>>> via folio_migrate_flags():
>>>
>>> if (folio_test_clear_active(folio)) {
>>> VM_BUG_ON_FOLIO(folio_test_unevictable(folio), folio);
>>> folio_set_active(newfolio);
>>> } else if (folio_test_clear_unevictable(folio))
>>> folio_set_unevictable(newfolio);
>>
>> Right.
>>
>>>
>>> For the above race case, the page won't be freed through "if (page_count(page) == 1)" code block.
>>> It will just be migrated and freed via put_page() after folio_migrate_flags() having cleared PG_active
>>> and PG_unevictable.
>>> "
>>> Or Am I miss something again? :)
>>
>> For #1, I'm still not sure what would happen on a speculative reference.
>>
>> It's worth summarizing that
>>
>> a) free_pages_prepare() will clear both flags via page->flags &=
>> ~PAGE_FLAGS_CHECK_AT_PREP;
>>
>> b) free_pages_prepare() will bail out if any flag is set in
>> check_free_page().
>>
>> As we've never seen b) in the wild, this certainly has low priority, and
>> maybe it really cannot happen right now.
>>
>> However, maybe really allowing these flags to be set when freeing the
>> page and removing the "page_count(page) == 1" case from migration code
>> would be the clean thing to do.
>
> IMHO, check_free_page is used to catch possible problem. There's the comment of PAGE_FLAGS_CHECK_AT_FREE:
>
> /*
> * Flags checked when a page is freed. Pages being freed should not have
> * these flags set. If they are, there is a problem.
> */
> #define PAGE_FLAGS_CHECK_AT_FREE
>
> There might be an assumption: when page is freed, it shouldn't be an active or unevictable page. It should be
> inactive and evictable. So allowing these flags to be set when freeing the page might not be a good idea?
Yeah, and we'd be lifting that restriction because there is good reason
to do so.
Maybe we *could* special case for isolated pages; however, that adds
runtime overhead. Of course, we could perform different checks for e.g.,
DEBUG_VM vs !DEBUG_VM.
--
Thanks,
David / dhildenb
On 2022/6/1 18:31, David Hildenbrand wrote:
> On 31.05.22 14:37, Miaohe Lin wrote:
>> On 2022/5/31 19:59, David Hildenbrand wrote:
>>> Sorry for the late reply, was on vacation.
>>
>> That's all right. Hope you have a great time. ;)
>>
>>>
>>>>>>
>>>>>> But for isolated page, PageLRU is cleared. So when the isolated page is released, __clear_page_lru_flags
>>>>>> won't be called. So we have to clear the PG_active and PG_unevictable here manully. So I think
>>>>>> this code block works. Or am I miss something again?
>>>>>
>>>>> Let's assume the following: page as freed by the owner and we enter
>>>>> unmap_and_move().
>>>>>
>>>>>
>>>>> #1: enter unmap_and_move() // page_count is 1
>>>>> #2: enter isolate_movable_page() // page_count is 1
>>>>> #2: get_page_unless_zero() // page_count is now 2
>>>>> #1: if (page_count(page) == 1) { // does not trigger
>>>>> #2: put_page(page); // page_count is now 1
>>>>> #1: put_page(page); // page_count is now 0 -> freed
>>>>>
>>>>>
>>>>> #1 will trigger __put_page() -> __put_single_page() ->
>>>>> __page_cache_release() will not clear the flags because it's not an LRU
>>>>> page at that point in time, right (-> isolated)?
>>>>
>>>> Sorry, you're right. I thought the old page will be freed via putback_lru_page which will
>>>> set PageLRU back instead of put_page directly. So if the above race occurs, PG_active and
>>>> PG_unevictable will remain set while page goes to the buddy and check_free_page will complain
>>>> about it. But it seems this is never witnessed?
>>>
>>> Maybe
>>>
>>> a) we were lucky so far and didn't trigger it
>>> b) the whole code block is dead code because we are missing something
>>> c) we are missing something else :)
>>
>> I think I found the things we missed in another email [1].
>> [1]: https://lore.kernel.org/all/[email protected]/
>>
>> Paste the main content of [1] here:
>>
>> "
>> There are 3 cases in unmap_and_move:
>>
>> 1.page is freed through "if (page_count(page) == 1)" code block. This works
>> as PG_active and PG_unevictable are cleared here.
>>
>> 2. Failed to migrate the page. The page won't be release so we don't care about it.
>
> Right, page is un-isolated.
>
>>
>> 3. The page is migrated successfully. The PG_active and PG_unevictable are cleared
>> via folio_migrate_flags():
>>
>> if (folio_test_clear_active(folio)) {
>> VM_BUG_ON_FOLIO(folio_test_unevictable(folio), folio);
>> folio_set_active(newfolio);
>> } else if (folio_test_clear_unevictable(folio))
>> folio_set_unevictable(newfolio);
>
> Right.
>
>>
>> For the above race case, the page won't be freed through "if (page_count(page) == 1)" code block.
>> It will just be migrated and freed via put_page() after folio_migrate_flags() having cleared PG_active
>> and PG_unevictable.
>> "
>> Or Am I miss something again? :)
>
> For #1, I'm still not sure what would happen on a speculative reference.
>
> It's worth summarizing that
>
> a) free_pages_prepare() will clear both flags via page->flags &=
> ~PAGE_FLAGS_CHECK_AT_PREP;
>
> b) free_pages_prepare() will bail out if any flag is set in
> check_free_page().
>
> As we've never seen b) in the wild, this certainly has low priority, and
> maybe it really cannot happen right now.
>
> However, maybe really allowing these flags to be set when freeing the
> page and removing the "page_count(page) == 1" case from migration code
> would be the clean thing to do.
IMHO, check_free_page is used to catch possible problem. There's the comment of PAGE_FLAGS_CHECK_AT_FREE:
/*
* Flags checked when a page is freed. Pages being freed should not have
* these flags set. If they are, there is a problem.
*/
#define PAGE_FLAGS_CHECK_AT_FREE
There might be an assumption: when page is freed, it shouldn't be an active or unevictable page. It should be
inactive and evictable. So allowing these flags to be set when freeing the page might not be a good idea?
Thanks!
>
On 2022/6/2 16:47, David Hildenbrand wrote:
> On 02.06.22 09:40, Miaohe Lin wrote:
>> On 2022/6/1 18:31, David Hildenbrand wrote:
>>> On 31.05.22 14:37, Miaohe Lin wrote:
>>>> On 2022/5/31 19:59, David Hildenbrand wrote:
>>>>> Sorry for the late reply, was on vacation.
>>>>
>>>> That's all right. Hope you have a great time. ;)
>>>>
>>>>>
>>>>>>>>
>>>>>>>> But for isolated page, PageLRU is cleared. So when the isolated page is released, __clear_page_lru_flags
>>>>>>>> won't be called. So we have to clear the PG_active and PG_unevictable here manully. So I think
>>>>>>>> this code block works. Or am I miss something again?
>>>>>>>
>>>>>>> Let's assume the following: page as freed by the owner and we enter
>>>>>>> unmap_and_move().
>>>>>>>
>>>>>>>
>>>>>>> #1: enter unmap_and_move() // page_count is 1
>>>>>>> #2: enter isolate_movable_page() // page_count is 1
>>>>>>> #2: get_page_unless_zero() // page_count is now 2
>>>>>>> #1: if (page_count(page) == 1) { // does not trigger
>>>>>>> #2: put_page(page); // page_count is now 1
>>>>>>> #1: put_page(page); // page_count is now 0 -> freed
>>>>>>>
>>>>>>>
>>>>>>> #1 will trigger __put_page() -> __put_single_page() ->
>>>>>>> __page_cache_release() will not clear the flags because it's not an LRU
>>>>>>> page at that point in time, right (-> isolated)?
>>>>>>
>>>>>> Sorry, you're right. I thought the old page will be freed via putback_lru_page which will
>>>>>> set PageLRU back instead of put_page directly. So if the above race occurs, PG_active and
>>>>>> PG_unevictable will remain set while page goes to the buddy and check_free_page will complain
>>>>>> about it. But it seems this is never witnessed?
>>>>>
>>>>> Maybe
>>>>>
>>>>> a) we were lucky so far and didn't trigger it
>>>>> b) the whole code block is dead code because we are missing something
>>>>> c) we are missing something else :)
>>>>
>>>> I think I found the things we missed in another email [1].
>>>> [1]: https://lore.kernel.org/all/[email protected]/
>>>>
>>>> Paste the main content of [1] here:
>>>>
>>>> "
>>>> There are 3 cases in unmap_and_move:
>>>>
>>>> 1.page is freed through "if (page_count(page) == 1)" code block. This works
>>>> as PG_active and PG_unevictable are cleared here.
>>>>
>>>> 2. Failed to migrate the page. The page won't be release so we don't care about it.
>>>
>>> Right, page is un-isolated.
>>>
>>>>
>>>> 3. The page is migrated successfully. The PG_active and PG_unevictable are cleared
>>>> via folio_migrate_flags():
>>>>
>>>> if (folio_test_clear_active(folio)) {
>>>> VM_BUG_ON_FOLIO(folio_test_unevictable(folio), folio);
>>>> folio_set_active(newfolio);
>>>> } else if (folio_test_clear_unevictable(folio))
>>>> folio_set_unevictable(newfolio);
>>>
>>> Right.
>>>
>>>>
>>>> For the above race case, the page won't be freed through "if (page_count(page) == 1)" code block.
>>>> It will just be migrated and freed via put_page() after folio_migrate_flags() having cleared PG_active
>>>> and PG_unevictable.
>>>> "
>>>> Or Am I miss something again? :)
>>>
>>> For #1, I'm still not sure what would happen on a speculative reference.
>>>
>>> It's worth summarizing that
>>>
>>> a) free_pages_prepare() will clear both flags via page->flags &=
>>> ~PAGE_FLAGS_CHECK_AT_PREP;
>>>
>>> b) free_pages_prepare() will bail out if any flag is set in
>>> check_free_page().
>>>
>>> As we've never seen b) in the wild, this certainly has low priority, and
>>> maybe it really cannot happen right now.
>>>
>>> However, maybe really allowing these flags to be set when freeing the
>>> page and removing the "page_count(page) == 1" case from migration code
>>> would be the clean thing to do.
>>
>> IMHO, check_free_page is used to catch possible problem. There's the comment of PAGE_FLAGS_CHECK_AT_FREE:
>>
>> /*
>> * Flags checked when a page is freed. Pages being freed should not have
>> * these flags set. If they are, there is a problem.
>> */
>> #define PAGE_FLAGS_CHECK_AT_FREE
>>
>> There might be an assumption: when page is freed, it shouldn't be an active or unevictable page. It should be
>> inactive and evictable. So allowing these flags to be set when freeing the page might not be a good idea?
>
> Yeah, and we'd be lifting that restriction because there is good reason
> to do so.
>
> Maybe we *could* special case for isolated pages; however, that adds
> runtime overhead. Of course, we could perform different checks for e.g.,
> DEBUG_VM vs !DEBUG_VM.
I found there is one assumption about PG_active and PG_unevictable, i.e. in __folio_clear_lru_flags:
/* this shouldn't happen, so leave the flags to bad_page() */
if (folio_test_active(folio) && folio_test_unevictable(folio))
return;
If PG_active and PG_unevictable are both set, this case will be caught in the bad_page() via check_free_page().
There might be some other assumptions about PG_active and PG_unevictable. So I think it's not safe to lift that
restriction.
But maybe we could limit this check within DEBUG_VM as you suggested. Am I supposed to do it?
Thanks!
>
On 07.06.22 04:20, Miaohe Lin wrote:
> On 2022/6/2 16:47, David Hildenbrand wrote:
>> On 02.06.22 09:40, Miaohe Lin wrote:
>>> On 2022/6/1 18:31, David Hildenbrand wrote:
>>>> On 31.05.22 14:37, Miaohe Lin wrote:
>>>>> On 2022/5/31 19:59, David Hildenbrand wrote:
>>>>>> Sorry for the late reply, was on vacation.
>>>>>
>>>>> That's all right. Hope you have a great time. ;)
>>>>>
>>>>>>
>>>>>>>>>
>>>>>>>>> But for isolated page, PageLRU is cleared. So when the isolated page is released, __clear_page_lru_flags
>>>>>>>>> won't be called. So we have to clear the PG_active and PG_unevictable here manully. So I think
>>>>>>>>> this code block works. Or am I miss something again?
>>>>>>>>
>>>>>>>> Let's assume the following: page as freed by the owner and we enter
>>>>>>>> unmap_and_move().
>>>>>>>>
>>>>>>>>
>>>>>>>> #1: enter unmap_and_move() // page_count is 1
>>>>>>>> #2: enter isolate_movable_page() // page_count is 1
>>>>>>>> #2: get_page_unless_zero() // page_count is now 2
>>>>>>>> #1: if (page_count(page) == 1) { // does not trigger
>>>>>>>> #2: put_page(page); // page_count is now 1
>>>>>>>> #1: put_page(page); // page_count is now 0 -> freed
>>>>>>>>
>>>>>>>>
>>>>>>>> #1 will trigger __put_page() -> __put_single_page() ->
>>>>>>>> __page_cache_release() will not clear the flags because it's not an LRU
>>>>>>>> page at that point in time, right (-> isolated)?
>>>>>>>
>>>>>>> Sorry, you're right. I thought the old page will be freed via putback_lru_page which will
>>>>>>> set PageLRU back instead of put_page directly. So if the above race occurs, PG_active and
>>>>>>> PG_unevictable will remain set while page goes to the buddy and check_free_page will complain
>>>>>>> about it. But it seems this is never witnessed?
>>>>>>
>>>>>> Maybe
>>>>>>
>>>>>> a) we were lucky so far and didn't trigger it
>>>>>> b) the whole code block is dead code because we are missing something
>>>>>> c) we are missing something else :)
>>>>>
>>>>> I think I found the things we missed in another email [1].
>>>>> [1]: https://lore.kernel.org/all/[email protected]/
>>>>>
>>>>> Paste the main content of [1] here:
>>>>>
>>>>> "
>>>>> There are 3 cases in unmap_and_move:
>>>>>
>>>>> 1.page is freed through "if (page_count(page) == 1)" code block. This works
>>>>> as PG_active and PG_unevictable are cleared here.
>>>>>
>>>>> 2. Failed to migrate the page. The page won't be release so we don't care about it.
>>>>
>>>> Right, page is un-isolated.
>>>>
>>>>>
>>>>> 3. The page is migrated successfully. The PG_active and PG_unevictable are cleared
>>>>> via folio_migrate_flags():
>>>>>
>>>>> if (folio_test_clear_active(folio)) {
>>>>> VM_BUG_ON_FOLIO(folio_test_unevictable(folio), folio);
>>>>> folio_set_active(newfolio);
>>>>> } else if (folio_test_clear_unevictable(folio))
>>>>> folio_set_unevictable(newfolio);
>>>>
>>>> Right.
>>>>
>>>>>
>>>>> For the above race case, the page won't be freed through "if (page_count(page) == 1)" code block.
>>>>> It will just be migrated and freed via put_page() after folio_migrate_flags() having cleared PG_active
>>>>> and PG_unevictable.
>>>>> "
>>>>> Or Am I miss something again? :)
>>>>
>>>> For #1, I'm still not sure what would happen on a speculative reference.
>>>>
>>>> It's worth summarizing that
>>>>
>>>> a) free_pages_prepare() will clear both flags via page->flags &=
>>>> ~PAGE_FLAGS_CHECK_AT_PREP;
>>>>
>>>> b) free_pages_prepare() will bail out if any flag is set in
>>>> check_free_page().
>>>>
>>>> As we've never seen b) in the wild, this certainly has low priority, and
>>>> maybe it really cannot happen right now.
>>>>
>>>> However, maybe really allowing these flags to be set when freeing the
>>>> page and removing the "page_count(page) == 1" case from migration code
>>>> would be the clean thing to do.
>>>
>>> IMHO, check_free_page is used to catch possible problem. There's the comment of PAGE_FLAGS_CHECK_AT_FREE:
>>>
>>> /*
>>> * Flags checked when a page is freed. Pages being freed should not have
>>> * these flags set. If they are, there is a problem.
>>> */
>>> #define PAGE_FLAGS_CHECK_AT_FREE
>>>
>>> There might be an assumption: when page is freed, it shouldn't be an active or unevictable page. It should be
>>> inactive and evictable. So allowing these flags to be set when freeing the page might not be a good idea?
>>
>> Yeah, and we'd be lifting that restriction because there is good reason
>> to do so.
>>
>> Maybe we *could* special case for isolated pages; however, that adds
>> runtime overhead. Of course, we could perform different checks for e.g.,
>> DEBUG_VM vs !DEBUG_VM.
>
> I found there is one assumption about PG_active and PG_unevictable, i.e. in __folio_clear_lru_flags:
>
> /* this shouldn't happen, so leave the flags to bad_page() */
> if (folio_test_active(folio) && folio_test_unevictable(folio))
> return;
>
> If PG_active and PG_unevictable are both set, this case will be caught in the bad_page() via check_free_page().
> There might be some other assumptions about PG_active and PG_unevictable. So I think it's not safe to lift that
> restriction.
>
> But maybe we could limit this check within DEBUG_VM as you suggested. Am I supposed to do it?
Well, if you want, you can look into ways of cleaning that up and
removing the "if there is more than one reference, the owner hasn't
freed the page" condition, because there are corner cases where the
owner might have freed the page but speculative references keep the
refcount temporarily incremented.
--
Thanks,
David / dhildenb