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
