On Sun, 26 Apr 2020 08:13:17 +0530 Anshuman Khandual <anshuman.khandual@xxxxxxx> wrote:
>
>
> On 04/26/2020 06:25 AM, Andrew Morton wrote:
> > On Tue, 14 Apr 2020 17:14:30 +0530 Anshuman Khandual <anshuman.khandual@xxxxxxx> wrote:
> >
> >> There are multiple similar definitions for arch_clear_hugepage_flags() on
> >> various platforms. This introduces HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS for those
> >> platforms that need to define their own arch_clear_hugepage_flags() while
> >> also providing a generic fallback definition for others to use. This help
> >> reduce code duplication.
> >>
> >> ...
> >>
> >> --- a/include/linux/hugetlb.h
> >> +++ b/include/linux/hugetlb.h
> >> @@ -544,6 +544,10 @@ static inline int is_hugepage_only_range(struct mm_struct *mm,
> >> }
> >> #endif
> >>
> >> +#ifndef HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS
> >> +static inline void arch_clear_hugepage_flags(struct page *page) { }
> >> +#endif
> >> +
> >> #ifndef arch_make_huge_pte
> >> static inline pte_t arch_make_huge_pte(pte_t entry, struct vm_area_struct *vma,
> >> struct page *page, int writable)
> >
> > This is the rather old-school way of doing it. The Linus-suggested way is
> >
> > #ifndef arch_clear_hugepage_flags
> > static inline void arch_clear_hugepage_flags(struct page *page)
> > {
> > }
> > #define arch_clear_hugepage_flags arch_clear_hugepage_flags
>
> Do we need that above line here ? Is not that implicit.
It depends if other header files want to test whether
arch_clear_hugepage_flags is already defined. If the header heorarchy
is well-defined and working properly, they shouldn't need to, because
we're reliably indluding the relevant arch header before (or early
within) include/linux/hugetlb.h.
It would be nice if
#define arch_clear_hugepage_flags arch_clear_hugepage_flags
#define arch_clear_hugepage_flags arch_clear_hugepage_flags
were to generate an compiler error but it doesn't. If it did we could
detect these incorrect inclusion orders.
> > #endif
> >
> > And the various arch headers do
> >
> > static inline void arch_clear_hugepage_flags(struct page *page)
> > {
> > <some implementation>
> > }
> > #define arch_clear_hugepage_flags arch_clear_hugepage_flags
> >
> > It's a small difference - mainly to avoid adding two variables to the
> > overall namespace where one would do.
>
> Understood, will change and resend.
That's OK - I've queued up that fix.
