| Message ID | 1536747974-25875-4-git-send-email-will.deacon@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up huge vmap and ioremap code | expand |
On Wed, 2018-09-12 at 11:26 +0100, Will Deacon wrote: > Now that the core code checks this for us, we don't need to do it in the > backend. > > Cc: Chintan Pandya <cpandya@codeaurora.org> > Cc: Toshi Kani <toshi.kani@hpe.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > arch/x86/mm/pgtable.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c > index ae394552fb94..b4919c44a194 100644 > --- a/arch/x86/mm/pgtable.c > +++ b/arch/x86/mm/pgtable.c > @@ -796,9 +796,6 @@ int pud_free_pmd_page(pud_t *pud, unsigned long addr) > pte_t *pte; > int i; > > - if (pud_none(*pud)) > - return 1; > - Do we need to remove this safe guard? I feel list this is same as kfree() accepting NULL. Thanks, -Toshi > pmd = (pmd_t *)pud_page_vaddr(*pud); > pmd_sv = (pmd_t *)__get_free_page(GFP_KERNEL); > if (!pmd_sv) > @@ -840,9 +837,6 @@ int pmd_free_pte_page(pmd_t *pmd, unsigned long addr) > { > pte_t *pte; > > - if (pmd_none(*pmd)) > - return 1; > - > pte = (pte_t *)pmd_page_vaddr(*pmd); > pmd_clear(pmd); >
On Fri, Sep 14, 2018 at 08:37:48PM +0000, Kani, Toshi wrote: > On Wed, 2018-09-12 at 11:26 +0100, Will Deacon wrote: > > Now that the core code checks this for us, we don't need to do it in the > > backend. > > > > Cc: Chintan Pandya <cpandya@codeaurora.org> > > Cc: Toshi Kani <toshi.kani@hpe.com> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Michal Hocko <mhocko@suse.com> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Signed-off-by: Will Deacon <will.deacon@arm.com> > > --- > > arch/x86/mm/pgtable.c | 6 ------ > > 1 file changed, 6 deletions(-) > > > > diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c > > index ae394552fb94..b4919c44a194 100644 > > --- a/arch/x86/mm/pgtable.c > > +++ b/arch/x86/mm/pgtable.c > > @@ -796,9 +796,6 @@ int pud_free_pmd_page(pud_t *pud, unsigned long addr) > > pte_t *pte; > > int i; > > > > - if (pud_none(*pud)) > > - return 1; > > - > > Do we need to remove this safe guard? I feel list this is same as > kfree() accepting NULL. I think two big differences with kfree() are (1) that this function has exactly one caller in the tree and (2) it's implemented per-arch. Therefore we're in a good position to give it some simple semantics and implement those. Of course, if the x86 people would like to keep the redundant check, that's up to them, but I think it makes the function more confusing and tempts people into calling it for present entries. Will
On Mon, 2018-09-17 at 12:33 +0100, Will Deacon wrote: > On Fri, Sep 14, 2018 at 08:37:48PM +0000, Kani, Toshi wrote: > > On Wed, 2018-09-12 at 11:26 +0100, Will Deacon wrote: > > > Now that the core code checks this for us, we don't need to do it in the > > > backend. > > > > > > Cc: Chintan Pandya <cpandya@codeaurora.org> > > > Cc: Toshi Kani <toshi.kani@hpe.com> > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > > Cc: Michal Hocko <mhocko@suse.com> > > > Cc: Andrew Morton <akpm@linux-foundation.org> > > > Signed-off-by: Will Deacon <will.deacon@arm.com> > > > --- > > > arch/x86/mm/pgtable.c | 6 ------ > > > 1 file changed, 6 deletions(-) > > > > > > diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c > > > index ae394552fb94..b4919c44a194 100644 > > > --- a/arch/x86/mm/pgtable.c > > > +++ b/arch/x86/mm/pgtable.c > > > @@ -796,9 +796,6 @@ int pud_free_pmd_page(pud_t *pud, unsigned long addr) > > > pte_t *pte; > > > int i; > > > > > > - if (pud_none(*pud)) > > > - return 1; > > > - > > > > Do we need to remove this safe guard? I feel list this is same as > > kfree() accepting NULL. > > I think two big differences with kfree() are (1) that this function has > exactly one caller in the tree and (2) it's implemented per-arch. Therefore > we're in a good position to give it some simple semantics and implement > those. Of course, if the x86 people would like to keep the redundant check, > that's up to them, but I think it makes the function more confusing and > tempts people into calling it for present entries. With patch 1/5 change to have pXd_present() check, I agree that we can remove this pXd_none() check to avoid any confusion. Reviewed-by: Toshi Kani <toshi.kani@hpe.com> Thanks, -Toshi
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c index ae394552fb94..b4919c44a194 100644 --- a/arch/x86/mm/pgtable.c +++ b/arch/x86/mm/pgtable.c @@ -796,9 +796,6 @@ int pud_free_pmd_page(pud_t *pud, unsigned long addr) pte_t *pte; int i; - if (pud_none(*pud)) - return 1; - pmd = (pmd_t *)pud_page_vaddr(*pud); pmd_sv = (pmd_t *)__get_free_page(GFP_KERNEL); if (!pmd_sv) @@ -840,9 +837,6 @@ int pmd_free_pte_page(pmd_t *pmd, unsigned long addr) { pte_t *pte; - if (pmd_none(*pmd)) - return 1; - pte = (pte_t *)pmd_page_vaddr(*pmd); pmd_clear(pmd);
Now that the core code checks this for us, we don't need to do it in the backend. Cc: Chintan Pandya <cpandya@codeaurora.org> Cc: Toshi Kani <toshi.kani@hpe.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Michal Hocko <mhocko@suse.com> Cc: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Will Deacon <will.deacon@arm.com> --- arch/x86/mm/pgtable.c | 6 ------ 1 file changed, 6 deletions(-)