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

[Mike Rapoport]

> 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

"advance by two"?

> progress if scan_sleep_millisecs happens to be short.
> 
> Lift it into khugepaged_full_pass() and drive it through sysfs: any store

Lift what?

> 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.

This very much elaborates on "what" and "how" and very little on "why".
I'd change the balance.

> Assisted-by: Claude-Code:claude-opus-5
> Tested-by: Muhammad Usama Anjum <usama.anjum@xxxxxxx>
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
>
> 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.
> + */

Hmm, the function name already tells that it only writes on change.
Do we need this long text along with it?

Surely "khugepaged_full_pass() is ithe only sanctioned mode" is out of
line for infrastrucure helper description.

-- 
Sincerely yours,
Mike.