mm: Rework zap ptes on swap entries
Subsystems (2)
v2 cover mm: Rework zap ptes on swap entries
v2:
- Resend to rebase to tag v5.16-rc1 (fa55b7dcdc43)
The goal of this small series is to replace the previous patch (which is the 5th patch of the series):
https://lore.kernel.org/linux-mm/[email protected]/
This patch used a more aggresive (but IMHO cleaner and correct..) approach by removing that trick to skip swap entries, then we handle swap entries always.
The behavior of "skipping swap entries" existed starting from the initial git commit that we'll try to skip swap entries when zapping ptes if zap_detail pointer specified.
I found that it's probably broken because of the introduction of page migration mechanism that does not exist yet in the world of 1st git commit, then we could errornously skip scanning the swap entries for file-backed memory, like shmem, while we should. I'm afraid we'll have RSS accounting wrong for those shmem pages during migration so there could have leftover SHMEM RSS accounts.
Patch 1 did that removal of the trick, details in the commit message.
Patch 2 is a further cleanup for zap pte swap handling that can be done after patch 1, in which there's no functional change intended.
The change should be on the slow path for zapping swap entries (e.g., we handle none/present ptes in early code path always, so they're totally not affected), but if anyone worries about specific workload that may be affected by this patchset, please let me know and I'll be happy to run some more tests. I could also overlook something that was buried in history, in that case please kindly point that out. Marking the patchset RFC for this.
Smoke tested only. Please review, thanks.
Peter Xu (2):
mm: Don't skip swap entry even if zap_details specified
mm: Rework swap handling of zap_pte_range
mm/memory.c | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-)
Signature · 1 line
2.32.0
RFC PATCH v2 1/2 mm: Don't skip swap entry even if zap_details specified
This check existed since the 1st git commit of Linux repository, but at that time there's no page migration yet so I think it's okay.
With page migration enabled, it should logically be possible that we zap some shmem pages during migration. When that happens, IIUC the old code could have the RSS counter accounted wrong on MM_SHMEMPAGES because we will zap the ptes without decreasing the counters for the migrating entries. I have no unit test to prove it as I don't know an easy way to trigger this condition, though.
Besides, the optimization itself is already confusing IMHO to me in a few points:
- The wording "skip swap entries" is confusing, because we're not skipping all swap entries - we handle device private/exclusive pages before that.
- The skip behavior is enabled as long as zap_details pointer passed over. It's very hard to figure that out for a new zap caller because it's unclear why we should skip swap entries when we have zap_details specified.
- With modern systems, especially performance critical use cases, swap entries should be rare, so I doubt the usefulness of this optimization since it should be on a slow path anyway.
- It is not aligned with what we do with huge pmd swap entries, where in zap_huge_pmd() we'll do the accounting unconditionally.
This patch drops that trick, so we handle swap ptes coherently. Meanwhile we should do the same mapping check upon migration entries too.
--- mm/memory.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
mm/memory.c +2 -4
@@ -1382,16 +1382,14 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,continue ; } -/* If details->check_mapping, we leave swap entries. */ -if (unlikely (details)) -continue ; -if (!non_swap_entry (entry)) rss[MM_SWAPENTS ]--;else if (is_migration_entry (entry)) {struct page *page; page =pfn_swap_entry_to_page (entry); +if (unlikely (zap_skip_check_mapping (details, page))) +continue ; rss[mm_counter (page)]--; }if (unlikely (!free_swap_and_cache (entry)))
Signature · 1 line
2.32.0
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 10 lines · AEDT Peter Xu
This check existed since the 1st git commit of Linux repository, but at that time there's no page migration yet so I think it's okay.
With page migration enabled, it should logically be possible that we zap some shmem pages during migration. When that happens, IIUC the old code could have the RSS counter accounted wrong on
MM_SHMEMPAGESbecause we will zap the ptes without decreasing the counters for the migrating entries. I have no unit test to prove it as I don't know an easy way to trigger this condition, though.Besides, the optimization itself is already confusing IMHO to me in a few points:
I've spent a bit of time looking at this and think it would be good to get cleaned up as I've found it hard to follow in the past. What I haven't been able to confirm is if anything relies on skipping swap entries or not. From you're description it sounds like skipping swap entries was done as an optimisation rather than for some functional reason is that correct?
Quoted 11 lines
- The wording "skip swap entries" is confusing, because we're not skipping all swap entries - we handle device private/exclusive pages before that.
- The skip behavior is enabled as long as
zap_detailspointer passed over. It's very hard to figure that out for a new zap caller because it's unclear why we should skip swap entries when we havezap_detailsspecified.- With modern systems, especially performance critical use cases, swap entries should be rare, so I doubt the usefulness of this optimization since it should be on a slow path anyway.
Quoted 5 lines
- It is not aligned with what we do with huge pmd swap entries, where in
zap_huge_pmd()we'll do the accounting unconditionally.This patch drops that trick, so we handle swap ptes coherently. Meanwhile we should do the same mapping check upon migration entries too.
I agree, and I'm not convinced the current handling is very good - if we skip zapping a migration entry then the page mapping might get restored when the migration entry is removed.
In practice I don't think that is a problem as the migration entry target page will be locked, and if I'm understanding things correctly callers of unmap_mapping_*() need to have the page(s) locked anyway if they want to be sure the page is unmapped. But it seems removing the migration entries better matches the intent and I can't think of a reason why they should be skipped.
Quoted 29 lines
Signed-off-by Peter Xu <[email protected]>--- mm/memory.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)mm/memory.c +2
-4@@ -1382,16 +1382,14 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,continue ; } -/* If details->check_mapping, we leave swap entries. */ -if (unlikely (details)) -continue ; -if (!non_swap_entry (entry)) rss[MM_SWAPENTS ]--;else if (is_migration_entry (entry)) {struct page *page; page =pfn_swap_entry_to_page (entry); +if (unlikely (zap_skip_check_mapping (details, page))) +continue ; rss[mm_counter (page)]--; }if (unlikely (!free_swap_and_cache (entry)))
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 10 lines · Alistair Popple
Quoted 9 lines · AEDT Peter Xu
This check existed since the 1st git commit of Linux repository, but at that time there's no page migration yet so I think it's okay.
With page migration enabled, it should logically be possible that we zap some shmem pages during migration. When that happens, IIUC the old code could have the RSS counter accounted wrong on
MM_SHMEMPAGESbecause we will zap the ptes without decreasing the counters for the migrating entries. I have no unit test to prove it as I don't know an easy way to trigger this condition, though.
Quoted 7 lines
Quoted 1 line
Besides, the optimization itself is already confusing IMHO to me in a few points:
I've spent a bit of time looking at this and think it would be good to get cleaned up as I've found it hard to follow in the past. What I haven't been able to confirm is if anything relies on skipping swap entries or not. From you're description it sounds like skipping swap entries was done as an optimisation rather than for some functional reason is that correct?
Thanks again for looking into this patch, Alistair. I appreciate it a lot.
I should say that it's how I understand this, and I could be wrong, that's the major reason why I marked this patch as RFC.
As I mentioned this behavior existed in the 1st commit of git history of Linux, that's the time when there's no special swap entries at all but all the swap entries are "real" swap entries for anonymous.
That's why I think it should be an optimization because when previously zap_details (along with zap_details->mapping in the old code) is non-null, and that's definitely not an anonymous page. Then skipping swap entry for file backed memory sounds like a good optimization.
However after that we've got all kinds of swap entries introduced, and as you spotted at least the migration entry should be able to exist to some file backed memory type (shmem).
Quoted 22 lines
Quoted 12 lines
- The wording "skip swap entries" is confusing, because we're not skipping all swap entries - we handle device private/exclusive pages before that.
- The skip behavior is enabled as long as
zap_detailspointer passed over. It's very hard to figure that out for a new zap caller because it's unclear why we should skip swap entries when we havezap_detailsspecified.- With modern systems, especially performance critical use cases, swap entries should be rare, so I doubt the usefulness of this optimization since it should be on a slow path anyway.
Quoted 5 lines
- It is not aligned with what we do with huge pmd swap entries, where in
zap_huge_pmd()we'll do the accounting unconditionally.This patch drops that trick, so we handle swap ptes coherently. Meanwhile we should do the same mapping check upon migration entries too.
I agree, and I'm not convinced the current handling is very good - if we skip zapping a migration entry then the page mapping might get restored when the migration entry is removed.
Quoted 5 lines
In practice I don't think that is a problem as the migration entry target page will be locked, and if I'm understanding things correctly callers of unmap_mapping_*() need to have the page(s) locked anyway if they want to be sure the page is unmapped. But it seems removing the migration entries better matches the intent and I can't think of a reason why they should be skipped.
Exactly, that's what I see this too.
I used to think there is a bug for shmem migration (if you still remember I mentioned it in some of my previous patchset cover letters), but then I found migration requires page lock then it's probably not a real bug at all. However that's never a convincing reason to ignore swap entries.
I wanted to "ignore" this problem by the "adding a flag to skip swap entry" patch, but as you saw it was very not welcomed anyway, so I have no choice to try find the fundamental reason for skipping swap entries. When I figured I cannot really find any good reason and skipping seems to be even buggy, hence this patch. If this is the right way, the zap pte path can be simplified quite a lot after patch 2 of this series.
Signature · 1 line
Peter Xu
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 19 lines · AEDT Peter Xu
Quoted 10 lines · Alistair Popple
Quoted 9 lines · AEDT Peter Xu
This check existed since the 1st git commit of Linux repository, but at that time there's no page migration yet so I think it's okay.
With page migration enabled, it should logically be possible that we zap some shmem pages during migration. When that happens, IIUC the old code could have the RSS counter accounted wrong on
MM_SHMEMPAGESbecause we will zap the ptes without decreasing the counters for the migrating entries. I have no unit test to prove it as I don't know an easy way to trigger this condition, though.Quoted 7 lines
Quoted 1 line
Besides, the optimization itself is already confusing IMHO to me in a few points:
I've spent a bit of time looking at this and think it would be good to get cleaned up as I've found it hard to follow in the past. What I haven't been able to confirm is if anything relies on skipping swap entries or not. From you're description it sounds like skipping swap entries was done as an optimisation rather than for some functional reason is that correct?
Quoted 3 lines
Thanks again for looking into this patch, Alistair. I appreciate it a lot.
I should say that it's how I understand this, and I could be wrong, that's the
That makes two of us!
Quoted 10 lines
major reason why I marked this patch as RFC.
As I mentioned this behavior existed in the 1st commit of git history of Linux, that's the time when there's no special swap entries at all but all the swap entries are "real" swap entries for anonymous.
That's why I think it should be an optimization because when previously
zap_details(along withzap_details->mappingin the old code) is non-null, and that's definitely not an anonymous page. Then skipping swap entry for file backed memory sounds like a good optimization.
Thanks. That was the detail I was trying to figure out. Ie. why might something want to skip swap entries. I will spend some more time looking to be sure though.
Quoted 32 lines
However after that we've got all kinds of swap entries introduced, and as you spotted at least the migration entry should be able to exist to some file backed memory type (shmem).
Quoted 22 lines
Quoted 12 lines
- The wording "skip swap entries" is confusing, because we're not skipping all swap entries - we handle device private/exclusive pages before that.
- The skip behavior is enabled as long as
zap_detailspointer passed over. It's very hard to figure that out for a new zap caller because it's unclear why we should skip swap entries when we havezap_detailsspecified.- With modern systems, especially performance critical use cases, swap entries should be rare, so I doubt the usefulness of this optimization since it should be on a slow path anyway.
Quoted 5 lines
- It is not aligned with what we do with huge pmd swap entries, where in
zap_huge_pmd()we'll do the accounting unconditionally.This patch drops that trick, so we handle swap ptes coherently. Meanwhile we should do the same mapping check upon migration entries too.
I agree, and I'm not convinced the current handling is very good - if we skip zapping a migration entry then the page mapping might get restored when the migration entry is removed.
Quoted 5 lines
In practice I don't think that is a problem as the migration entry target page will be locked, and if I'm understanding things correctly callers of unmap_mapping_*() need to have the page(s) locked anyway if they want to be sure the page is unmapped. But it seems removing the migration entries better matches the intent and I can't think of a reason why they should be skipped.
Quoted 6 lines
Exactly, that's what I see this too.
I used to think there is a bug for shmem migration (if you still remember I mentioned it in some of my previous patchset cover letters), but then I found migration requires page lock then it's probably not a real bug at all. However that's never a convincing reason to ignore swap entries.
Right, it also took me a while to convince myself there wasn't a bug there so if for some reason this patch doesn't end up going in I think we should still treat migration entries the same way as device-private entries.
Quoted 6 lines
I wanted to "ignore" this problem by the "adding a flag to skip swap entry" patch, but as you saw it was very not welcomed anyway, so I have no choice to try find the fundamental reason for skipping swap entries. When I figured I cannot really find any good reason and skipping seems to be even buggy, hence this patch. If this is the right way, the zap pte path can be simplified quite a lot after patch 2 of this series.
Yep, I think it's definitely worth trying to figure out. And if it turns out there is some good reason for skipping we better make sure to document it in a comment somewhere so none of this good research is lost. However I haven't yet come up with a reason why they need to be skipped either.
- Alistair
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 3 lines · Alistair Popple
Thanks. That was the detail I was trying to figure out. Ie. why might something want to skip swap entries. I will spend some more time looking to be sure though.
Sure, please take your time, and thanks again for working together on this.
Signature · 1 line
Peter Xu
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
In reply to Peter XuQuoted 2 lines · Peter Xu
This check existed since the 1st git commit of Linux repository, but at that time there's no page migration yet so I think it's okay.
//git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git helps with the history. Yes, the check was okay back then, but a lot of changes have come in since: I'll tell more of those below.
You are absolutely right to clean this up and fix the bugs that have crept in, but I think the patch itself is not quite right yet.
Quoted 3 lines
With page migration enabled, it should logically be possible that we zap some shmem pages during migration.
Yes.
Quoted 4 lines
When that happens, IIUC the old code could have
the RSS counter accounted wrong onMM_SHMEMPAGESbecause we will zap the ptes without decreasing the counters for the migrating entries. I have no unit test to prove it as I don't know an easy way to trigger this condition, though.
In the no-details case, yes, it does look like that. I ought to try and reproduce that, but responding to mails seems more important.
Quoted 2 lines
Besides, the optimization itself is already confusing IMHO to me in a few points:
It is confusing and unnecessary and wrong, I agree.
Quoted 3 lines
- The wording "skip swap entries" is confusing, because we're not skipping all swap entries - we handle device private/exclusive pages before that.
I'm entirely ignorant of device pages, so cannot comment on your 2/2, but of course it's good if you can bring the cases closer together.
Quoted 4 lines
- The skip behavior is enabled as long as
zap_detailspointer passed over. It's very hard to figure that out for a new zap caller because it's unclear why we should skip swap entries when we havezap_detailsspecified.
History below will clarify that.
Quoted 7 lines
- With modern systems, especially performance critical use cases, swap entries should be rare, so I doubt the usefulness of this optimization since it should be on a slow path anyway.
- It is not aligned with what we do with huge pmd swap entries, where in
zap_huge_pmd()we'll do the accounting unconditionally.
The patch below does not align with what's done in zap_huge_pmd() either; but I think zap_huge_pmd() is okay without "details" because its only swap entries are migration entries, and we do not use huge pages when COWing from file huge pages.
Quoted 14 lines
This patch drops that trick, so we handle swap ptes coherently. Meanwhile we should do the same mapping check upon migration entries too.
Signed-off-by Peter Xu <[email protected]>--- mm/memory.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)mm/memory.c +0
-0
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1388
Quoted 7 lines
mm/memory.c +0
-4@@ -1382,16 +1382,14 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,continue ; } -/* If details->check_mapping, we leave swap entries. */ -if (unlikely (details)) -continue ; -
Good.
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1392
Quoted 8 lines
mm/memory.c +2
-0if (!non_swap_entry (entry)) rss[MM_SWAPENTS ]--;else if (is_migration_entry (entry)) {struct page *page; page =pfn_swap_entry_to_page (entry); +if (unlikely (zap_skip_check_mapping (details, page))) +continue ;
Good; though I don't think it's worth an "unlikely", and I say again, more forcefully, that "zap_skip_check_mapping" is a terrible name.
David suggested naming its inversion should_zap_page(), yes, that's much better; I'd toyed with do_not_zap_page() and zap_skip_page(), but should_zap_page() for the inversion sounds good to me.
And I'm pleased to see in one of Matthew's diffs that he intends to bring zap_details and zap_skip_check_mapping() back into mm/memory.c from include/linux/mm.h.
Quoted 5 lines
mm/memory.c +0
-0rss[mm_counter (page)]--; }if (unlikely (!free_swap_and_cache (entry)))Signature · 1 line
2.32.0
History. zap_details came in 2.6.6, and it was mostly to manage truncation on the non-linear vmas we had at that time (remap_file_pages syscall packing non-sequential offsets into a single vma, with pte_file entries), where index could not be deduced from position of pte in vma:
truncation range had to be passed down in zap_details; and an madvise needed it too, so it could not be private to mm/memory.c then.
But at the same time, I added the even_cows argument to unmap_mapping_range(), to distinguish truncation (which POSIX requires to unmap even COWed pages) from invalidation (for page cache coherence, which shouldn't touch private COWs). However, there appear to be no users of that in 2.6.6, though I wouldn't have added that complication just for the fun of confusing everyone: best guess would be that there was parallel development, and the use for !even_cows got removed in the very same release that it was being added.
(PageAnon was brand new in 2.6.6: maybe it could have been used instead of comparing details->check_mapping, or maybe there's some other case I've forgotten which actually needs the exact mapping check.)
Eventually a few drivers came to use unmap_shared_mapping_range(), the !even_cows caller; but more to the point, hole-punching came in, and I felt very strongly that hole-punching on a file had no right to delete private user data. So then !even_cows became useful.
IIRC, I've seen Linus say he *detests* zap_details. It had much better justification in the days of non-linear, and I was sad to add single_page to it quite recently; but hope that can go away later (when try_to_unmap_one() is properly extended to THPs).
Now, here's what may clear up a lot of the confusion. The 2.6.7 zap_pte_range() got a line at the head of zap_pte_range()
if (details && !details->check_mapping && !details->nonlinear_vma) details = NULL; which paired with the /* * If details->check_mapping, we leave swap entries; * if details->nonlinear_vma, we leave file entries. */ if (unlikely(details)) continue;
lower down. I haven't followed up the actual commits, but ChangeLog-2.6.7 implies that 2.6.6 had a "details = NULL;" placed elsewhere but buggily. In 2.6.12 it moved from zap_pte_range() to unmap_page_range(). It was intended, not so much to optimize, as to simplify the flow; but in fact has caused all this confusion.
When Kirill discontinued non-linear mapping support in 4.0 (no tears were shed and) the nonlinear_vma comment above got deleted, leaving just the then more puzzling check_mapping comment.
Then in 4.6 the "details = NULL;" seems to have got deleted as part of aac453635549 ("mm, oom: introduce oom reaper"), which added some new details (ignore_dirty and check_swap_entries); which got reverted in 4.11, but apparently the "details = NULL;" not restored.
My stamina is suddenly exhausted, without actually pinpointing a commit for "Fixes:" in your eventual cleanup. Sorry, I've had enough!
Hugh
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 8 lines · Hugh Dickins
Quoted 2 lines · Peter Xu
This check existed since the 1st git commit of Linux repository, but at that time there's no page migration yet so I think it's okay.
//git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git helps with the history. Yes, the check was okay back then, but a lot of changes have come in since: I'll tell more of those below.
Thanks for looking at this. By the way, the link is greatly helpful. It's always good to be able to read into the history.
Quoted 3 lines
You are absolutely right to clean this up and fix the bugs that have crept in, but I think the patch itself is not quite right yet.
Do you mean the pmd path on checking mapping? If so I agree, and I'll add that in v2 (even if as you pointed out that shouldn't be a real bug, iiuc, as you analyzed below).
Let me know if I missed anything else..
Quoted 7 lines
Quoted 4 lines
With page migration enabled, it should logically be possible that we zap some shmem pages during migration.
Yes.
Quoted 7 lines
Quoted 4 lines
When that happens, IIUC the old code could have
the RSS counter accounted wrong onMM_SHMEMPAGESbecause we will zap the ptes without decreasing the counters for the migrating entries. I have no unit test to prove it as I don't know an easy way to trigger this condition, though.In the no-details case, yes, it does look like that. I ought to try and reproduce that, but responding to mails seems more important.
Please let me know if you know how to reproduce it, since I don't know yet a real reproducer.
What I can do, though, is if with below patch applied to current linux master:
=============
mm/memory.c +3 -1
@@ -1383,8 +1383,10 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, }/* If details->check_mapping, we leave swap entries. */ -if (unlikely (details)) +if (unlikely (details)) { +WARN_ON_ONCE (is_migration_entry (entry));continue ; + }if (!non_swap_entry (entry)) rss[MM_SWAPENTS ]--;
=============
Then I can easily trigger it if with the help of a test program attached (zap-migrate.c).
IMHO it won't really reproduce because it seems all relevant shmem operations (e.g. hole punch, unmap..) will take the page lock so it won't really race with migration (which needs the page lock too), as mentioned in previous cover letters when I was still working on the old versions of this. But it could be possible that I missed something.
While the WARN_ON_ONCE() can trigger only because of the fast path in hole punching we have the pre-unmap without lock:
if ((u64)unmap_end > (u64)unmap_start) unmap_mapping_range(mapping, unmap_start, 1 + unmap_end - unmap_start, 0); shmem_truncate_range(inode, offset, offset + len - 1);
But that will be fixed up right afterwards in shmem_truncate_range(), afaict, which is with-lock. Hence we have a small window to at least trigger the warning above, even if it won't really mess up the accounting finally.
Quoted 62 lines
Quoted 3 lines
Besides, the optimization itself is already confusing IMHO to me in a few points:
It is confusing and unnecessary and wrong, I agree.
Quoted 3 lines
- The wording "skip swap entries" is confusing, because we're not skipping all swap entries - we handle device private/exclusive pages before that.
I'm entirely ignorant of device pages, so cannot comment on your 2/2, but of course it's good if you can bring the cases closer together.
Quoted 4 lines
- The skip behavior is enabled as long as
zap_detailspointer passed over. It's very hard to figure that out for a new zap caller because it's unclear why we should skip swap entries when we havezap_detailsspecified.History below will clarify that.
Quoted 7 lines
- With modern systems, especially performance critical use cases, swap entries should be rare, so I doubt the usefulness of this optimization since it should be on a slow path anyway.
- It is not aligned with what we do with huge pmd swap entries, where in
zap_huge_pmd()we'll do the accounting unconditionally.The patch below does not align with what's done in
zap_huge_pmd()either; but I thinkzap_huge_pmd()is okay without "details" because its only swap entries are migration entries, and we do not use huge pages when COWing from file huge pages.Quoted 14 lines
This patch drops that trick, so we handle swap ptes coherently. Meanwhile we should do the same mapping check upon migration entries too.
Signed-off-by Peter Xu <[email protected]>--- mm/memory.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)mm/memory.c +0
-0mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1388
Quoted 7 lines
mm/memory.c +0
-4@@ -1382,16 +1382,14 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,continue ; } -/* If details->check_mapping, we leave swap entries. */ -if (unlikely (details)) -continue ; -Good.
Quoted 5 lines
mm/memory.c +0
-0if (!non_swap_entry (entry)) rss[MM_SWAPENTS ]--;else if (is_migration_entry (entry)) {struct page *page;
Quoted 5 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1392
Quoted 3 lines
mm/memory.c +2
-0page =pfn_swap_entry_to_page (entry); +if (unlikely (zap_skip_check_mapping (details, page))) +continue ;Good; though I don't think it's worth an "unlikely", and I say again,
Sure, I'll drop this "unlikely". Meanwhile, shall we drop all the rest of the "unlikely" too around this check?
Quoted 5 lines
more forcefully, that "
zap_skip_check_mapping" is a terrible name.David suggested naming its inversion
should_zap_page(), yes, that's much better; I'd toyed withdo_not_zap_page()andzap_skip_page(), butshould_zap_page()for the inversion sounds good to me.
Ah sure, I'll rename it to should_zap_page() in a new patch before this.
Quoted 4 lines
And I'm pleased to see in one of Matthew's diffs that he intends to bring
zap_detailsandzap_skip_check_mapping()back intomm/memory.cfrominclude/linux/mm.h.
Yeah it's only used in memory.c. I put it there initially because I wanted zap_details user can reference what's that check mapping is all about, but maybe that's not necessary.
If you or Matthew could provide a link to that patch, I'll be happy to pick that up too with this series. Or I can also do nothing assuming Matthew will handle it elsewhere.
Quoted 58 lines
Quoted 6 lines
mm/memory.c +0
-0rss[mm_counter (page)]--; }if (unlikely (!free_swap_and_cache (entry)))Signature · 1 line
2.32.0History.
zap_detailscame in 2.6.6, and it was mostly to manage truncation on the non-linear vmas we had at that time (remap_file_pagessyscall packing non-sequential offsets into a single vma, withpte_fileentries), where index could not be deduced from position of pte in vma:
truncation range had to be passed down inzap_details; and an madvise needed it too, so it could not be private tomm/memory.cthen.But at the same time, I added the
even_cowsargument tounmap_mapping_range(), to distinguish truncation (which POSIX requires to unmap even COWed pages) from invalidation (for page cache coherence, which shouldn't touch private COWs). However, there appear to be no users of that in 2.6.6, though I wouldn't have added that complication just for the fun of confusing everyone: best guess would be that there was parallel development, and the use for !even_cows got removed in the very same release that it was being added.(PageAnon was brand new in 2.6.6: maybe it could have been used instead of comparing
details->check_mapping, or maybe there's some other case I've forgotten which actually needs the exact mapping check.)Eventually a few drivers came to use
unmap_shared_mapping_range(), the !even_cows caller; but more to the point, hole-punching came in, and I felt very strongly that hole-punching on a file had no right to delete private user data. So then !even_cows became useful.IIRC, I've seen Linus say he *detests*
zap_details. It had much better justification in the days of non-linear, and I was sad to addsingle_pageto it quite recently; but hope that can go away later (whentry_to_unmap_one()is properly extended to THPs).Now, here's what may clear up a lot of the confusion. The 2.6.7
zap_pte_range()got a line at the head ofzap_pte_range()if (details && !details->check_mapping && !details->nonlinear_vma) details = NULL; which paired with the /* * If details->check_mapping, we leave swap entries; * if details->nonlinear_vma, we leave file entries. */ if (unlikely(details)) continue;lower down. I haven't followed up the actual commits, but ChangeLog-2.6.7 implies that 2.6.6 had a "details = NULL;" placed elsewhere but buggily. In 2.6.12 it moved from
zap_pte_range()tounmap_page_range(). It was intended, not so much to optimize, as to simplify the flow; but in fact has caused all this confusion.When Kirill discontinued non-linear mapping support in 4.0 (no tears were shed and) the
nonlinear_vmacomment above got deleted, leaving just the then more puzzlingcheck_mappingcomment.
Quoted 7 lines
Then in 4.6 the "details = NULL;" seems to have got deleted as part of
aac453635549("mm, oom: introduce oom reaper"), which added some new details (ignore_dirtyandcheck_swap_entries); which got reverted in 4.11, but apparently the "details = NULL;" not restored.My stamina is suddenly exhausted, without actually pinpointing a commit for "Fixes:" in your eventual cleanup. Sorry, I've had enough!
Yeah it's in most cases a pain for digging all these trivial details, thanks for digging already most of it out of the mist.
That's really what I hope this patch can help: not only because the uffd work will rely on it, but also on resolving this early (we do use some wordings like "technical debt" sometimes, I think it's the same here but different form) when the above "history.git" is still functional so we can still reference.
With your help and the history.git I can try a better commit message because obviously some of the contents needs amending (it's not a pure optimization at all), but I assume the patch content will be mostly the same, with the tweaks applied.
Per stated so far I don't know any real reproducer so maybe it's not a real issue in any production system? Maybe that'll make it a bit easier, because then we don't strongly require a Fixes (which could be another hard question to answer besides the issue itself).
Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 62 lines · Peter Xu
Quoted 8 lines · Hugh Dickins
Quoted 2 lines · Peter Xu
This check existed since the 1st git commit of Linux repository, but at that time there's no page migration yet so I think it's okay.
//git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git helps with the history. Yes, the check was okay back then, but a lot of changes have come in since: I'll tell more of those below.
Thanks for looking at this. By the way, the link is greatly helpful. It's always good to be able to read into the history.
Quoted 3 lines
You are absolutely right to clean this up and fix the bugs that have crept in, but I think the patch itself is not quite right yet.
Do you mean the pmd path on checking mapping? If so I agree, and I'll add that in v2 (even if as you pointed out that shouldn't be a real bug, iiuc, as you analyzed below).
Let me know if I missed anything else..
Quoted 7 lines
Quoted 4 lines
With page migration enabled, it should logically be possible that we zap some shmem pages during migration.
Yes.
Quoted 7 lines
Quoted 4 lines
When that happens, IIUC the old code could have
the RSS counter accounted wrong onMM_SHMEMPAGESbecause we will zap the ptes without decreasing the counters for the migrating entries. I have no unit test to prove it as I don't know an easy way to trigger this condition, though.In the no-details case, yes, it does look like that. I ought to try and reproduce that, but responding to mails seems more important.
Please let me know if you know how to reproduce it, since I don't know yet a real reproducer.
What I can do, though, is if with below patch applied to current linux master:
=============
mm/memory.c +3
-1@@ -1383,8 +1383,10 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, }/* If details->check_mapping, we leave swap entries. */ -if (unlikely (details)) +if (unlikely (details)) { +WARN_ON_ONCE (is_migration_entry (entry));continue ; + }if (!non_swap_entry (entry)) rss[MM_SWAPENTS ]--;=============
Quoted 2 lines
Then I can easily trigger it if with the help of a test program attached (
zap-migrate.c).
(Attaching for real; also copy Matthew)
Quoted 199 lines
IMHO it won't really reproduce because it seems all relevant shmem operations (e.g. hole punch, unmap..) will take the page lock so it won't really race with migration (which needs the page lock too), as mentioned in previous cover letters when I was still working on the old versions of this. But it could be possible that I missed something.
While the
WARN_ON_ONCE()can trigger only because of the fast path in hole punching we have the pre-unmap without lock:if ((u64)unmap_end > (u64)unmap_start) unmap_mapping_range(mapping, unmap_start, 1 + unmap_end - unmap_start, 0); shmem_truncate_range(inode, offset, offset + len - 1);But that will be fixed up right afterwards in
shmem_truncate_range(), afaict, which is with-lock. Hence we have a small window to at least trigger the warning above, even if it won't really mess up the accounting finally.Quoted 62 lines
Quoted 3 lines
Besides, the optimization itself is already confusing IMHO to me in a few points:
It is confusing and unnecessary and wrong, I agree.
Quoted 3 lines
- The wording "skip swap entries" is confusing, because we're not skipping all swap entries - we handle device private/exclusive pages before that.
I'm entirely ignorant of device pages, so cannot comment on your 2/2, but of course it's good if you can bring the cases closer together.
Quoted 4 lines
- The skip behavior is enabled as long as
zap_detailspointer passed over. It's very hard to figure that out for a new zap caller because it's unclear why we should skip swap entries when we havezap_detailsspecified.History below will clarify that.
Quoted 7 lines
- With modern systems, especially performance critical use cases, swap entries should be rare, so I doubt the usefulness of this optimization since it should be on a slow path anyway.
- It is not aligned with what we do with huge pmd swap entries, where in
zap_huge_pmd()we'll do the accounting unconditionally.The patch below does not align with what's done in
zap_huge_pmd()either; but I thinkzap_huge_pmd()is okay without "details" because its only swap entries are migration entries, and we do not use huge pages when COWing from file huge pages.Quoted 14 lines
This patch drops that trick, so we handle swap ptes coherently. Meanwhile we should do the same mapping check upon migration entries too.
Signed-off-by Peter Xu <[email protected]>--- mm/memory.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)mm/memory.c +0
-0mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1388
Quoted 7 lines
mm/memory.c +0
-4@@ -1382,16 +1382,14 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,continue ; } -/* If details->check_mapping, we leave swap entries. */ -if (unlikely (details)) -continue ; -Good.
Quoted 5 lines
mm/memory.c +0
-0if (!non_swap_entry (entry)) rss[MM_SWAPENTS ]--;else if (is_migration_entry (entry)) {struct page *page;Quoted 5 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1392
Quoted 3 lines
mm/memory.c +2
-0page =pfn_swap_entry_to_page (entry); +if (unlikely (zap_skip_check_mapping (details, page))) +continue ;Good; though I don't think it's worth an "unlikely", and I say again,
Sure, I'll drop this "unlikely". Meanwhile, shall we drop all the rest of the "unlikely" too around this check?
Quoted 5 lines
more forcefully, that "
zap_skip_check_mapping" is a terrible name.David suggested naming its inversion
should_zap_page(), yes, that's much better; I'd toyed withdo_not_zap_page()andzap_skip_page(), butshould_zap_page()for the inversion sounds good to me.Ah sure, I'll rename it to
should_zap_page()in a new patch before this.Quoted 4 lines
And I'm pleased to see in one of Matthew's diffs that he intends to bring
zap_detailsandzap_skip_check_mapping()back intomm/memory.cfrominclude/linux/mm.h.Yeah it's only used in
memory.c. I put it there initially because I wantedzap_detailsuser can reference what's that check mapping is all about, but maybe that's not necessary.If you or Matthew could provide a link to that patch, I'll be happy to pick that up too with this series. Or I can also do nothing assuming Matthew will handle it elsewhere.
Quoted 58 lines
Quoted 6 lines
mm/memory.c +0
-0rss[mm_counter (page)]--; }if (unlikely (!free_swap_and_cache (entry)))Signature · 1 line
2.32.0History.
zap_detailscame in 2.6.6, and it was mostly to manage truncation on the non-linear vmas we had at that time (remap_file_pagessyscall packing non-sequential offsets into a single vma, withpte_fileentries), where index could not be deduced from position of pte in vma:
truncation range had to be passed down inzap_details; and an madvise needed it too, so it could not be private tomm/memory.cthen.But at the same time, I added the
even_cowsargument tounmap_mapping_range(), to distinguish truncation (which POSIX requires to unmap even COWed pages) from invalidation (for page cache coherence, which shouldn't touch private COWs). However, there appear to be no users of that in 2.6.6, though I wouldn't have added that complication just for the fun of confusing everyone: best guess would be that there was parallel development, and the use for !even_cows got removed in the very same release that it was being added.(PageAnon was brand new in 2.6.6: maybe it could have been used instead of comparing
details->check_mapping, or maybe there's some other case I've forgotten which actually needs the exact mapping check.)Eventually a few drivers came to use
unmap_shared_mapping_range(), the !even_cows caller; but more to the point, hole-punching came in, and I felt very strongly that hole-punching on a file had no right to delete private user data. So then !even_cows became useful.IIRC, I've seen Linus say he *detests*
zap_details. It had much better justification in the days of non-linear, and I was sad to addsingle_pageto it quite recently; but hope that can go away later (whentry_to_unmap_one()is properly extended to THPs).Now, here's what may clear up a lot of the confusion. The 2.6.7
zap_pte_range()got a line at the head ofzap_pte_range()if (details && !details->check_mapping && !details->nonlinear_vma) details = NULL; which paired with the /* * If details->check_mapping, we leave swap entries; * if details->nonlinear_vma, we leave file entries. */ if (unlikely(details)) continue;lower down. I haven't followed up the actual commits, but ChangeLog-2.6.7 implies that 2.6.6 had a "details = NULL;" placed elsewhere but buggily. In 2.6.12 it moved from
zap_pte_range()tounmap_page_range(). It was intended, not so much to optimize, as to simplify the flow; but in fact has caused all this confusion.When Kirill discontinued non-linear mapping support in 4.0 (no tears were shed and) the
nonlinear_vmacomment above got deleted, leaving just the then more puzzlingcheck_mappingcomment.Quoted 7 lines
Then in 4.6 the "details = NULL;" seems to have got deleted as part of
aac453635549("mm, oom: introduce oom reaper"), which added some new details (ignore_dirtyandcheck_swap_entries); which got reverted in 4.11, but apparently the "details = NULL;" not restored.My stamina is suddenly exhausted, without actually pinpointing a commit for "Fixes:" in your eventual cleanup. Sorry, I've had enough!
Yeah it's in most cases a pain for digging all these trivial details, thanks for digging already most of it out of the mist.
That's really what I hope this patch can help: not only because the uffd work will rely on it, but also on resolving this early (we do use some wordings like "technical debt" sometimes, I think it's the same here but different form) when the above "history.git" is still functional so we can still reference.
With your help and the history.git I can try a better commit message because obviously some of the contents needs amending (it's not a pure optimization at all), but I assume the patch content will be mostly the same, with the tweaks applied.
Per stated so far I don't know any real reproducer so maybe it's not a real issue in any production system? Maybe that'll make it a bit easier, because then we don't strongly require a Fixes (which could be another hard question to answer besides the issue itself).
Thanks,
Signature · 1 line
Peter Xu
Signature · 1 line
Peter Xu
#define _GNU_SOURCE /* See feature_test_macros(7) */
#include <fcntl.h>
#include <stdio.h>
#include <numaif.h>
#include <unistd.h>
#include <sys/mman.h>
#include <assert.h>
#include <stdlib.h>
#include <pthread.h>
#include <semaphore.h>
#include <time.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <string.h>
#define BSIZE (1UL << 28) /* 64MB */
#define DELAY 30 /* Something bigger than 10 */
int page_size, npages, *status, *nodes;pthread_t thread;
char *buffer;
void **pages;sem_t sem;
char str[4096];
int shmem_fd;
void *evictor_thread(void *data)
{
int i, ret;
printf("evictor created\n");
for (i = 0; i < npages; i++) {
sem_wait(&sem);
usleep(random() % DELAY);
ret = fallocate(shmem_fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
i * page_size, page_size);
assert(ret == 0);
}
printf("evictor quitted\n");
return NULL;
}void do_init(void)
{
int i, ret;
srand(time(NULL));
sem_init(&sem, 0, 0);
page_size = getpagesize();
npages = BSIZE / page_size;
shmem_fd = memfd_create("zap-migrate", 0);
assert(shmem_fd >= 0);
ret = ftruncate(shmem_fd, BSIZE);
assert(ret == 0);
buffer = mmap(NULL, BSIZE, PROT_READ | PROT_WRITE,
MAP_SHARED, shmem_fd, 0);
assert(buffer != MAP_FAILED);
pages = calloc(npages, sizeof(void *));
status = calloc(npages, sizeof(int));
nodes = calloc(npages, sizeof(int));
assert(pages && status && nodes);
for (i = 0; i < npages; i++) {
*(buffer + page_size * i) = i;
}
ret = pthread_create(&thread, NULL, evictor_thread, NULL);
assert(ret == 0);
}void do_quit(void)
{
free(pages);
free(status);
free(nodes);
munmap(buffer, BSIZE);
close(shmem_fd);
}void move_all_to_node(int node)
{
long ret;
int i;
for (i = 0; i < npages; i++) {
pages[i] = buffer + page_size * i;
nodes[i] = node;
status[i] = 0;
}
ret = move_pages(0, npages, pages, nodes, status, MPOL_MF_MOVE);
assert(ret == 0);
printf("%s node=%d\n", __func__, node);
}void move_each_to_node(int node)
{
long ret;
int i;
for (i = 0; i < npages; i++) {
pages[0] = buffer + page_size * i;
nodes[0] = node;
status[0] = -1;
/* Kick the evictor */
sem_post(&sem);
usleep(10);
ret = move_pages(0, 1, pages, nodes, status, MPOL_MF_MOVE);
assert(ret == 0);
}
printf("%s node=%d\n", __func__, node);
}void check(void)
{
pid_t pid = getpid();
int fd, ret;
char *p, *end;
sprintf(str, "/proc/%d/smaps_rollup", pid);
printf("Dump file: %s\n==========\n", str);
fd = open(str, O_RDONLY);
assert(fd >= 0);
while (1) {
ret = read(fd, str, sizeof(str)-1);
assert(ret >= 0);
if (ret == 0)
break;
}
p = strstr(str, "Pss_Shmem:");
end = strchr(p, '\n');
*end = 0;
printf("%s\n", p);
close(fd);
}void main(void)
{
do_init();
move_all_to_node(0);
move_each_to_node(1);
/* Make sure madvise() all ran */
pthread_join(thread, NULL);
/* Should be "0KB" */
check();
do_quit();
}Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 8 lines · Peter Xu
Quoted 7 lines · Peter Xu
Quoted 4 lines · Hugh Dickins
On Mon, 15 Nov 2021, Peter Xu wrote:
You are absolutely right to clean this up and fix the bugs that have crept in, but I think the patch itself is not quite right yet.
Do you mean the pmd path on checking mapping?
Sorry, no, I don't mean that at all. We agree that we cannot see an actual bug there, so nothing to fix; and re-exporting all the zap_details stuff to make it available to mm/huge_memory.c would be rather sad. You might wish to do some future-proofing, but let's just leave that to the future.
I mean that the handling of swap-like entries in zap_pte_range(), after you've removed the "if (unlikely(details)) continue;", is not right yet.
Quoted 23 lines
Quoted 23 lines
If so I agree, and I'll add that
in v2 (even if as you pointed out that shouldn't be a real bug, iiuc, as you analyzed below).Let me know if I missed anything else..
What I can do, though, is if with below patch applied to current linux master:
=============
mm/memory.c +3
-1@@ -1383,8 +1383,10 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, }/* If details->check_mapping, we leave swap entries. */ -if (unlikely (details)) +if (unlikely (details)) { +WARN_ON_ONCE (is_migration_entry (entry));continue ; + }
Quoted 8 lines
Quoted 6 lines
mm/memory.c +0
-0if (!non_swap_entry (entry)) rss[MM_SWAPENTS ]--;=============
Then I can easily trigger it if with the help of a test program attached (
zap-migrate.c).(Attaching for real; also copy Matthew)
Yes, I'm not at all surprised that you can see migration entries there.
Quoted 7 lines
Quoted 7 lines
IMHO it won't really reproduce because it seems all relevant shmem operations (e.g. hole punch, unmap..) will take the page lock so it won't really race with migration (which needs the page lock too), as mentioned in previous cover letters when I was still working on the old versions of this. But it could be possible that I missed something.
The latter.
There may still be some cases in which unmap_mapping_range() is called on a single page with that page held locked, I have not checked; but in general it is called on a range of pages, and would not have them locked.
The usual pattern when truncating/hole-punching/invalidating is to call unmap_mapping_range() first, to do the bulk of the work efficiently, scanning down the page tables without holding any page lock; but then unmap racily faulted stragglers one by one while holding page lock using unmap_mapping_page() just before deleting each from page cache.
So some relevant operations are done without page lock and some relevant operations are done with page lock: both need to be considered, but you are right that the page-locked ones will not encounter a migration entry of that page (I haven't tried to see if that fact is useful here or not).
You may need to substitute "pages" or "page" or "folio" in the above.
Quoted 8 lines
Quoted 8 lines
While the
WARN_ON_ONCE()can trigger only because of the fast path in hole punching we have the pre-unmap without lock:if ((u64)unmap_end > (u64)unmap_start) unmap_mapping_range(mapping, unmap_start, 1 + unmap_end - unmap_start, 0); shmem_truncate_range(inode, offset, offset + len - 1);
Oh, I should have read on, I've been wasting my time explaining above:
there you said the page must be locked, here you find that is not so.
Quoted 4 lines
Quoted 4 lines
But that will be fixed up right afterwards in
shmem_truncate_range(), afaict, which is with-lock. Hence we have a small window to at least trigger the warning above, even if it won't really mess up the accounting finally.
Well, there is still an opportunity to do the wrong thing.
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1388
Quoted 10 lines
mm/memory.c +0
-4@@ -1382,16 +1382,14 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,continue ; } -/* If details->check_mapping, we leave swap entries. */ -if (unlikely (details)) -continue ; -Good.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1392
Quoted 2 lines
mm/memory.c +0
-0if (!non_swap_entry (entry)) rss[MM_SWAPENTS ]--;
Right, I did not say at all what is wrong here, I suppose I intended to come back but then got tired - sorry. Here we are dealing with a genuine swap entry. No problem for exit or munmap or MADV_DONTNEED. No problem for truncation (but apparently there has been an unnoticed problem, that swapped-out COWed pages beyond the truncation point were not being deleted, while that "details continue" was still there). But a problem for hole-punching: without the "details continue", this will go on to delete COWed pages which ought to be preserved - needs a should_zap_page() check, as you do with the migration entry.
Except that here we have no page to check, so it looks like you'll have to change should_zap_page() to deal with this case too, or just check details->check_mapping directly. Which raises the question again of why I did not just use a boolean flag there originally: aah, I think I've found why. In those days there was a horrible "optimization", for better performance on some benchmark I guess, which when you read from /dev/zero into a private mapping, would map the zero page there (look up read_zero_pagealigned() and zeromap_page_range() if you dare). So there was another category of page to be skipped along with the anon COWs, and I didn't want multiple tests in the zap loop, so checking check_mapping against page->mapping did both. I think nowadays you could do it by checking for PageAnon page (or genuine swap entry) instead.
Quoted 12 lines
Quoted 12 lines
Quoted 8 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1392
Quoted 6 lines
mm/memory.c +2
-0else if (is_migration_entry (entry)) {struct page *page; page =pfn_swap_entry_to_page (entry); +if (unlikely (zap_skip_check_mapping (details, page))) +continue ;Good; though I don't think it's worth an "unlikely", and I say again,
Sure, I'll drop this "unlikely". Meanwhile, shall we drop all the rest of the "unlikely" too around this check?
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
more forcefully, that "
zap_skip_check_mapping" is a terrible name.David suggested naming its inversion
should_zap_page(), yes, that's much better; I'd toyed withdo_not_zap_page()andzap_skip_page(), butshould_zap_page()for the inversion sounds good to me.Ah sure, I'll rename it to
should_zap_page()in a new patch before this.
Thanks; though now even that name is looking dubious - should_zap_entry()?
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
And I'm pleased to see in one of Matthew's diffs that he intends to bring
zap_detailsandzap_skip_check_mapping()back intomm/memory.cfrominclude/linux/mm.h.
Quoted 7 lines
Quoted 7 lines
Yeah it's only used in
memory.c. I put it there initially because I wantedzap_detailsuser can reference what's that check mapping is all about, but maybe that's not necessary.If you or Matthew could provide a link to that patch, I'll be happy to pick that up too with this series. Or I can also do nothing assuming Matthew will handle it elsewhere.
In Linus's tree today: 3506659e18a6 ("mm: Add unmap_mapping_folio()").
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1394
Quoted 4 lines
mm/memory.c +0
-0rss[mm_counter (page)]--; }
I have given no thought as to whether more "else"s are needed there.
Quoted 1 line
Quoted 1 line
Quoted 1 line
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1395
Quoted 1 line
mm/memory.c +0
-0if (unlikely (!free_swap_and_cache (entry)))
(I think you moved that to a more relevant place in a later patch: good.)
I know you're also asking for test suggestions, but I'll leave it to you:
hole-punching, truncation, swapping, compaction (to force migration).
Hugh
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Hi, Hugh,
Quoted 38 lines · Hugh Dickins
Quoted 5 lines
Quoted 5 lines
IMHO it won't really reproduce because it seems all relevant shmem operations (e.g. hole punch, unmap..) will take the page lock so it won't really race with migration (which needs the page lock too), as mentioned in previous cover letters when I was still working on the old versions of this. But it could be possible that I missed something.
The latter.
There may still be some cases in which
unmap_mapping_range()is called on a single page with that page held locked, I have not checked; but in general it is called on a range of pages, and would not have them locked.The usual pattern when truncating/hole-punching/invalidating is to call
unmap_mapping_range()first, to do the bulk of the work efficiently, scanning down the page tables without holding any page lock; but then unmap racily faulted stragglers one by one while holding page lock usingunmap_mapping_page()just before deleting each from page cache.So some relevant operations are done without page lock and some relevant operations are done with page lock: both need to be considered, but you are right that the page-locked ones will not encounter a migration entry of that page (I haven't tried to see if that fact is useful here or not).
You may need to substitute "pages" or "page" or "folio" in the above.
Quoted 8 lines
Quoted 8 lines
While the
WARN_ON_ONCE()can trigger only because of the fast path in hole punching we have the pre-unmap without lock:if ((u64)unmap_end > (u64)unmap_start) unmap_mapping_range(mapping, unmap_start, 1 + unmap_end - unmap_start, 0); shmem_truncate_range(inode, offset, offset + len - 1);Oh, I should have read on, I've been wasting my time explaining above:
there you said the page must be locked, here you find that is not so.Quoted 1 line
Quoted 1 line
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
But that will be fixed up right afterwards in
shmem_truncate_range(), afaict, which is with-lock. Hence we have a small window to at least trigger the warning above, even if it won't really mess up the accounting finally.Well, there is still an opportunity to do the wrong thing.
So what I wanted to express is we can't trigger any real issue, so no way to write a valid reproducer even if we know it did it wrong.. And that's why I provided a "fake" one..
Let me explain a little bit further. Sorry I know you know most of below, so it could be that you spend 5 minutes reading nonsense.. but I want to 100% sure we're on the same page.
Firstly, current code will have a problem if we:
(1) Passed over a valid "zap_details" pointer to zap_pte_range, and,
(2) At the meantime if there can be a swap entry of any kind of below:
(2.1) real swap entry
(2.2) migration swap entry
Then we got doomed because we could have errornously skipped some swap entry that was also under the mapping while we shouldn't.
I didn't mention device exclusive/private entries are fine because they're checked before the "details continue" check.
I didn't mention hwpoison entry too; but I'll discuss it later..
Then, when do we have cases above (1) or (2)?
(1) is impossible because when there's zap_details in current code it means the page is backing a fs mapping, hence not PageAnon. We can only hit (2).
What can hit (2)? I only know shmem...
Next, as a conclusion, what we want to reproduce is: some caller calls zap_pte_range() with zap_details, but accidentally there's a migration entry.
It's fairly easy to find this, as what my "fake" reproducer was doing.
However, as I stated before, all these use cases always have another step to take the lock and redo the range. Then even if some migration entry got wrongly skipped it'll always be fixed. What we need to find is some caller that calls zap_pte_range() without later taking the page lock and redo that. That's the only possibility to trigger a real issue on the shmem accounting.
To prove this, let's see all the possible call stacks...
zap_pte_range() will only be called with zap_details pointer with below two functions:
unmap_mapping_folio
unmap_mapping_pages
unmap_mapping_folio is already having the page lock so it's not going to work. Then the callers of unmap_mapping_pages are:
unmap_mapping_pages
[1] collapse_file
[?] unmap_mapping_range
[?] unmap_shared_mapping_range
[?] vmw_bo_dirty_unmap (gpu/drm)
[?] inode_go_sync (gfs2)
[1] shmem_setattr
[1] shmem_fallocate
[1] truncate_pagecache
[1] truncate_pagecache_range
[1] invalidate_inode_pages2_rangeIt's a reversed tree of call stacks, when I marked [1] means this call site does the "unmap then take page lock and redo" thing, so it can't reproduce.
The only outliers are:
[?] vmw_bo_dirty_unmap (gpu/drm)
[?] inode_go_sync (gfs2)Which I marked with [?].
However I don't think they'll have migration entries at all, or am I wrong?
I hope I explained why I think we can't write a reproducer.. but I still think we should fix the problem, because we can't tell whether it'll go wrong in the future with a new caller who will not take the page lock to redo the zap.
Quoted 17 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1388
Quoted 11 lines
mm/memory.c +0
-4@@ -1382,16 +1382,14 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,continue ; } -/* If details->check_mapping, we leave swap entries. */ -if (unlikely (details)) -continue ; -Good.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1392
Quoted 2 lines
mm/memory.c +0
-0if (!non_swap_entry (entry)) rss[MM_SWAPENTS ]--;
Quoted 9 lines
Right, I did not say at all what is wrong here, I suppose I intended to come back but then got tired - sorry. Here we are dealing with a genuine swap entry. No problem for exit or munmap or
MADV_DONTNEED. No problem for truncation (but apparently there has been an unnoticed problem, that swapped-out COWed pages beyond the truncation point were not being deleted, while that "details continue" was still there). But a problem for hole-punching: without the "details continue", this will go on to delete COWed pages which ought to be preserved - needs ashould_zap_page()check, as you do with the migration entry.
I see, thanks for pointing out.
Quoted 4 lines
Except that here we have no page to check, so it looks like you'll have to change
should_zap_page()to deal with this case too, or just checkdetails->check_mappingdirectly.
Yeah I prefer this, as we don't have the page* pointer anyway.
Quoted 11 lines
Which raises the question again
of why I did not just use a boolean flag there originally: aah, I think I've found why. In those days there was a horrible "optimization", for better performance on some benchmark I guess, which when you read from/dev/zerointo a private mapping, would map the zero page there (look upread_zero_pagealigned()andzeromap_page_range()if you dare). So there was another category of page to be skipped along with the anon COWs, and I didn't want multiple tests in the zap loop, so checkingcheck_mappingagainstpage->mappingdid both. I think nowadays you could do it by checking for PageAnon page (or genuine swap entry) instead.
It must be PageAnon already, isn't it?
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 9 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1392
Quoted 7 lines
mm/memory.c +2
-0else if (is_migration_entry (entry)) {struct page *page; page =pfn_swap_entry_to_page (entry); +if (unlikely (zap_skip_check_mapping (details, page))) +continue ;Good; though I don't think it's worth an "unlikely", and I say again,
Sure, I'll drop this "unlikely". Meanwhile, shall we drop all the rest of the "unlikely" too around this check?
Quoted 2 lines
more forcefully, that "
zap_skip_check_mapping" is a terrible name.
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
David suggested naming its inversion
should_zap_page(), yes, that's much better; I'd toyed withdo_not_zap_page()andzap_skip_page(), butshould_zap_page()for the inversion sounds good to me.Ah sure, I'll rename it to
should_zap_page()in a new patch before this.Thanks; though now even that name is looking dubious -
should_zap_entry()?
I guess my plan is simply keep should_zap_page() and check explicitly for real swap entries.
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 6 lines
And I'm pleased to see in one of Matthew's diffs that he intends to bring
zap_detailsandzap_skip_check_mapping()back intomm/memory.cfrominclude/linux/mm.h.Yeah it's only used in
memory.c. I put it there initially because I wantedzap_detailsuser can reference what's that check mapping is all about, but maybe that's not necessary.
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
If you or Matthew could provide a link to that patch, I'll be happy to pick that up too with this series. Or I can also do nothing assuming Matthew will handle it elsewhere.
In Linus's tree today:
3506659e18a6("mm: Addunmap_mapping_folio()").
Rebased.
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1394
Quoted 5 lines
mm/memory.c +0
-0rss[mm_counter (page)]--; }I have given no thought as to whether more "else"s are needed there.
It's hwpoison that's in the else. Nothing else should.
I didn't mention it probably because I forgot. I did think about it when drafting, and IMHO we should simply zap that hwpoison entry because:
(1) Zap means the user knows this data is meaningless, so at least we shouldn't SIGBUS for that anymore.
(2) If we keep it there, it could errornously trigger SIGBUS later if the guest accessed that pte again somehow.
I plan to mention that in the commit message, but I can also add some comments directly into the code. Let me know your thoughts.
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1395
Quoted 2 lines
mm/memory.c +0
-0if (unlikely (!free_swap_and_cache (entry)))(I think you moved that to a more relevant place in a later patch: good.)
I know you're also asking for test suggestions, but I'll leave it to you:
hole-punching, truncation, swapping, compaction (to force migration).
Please read above - again I don't think we can write a reproducer, and that's why I provided the "fake" reproducer only.
I could always be missing something, though. I'd be glad to be corrected, again.
Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 18 lines · Peter Xu
Quoted 3 lines
Except that here we have no page to check, so it looks like you'll have to change
should_zap_page()to deal with this case too, or just checkdetails->check_mappingdirectly.Yeah I prefer this, as we don't have the page* pointer anyway.
Quoted 11 lines
Which raises the question again
of why I did not just use a boolean flag there originally: aah, I think I've found why. In those days there was a horrible "optimization", for better performance on some benchmark I guess, which when you read from/dev/zerointo a private mapping, would map the zero page there (look upread_zero_pagealigned()andzeromap_page_range()if you dare). So there was another category of page to be skipped along with the anon COWs, and I didn't want multiple tests in the zap loop, so checkingcheck_mappingagainstpage->mappingdid both. I think nowadays you could do it by checking for PageAnon page (or genuine swap entry) instead.
Quoted 1 line
It must be PageAnon already, isn't it?
I think I see what you meant now..
I assume the special case is gone, how about I switch zap_mappings back into a boolean altogether in this patchset? Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 19 lines · Peter Xu
Quoted 18 lines · Peter Xu
Quoted 3 lines
Except that here we have no page to check, so it looks like you'll have to change
should_zap_page()to deal with this case too, or just checkdetails->check_mappingdirectly.Yeah I prefer this, as we don't have the page* pointer anyway.
Quoted 11 lines
Which raises the question again
of why I did not just use a boolean flag there originally: aah, I think I've found why. In those days there was a horrible "optimization", for better performance on some benchmark I guess, which when you read from/dev/zerointo a private mapping, would map the zero page there (look upread_zero_pagealigned()andzeromap_page_range()if you dare). So there was another category of page to be skipped along with the anon COWs, and I didn't want multiple tests in the zap loop, so checkingcheck_mappingagainstpage->mappingdid both. I think nowadays you could do it by checking for PageAnon page (or genuine swap entry) instead.
Quoted 6 lines
Quoted 1 line
It must be PageAnon already, isn't it?
I think I see what you meant now..
I assume the special case is gone, how about I switch
zap_mappingsback into a boolean altogether in this patchset? Thanks,
Oh, one more thing..
When reading the history and also your explanations above, I figured one thing that may not be right for a long time, on zero page handling of zapping.
If to quote your comment above, we should keep the zero page entries too if zap_details.zap_mapping is specified. However it's not true for a long time, I guess starting from when vm_normal_page() returns NULL for zero pfns. I also have a very strong feeling that in the old world the "page*" is non-NULL for zero pages here.
So... I'm boldly thinking whether we should also need another fix upon the zero page handling here too, probably before this whole patchset (so it'll be the 1st patch, it should directly apply to master) because if I'm right on above it can be seen as another separate bug fix:
---8<---
mm/memory.c +10 -3
@@ -1320,11 +1320,18 @@ struct zap_details {static inline bool zap_skip_check_mapping (struct zap_details *details,struct page *page) { -if (!details || !page) +/* No detail pointer or no zap_mapping pointer, zap all */ +if (!details || !details->zap_mapping)return false; -return details->zap_mapping && - (details->zap_mapping !=page_rmapping (page)); +/* +* For example, the zero page. If the user wants to keep the private +* pages, zero page should also be in count. +*/ +if (!page) +return true; + +return details->zap_mapping !=page_rmapping (page); } static unsigned long zap_pte_range(struct mmu_gather *tlb,
---8<---
page can be NULL for e.g. PFNMAP and when error occured too above. I assume we don't need to bother with them (e.g., I don't think PFNMAP or MIXED should specify even_cows=false at all, because there's no page cache layer), though. Mostly it's about how we should handle zero page right.
Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 46 lines · Peter Xu
Oh, one more thing..
When reading the history and also your explanations above, I figured one thing that may not be right for a long time, on zero page handling of zapping.
If to quote your comment above, we should keep the zero page entries too if zap_details.zap_mapping is specified. However it's not true for a long time, I guess starting from when
vm_normal_page()returns NULL for zero pfns. I also have a very strong feeling that in the old world the "page*" is non-NULL for zero pages here.So... I'm boldly thinking whether we should also need another fix upon the zero page handling here too, probably before this whole patchset (so it'll be the 1st patch, it should directly apply to master) because if I'm right on above it can be seen as another separate bug fix:
---8<---
mm/memory.c +10
-4@@ -1320,11 +1320,18 @@ struct zap_details {static inline bool zap_skip_check_mapping (struct zap_details *details,struct page *page) { -if (!details || !page) +/* No detail pointer or no zap_mapping pointer, zap all */ +if (!details || !details->zap_mapping)return false; -return details->zap_mapping && - (details->zap_mapping !=page_rmapping (page)); +/* +* For example, the zero page. If the user wants to keep the private +* pages, zero page should also be in count. +*/ +if (!page) +return true; + +return details->zap_mapping !=page_rmapping (page); } static unsigned long zap_pte_range(struct mmu_gather *tlb, ---8<---
Quoted 4 lines
page can be NULL for e.g. PFNMAP and when error occured too above. I assume we don't need to bother with them (e.g., I don't think PFNMAP or MIXED should specify even_cows=false at all, because there's no page cache layer), though. Mostly it's about how we should handle zero page right.
I have not understood the above.
I don't know of any problem that needs fixing with the zero page:
how do you suppose the zero page gets into a truncatable or hole-punchable mapping? We use it for read faults in anonymous mappings. And I told the story of how once-upon-a-time it could get inserted into any mapping by reading from /dev/zero, but that odd case was dropped years ago. And I am open to (even encouraging) a change to make use of zero page for read faults of holes in shmem: but that's potential future work, which would require some changes elsewhere (though perhaps none here: the zero page could never be used for the result of a COW).
Please explain the zero page problem you hope to fix here.
Hugh
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 11 lines · Hugh Dickins
I don't know of any problem that needs fixing with the zero page:
how do you suppose the zero page gets into a truncatable or hole-punchable mapping? We use it for read faults in anonymous mappings. And I told the story of how once-upon-a-time it could get inserted into any mapping by reading from/dev/zero, but that odd case was dropped years ago. And I am open to (even encouraging) a change to make use of zero page for read faults of holes in shmem: but that's potential future work, which would require some changes elsewhere (though perhaps none here: the zero page could never be used for the result of a COW).Please explain the zero page problem you hope to fix here.
After I tried to learn the old/new worlds of zero page somehow I thought there can be zero pfns installed, but it seems not at all..
Please ignore above, sorry for the noise.
Signature · 1 line
Peter Xu
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
In reply to Peter XuQuoted 68 lines · Peter Xu
Hi, Hugh,
Quoted 38 lines · Hugh Dickins
Quoted 5 lines
Quoted 5 lines
IMHO it won't really reproduce because it seems all relevant shmem operations (e.g. hole punch, unmap..) will take the page lock so it won't really race with migration (which needs the page lock too), as mentioned in previous cover letters when I was still working on the old versions of this. But it could be possible that I missed something.
The latter.
There may still be some cases in which
unmap_mapping_range()is called on a single page with that page held locked, I have not checked; but in general it is called on a range of pages, and would not have them locked.The usual pattern when truncating/hole-punching/invalidating is to call
unmap_mapping_range()first, to do the bulk of the work efficiently, scanning down the page tables without holding any page lock; but then unmap racily faulted stragglers one by one while holding page lock usingunmap_mapping_page()just before deleting each from page cache.So some relevant operations are done without page lock and some relevant operations are done with page lock: both need to be considered, but you are right that the page-locked ones will not encounter a migration entry of that page (I haven't tried to see if that fact is useful here or not).
You may need to substitute "pages" or "page" or "folio" in the above.
Quoted 8 lines
Quoted 8 lines
While the
WARN_ON_ONCE()can trigger only because of the fast path in hole punching we have the pre-unmap without lock:if ((u64)unmap_end > (u64)unmap_start) unmap_mapping_range(mapping, unmap_start, 1 + unmap_end - unmap_start, 0); shmem_truncate_range(inode, offset, offset + len - 1);Oh, I should have read on, I've been wasting my time explaining above:
there you said the page must be locked, here you find that is not so.Quoted 1 line
Quoted 1 line
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
But that will be fixed up right afterwards in
shmem_truncate_range(), afaict, which is with-lock. Hence we have a small window to at least trigger the warning above, even if it won't really mess up the accounting finally.Well, there is still an opportunity to do the wrong thing.
So what I wanted to express is we can't trigger any real issue, so no way to write a valid reproducer even if we know it did it wrong.. And that's why I provided a "fake" one..
Let me explain a little bit further. Sorry I know you know most of below, so it could be that you spend 5 minutes reading nonsense.. but I want to 100% sure we're on the same page.
Firstly, current code will have a problem if we:
(1) Passed over a valid "
zap_details" pointer tozap_pte_range, and,
(2) At the meantime if there can be a swap entry of any kind of below:
(2.1) real swap entry
(2.2) migration swap entryThen we got doomed because we could have errornously skipped some swap entry that was also under the mapping while we shouldn't.
I didn't mention device exclusive/private entries are fine because they're checked before the "details continue" check.
Quoted 6 lines
I didn't mention hwpoison entry too; but I'll discuss it later..
Then, when do we have cases above (1) or (2)?
(1) is impossible because when there's
zap_detailsin current code it means the page is backing a fs mapping, hence not PageAnon. We can only hit (2).
No: a MAP_PRIVATE fs mapping has a PageAnon page wherever CopyOnWrite has been done.
Quoted 2 lines
What can hit (2)? I only know shmem...
No: I cannot see any special case for shmem versus file versus anon in try_to_migrate_one().
Quoted 11 lines
Next, as a conclusion, what we want to reproduce is: some caller calls
zap_pte_range()withzap_details, but accidentally there's a migration entry.It's fairly easy to find this, as what my "fake" reproducer was doing.
However, as I stated before, all these use cases always have another step to take the lock and redo the range. Then even if some migration entry got wrongly skipped it'll always be fixed. What we need to find is some caller that calls
zap_pte_range()without later taking the page lock and redo that. That's the only possibility to trigger a real issue on the shmem accounting.
I agree that the fallback "if (folio_mapped() unmap_mapping_folio()", while holding folio lock, ensures that there cannot be a migration entry substituted for present pte at that time, so no problem if migration entry was wrongly skipped on the earlier unlocked pass.
But you're forgetting the complementary mistake: that the earlier unlocked pass might have zapped a migration entry (corresponding to an anon COWed page) when it should have skipped it (while punching a hole).
Quoted 27 lines
To prove this, let's see all the possible call stacks...
zap_pte_range()will only be called withzap_detailspointer with below two functions:
unmap_mapping_folio
unmap_mapping_pages
unmap_mapping_foliois already having the page lock so it's not going to work. Then the callers ofunmap_mapping_pagesare:unmap_mapping_pages [1] collapse_file [?] unmap_mapping_range [?] unmap_shared_mapping_range [?] vmw_bo_dirty_unmap (gpu/drm) [?] inode_go_sync (gfs2) [1] shmem_setattr [1] shmem_fallocate [1] truncate_pagecache [1] truncate_pagecache_range [1] invalidate_inode_pages2_rangeIt's a reversed tree of call stacks, when I marked [1] means this call site does the "unmap then take page lock and redo" thing, so it can't reproduce.
Quoted 8 lines
The only outliers are:
[?] vmw_bo_dirty_unmap (gpu/drm) [?] inode_go_sync (gfs2)Which I marked with [?].
However I don't think they'll have migration entries at all, or am I wrong?
To be honest, I have not made the effort to understand your point there, since I already suggested above that three of your assumptions are wrong. But I think we should presume that the upper levels have chosen the right setting for even_cows, and zap_pte_range() just needs to respect it.
Quoted 53 lines
I hope I explained why I think we can't write a reproducer.. but I still think we should fix the problem, because we can't tell whether it'll go wrong in the future with a new caller who will not take the page lock to redo the zap.
Quoted 17 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1388
Quoted 11 lines
mm/memory.c +0
-4@@ -1382,16 +1382,14 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,continue ; } -/* If details->check_mapping, we leave swap entries. */ -if (unlikely (details)) -continue ; -Good.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1392
Quoted 2 lines
mm/memory.c +0
-0if (!non_swap_entry (entry)) rss[MM_SWAPENTS ]--;Quoted 9 lines
Right, I did not say at all what is wrong here, I suppose I intended to come back but then got tired - sorry. Here we are dealing with a genuine swap entry. No problem for exit or munmap or
MADV_DONTNEED. No problem for truncation (but apparently there has been an unnoticed problem, that swapped-out COWed pages beyond the truncation point were not being deleted, while that "details continue" was still there). But a problem for hole-punching: without the "details continue", this will go on to delete COWed pages which ought to be preserved - needs ashould_zap_page()check, as you do with the migration entry.I see, thanks for pointing out.
Quoted 4 lines
Except that here we have no page to check, so it looks like you'll have to change
should_zap_page()to deal with this case too, or just checkdetails->check_mappingdirectly.Yeah I prefer this, as we don't have the page* pointer anyway.
Quoted 11 lines
Which raises the question again
of why I did not just use a boolean flag there originally: aah, I think I've found why. In those days there was a horrible "optimization", for better performance on some benchmark I guess, which when you read from/dev/zerointo a private mapping, would map the zero page there (look upread_zero_pagealigned()andzeromap_page_range()if you dare). So there was another category of page to be skipped along with the anon COWs, and I didn't want multiple tests in the zap loop, so checkingcheck_mappingagainstpage->mappingdid both. I think nowadays you could do it by checking for PageAnon page (or genuine swap entry) instead.
Quoted 1 line
It must be PageAnon already, isn't it?
I did not understand what you were asking there; but in your followup mail, I think you came to understand what I meant better. Yes, I believe you could safely replace struct address_space *zap_mapping by a more understandable boolean (skip_cows? its inverse would be easier to understand, but we don't want almost everyone to have to pass a zap_details initialized to true there).
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 10 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1392
Quoted 8 lines
mm/memory.c +2
-0else if (is_migration_entry (entry)) {struct page *page; page =pfn_swap_entry_to_page (entry); +if (unlikely (zap_skip_check_mapping (details, page))) +continue ;Good; though I don't think it's worth an "unlikely", and I say again,
Sure, I'll drop this "unlikely". Meanwhile, shall we drop all the rest of the "unlikely" too around this check?
Quoted 5 lines
more forcefully, that "
zap_skip_check_mapping" is a terrible name.David suggested naming its inversion
should_zap_page(), yes, that's much better; I'd toyed withdo_not_zap_page()andzap_skip_page(), butshould_zap_page()for the inversion sounds good to me.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Ah sure, I'll rename it to
should_zap_page()in a new patch before this.Thanks; though now even that name is looking dubious -
should_zap_entry()?I guess my plan is simply keep
should_zap_page()and check explicitly for real swap entries.
I won't know whether that's right or not until seeing the actual patch. The important thing to keep in mind is that the treatment of a migration entry needs to match the nature of the page it is migrating:
if skip_cows, then the migration entry of a PageAnon should be skipped not zapped, just as a PageAnon or a real swap entry would be.
The fact that a migration entry is encoded much like a swap entry, and a real swap entry corresponds to a PageAnon, is misleading here.
Quoted 33 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 7 lines
And I'm pleased to see in one of Matthew's diffs that he intends to bring
zap_detailsandzap_skip_check_mapping()back intomm/memory.cfrominclude/linux/mm.h.Yeah it's only used in
memory.c. I put it there initially because I wantedzap_detailsuser can reference what's that check mapping is all about, but maybe that's not necessary.Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
If you or Matthew could provide a link to that patch, I'll be happy to pick that up too with this series. Or I can also do nothing assuming Matthew will handle it elsewhere.
In Linus's tree today:
3506659e18a6("mm: Addunmap_mapping_folio()").Rebased.
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1394
Quoted 5 lines
mm/memory.c +0
-0rss[mm_counter (page)]--; }I have given no thought as to whether more "else"s are needed there.
It's hwpoison that's in the else. Nothing else should.
I didn't mention it probably because I forgot. I did think about it when drafting, and IMHO we should simply zap that hwpoison entry because:
Quoted 8 lines
(1) Zap means the user knows this data is meaningless, so at least we shouldn't SIGBUS for that anymore.
(2) If we keep it there, it could errornously trigger SIGBUS later if the guest accessed that pte again somehow.
I plan to mention that in the commit message, but I can also add some comments directly into the code. Let me know your thoughts.
It's comes down, again, to what punching a hole in a file should do to the private data that has been COWed from it. Strictly, it should not interfere with it, even when the COWed page has become poisonous:
the entry should be left in to generate SIGBUS. Whereas ordinary unmapping or truncating or MADV_DONTNEEDing would zap it.
Quoted 11 lines
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 1395
Quoted 3 lines
mm/memory.c +0
-0if (unlikely (!free_swap_and_cache (entry)))(I think you moved that to a more relevant place in a later patch: good.)
I know you're also asking for test suggestions, but I'll leave it to you:
hole-punching, truncation, swapping, compaction (to force migration).Please read above - again I don't think we can write a reproducer, and that's why I provided the "fake" reproducer only.
I haven't studied your "fake" reproducer at all, I guess I'm not all that interested in fakes. An actual reproducer would be able to show that the current code (5.17-rc1 and recent) is zapping COWed pages in MAP_PRIVATE areas when a hole is punched in the corresponding file. Or, would it show that I've been wrong all along in what I've said on this?
(Yes, our repeated back and forth on this does make me wonder whether it would have been easier to define hole-punch as deleting COWs too:
but it's years too late to change our minds on that; and I think a closer examination might show that it's difficult to guarantee the deletion of COWs when punching a hole - traditionally, truncation has leant on i_size for consistent implementation, and hole-punching has been much trickier, because it does not change i_size at all.)
Hugh
Quoted 8 lines
I could always be missing something, though. I'd be glad to be corrected, again.
Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 6 lines · Hugh Dickins
Quoted 5 lines
However, as I stated before, all these use cases always have another step to take the lock and redo the range. Then even if some migration entry got wrongly skipped it'll always be fixed. What we need to find is some caller that calls
zap_pte_range()without later taking the page lock and redo that. That's the only possibility to trigger a real issue on the shmem accounting.
Quoted 8 lines
I agree that the fallback "if (
folio_mapped()unmap_mapping_folio()", while holding folio lock, ensures that there cannot be a migration entry substituted for present pte at that time, so no problem if migration entry was wrongly skipped on the earlier unlocked pass.But you're forgetting the complementary mistake: that the earlier unlocked pass might have zapped a migration entry (corresponding to an anon COWed page) when it should have skipped it (while punching a hole).
IMHO we won't wrongly zap a migration entry because when it's file backed we've got non-NULL zap_details, so we'll skip all migration entries. IOW, we can only wrongly skip some entries, not wrongly zap some.
But I get your point, and thanks for pointing out what I missed - I think I forgot the private mappings completely somehow when writting that up..
I have a quick idea on reproducer now (perhaps file size shrinking on private pages being swapped out), I'll try to write a real reproducer and update later.
[...]
Quoted 6 lines
I did not understand what you were asking there; but in your followup mail, I think you came to understand what I meant better. Yes, I believe you could safely replace struct
address_space*zap_mappingby a more understandable boolean (skip_cows? its inverse would be easier to understand, but we don't want almost everyone to have to pass azap_detailsinitialized to true there).
The only even_cows==true for zap_details is with unmap_mapping_range(), where its caller passed over even_cows==true as parameter. So IMHO that helper helped to construct the zap_details anyway.
I'll try it out starting with naming it zap_details.even_cows; I'll make it the last patch as a cleanup.
Quoted 16 lines
Quoted 16 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +0
-0rss[mm_counter(page)]--; }I have given no thought as to whether more "else"s are needed there.
It's hwpoison that's in the else. Nothing else should.
I didn't mention it probably because I forgot. I did think about it when drafting, and IMHO we should simply zap that hwpoison entry because:
(1) Zap means the user knows this data is meaningless, so at least we shouldn't SIGBUS for that anymore.
(2) If we keep it there, it could errornously trigger SIGBUS later if the guest accessed that pte again somehow.
Quoted 8 lines
Quoted 2 lines
I plan to mention that in the commit message, but I can also add some comments directly into the code. Let me know your thoughts.
It's comes down, again, to what punching a hole in a file should do to the private data that has been COWed from it. Strictly, it should not interfere with it, even when the COWed page has become poisonous:
the entry should be left in to generate SIGBUS. Whereas ordinary unmapping or truncating orMADV_DONTNEEDingwould zap it.
Makes sense, I'll take care of that in the new version too. Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 2 lines · Peter Xu
I have a quick idea on reproducer now (perhaps file size shrinking on private pages being swapped out), I'll try to write a real reproducer and update later.
Here's the reproducer..
===8<===
#define _GNU_SOURCE /* See feature_test_macros(7) */
#include <stdio.h>
#include <assert.h>
#include <unistd.h>
#include <sys/mman.h>
#include <sys/types.h>
int page_size;
int shmem_fd;
char *buffer;
void main(void)
{
int ret;
char val;
page_size = getpagesize();
shmem_fd = memfd_create("test", 0);
assert(shmem_fd >= 0);
ret = ftruncate(shmem_fd, page_size * 2);
assert(ret == 0);
buffer = mmap(NULL, page_size * 2, PROT_READ | PROT_WRITE,
MAP_PRIVATE, shmem_fd, 0);
assert(buffer != MAP_FAILED);
/* Write private page, swap it out */
buffer[page_size] = 1;
madvise(buffer, page_size * 2, MADV_PAGEOUT);
/* This should drop private buffer[page_size] already */
ret = ftruncate(shmem_fd, page_size);
assert(ret == 0);
/* Recover the size */
ret = ftruncate(shmem_fd, page_size * 2);
assert(ret == 0);
/* Re-read the data, it should be all zero */
val = buffer[page_size];
if (val == 0)
printf("Good\n");
else
printf("BUG\n");
}===8<===
I'm looking for a correct Fixes for the 1st patch, but afaict it seems not right even in the initial git commit, which traces back to the "history" git repo of your commit:
dd9fd0e03de6 ("[PATCH] rmap: nonlinear truncation", 2004-04-17)
Where we have:
/* * If details->check_mapping, we leave swap entries; * if details->nonlinear_vma, we leave file entries. */ if (unlikely(details)) continue; if (!pte_file(ptent)) free_swap_and_cache(pte_to_swp_entry(ptent));
As I don't know how it could be right if the user only specified details->nonlinear_vma but keeping details->check_mapping==NULL.. there, and there're definitely callers for that, e.g.:
static long madvise_dontneed(struct vm_area_struct * vma,
unsigned long start, unsigned long end)
{
if ((vma->vm_flags & VM_LOCKED) || is_vm_hugetlb_page(vma))
return -EINVAL;
if (unlikely(vma->vm_flags & VM_NONLINEAR)) {
struct zap_details details = {
.nonlinear_vma = vma,
.last_index = ULONG_MAX,
};
zap_page_range(vma, start, end - start, &details);
} else
zap_page_range(vma, start, end - start, NULL);
return 0;
}So I plan to add the Fixes to the initial git commit, 1da177e4c3f4 ("Linux-2.6.12-rc2", 2005-04-16).
Does it sound right to you?
Thanks,
--
Peter Xu
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
In reply to Peter XuQuoted 9 lines · Peter Xu
This check existed since the 1st git commit of Linux repository, but at that time there's no page migration yet so I think it's okay.
With page migration enabled, it should logically be possible that we zap some shmem pages during migration. When that happens, IIUC the old code could have the RSS counter accounted wrong on
MM_SHMEMPAGESbecause we will zap the ptes without decreasing the counters for the migrating entries. I have no unit test to prove it as I don't know an easy way to trigger this condition, though.
Quoted 4 lines
Besides, the optimization itself is already confusing IMHO to me in a few points:
- The wording "skip swap entries" is confusing, because we're not skipping all swap entries - we handle device private/exclusive pages before that.
I think one part of the confusion is "swap vs non-swap" entries. For !pte_none() && !pte_present() we can have
* swap entry
* non-swap entry
** device exclusive entry
** device private entry
** HWpoison entry
** migration entry
So the comment claims to skip "swap entries" but also skips HWpoison and migration entries, and I think that's the confusing part. Both only apply to PageAnon().
IIUC, the only way we could get details != NULL is via unmap_mapping_page()+unmap_mapping_pages().
I do wonder if any of the callers really cares about PageAnon() pages where this would be relevant.
Am I wrong or is unmap_mapping_pages() never called with "even_cows == true" and we can remove that paremeter:
git grep -C2 unmap_mapping_pagesfs/afs/callback.c- struct afs_vnode *vnode = container_of(work, struct afs_vnode, cb_work);fs/afs/callback.c-fs/afs/callback.c: unmap_mapping_pages(vnode->vfs_inode.i_mapping, 0, 0, false);fs/afs/callback.c-}fs/afs/callback.c-
--fs/dax.c- if (dax_is_zero_entry(entry)) {fs/dax.c- xas_unlock_irq(xas);fs/dax.c: unmap_mapping_pages(mapping,fs/dax.c- xas->xa_index & ~PG_PMD_COLOUR,fs/dax.c- PG_PMD_NR, false);
--
fs/dax.c- * get_user_pages() slow path. The slow path is protected by fs/dax.c- * pte_lock() and pmd_lock(). New references are not taken without fs/dax.c: * holding those locks, and unmap_mapping_pages() will not zero the fs/dax.c- * pte or pmd without holding the respective lock, so we are fs/dax.c- * guaranteed to either see new references or prevent new fs/dax.c- * references from being established. fs/dax.c- */
fs/dax.c: unmap_mapping_pages(mapping, start_idx, end_idx - start_idx + 1, 0);fs/dax.c-fs/dax.c- xas_lock_irq(&xas);
--fs/dax.c- /* we are replacing a zero page with block mapping */fs/dax.c- if (dax_is_pmd_entry(entry))fs/dax.c: unmap_mapping_pages(mapping, index & ~PG_PMD_COLOUR,fs/dax.c- PG_PMD_NR, false);fs/dax.c- else /* pte entry */fs/dax.c: unmap_mapping_pages(mapping, index, 1, false);fs/dax.c- }fs/dax.c-
--include/linux/mm.h- bool *unlocked);include/linux/mm.h-void unmap_mapping_page(struct page *page); include/linux/mm.h:void unmap_mapping_pages(struct address_space *mapping,include/linux/mm.h- pgoff_t start, pgoff_t nr, bool even_cows);include/linux/mm.h-void unmap_mapping_range(struct address_space *mapping, --include/linux/mm.h-}include/linux/mm.h-static inline void unmap_mapping_page(struct page *page) { } include/linux/mm.h:static inline void unmap_mapping_pages(struct address_space *mapping,include/linux/mm.h- pgoff_t start, pgoff_t nr, bool even_cows) { }include/linux/mm.h-static inline void unmap_mapping_range(struct address_space *mapping, --mm/khugepaged.c-mm/khugepaged.c- if (page_mapped(page))mm/khugepaged.c: unmap_mapping_pages(mapping, index, 1, false);mm/khugepaged.c-mm/khugepaged.c- xas_lock_irq(&xas);
--mm/memory.c- * Unmap this page from any userspace process which still has it mmaped. mm/memory.c- * Typically, for efficiency, the range of nearby pages has already been mm/memory.c: * unmapped by unmap_mapping_pages() or unmap_mapping_range(). But once mm/memory.c- * truncation or invalidation holds the lock on a page, it may find that mm/memory.c- * the page has been remapped again: and then uses unmap_mapping_page() --mm/memory.c-mm/memory.c-/**mm/memory.c: * unmap_mapping_pages() - Unmap pages from processes. mm/memory.c- * @mapping: The address space containing pages to be unmapped. mm/memory.c- * @start: Index of first page to be unmapped.
--mm/memory.c- * cache.mm/memory.c- */
mm/memory.c:void unmap_mapping_pages(struct address_space *mapping, pgoff_t start,mm/memory.c- pgoff_t nr, bool even_cows)mm/memory.c-{
--mm/memory.c- i_mmap_unlock_write(mapping);mm/memory.c-}
mm/memory.c:EXPORT_SYMBOL_GPL(unmap_mapping_pages);mm/memory.c-mm/memory.c-/**
--mm/memory.c- }mm/memory.c-mm/memory.c: unmap_mapping_pages(mapping, hba, hlen, even_cows);mm/memory.c-}
mm/memory.c-EXPORT_SYMBOL(unmap_mapping_range);
--mm/truncate.c- * zap the rest of the file in one hit.mm/truncate.c- */mm/truncate.c: unmap_mapping_pages(mapping, index,mm/truncate.c- (1 + end - index), false);mm/truncate.c- did_range_unmap = 1;
--mm/truncate.c- */mm/truncate.c- if (dax_mapping(mapping)) {mm/truncate.c: unmap_mapping_pages(mapping, start, end - start + 1, false);mm/truncate.c- }mm/truncate.c-out:
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 25 lines · AEDT David Hildenbrand
Quoted 9 lines · Peter Xu
This check existed since the 1st git commit of Linux repository, but at that time there's no page migration yet so I think it's okay.
With page migration enabled, it should logically be possible that we zap some shmem pages during migration. When that happens, IIUC the old code could have the RSS counter accounted wrong on
MM_SHMEMPAGESbecause we will zap the ptes without decreasing the counters for the migrating entries. I have no unit test to prove it as I don't know an easy way to trigger this condition, though.Quoted 4 lines
Besides, the optimization itself is already confusing IMHO to me in a few points:
- The wording "skip swap entries" is confusing, because we're not skipping all swap entries - we handle device private/exclusive pages before that.
I think one part of the confusion is "swap vs non-swap" entries. For !pte_none() && !pte_present() we can have
* swap entry
* non-swap entry
** device exclusive entry
** device private entry
** HWpoison entry
** migration entry
Quoted 3 lines
So the comment claims to skip "swap entries" but also skips HWpoison and migration entries, and I think that's the confusing part. Both only apply to
PageAnon().
I must be missing something but why do these only apply to PageAnon()?
Quoted 6 lines
IIUC, the only way we could get details != NULL is via unmap_mapping_page()+unmap_mapping_pages().
I do wonder if any of the callers really cares about
PageAnon()pages where this would be relevant.Am I wrong or is
unmap_mapping_pages()never called with "even_cows== true" and we can remove that paremeter:
Except that unmap_mapping_range() takes even_cows as a parameter and passes that to unmap_mapping_pages(), and from what I can tell there are callers of unmap_mapping_range() that set even_cows = true.
Quoted 95 lines
git grep -C2
unmap_mapping_pagesfs/afs/callback.c-structafs_vnode*vnode =container_of(work, struct afs_vnode, cb_work);fs/afs/callback.c-fs/afs/callback.c:unmap_mapping_pages(vnode->vfs_inode.i_mapping, 0, 0, false);fs/afs/callback.c-}fs/afs/callback.c-
--fs/dax.c-if (dax_is_zero_entry(entry)) {fs/dax.c-xas_unlock_irq(xas);fs/dax.c: unmap_mapping_pages(mapping,fs/dax.c-xas->xa_index& ~PG_PMD_COLOUR,fs/dax.c-PG_PMD_NR, false);
--fs/dax.c- * get_user_pages() slow path. The slow path is protected by fs/dax.c- * pte_lock() and pmd_lock(). New references are not taken without fs/dax.c: * holding those locks, and unmap_mapping_pages() will not zero the fs/dax.c- * pte or pmd without holding the respective lock, so we are fs/dax.c- * guaranteed to either see new references or prevent new fs/dax.c- * references from being established. fs/dax.c- */
fs/dax.c:unmap_mapping_pages(mapping, start_idx, end_idx - start_idx + 1, 0);fs/dax.c-fs/dax.c-xas_lock_irq(&xas);
--fs/dax.c-/* we are replacing a zero page with block mapping */fs/dax.c-if (dax_is_pmd_entry(entry))fs/dax.c: unmap_mapping_pages(mapping, index & ~PG_PMD_COLOUR,fs/dax.c-PG_PMD_NR, false);fs/dax.c-else /* pte entry */fs/dax.c:unmap_mapping_pages(mapping, index, 1, false);fs/dax.c-}fs/dax.c-
--include/linux/mm.h-bool *unlocked);include/linux/mm.h-voidunmap_mapping_page(struct page *page); include/linux/mm.h:void unmap_mapping_pages(structaddress_space*mapping,include/linux/mm.h-pgoff_tstart,pgoff_tnr, booleven_cows);include/linux/mm.h-voidunmap_mapping_range(structaddress_space*mapping, --include/linux/mm.h-}include/linux/mm.h-staticinline voidunmap_mapping_page(struct page *page){ } include/linux/mm.h:static inline void unmap_mapping_pages(structaddress_space*mapping,include/linux/mm.h-pgoff_tstart,pgoff_tnr, booleven_cows) { }include/linux/mm.h-staticinline void unmap_mapping_range(structaddress_space*mapping, --mm/khugepaged.c-mm/khugepaged.c-if (page_mapped(page))mm/khugepaged.c:unmap_mapping_pages(mapping, index, 1, false);mm/khugepaged.c-mm/khugepaged.c-xas_lock_irq(&xas);
--mm/memory.c-* Unmap this page from any userspace process which still has it mmaped.mm/memory.c-* Typically, for efficiency, the range of nearby pages has already beenmm/memory.c: * unmapped byunmap_mapping_pages()orunmap_mapping_range(). But oncemm/memory.c-* truncation or invalidation holds the lock on a page, it may find thatmm/memory.c-* the page has been remapped again: and then usesunmap_mapping_page()--mm/memory.c-mm/memory.c-/**mm/memory.c: *unmap_mapping_pages()- Unmap pages from processes.mm/memory.c-* @mapping: The address space containing pages to be unmapped.mm/memory.c-* @start: Index of first page to be unmapped.
--mm/memory.c-* cache.mm/memory.c-*/
mm/memory.c:void unmap_mapping_pages(structaddress_space*mapping,pgoff_tstart,mm/memory.c-pgoff_tnr, booleven_cows)mm/memory.c-{
--mm/memory.c-i_mmap_unlock_write(mapping);mm/memory.c-}
mm/memory.c:EXPORT_SYMBOL_GPL(unmap_mapping_pages);mm/memory.c-mm/memory.c-/**
--mm/memory.c-}mm/memory.c-mm/memory.c:unmap_mapping_pages(mapping, hba, hlen, even_cows);mm/memory.c-}
mm/memory.c-EXPORT_SYMBOL(unmap_mapping_range);
--mm/truncate.c-* zap the rest of the file in one hit.mm/truncate.c-*/mm/truncate.c: unmap_mapping_pages(mapping, index,mm/truncate.c-(1 + end - index), false);mm/truncate.c-did_range_unmap= 1;
--mm/truncate.c-*/mm/truncate.c-if (dax_mapping(mapping)) {mm/truncate.c:unmap_mapping_pages(mapping, start, end - start + 1, false);mm/truncate.c-}mm/truncate.c-out:
Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified
Quoted 26 lines · Alistair Popple
Quoted 25 lines · AEDT David Hildenbrand
Quoted 9 lines · Peter Xu
This check existed since the 1st git commit of Linux repository, but at that time there's no page migration yet so I think it's okay.
With page migration enabled, it should logically be possible that we zap some shmem pages during migration. When that happens, IIUC the old code could have the RSS counter accounted wrong on
MM_SHMEMPAGESbecause we will zap the ptes without decreasing the counters for the migrating entries. I have no unit test to prove it as I don't know an easy way to trigger this condition, though.Quoted 4 lines
Besides, the optimization itself is already confusing IMHO to me in a few points:
- The wording "skip swap entries" is confusing, because we're not skipping all swap entries - we handle device private/exclusive pages before that.
I think one part of the confusion is "swap vs non-swap" entries. For !pte_none() && !pte_present() we can have
* swap entry
* non-swap entry
** device exclusive entry
** device private entry
** HWpoison entry
** migration entry
Quoted 5 lines
Quoted 3 lines
So the comment claims to skip "swap entries" but also skips HWpoison and migration entries, and I think that's the confusing part. Both only apply to
PageAnon().I must be missing something but why do these only apply to
PageAnon()?
My memory might be wrong. I remember that for PageAnon() we need migration/hwpoison entries because there is no way we could refault the page from a mapping once we zap the entry. For everything else, we could zap and refault. But looks like we indeed also use migration/hwpoison entries for pages with a mapping, although it might not be strictly required.
Quoted 11 lines
Quoted 7 lines
IIUC, the only way we could get details != NULL is via unmap_mapping_page()+unmap_mapping_pages().
I do wonder if any of the callers really cares about
PageAnon()pages where this would be relevant.Am I wrong or is
unmap_mapping_pages()never called with "even_cows== true" and we can remove that paremeter:Except that
unmap_mapping_range()takeseven_cowsas a parameter and passes that tounmap_mapping_pages(), and from what I can tell there are callers ofunmap_mapping_range()that seteven_cows = true.
You're right.
Signature · 3 lines
Thanks, David / dhildenb
RFC PATCH v2 2/2 mm: Rework swap handling of zap_pte_range
In reply to Peter XuClean the code up by merging the device private/exclusive swap entry handling with the rest, then we merge the pte clear operation too.
struct* page is defined in multiple places in the function, move it upward.
free_swap_and_cache() is only useful for !non_swap_entry() case, put it into the condition.
No functional change intended.
--- mm/memory.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-)
mm/memory.c +8 -17
@@ -1326,6 +1326,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,arch_enter_lazy_mmu_mode (); do { pte_t ptent = *pte; + struct page *page; + if (pte_none(ptent)) continue;
@@ -1333,8 +1335,6 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,break ;if (pte_present (ptent)) { -struct page *page; - page =vm_normal_page (vma, addr, ptent);if (unlikely (zap_skip_check_mapping (details, page)))continue ;
@@ -1368,32 +1368,23 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, entry =pte_to_swp_entry (ptent);if (is_device_private_entry (entry) ||is_device_exclusive_entry (entry)) { -struct page *page =pfn_swap_entry_to_page (entry); - + page =pfn_swap_entry_to_page (entry);if (unlikely (zap_skip_check_mapping (details, page)))continue ; -pte_clear_not_present_full (mm, addr, pte, tlb->fullmm); rss[mm_counter (page)]--; -if (is_device_private_entry (entry))page_remove_rmap (page, false); -put_page (page); -continue ; - } - -if (!non_swap_entry (entry)) - rss[MM_SWAPENTS ]--; -else if (is_migration_entry (entry)) { -struct page *page; - + }else if (is_migration_entry (entry)) { page =pfn_swap_entry_to_page (entry);if (unlikely (zap_skip_check_mapping (details, page)))continue ; rss[mm_counter (page)]--; + }else if (!non_swap_entry (entry)) { + rss[MM_SWAPENTS ]--; +if (unlikely (!free_swap_and_cache (entry))) +print_bad_pte (vma, addr, ptent,NULL ); } -if (unlikely (!free_swap_and_cache (entry))) -print_bad_pte (vma, addr, ptent,NULL );pte_clear_not_present_full (mm, addr, pte, tlb->fullmm); }while (pte++, addr +=PAGE_SIZE , addr != end);
Signature · 1 line
2.32.0
Re: [PATCH RFC v2 2/2] mm: Rework swap handling of zap_pte_range
Quoted 4 lines · Peter Xu
Clean the code up by merging the device private/exclusive swap entry handling with the rest, then we merge the pte clear operation too.
struct* page is defined in multiple places in the function, move it upward.
Is that actually a good thing? There was a time when declaring variables more locally helped compilers with liveness analysis and register allocation. Compilers are probably smarter now.
Re: [PATCH RFC v2 2/2] mm: Rework swap handling of zap_pte_range
Quoted 10 lines · Matthew Wilcox
Quoted 4 lines · Peter Xu
Clean the code up by merging the device private/exclusive swap entry handling with the rest, then we merge the pte clear operation too.
struct* page is defined in multiple places in the function, move it upward.
Is that actually a good thing? There was a time when declaring variables more locally helped compilers with liveness analysis and register allocation. Compilers are probably smarter now.
I see, I don't know the history of that, but I did give it a shot with a patch that recovered all the "struct page*" back to the origin, I found that it'll generated exactly the same assembly of unmap_page_range (actually, the whole mm/memory.o) no matter what.
I only tested on an aarch64 system, with below gcc version:
$ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/libexec/gcc/aarch64-redhat-linux/11/lto-wrapper Target: aarch64-redhat-linux Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl=/builddir/build/BUILD/gcc-11.0.1-20210324/obj-aarch64-redhat-linux/isl-install --enable-gnu-indirect-function --build=aarch64-redhat-linux Thread model: posix Supported LTO compression algorithms: zlib zstd gcc version 11.0.1 20210324 (Red Hat 11.0.1-0) (GCC)
Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH RFC v2 2/2] mm: Rework swap handling of zap_pte_range
In reply to Matthew WilcoxQuoted 7 lines · Matthew Wilcox
Quoted 4 lines · Peter Xu
Clean the code up by merging the device private/exclusive swap entry handling with the rest, then we merge the pte clear operation too.
struct* page is defined in multiple places in the function, move it upward.
Is that actually a good thing? There was a time when declaring
Yes. It is a very good thing. Having multiple cases of shadowed variables (in this case I'm using programming language terminology, or what I remember it as, anyway) provides lots of opportunities to create hard-to-spot bugs.
Quoted 3 lines
variables more locally helped compilers with liveness analysis and register allocation. Compilers are probably smarter now.
...as long as the above checks out, and I see from Peter's response that we're OK.
thanks,
Signature · 2 lines
John Hubbard NVIDIA
Re: [PATCH RFC v2 2/2] mm: Rework swap handling of zap_pte_range
Quoted 5 lines · John Hubbard
Quoted 4 lines · Matthew Wilcox
Quoted 3 lines · Peter Xu
Clean the code up by merging the device private/exclusive swap entry handling with the rest, then we merge the pte clear operation too.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
struct* page is defined in multiple places in the function, move it upward.
Is that actually a good thing? There was a time when declaring
Yes. It is a very good thing. Having multiple cases of shadowed variables (in this case I'm using programming language terminology, or what I remember it as, anyway) provides lots of opportunities to create hard-to-spot bugs.
I think you're misremembering. These are shadowed variables:
int a;
int b(void)
{
int a;
if (c) {
int a;
}
}This is not:
int b(void)
{
if (c) {
int a;
} else {
int a;
}
}I really wish we could turn on -Wshadow, but we get compilation warnings from header files right now. Or we did last time I checked.
Re: [PATCH RFC v2 2/2] mm: Rework swap handling of zap_pte_range
Quoted 8 lines · Matthew Wilcox
Quoted 7 lines · John Hubbard
Quoted 6 lines · Matthew Wilcox
Quoted 4 lines · Peter Xu
Clean the code up by merging the device private/exclusive swap entry handling with the rest, then we merge the pte clear operation too.
struct* page is defined in multiple places in the function, move it upward.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Is that actually a good thing? There was a time when declaring
Yes. It is a very good thing. Having multiple cases of shadowed variables (in this case I'm using programming language terminology, or what I remember it as, anyway) provides lots of opportunities to create hard-to-spot bugs.
I think you're misremembering. These are shadowed variables:
OK, I remembered correctly, but read the diffs a little too quickly, and...
Quoted 11 lines
int a;
int b(void)
{ int a; if (c) { int a; } }
Quoted 4 lines
This is not:
int b(void)
{
...missed that there is no longer a "int a" at the top level. But it does still present a small land mine, in that just adding a top level "int a" creates all these shadowed variables (not necessarily bugs, yet, I know).
It's less of an issue here, then I first thought. Generally, it's probably best to either use "int a" throughout, or differently named variables at lower levels...or make smaller functions. Because if a variable name is reused a lot in the same function then there is likely a relationship of sorts between the instances, and it's worth deciding what that is.
Quoted 10 lines
if (c) { int a; } else { int a; } }I really wish we could turn on -Wshadow, but we get compilation warnings from header files right now. Or we did last time I checked.
...and as you say, it would be nice if the programmer could just let the compiler figure out if there is a real problem. The elaborate rituals to stay out of harm's way are not as good as a tool that checks. :)
thanks,
Signature · 2 lines
John Hubbard NVIDIA