Re: [PATCH v4 10/19] selftests/mm: add khugepaged completion barrier helper

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Sat, Aug 15, 2026 at 02:58:52AM +0100, Kiryl Shutsemau wrote:
> From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>
>
> Race and functional tests need to drive khugepaged in step: set up a
> layout, let one full scan pass over it, check the result.  The khugepaged
> selftest already waits for full_scans to advance by two, but only makes
> progress if scan_sleep_millisecs happens to be short.
>
> Lift it into khugepaged_full_pass() and drive it through sysfs: any store
> to scan_sleep_millisecs wakes the daemon, so the barrier completes whatever
> the scan cadence.  A store can be lost when the daemon is between scans, so
> it keeps storing until the pass lands; a store to an awake daemon costs
> nothing and queues no extra pass.
>
> One wake completes one pass only if the whole mm list fits in a scan batch,
> so callers need a large pages_to_scan.
>
> Settings pushes must not start passes either.  A store to either sleep knob
> wakes the daemon, so thp_write_settings() now writes a khugepaged knob only
> when its value changes.  The other knobs do not wake, but writing them
> uniformly costs nothing.  thp_update_num() is exported for tests that want
> the same restraint.
>
> Assisted-by: Claude-Code:claude-opus-5
> Tested-by: Muhammad Usama Anjum <usama.anjum@arm.com>
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> ---
>  .../testing/selftests/mm/hugepage_settings.c  | 74 ++++++++++++++++---
>  .../testing/selftests/mm/hugepage_settings.h  |  3 +
>  2 files changed, 68 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/hugepage_settings.c b/tools/testing/selftests/mm/hugepage_settings.c
> index d7917dce3aba..992efee17b71 100644
> --- a/tools/testing/selftests/mm/hugepage_settings.c
> +++ b/tools/testing/selftests/mm/hugepage_settings.c
> @@ -183,6 +183,19 @@ void thp_read_settings(struct thp_settings *settings)
>  	}
>  }
>
> +/*
> + * Write only on change: a store to either sleep knob wakes khugepaged --
> + * __sleep_millisecs_store() clears khugepaged_sleep_expire and wakes the
> + * queue -- and settings pushes/pops must not start scan passes nobody
> + * asked for; khugepaged_full_pass() is the only sanctioned wake.  The
> + * other knobs do not wake, but writing them the same way costs nothing.
> + */

This seems quite schloppy and way too much information in a single dense
comment.

You/claude don't need to give your life story in every comment :)

