Re: Unmap page in uvm_anon_release()
On 10/09/22(Sat) 15:12, Mark Kettenis wrote: > > Date: Sat, 10 Sep 2022 14:18:02 +0200 > > From: Martin Pieuchot > > > > Diff below fixes a bug exposed when swapping on arm64. When an anon is > > released make sure the all the pmap references to the related page are > > removed. > > I'm a little bit puzzled by this. So these pages are still mapped > even though there are no references to the anon anymore? I don't know. I just realised that all the code paths leading to uvm_pagefree() get rid of the pmap references by calling page_protect() except a couple of them in the aiodone daemon and the clustering code in the pager. This can't hurt and make the existing code coherent. Maybe it just hides the bug, I don't know.
Re: Unmap page in uvm_anon_release()
> Date: Sat, 10 Sep 2022 14:18:02 +0200
> From: Martin Pieuchot
>
> Diff below fixes a bug exposed when swapping on arm64. When an anon is
> released make sure the all the pmap references to the related page are
> removed.
I'm a little bit puzzled by this. So these pages are still mapped
even though there are no references to the anon anymore?
> We could move the pmap_page_protect(pg, PROT_NONE) inside uvm_pagefree()
> to avoid future issue but that's for a later refactoring.
I don't think that makes sense. In cases where pages are explicitly
mapped (instead of faulted in) we call pmap_remove() before we end up
calling uvm_pagefree(). Calling pmap_page_protect() in that case
doesn't make sense.
> With this diff I can no longer reproduce the SIGBUS issue on the
> rockpro64 and swapping is stable as long as I/O from sdmmc(4) work.
>
> This should be good enough to commit the diff that got reverted, but I'll
> wait to be sure there's no regression.
>
> ok?
>
> Index: uvm/uvm_anon.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_anon.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 uvm_anon.c
> --- uvm/uvm_anon.c26 Mar 2021 13:40:05 - 1.54
> +++ uvm/uvm_anon.c10 Sep 2022 12:10:34 -
> @@ -255,6 +255,7 @@ uvm_anon_release(struct vm_anon *anon)
> KASSERT(anon->an_ref == 0);
>
> uvm_lock_pageq();
> + pmap_page_protect(pg, PROT_NONE);
> uvm_pagefree(pg);
> uvm_unlock_pageq();
> KASSERT(anon->an_page == NULL);
> Index: uvm/uvm_fault.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
> retrieving revision 1.132
> diff -u -p -r1.132 uvm_fault.c
> --- uvm/uvm_fault.c 31 Aug 2022 01:27:04 - 1.132
> +++ uvm/uvm_fault.c 10 Sep 2022 12:10:34 -
> @@ -396,7 +396,6 @@ uvmfault_anonget(struct uvm_faultinfo *u
>* anon and try again.
>*/
> if (pg->pg_flags & PG_RELEASED) {
> - pmap_page_protect(pg, PROT_NONE);
> KASSERT(anon->an_ref == 0);
> /*
>* Released while we had unlocked amap.
>
>
Unmap page in uvm_anon_release()
Diff below fixes a bug exposed when swapping on arm64. When an anon is
released make sure the all the pmap references to the related page are
removed.
We could move the pmap_page_protect(pg, PROT_NONE) inside uvm_pagefree()
to avoid future issue but that's for a later refactoring.
With this diff I can no longer reproduce the SIGBUS issue on the
rockpro64 and swapping is stable as long as I/O from sdmmc(4) work.
This should be good enough to commit the diff that got reverted, but I'll
wait to be sure there's no regression.
ok?
Index: uvm/uvm_anon.c
===
RCS file: /cvs/src/sys/uvm/uvm_anon.c,v
retrieving revision 1.54
diff -u -p -r1.54 uvm_anon.c
--- uvm/uvm_anon.c 26 Mar 2021 13:40:05 - 1.54
+++ uvm/uvm_anon.c 10 Sep 2022 12:10:34 -
@@ -255,6 +255,7 @@ uvm_anon_release(struct vm_anon *anon)
KASSERT(anon->an_ref == 0);
uvm_lock_pageq();
+ pmap_page_protect(pg, PROT_NONE);
uvm_pagefree(pg);
uvm_unlock_pageq();
KASSERT(anon->an_page == NULL);
Index: uvm/uvm_fault.c
===
RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
retrieving revision 1.132
diff -u -p -r1.132 uvm_fault.c
--- uvm/uvm_fault.c 31 Aug 2022 01:27:04 - 1.132
+++ uvm/uvm_fault.c 10 Sep 2022 12:10:34 -
@@ -396,7 +396,6 @@ uvmfault_anonget(struct uvm_faultinfo *u
* anon and try again.
*/
if (pg->pg_flags & PG_RELEASED) {
- pmap_page_protect(pg, PROT_NONE);
KASSERT(anon->an_ref == 0);
/*
* Released while we had unlocked amap.
