Re: [PATCH v10 4/4] mm: hugetlb_vmemmap: add hugetlb_optimize_vmemmap sysctl

[Muchun Song]

On Wed, May 11, 2022 at 05:45:57PM +0800, Muchun Song wrote:
> On Tue, May 10, 2022 at 05:39:40PM -0700, Mike Kravetz wrote:
> > On 5/10/22 14:30, Mike Kravetz wrote:
> > > On 5/8/22 23:27, Muchun Song wrote:
> > >> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> > >> index 029fb7e26504..917112661b5c 100644
> > >> --- a/include/linux/memory_hotplug.h
> > >> +++ b/include/linux/memory_hotplug.h
> > >> @@ -351,4 +351,13 @@ void arch_remove_linear_mapping(u64 start, u64 size);
> > >>  extern bool mhp_supports_memmap_on_memory(unsigned long size);
> > >>  #endif /* CONFIG_MEMORY_HOTPLUG */
> > >>  
> > >> +#ifdef CONFIG_MHP_MEMMAP_ON_MEMORY
> > >> +bool mhp_memmap_on_memory(void);
> > >> +#else
> > >> +static inline bool mhp_memmap_on_memory(void)
> > >> +{
> > >> +	return false;
> > >> +}
> > >> +#endif
> > >> +
> > >>  #endif /* __LINUX_MEMORY_HOTPLUG_H */
> > >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > >> index 8605d7eb7f5c..86158eb9da70 100644
> > >> --- a/mm/hugetlb.c
> > >> +++ b/mm/hugetlb.c
> > >> @@ -1617,6 +1617,9 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn);
> > >>  
> > >>  static inline void flush_free_hpage_work(struct hstate *h)
> > >>  {
> > >> +	if (!hugetlb_optimize_vmemmap_enabled())
> > >> +		return;
> > >> +
> > > 
> > > Hi Muchun,
> > > 
> > > In v9 I was suggesting that we may be able to eliminate the static_branch_inc/dec from the vmemmap free/alloc paths.  With this patch
> > > I believe hugetlb_optimize_vmemmap_enabled() is really checking
> > > 'has hugetlb vmemmap optimization been enabled' OR 'are there still vmemmap
> > > optimized hugetlb pages in the system'.  That may be confusing.
> > > 
> > 
> > Sorry, I forgot about the use of hugetlb_optimize_vmemmap_enabled in
> > page_fixed_fake_head.  We need to know if there are any vmemmap optimized
> > hugetlb pages in the system in this performance sensitive path.  So,
> > static_branch_inc/dec is indeed a good idea.
> >
> 
> Agree.
> 
> > Please disregard my attempt below at removing static_branch_inc/dec.
> > 
> > I still find the name hugetlb_optimize_vmemmap_enabled a bit confusing as
> > it tests two conditions (enabled and pages in use).
> >
> 
> Right. It tests two conditions.
> 
> > You have already 'open coded' just the check for enabled in the routine
> > hugetlb_vmemmap_free with:
> > 
> > 	READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF
> > 
> > How about having hugetlb_optimize_vmemmap_enabled() just check
> > vmemmap_optimize_mode in a manner like above?  Then rename
> 
> I'm wondering is it necessary to do this? vmemmap_optimize_mode
> is a internal state in hugetlb_vmemmap.c, at leaset now there is
> no outside users who care about this.  Open-coding may be not
> an issue (I guess)?  If one day someone cares it, maybe it it
> the time to do this and rename hugetlb_optimize_vmemmap_enabled()?
> I'm not against doing this, just expressing some of my thoughts.
> What do you think, Mike?
> 
> > hugetlb_optimize_vmemmap_enabled to something like:
> > hugetlb_optimized_vmemmap_possible().  Sorry, I can think if a great name.
> > 
> 
> At least I cannot come up with an appropriate name.
> hugetlb_optimize_vmemmap_may_enabled()? It's not easy to come
> up with a good name.
>

Instead of renaming, how about remove hugetlb_optimize_vmemmap_enabled()
directly?  I found there are only two places (mm/memory_hotplug.c and
arch/arm64/mm/flush.c) except include/linux/page-flags.h where use this
helper.

In arch/arm64/mm/flush.c, we could replace it with

  if (PageHuge(page) && HPageVmemmapOptimized(compound_head(page)))

In mm/memory_hotplug.c, I have a plan to remove it as well (I'll
post them out after this patch merged).

Finally, there is no outside users of it, we could remove it and squash
it into page_fixed_fake_head(). What do you think this, Mike?