[3/5] x86: pgtable: Drop pXd_none() checks from pXd_free_pYd_table()

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(-)

-- 
2.1.4

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