From: Chintan Pandya <cpandya@codeaurora.org> To: Will Deacon <will.deacon@arm.com> Cc: catalin.marinas@arm.com, mark.rutland@arm.com, toshi.kani@hpe.com, arnd@arndb.de, ard.biesheuvel@linaro.org, marc.zyngier@arm.com, james.morse@arm.com, kristina.martsenko@arm.com, takahiro.akashi@linaro.org, gregkh@linuxfoundation.org, tglx@linutronix.de, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, akpm@linux-foundation.org Subject: Re: [PATCH v5 3/4] arm64: Implement page table free interfaces Date: Wed, 28 Mar 2018 12:29:25 +0530 [thread overview] Message-ID: <3a9610af-c35d-2624-87a6-71f893740fa4@codeaurora.org> (raw) In-Reply-To: <20180327180048.GJ18435@arm.com> On 3/27/2018 11:30 PM, Will Deacon wrote: > Hi Chintan, Hi Will, > > On Tue, Mar 27, 2018 at 06:54:59PM +0530, Chintan Pandya wrote: >> Implement pud_free_pmd_page() and pmd_free_pte_page(). >> >> Implementation requires, >> 1) Freeing of the un-used next level page tables >> 2) Clearing off the current pud/pmd entry >> 3) Invalidate TLB which could have previously >> valid but not stale entry >> >> Signed-off-by: Chintan Pandya <cpandya@codeaurora.org> >> --- >> V4->V5: >> - Using __flush_tlb_kernel_pgtable instead of >> flush_tlb_kernel_range >> >> >> arch/arm64/mm/mmu.c | 33 +++++++++++++++++++++++++++++++-- >> 1 file changed, 31 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >> index da98828..3552c7a 100644 >> --- a/arch/arm64/mm/mmu.c >> +++ b/arch/arm64/mm/mmu.c >> @@ -45,6 +45,7 @@ >> #include <asm/memblock.h> >> #include <asm/mmu_context.h> >> #include <asm/ptdump.h> >> +#include <asm/tlbflush.h> >> >> #define NO_BLOCK_MAPPINGS BIT(0) >> #define NO_CONT_MAPPINGS BIT(1) >> @@ -973,12 +974,40 @@ int pmd_clear_huge(pmd_t *pmdp) >> return 1; >> } >> >> +static int __pmd_free_pte_page(pmd_t *pmd, unsigned long addr, bool tlb_inv) >> +{ >> + pmd_t *table; >> + >> + if (pmd_val(*pmd)) { > > Please can you follow what I did in 20a004e7b017 ("arm64: mm: Use > READ_ONCE/WRITE_ONCE when accessing page tables") and: > > 1. Use consistent naming, so pmd_t * pmdp. > 2. Use READ_ONCE to dereference the entry once into a local. > > Similarly for the pud code below. Sure. I'll fix this in v6. > >> + table = __va(pmd_val(*pmd)); >> + pmd_clear(pmd); >> + if (tlb_inv) >> + __flush_tlb_kernel_pgtable(addr); >> + >> + free_page((unsigned long) table); > > Hmm. Surely it's only safe to call free_page if !tlb_inv in situations when > the page table is already disconnected at a higher level? That doesn't > appear to be the case with the function below, which still has the pud > installed. What am I missing? > Point ! Without the invalidation, free'ing a page is not safe. Better, I do __flush_tlb_kernel_pgtable() every time. This might not be as costly as flush_tlb_kernel_range(). >> + } >> + return 1; >> +} >> + >> int pud_free_pmd_page(pud_t *pud, unsigned long addr) >> { >> - return pud_none(*pud); >> + pmd_t *table; >> + int i; >> + >> + if (pud_val(*pud)) { >> + table = __va(pud_val(*pud)); >> + for (i = 0; i < PTRS_PER_PMD; i++) >> + __pmd_free_pte_page(&table[i], addr + (i * PMD_SIZE), >> + false); >> + >> + pud_clear(pud); >> + flush_tlb_kernel_range(addr, addr + PUD_SIZE); > > Why aren't you using __flush_tlb_kernel_pgtable here? > Now that I will call __flush_tlb_kernel_pgtable() for every PMD, I can use __flush_tlb_kernel_pgtable() here as well. Previously, the thought was, while invalidating PUD by VA would not work always because PUD may have next level of valid mapping still present in the table (valid next PMD but invalid next-to-next PTE). In this case doing just __flush_tlb_kernel_pgtable() for PUD might not be enough. We need to invalidate subsequent tables as well which I was skipping for optimization. So, I used flush_tlb_kernel_range(). I will upload v6. > Will > Chintan -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
WARNING: multiple messages have this Message-ID (diff)
From: cpandya@codeaurora.org (Chintan Pandya) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v5 3/4] arm64: Implement page table free interfaces Date: Wed, 28 Mar 2018 12:29:25 +0530 [thread overview] Message-ID: <3a9610af-c35d-2624-87a6-71f893740fa4@codeaurora.org> (raw) In-Reply-To: <20180327180048.GJ18435@arm.com> On 3/27/2018 11:30 PM, Will Deacon wrote: > Hi Chintan, Hi Will, > > On Tue, Mar 27, 2018 at 06:54:59PM +0530, Chintan Pandya wrote: >> Implement pud_free_pmd_page() and pmd_free_pte_page(). >> >> Implementation requires, >> 1) Freeing of the un-used next level page tables >> 2) Clearing off the current pud/pmd entry >> 3) Invalidate TLB which could have previously >> valid but not stale entry >> >> Signed-off-by: Chintan Pandya <cpandya@codeaurora.org> >> --- >> V4->V5: >> - Using __flush_tlb_kernel_pgtable instead of >> flush_tlb_kernel_range >> >> >> arch/arm64/mm/mmu.c | 33 +++++++++++++++++++++++++++++++-- >> 1 file changed, 31 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >> index da98828..3552c7a 100644 >> --- a/arch/arm64/mm/mmu.c >> +++ b/arch/arm64/mm/mmu.c >> @@ -45,6 +45,7 @@ >> #include <asm/memblock.h> >> #include <asm/mmu_context.h> >> #include <asm/ptdump.h> >> +#include <asm/tlbflush.h> >> >> #define NO_BLOCK_MAPPINGS BIT(0) >> #define NO_CONT_MAPPINGS BIT(1) >> @@ -973,12 +974,40 @@ int pmd_clear_huge(pmd_t *pmdp) >> return 1; >> } >> >> +static int __pmd_free_pte_page(pmd_t *pmd, unsigned long addr, bool tlb_inv) >> +{ >> + pmd_t *table; >> + >> + if (pmd_val(*pmd)) { > > Please can you follow what I did in 20a004e7b017 ("arm64: mm: Use > READ_ONCE/WRITE_ONCE when accessing page tables") and: > > 1. Use consistent naming, so pmd_t * pmdp. > 2. Use READ_ONCE to dereference the entry once into a local. > > Similarly for the pud code below. Sure. I'll fix this in v6. > >> + table = __va(pmd_val(*pmd)); >> + pmd_clear(pmd); >> + if (tlb_inv) >> + __flush_tlb_kernel_pgtable(addr); >> + >> + free_page((unsigned long) table); > > Hmm. Surely it's only safe to call free_page if !tlb_inv in situations when > the page table is already disconnected at a higher level? That doesn't > appear to be the case with the function below, which still has the pud > installed. What am I missing? > Point ! Without the invalidation, free'ing a page is not safe. Better, I do __flush_tlb_kernel_pgtable() every time. This might not be as costly as flush_tlb_kernel_range(). >> + } >> + return 1; >> +} >> + >> int pud_free_pmd_page(pud_t *pud, unsigned long addr) >> { >> - return pud_none(*pud); >> + pmd_t *table; >> + int i; >> + >> + if (pud_val(*pud)) { >> + table = __va(pud_val(*pud)); >> + for (i = 0; i < PTRS_PER_PMD; i++) >> + __pmd_free_pte_page(&table[i], addr + (i * PMD_SIZE), >> + false); >> + >> + pud_clear(pud); >> + flush_tlb_kernel_range(addr, addr + PUD_SIZE); > > Why aren't you using __flush_tlb_kernel_pgtable here? > Now that I will call __flush_tlb_kernel_pgtable() for every PMD, I can use __flush_tlb_kernel_pgtable() here as well. Previously, the thought was, while invalidating PUD by VA would not work always because PUD may have next level of valid mapping still present in the table (valid next PMD but invalid next-to-next PTE). In this case doing just __flush_tlb_kernel_pgtable() for PUD might not be enough. We need to invalidate subsequent tables as well which I was skipping for optimization. So, I used flush_tlb_kernel_range(). I will upload v6. > Will > Chintan -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
next prev parent reply other threads:[~2018-03-28 6:59 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-03-27 13:24 [PATCH v5 0/4] Fix issues with huge mapping in ioremap for ARM64 Chintan Pandya 2018-03-27 13:24 ` Chintan Pandya 2018-03-27 13:24 ` Chintan Pandya 2018-03-27 13:24 ` Chintan Pandya 2018-03-27 13:24 ` [PATCH v5 1/4] ioremap: Update pgtable free interfaces with addr Chintan Pandya 2018-03-27 13:24 ` Chintan Pandya 2018-03-28 11:50 ` kbuild test robot 2018-03-28 11:50 ` kbuild test robot 2018-03-28 11:50 ` kbuild test robot 2018-03-28 11:50 ` kbuild test robot 2018-03-28 12:22 ` Chintan Pandya 2018-03-28 12:22 ` Chintan Pandya 2018-03-28 13:12 ` kbuild test robot 2018-03-28 13:12 ` kbuild test robot 2018-03-28 13:12 ` kbuild test robot 2018-03-28 13:12 ` kbuild test robot 2018-03-27 13:24 ` [PATCH v5 2/4] arm64: tlbflush: Introduce __flush_tlb_kernel_pgtable Chintan Pandya 2018-03-27 13:24 ` Chintan Pandya 2018-03-27 13:24 ` [PATCH v5 3/4] arm64: Implement page table free interfaces Chintan Pandya 2018-03-27 13:24 ` Chintan Pandya 2018-03-27 18:00 ` Will Deacon 2018-03-27 18:00 ` Will Deacon 2018-03-28 6:59 ` Chintan Pandya [this message] 2018-03-28 6:59 ` Chintan Pandya 2018-03-27 13:25 ` [PATCH v5 4/4] Revert "arm64: Enforce BBM for huge IO/VMAP mappings" Chintan Pandya 2018-03-27 13:25 ` Chintan Pandya
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3a9610af-c35d-2624-87a6-71f893740fa4@codeaurora.org \ --to=cpandya@codeaurora.org \ --cc=akpm@linux-foundation.org \ --cc=ard.biesheuvel@linaro.org \ --cc=arnd@arndb.de \ --cc=catalin.marinas@arm.com \ --cc=gregkh@linuxfoundation.org \ --cc=james.morse@arm.com \ --cc=kristina.martsenko@arm.com \ --cc=linux-arch@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=marc.zyngier@arm.com \ --cc=mark.rutland@arm.com \ --cc=takahiro.akashi@linaro.org \ --cc=tglx@linutronix.de \ --cc=toshi.kani@hpe.com \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.