Re: [PATCH v3 2/2] mm: remove odd HAVE_PTE_SPECIAL

[Christophe LEROY]

Le 11/04/2018 Ã 11:03, Laurent Dufour a ÃcritÂ:
> On 11/04/2018 10:58, Christophe LEROY wrote:
> > Le 11/04/2018 Ã 10:03, Laurent Dufour a ÃcritÂ:
> > > Remove the additional define HAVE_PTE_SPECIAL and rely directly on
> > > CONFIG_ARCH_HAS_PTE_SPECIAL.
> > >
> > > There is no functional change introduced by this patch
> > >
> > > Signed-off-by: Laurent Dufour <ldufour@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >  Â mm/memory.c | 19 ++++++++-----------
> > >  Â 1 file changed, 8 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index 96910c625daa..7f7dc7b2a341 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -817,17 +817,12 @@ static void print_bad_pte(struct vm_area_struct *vma,
> > > unsigned long addr,
> > >  ÂÂ * PFNMAP mappings in order to support COWable mappings.
> > >  ÂÂ *
> > >  ÂÂ */
> > > -#ifdef CONFIG_ARCH_HAS_PTE_SPECIAL
> > > -# define HAVE_PTE_SPECIAL 1
> > > -#else
> > > -# define HAVE_PTE_SPECIAL 0
> > > -#endif
> > >  Â struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
> > >  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pte_t pte, bool with_public_device)
> > >  Â {
> > >  ÂÂÂÂÂ unsigned long pfn = pte_pfn(pte);
> > >  Â -ÂÂÂ if (HAVE_PTE_SPECIAL) {
> > > +ÂÂÂ if (IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL)) {
> > >  ÂÂÂÂÂÂÂÂÂ if (likely(!pte_special(pte)))
> > >  ÂÂÂÂÂÂÂÂÂÂÂÂÂ goto check_pfn;
> > >  ÂÂÂÂÂÂÂÂÂ if (vma->vm_ops && vma->vm_ops->find_special_page)
> > > @@ -862,7 +857,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma,
> > > unsigned long addr,
> > >  ÂÂÂÂÂÂÂÂÂ return NULL;
> > >  ÂÂÂÂÂ }
> > >  Â -ÂÂÂ /* !HAVE_PTE_SPECIAL case follows: */
> > > +ÂÂÂ /* !CONFIG_ARCH_HAS_PTE_SPECIAL case follows: */
> > >  Â ÂÂÂÂÂ if (unlikely(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP))) {
> > >  ÂÂÂÂÂÂÂÂÂ if (vma->vm_flags & VM_MIXEDMAP) {
> > > @@ -881,7 +876,8 @@ struct page *_vm_normal_page(struct vm_area_struct *vma,
> > > unsigned long addr,
> > >  Â ÂÂÂÂÂ if (is_zero_pfn(pfn))
> > >  ÂÂÂÂÂÂÂÂÂ return NULL;
> > > -check_pfn:
> > > +
> > > +check_pfn: __maybe_unused
> >
> > See below
> >
> > >  ÂÂÂÂÂ if (unlikely(pfn > highest_memmap_pfn)) {
> > >  ÂÂÂÂÂÂÂÂÂ print_bad_pte(vma, addr, pte, NULL);
> > >  ÂÂÂÂÂÂÂÂÂ return NULL;
> > > @@ -891,7 +887,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma,
> > > unsigned long addr,
> > >  ÂÂÂÂÂÂ * NOTE! We still have PageReserved() pages in the page tables.
> > >  ÂÂÂÂÂÂ * eg. VDSO mappings can cause them to exist.
> > >  ÂÂÂÂÂÂ */
> > > -out:
> > > +out: __maybe_unused
> >
> > Why do you need that change ?
> >
> > There is no reason for the compiler to complain. It would complain if the goto
> > was within a #ifdef, but all the purpose of using IS_ENABLED() is to allow the
> > compiler to properly handle all possible cases. That's all the force of
> > IS_ENABLED() compared to ifdefs, and that the reason why they are plebicited,
> > ref Linux Codying style for a detailed explanation.
>
> Fair enough.
>
> Should I submit a v4 just to remove these so ugly __maybe_unused ?

Most likely, unless the mm maintainer agrees to remove them by himself 
when applying your patch ?

Christophe