Re: [PATCH] mm: always consider THP when adjusting min_free_kbytes

[Khalid Aziz]

On Wed, 2020-02-05 at 17:36 -0800, Mike Kravetz wrote:
> On 2/4/20 4:33 PM, Mike Kravetz wrote:
> > Today, we never overwrite a user defined value that is larger than
> > that calculated by the code.  However, we will owerwrite a user
> > defined
> > value if the code calculates a larger value.
> 
> If we overwrite a user defined value, we do not inform the user.
> 
> > I'm starting to think the best option is to NEVER overwrite a user
> > defined
> > value.
> 
> Below is an updated patch which never overwrites a user defined
> value.
> We would only potentially overwrite a user value on a memory
> offline/online
> or khugepaged start/stop operation.  Since no validation of user
> defined
> values is performed, it seems the desired behavior is to never
> overwrite them?  
> 
> If user has specified a value and we calculate a value, a message
> containing
> both values is logged.
> 
> I am not aware of anyone complaining about current behavior.  While
> working
> a customer issue and playing with min_free_kbytes, I noticed the
> behavior
> and thought it does not do what one would expect.
> 
> From: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
> Subject: [PATCH v2] mm: Don't overwrite user min_free_kbytes,
> consider THP
>  when adjusting
> 
> The value of min_free_kbytes is calculated in two routines:
> 1) init_per_zone_wmark_min based on available memory
> 2) set_recommended_min_free_kbytes may reserve extra space for
>    THP allocations
> 
> In both of these routines, a user defined min_free_kbytes value will
> be overwritten if the value calculated in the code is larger. No
> message
> is logged if the user value is overwritten.
> 
> Change code to never overwrite user defined value.  However, do log a
> message (once per value) showing the value calculated in code.
> 
> At system initialization time, both init_per_zone_wmark_min and
> set_recommended_min_free_kbytes are called to set the initial value
> for min_free_kbytes.  When memory is offlined or onlined,
> min_free_kbytes
> is recalculated and adjusted based on the amount of memory.  However,
> the adjustment for THP is not considered.  Here is an example from a
> 2
> node system with 8GB of memory.
> 
>  # cat /proc/sys/vm/min_free_kbytes
>  90112
>  # echo 0 > /sys/devices/system/node/node1/memory56/online
>  # cat /proc/sys/vm/min_free_kbytes
>  11243
>  # echo 1 > /sys/devices/system/node/node1/memory56/online
>  # cat /proc/sys/vm/min_free_kbytes
>  11412
> 
> One would expect that min_free_kbytes would return to it's original
> value after the offline/online operations.
> 
> Create a simple interface for THP/khugepaged based adjustment and
> call this whenever min_free_kbytes is adjusted.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
> ---
>  include/linux/khugepaged.h |  5 ++++
>  mm/internal.h              |  2 ++
>  mm/khugepaged.c            | 56 ++++++++++++++++++++++++++++++++--
> ----
>  mm/page_alloc.c            | 35 ++++++++++++++++--------
>  4 files changed, 78 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h
> index bc45ea1efbf7..faa92923dbe5 100644
> --- a/include/linux/khugepaged.h
> +++ b/include/linux/khugepaged.h
> @@ -15,6 +15,7 @@ extern int __khugepaged_enter(struct mm_struct
> *mm);
>  extern void __khugepaged_exit(struct mm_struct *mm);
>  extern int khugepaged_enter_vma_merge(struct vm_area_struct *vma,
>  				      unsigned long vm_flags);
> +extern int khugepaged_adjust_min_free_kbytes(int mfk_value);
>  #ifdef CONFIG_SHMEM
>  extern void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned
> long addr);
>  #else
> @@ -81,6 +82,10 @@ static inline int
> khugepaged_enter_vma_merge(struct vm_area_struct *vma,
>  {
>  	return 0;
>  }
> +static inline int khugepaged_adjust_min_free_kbytes(int mfk_value)
> +{
> +	return 0;
> +}
>  static inline void collapse_pte_mapped_thp(struct mm_struct *mm,
>  					   unsigned long addr)
>  {
> diff --git a/mm/internal.h b/mm/internal.h
> index 3cf20ab3ca01..57bbc157124e 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -164,6 +164,8 @@ extern void prep_compound_page(struct page *page,
> unsigned int order);
>  extern void post_alloc_hook(struct page *page, unsigned int order,
>  					gfp_t gfp_flags);
>  extern int user_min_free_kbytes;
> +#define UNSET_USER_MFK_VALUE -1
> +extern int calc_min_free_kbytes;
>  
>  extern void zone_pcp_update(struct zone *zone);
>  extern void zone_pcp_reset(struct zone *zone);
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index b679908743cb..6af72cb7e337 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2138,8 +2138,9 @@ static int khugepaged(void *none)
>  	return 0;
>  }
>  
> -static void set_recommended_min_free_kbytes(void)
> +int __khugepaged_adjust_min_free_kbytes(int mfk_value)
>  {
> +	int ret = 0;
>  	struct zone *zone;
>  	int nr_zones = 0;
>  	unsigned long recommended_min;
> @@ -2172,19 +2173,40 @@ static void
> set_recommended_min_free_kbytes(void)
>  			      (unsigned long) nr_free_buffer_pages() /
> 20);
>  	recommended_min <<= (PAGE_SHIFT-10);
>  
> -	if (recommended_min > min_free_kbytes) {
> -		if (user_min_free_kbytes >= 0)
> -			pr_info("raising min_free_kbytes from %d to %lu
> to help transparent hugepage allocations\n",
> -				min_free_kbytes, recommended_min);
> +	if (recommended_min > mfk_value)
> +		ret = (int)recommended_min - mfk_value;
> +
> +	return ret;
> +}
>  
> -		min_free_kbytes = recommended_min;
> +static void set_recommended_min_free_kbytes(void)
> +{
> +	int av = __khugepaged_adjust_min_free_kbytes(min_free_kbytes);
> +
> +	if (av) {
> +		if (user_min_free_kbytes != UNSET_USER_MFK_VALUE) {
> +			/* Do not change user defined value. */
> +			if ((min_free_kbytes + av) !=
> calc_min_free_kbytes) {
> +				/*
> +				 * Save calculated value so we only
> print
> +				 * warning once per value.
> +				 */
> +				calc_min_free_kbytes = min_free_kbytes
> + av;
> +				pr_warn("min_free_kbytes is not updated
> to %d because user defined value %d is preferred\n",
> +					calc_min_free_kbytes,
> +					user_min_free_kbytes);
> +			}
> +		} else {
> +			min_free_kbytes += av;
> +			setup_per_zone_wmarks();
> +		}
>  	}
> -	setup_per_zone_wmarks();
>  }
>  
> -int start_stop_khugepaged(void)
> +static struct task_struct *khugepaged_thread __read_mostly;
> +
> +int __ref start_stop_khugepaged(void)
>  {
> -	static struct task_struct *khugepaged_thread __read_mostly;
>  	static DEFINE_MUTEX(khugepaged_mutex);
>  	int err = 0;
>  
> @@ -2207,8 +2229,24 @@ int start_stop_khugepaged(void)
>  	} else if (khugepaged_thread) {
>  		kthread_stop(khugepaged_thread);
>  		khugepaged_thread = NULL;
> +		init_per_zone_wmark_min();
>  	}
>  fail:
>  	mutex_unlock(&khugepaged_mutex);
>  	return err;
>  }
> +
> +int khugepaged_adjust_min_free_kbytes(int mfk_value)
> +{
> +	int ret = 0;
> +
> +	/*
> +	 * This is a bit racy, and we could miss transitions.  However,
> +	 * start/stop code above will make additional adjustments at
> the
> +	 * end of transitions.
> +	 */
> +	if (khugepaged_enabled() && khugepaged_thread)
> +		ret = __khugepaged_adjust_min_free_kbytes(mfk_value);
> +
> +	return ret;
> +}
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index d047bf7d8fd4..73162a5bf296 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -68,6 +68,7 @@
>  #include <linux/lockdep.h>
>  #include <linux/nmi.h>
>  #include <linux/psi.h>
> +#include <linux/khugepaged.h>
>  
>  #include <asm/sections.h>
>  #include <asm/tlbflush.h>
> @@ -314,7 +315,8 @@ compound_page_dtor * const compound_page_dtors[]
> = {
>  };
>  
>  int min_free_kbytes = 1024;
> -int user_min_free_kbytes = -1;
> +int user_min_free_kbytes = UNSET_USER_MFK_VALUE;
> +int calc_min_free_kbytes = -1;
>  #ifdef CONFIG_DISCONTIGMEM
>  /*
>   * DiscontigMem defines memory ranges as separate pg_data_t even if
> the ranges
> @@ -7819,17 +7821,28 @@ int __meminit init_per_zone_wmark_min(void)
>  
>  	lowmem_kbytes = nr_free_buffer_pages() * (PAGE_SIZE >> 10);
>  	new_min_free_kbytes = int_sqrt(lowmem_kbytes * 16);
> -
> -	if (new_min_free_kbytes > user_min_free_kbytes) {
> -		min_free_kbytes = new_min_free_kbytes;
> -		if (min_free_kbytes < 128)
> -			min_free_kbytes = 128;
> -		if (min_free_kbytes > 65536)
> -			min_free_kbytes = 65536;
> -	} else {
> -		pr_warn("min_free_kbytes is not updated to %d because
> user defined value %d is preferred\n",
> +	if (new_min_free_kbytes < 128)
> +		new_min_free_kbytes = 128;
> +	if (new_min_free_kbytes > 65536)
> +		new_min_free_kbytes = 65536;
> +	new_min_free_kbytes +=
> +		khugepaged_adjust_min_free_kbytes(new_min_free_kbytes);
> +
> +	if (user_min_free_kbytes != UNSET_USER_MFK_VALUE) {
> +		/* Do not change user defined value. */
> +		if (new_min_free_kbytes != calc_min_free_kbytes) {
> +			/*
> +			 * Save calculated value so we only print
> warning
> +			 * once per value.
> +			 */
> +			calc_min_free_kbytes = new_min_free_kbytes;
> +			pr_warn("min_free_kbytes is not updated to %d
> because user defined value %d is preferred\n",
>  				new_min_free_kbytes,
> user_min_free_kbytes);
> +		}
> +		goto out;
>  	}
> +
> +	min_free_kbytes = new_min_free_kbytes;
>  	setup_per_zone_wmarks();
>  	refresh_zone_stat_thresholds();
>  	setup_per_zone_lowmem_reserve();
> @@ -7838,7 +7851,7 @@ int __meminit init_per_zone_wmark_min(void)
>  	setup_min_unmapped_ratio();
>  	setup_min_slab_ratio();
>  #endif
> -
> +out:
>  	return 0;
>  }
>  core_initcall(init_per_zone_wmark_min)

This looks good to me.

Reviewed-by: Khalid Aziz <khalid.aziz@xxxxxxxxxx>