Re: [PATCH RFC v2 12/20] x86: mm: define pudp_set_access_flags() when CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD is enabled only.

[Yeoreum Yun]

Hi Dave,

> > diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> > index f32facdb3035..edad847a2ecd 100644
> > --- a/arch/x86/mm/pgtable.c
> > +++ b/arch/x86/mm/pgtable.c
> > @@ -411,6 +411,7 @@ int pmdp_set_access_flags(struct vm_area_struct *vma,
> >  	return changed;
> >  }
> >  
> > +#ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
> >  int pudp_set_access_flags(struct vm_area_struct *vma, unsigned long address,
> >  			  pud_t *pudp, pud_t entry, int dirty)
> >  {
> > @@ -430,6 +431,7 @@ int pudp_set_access_flags(struct vm_area_struct *vma, unsigned long address,
> >  
> >  	return changed;
> >  }
> > +#endif /* CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */
> >  #endif
> >  
> >  bool ptep_test_and_clear_young(struct vm_area_struct *vma,
> 
> #ifdefs in .c files are evil.
> 
> The changelog doesn't make a strong enough case for why this evil should
> be tolerated.
> 
> These are also _precisely_ the kind of #ifdefs that cause compilation
> problems. This one is:
> 
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> /// function here
> #ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
> /// another function here
> #endif
> #endif
> 
> So there end up being a couple of dependent config options in play. If
> there are compile problems, this makes them harder to find.
> 
> What is the _actual_ goal here? Saving 50 bytes of kernel text?

TBH, this came from for v1's change of behavior set_pud() where
triggered compiliation problem with v2 this change wouldn't require.

However, wrappering pudp_set_access_flags() with
CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD seems reasonable for not only
saving a kernel text but also keeping consistency with the other pattern
like pudp_invalidate(), pudp_establish() and etc.

Am I mising something?

-- 
Sincerely,
Yeoreum Yun