Smaller, clearer, human-written please.

	/*

> +void thp_update_num(const char *name, unsigned long num)
> +{
> +	if (thp_read_num(name) != num)
> +		thp_write_num(name, num);
> +}
> +
>  void thp_write_settings(struct thp_settings *settings)
>  {
>  	struct khugepaged_settings *khugepaged = &settings->khugepaged;
> @@ -198,15 +211,15 @@ void thp_write_settings(struct thp_settings *settings)
>  			shmem_enabled_strings[settings->shmem_enabled]);
>  	thp_write_num("use_zero_page", settings->use_zero_page);
>
> -	thp_write_num("khugepaged/defrag", khugepaged->defrag);
> -	thp_write_num("khugepaged/alloc_sleep_millisecs",
> -			khugepaged->alloc_sleep_millisecs);
> -	thp_write_num("khugepaged/scan_sleep_millisecs",
> -			khugepaged->scan_sleep_millisecs);
> -	thp_write_num("khugepaged/max_ptes_none", khugepaged->max_ptes_none);
> -	thp_write_num("khugepaged/max_ptes_swap", khugepaged->max_ptes_swap);
> -	thp_write_num("khugepaged/max_ptes_shared", khugepaged->max_ptes_shared);
> -	thp_write_num("khugepaged/pages_to_scan", khugepaged->pages_to_scan);
> +	thp_update_num("khugepaged/defrag", khugepaged->defrag);
> +	thp_update_num("khugepaged/alloc_sleep_millisecs",
> +		       khugepaged->alloc_sleep_millisecs);
> +	thp_update_num("khugepaged/scan_sleep_millisecs",
> +		       khugepaged->scan_sleep_millisecs);
> +	thp_update_num("khugepaged/max_ptes_none", khugepaged->max_ptes_none);
> +	thp_update_num("khugepaged/max_ptes_swap", khugepaged->max_ptes_swap);
> +	thp_update_num("khugepaged/max_ptes_shared", khugepaged->max_ptes_shared);
> +	thp_update_num("khugepaged/pages_to_scan", khugepaged->pages_to_scan);
>
>  	if (dev_queue_read_ahead_path[0])
>  		write_num(dev_queue_read_ahead_path, settings->read_ahead_kb);
> @@ -230,6 +243,49 @@ void thp_write_settings(struct thp_settings *settings)
>  	}
>  }
>
> +/*
> + * Completion barrier for khugepaged: wait until a full scan pass that
> + * started after this call has finished. full_scans must advance by two;
> + * a +1 step may complete a pass that examined this mm before the
> + * caller's setup was in place.
> + *
> + * Any store to scan_sleep_millisecs wakes the daemon, so the barrier works
> + * whatever the configured scan cadence -- but a store can be lost.
> + * __sleep_millisecs_store() clears khugepaged_sleep_expire and wakes the
> + * queue; if the daemon is between scans rather than sleeping, it sets
> + * khugepaged_sleep_expire itself on the way into khugepaged_wait_work() and
> + * then sleeps for the full interval, having never seen the store.  So keep
> + * storing until the pass lands; a store while the daemon is awake costs
> + * nothing and does not queue an extra pass.
> + *
> + * One wake completes one full pass only if the whole mm list fits in
> + * one scan batch, so callers must pair this with a large
> + * pages_to_scan.
> + */

Same comment about the comment. This is dense and schloppy. Clarity and
terseness please.

> +bool khugepaged_full_pass(unsigned int timeout_s)
> +{
> +	unsigned long deadline_ms = timeout_s * 1000UL;
> +	unsigned long sleep_ms =
> +		thp_read_num("khugepaged/scan_sleep_millisecs");
> +	unsigned long elapsed_ms = 0;
> +	int pass;
> +
> +	for (pass = 0; pass < 2; pass++) {
> +		unsigned long target =
> +			thp_read_num("khugepaged/full_scans") + 1;
> +
> +		while (thp_read_num("khugepaged/full_scans") < target) {
> +			if (elapsed_ms >= deadline_ms)
> +				return false;
> +			thp_write_num("khugepaged/scan_sleep_millisecs",
> +				      sleep_ms);
> +			usleep(10 * 1000);
> +			elapsed_ms += 10;
> +		}
> +	}
> +	return true;
> +}
> +
>  struct thp_settings *thp_current_settings(void)
>  {
>  	if (!settings_index) {
> diff --git a/tools/testing/selftests/mm/hugepage_settings.h b/tools/testing/selftests/mm/hugepage_settings.h
> index 726c73c43c05..ba7d38370d43 100644
> --- a/tools/testing/selftests/mm/hugepage_settings.h
> +++ b/tools/testing/selftests/mm/hugepage_settings.h
> @@ -70,6 +70,7 @@ int thp_read_string(const char *name, const char * const strings[]);
>  void thp_write_string(const char *name, const char *val);
>  unsigned long thp_read_num(const char *name);
>  void thp_write_num(const char *name, unsigned long num);
> +void thp_update_num(const char *name, unsigned long num);
>
>  void thp_write_settings(struct thp_settings *settings);
>  void thp_read_settings(struct thp_settings *settings);
> @@ -83,6 +84,8 @@ static inline void thp_save_settings(void)
>  	hugepage_save_settings(/* thp = */ true, /* hugetlb = */ false);
>  }
>
> +bool khugepaged_full_pass(unsigned int timeout_s);
> +
>  void thp_set_read_ahead_path(char *path);
>  unsigned long thp_supported_orders(void);
>  unsigned long thp_shmem_supported_orders(void);
> --
> 2.54.0
>

--
Cheers, Lorenzo