| Message ID | 20191106154105.15176-1-catalin.marinas@arm.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 6767df245f4736d0cf0c6fb7cf9cf94b27414245 |
| Headers | show |
| Series | arm64: Do not mask out PTE_RDONLY in pte_same() | expand |
On Wed, Nov 06, 2019 at 03:41:05PM +0000, Catalin Marinas wrote: > Following commit 73e86cb03cf2 ("arm64: Move PTE_RDONLY bit handling out > of set_pte_at()"), the PTE_RDONLY bit is no longer managed by > set_pte_at() but built into the PAGE_* attribute definitions. > Consequently, pte_same() must include this bit when checking two PTEs > for equality. > > Remove the arm64-specific pte_same() function, practically reverting > commit 747a70e60b72 ("arm64: Fix copy-on-write referencing in HugeTLB") > > Fixes: 73e86cb03cf2 ("arm64: Move PTE_RDONLY bit handling out of set_pte_at()") > Cc: <stable@vger.kernel.org> # 4.14.x- > Cc: Will Deacon <will@kernel.org> > Cc: Steve Capper <steve.capper@arm.com> > Reported-by: John Stultz <john.stultz@linaro.org> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > --- > > Steve, > > Could you please check that the original problem fixed by commit > 747a70e60b72 no longer exists after reverting it in 4.14 onwards? In the meantime, I've thrown this at the CI to check that they come back clean. Will
On Wed, Nov 06, 2019 at 03:41:05PM +0000, Catalin Marinas wrote: > Following commit 73e86cb03cf2 ("arm64: Move PTE_RDONLY bit handling out > of set_pte_at()"), the PTE_RDONLY bit is no longer managed by > set_pte_at() but built into the PAGE_* attribute definitions. > Consequently, pte_same() must include this bit when checking two PTEs > for equality. > > Remove the arm64-specific pte_same() function, practically reverting > commit 747a70e60b72 ("arm64: Fix copy-on-write referencing in HugeTLB") > > Fixes: 73e86cb03cf2 ("arm64: Move PTE_RDONLY bit handling out of set_pte_at()") > Cc: <stable@vger.kernel.org> # 4.14.x- > Cc: Will Deacon <will@kernel.org> > Cc: Steve Capper <steve.capper@arm.com> > Reported-by: John Stultz <john.stultz@linaro.org> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > --- > > Steve, > > Could you please check that the original problem fixed by commit > 747a70e60b72 no longer exists after reverting it in 4.14 onwards? In the meantime, I've pushed this out to for-next/fixes since the CI came back clean and it fixes John's issue (which I've confirmed locally too). Interestingly, I'm not at all sure the problem is related to the Mali driver. Some tracing suggests that the ART JIT thread is stuck on a write fault, which could be explained by a broken pte_same(). Steve -- if you could please run the libhugetlbfs test suite as described in 747a70e60b72 before tomorrow, that would be really great. Thanks, Will
On Thu, Nov 07, 2019 at 11:48:26AM +0000, Will Deacon wrote: > On Wed, Nov 06, 2019 at 03:41:05PM +0000, Catalin Marinas wrote: > > Following commit 73e86cb03cf2 ("arm64: Move PTE_RDONLY bit handling out > > of set_pte_at()"), the PTE_RDONLY bit is no longer managed by > > set_pte_at() but built into the PAGE_* attribute definitions. > > Consequently, pte_same() must include this bit when checking two PTEs > > for equality. > > > > Remove the arm64-specific pte_same() function, practically reverting > > commit 747a70e60b72 ("arm64: Fix copy-on-write referencing in HugeTLB") > > > > Fixes: 73e86cb03cf2 ("arm64: Move PTE_RDONLY bit handling out of set_pte_at()") > > Cc: <stable@vger.kernel.org> # 4.14.x- > > Cc: Will Deacon <will@kernel.org> > > Cc: Steve Capper <steve.capper@arm.com> > > Reported-by: John Stultz <john.stultz@linaro.org> > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > --- > > > > Steve, > > > > Could you please check that the original problem fixed by commit > > 747a70e60b72 no longer exists after reverting it in 4.14 onwards? > > In the meantime, I've pushed this out to for-next/fixes since the CI came > back clean and it fixes John's issue (which I've confirmed locally too). > Interestingly, I'm not at all sure the problem is related to the Mali > driver. Some tracing suggests that the ART JIT thread is stuck on a write > fault, which could be explained by a broken pte_same(). > > Steve -- if you could please run the libhugetlbfs test suite as described > in 747a70e60b72 before tomorrow, that would be really great. > Hey Will, Catalin, Apologies for my late reply, I've been travelling longer than originally planned. I have tested for-next/fixes (with the pte_same removed), under libhugetlbfs and I have been unable to reproduce the original memory leak that prompted the pte_same logic in the first place. So this patch looks good to me. Will check this out for 4.14 too now. Cheers, -- Steve IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Fri, Nov 08, 2019 at 02:08:56AM +0000, Steve Capper wrote: > On Thu, Nov 07, 2019 at 11:48:26AM +0000, Will Deacon wrote: > > On Wed, Nov 06, 2019 at 03:41:05PM +0000, Catalin Marinas wrote: > > > Following commit 73e86cb03cf2 ("arm64: Move PTE_RDONLY bit handling out > > > of set_pte_at()"), the PTE_RDONLY bit is no longer managed by > > > set_pte_at() but built into the PAGE_* attribute definitions. > > > Consequently, pte_same() must include this bit when checking two PTEs > > > for equality. > > > > > > Remove the arm64-specific pte_same() function, practically reverting > > > commit 747a70e60b72 ("arm64: Fix copy-on-write referencing in HugeTLB") > > > > > > Fixes: 73e86cb03cf2 ("arm64: Move PTE_RDONLY bit handling out of set_pte_at()") > > > Cc: <stable@vger.kernel.org> # 4.14.x- > > > Cc: Will Deacon <will@kernel.org> > > > Cc: Steve Capper <steve.capper@arm.com> > > > Reported-by: John Stultz <john.stultz@linaro.org> > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > > --- > > > > > > Steve, > > > > > > Could you please check that the original problem fixed by commit > > > 747a70e60b72 no longer exists after reverting it in 4.14 onwards? > > > > In the meantime, I've pushed this out to for-next/fixes since the CI came > > back clean and it fixes John's issue (which I've confirmed locally too). > > Interestingly, I'm not at all sure the problem is related to the Mali > > driver. Some tracing suggests that the ART JIT thread is stuck on a write > > fault, which could be explained by a broken pte_same(). > > > > Steve -- if you could please run the libhugetlbfs test suite as described > > in 747a70e60b72 before tomorrow, that would be really great. > > > > Hey Will, Catalin, > Apologies for my late reply, I've been travelling longer than originally > planned. > > I have tested for-next/fixes (with the pte_same removed), under > libhugetlbfs and I have been unable to reproduce the original memory > leak that prompted the pte_same logic in the first place. So this patch > looks good to me. > > Will check this out for 4.14 too now. > I can confirm that 4.14 with Catalin's patch cherry-picked also passes the hugetlbfs cow tests. Cheers,
On Fri, Nov 08, 2019 at 02:37:04AM +0000, Steve Capper wrote: > On Fri, Nov 08, 2019 at 02:08:56AM +0000, Steve Capper wrote: > > On Thu, Nov 07, 2019 at 11:48:26AM +0000, Will Deacon wrote: > > > On Wed, Nov 06, 2019 at 03:41:05PM +0000, Catalin Marinas wrote: > > > > Following commit 73e86cb03cf2 ("arm64: Move PTE_RDONLY bit handling out > > > > of set_pte_at()"), the PTE_RDONLY bit is no longer managed by > > > > set_pte_at() but built into the PAGE_* attribute definitions. > > > > Consequently, pte_same() must include this bit when checking two PTEs > > > > for equality. > > > > > > > > Remove the arm64-specific pte_same() function, practically reverting > > > > commit 747a70e60b72 ("arm64: Fix copy-on-write referencing in HugeTLB") > > > > > > > > Fixes: 73e86cb03cf2 ("arm64: Move PTE_RDONLY bit handling out of set_pte_at()") > > > > Cc: <stable@vger.kernel.org> # 4.14.x- > > > > Cc: Will Deacon <will@kernel.org> > > > > Cc: Steve Capper <steve.capper@arm.com> > > > > Reported-by: John Stultz <john.stultz@linaro.org> > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > > > --- > > > > > > > > Steve, > > > > > > > > Could you please check that the original problem fixed by commit > > > > 747a70e60b72 no longer exists after reverting it in 4.14 onwards? > > > > > > In the meantime, I've pushed this out to for-next/fixes since the CI came > > > back clean and it fixes John's issue (which I've confirmed locally too). > > > Interestingly, I'm not at all sure the problem is related to the Mali > > > driver. Some tracing suggests that the ART JIT thread is stuck on a write > > > fault, which could be explained by a broken pte_same(). > > > > > > Steve -- if you could please run the libhugetlbfs test suite as described > > > in 747a70e60b72 before tomorrow, that would be really great. > > > > > > > Hey Will, Catalin, > > Apologies for my late reply, I've been travelling longer than originally > > planned. > > > > I have tested for-next/fixes (with the pte_same removed), under > > libhugetlbfs and I have been unable to reproduce the original memory > > leak that prompted the pte_same logic in the first place. So this patch > > looks good to me. > > > > Will check this out for 4.14 too now. > > I can confirm that 4.14 with Catalin's patch cherry-picked also passes > the hugetlbfs cow tests. Great. Thanks for testing Steve. The next thing is improving pte_modify() to avoid an unnecessary fault as we can end up with dirty|write|rdonly pte (and subsequently ptep_set_access_flags() bailed out earlier due to broken pte_same()). Anyway, that's not critical now, just minor perf improvement. I'll post a patch along the lines of https://paste.debian.net/hidden/66246019/.
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h index 5716fe86e7b9..5d15b4735a0e 100644 --- a/arch/arm64/include/asm/pgtable.h +++ b/arch/arm64/include/asm/pgtable.h @@ -283,23 +283,6 @@ static inline void set_pte_at(struct mm_struct *mm, unsigned long addr, set_pte(ptep, pte); } -#define __HAVE_ARCH_PTE_SAME -static inline int pte_same(pte_t pte_a, pte_t pte_b) -{ - pteval_t lhs, rhs; - - lhs = pte_val(pte_a); - rhs = pte_val(pte_b); - - if (pte_present(pte_a)) - lhs &= ~PTE_RDONLY; - - if (pte_present(pte_b)) - rhs &= ~PTE_RDONLY; - - return (lhs == rhs); -} - /* * Huge pte definitions. */
Following commit 73e86cb03cf2 ("arm64: Move PTE_RDONLY bit handling out of set_pte_at()"), the PTE_RDONLY bit is no longer managed by set_pte_at() but built into the PAGE_* attribute definitions. Consequently, pte_same() must include this bit when checking two PTEs for equality. Remove the arm64-specific pte_same() function, practically reverting commit 747a70e60b72 ("arm64: Fix copy-on-write referencing in HugeTLB") Fixes: 73e86cb03cf2 ("arm64: Move PTE_RDONLY bit handling out of set_pte_at()") Cc: <stable@vger.kernel.org> # 4.14.x- Cc: Will Deacon <will@kernel.org> Cc: Steve Capper <steve.capper@arm.com> Reported-by: John Stultz <john.stultz@linaro.org> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> --- Steve, Could you please check that the original problem fixed by commit 747a70e60b72 no longer exists after reverting it in 4.14 onwards? Thanks. arch/arm64/include/asm/pgtable.h | 17 ----------------- 1 file changed, 17 deletions(-)