Messages in this thread | | | Subject | Re: [PATCH v2] hugetlb: allow to free gigantic pages regardless of the configuration | From | Alexandre Ghiti <> | Date | Thu, 14 Feb 2019 11:59:18 +0100 |
| |
On 02/14/2019 10:52 AM, Vlastimil Babka wrote: > On 2/13/19 8:30 PM, Dave Hansen wrote: >>> -#if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA) >>> +#ifdef CONFIG_COMPACTION_CORE >>> static __init int gigantic_pages_init(void) >>> { >>> /* With compaction or CMA we can allocate gigantic pages at runtime */ >>> diff --git a/fs/Kconfig b/fs/Kconfig >>> index ac474a61be37..8fecd3ea5563 100644 >>> --- a/fs/Kconfig >>> +++ b/fs/Kconfig >>> @@ -207,8 +207,9 @@ config HUGETLB_PAGE >>> config MEMFD_CREATE >>> def_bool TMPFS || HUGETLBFS >>> >>> -config ARCH_HAS_GIGANTIC_PAGE >>> +config COMPACTION_CORE >>> bool >>> + default y if (MEMORY_ISOLATION && MIGRATION) || CMA >> This takes a hard dependency (#if) and turns it into a Kconfig *default* >> that can be overridden. That seems like trouble. >> >> Shouldn't it be: >> >> config COMPACTION_CORE >> def_bool y >> depends on (MEMORY_ISOLATION && MIGRATION) || CMA > Agreed. Also I noticed that it now depends on MIGRATION instead of > COMPACTION. That intention is correct IMHO, but will fail to > compile/link when both COMPACTION and CMA are disabled, and would need > more changes in mm/internal.h and mm/compaction.c (possibly just > replacing CMA in all "if defined CONFIG_COMPACTION || defined > CONFIG_CMA" instances with COMPACTION_CORE, but there might be more > problems, wanna try? :)
Let's be honest, that's a "typo" :) Migration is logical to me but that's because I don't know much about compaction. Thanks for noticing it. I'll take a look at what you propose to do too.
> > Also, I realized that COMPACTION_CORE is a wrong name, sorry about that. > What the config really provides is alloc_contig_range(), so it should be > named either CONFIG_CMA_CORE (as it provides contiguous memory > allocation, but not the related reservation and accounting), or > something like CONFIG_CONTIG_ALLOC. I would also move it from fs/Kconfig > to mm/Kconfig.
No problem, I was not inspired either. I'll send a v3 with the renaming you propose.
> Thanks!
Thank you.
Alex
| |