Re: [PATCH 0/2] mm: thp: reduce unnecessary start_stop_khugepaged() calls

[Lorenzo Stoakes (Oracle)]

On Wed, Mar 04, 2026 at 11:18:37AM +0000, Kiryl Shutsemau wrote:
> On Wed, Mar 04, 2026 at 02:22:32AM -0800, Breno Leitao wrote:
> > Breno Leitao (2):
> >       mm: thp: avoid calling start_stop_khugepaged() in anon_enabled_store()
> >       mm: thp: avoid calling start_stop_khugepaged() in enabled_store()
>
> I think the same can be achieved cleaner from within start_stop_khugepaged().

I'm kind of with you in doing it here but...

>
> Completely untested patch is below.
>
> One noticeable difference that with the patch we don't kick
> khugepaged_wait if khugepaged_thread is there. But I don't think it
> should make a difference.
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 1dd3cfca610d..80f818d3a094 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2683,18 +2683,18 @@ static void set_recommended_min_free_kbytes(void)
>
>  int start_stop_khugepaged(void)
>  {
> -	int err = 0;
> +	guard(mutex)(&khugepaged_mutex);

This is nice :) we need more guard usage in mm.

> +
> +	/* Check if anything has to be done */
> +	if (hugepage_pmd_enabled() == !!khugepaged_thread)

This is horrible, better to break out the latter expression as a small static
function like:

static bool is_khugepaged_thread_running(void)
{
	if (khugepaged_thread)
		return true;
	return false;
}

> +		return 0;
>
> -	mutex_lock(&khugepaged_mutex);
>  	if (hugepage_pmd_enabled()) {

Can we race on this function call, meaning check above might vary from one here?

Better to have

bool enabled = hugepage_pmd_enabled();

etc. etc.

> -		if (!khugepaged_thread)
> -			khugepaged_thread = kthread_run(khugepaged, NULL,
> -							"khugepaged");
> +		khugepaged_thread = kthread_run(khugepaged, NULL, "khugepaged");
>  		if (IS_ERR(khugepaged_thread)) {
>  			pr_err("khugepaged: kthread_run(khugepaged) failed\n");
> -			err = PTR_ERR(khugepaged_thread);
>  			khugepaged_thread = NULL;
> -			goto fail;
> +			return PTR_ERR(khugepaged_thread);
>  		}
>
>  		if (!list_empty(&khugepaged_scan.mm_head))
> @@ -2703,10 +2703,9 @@ int start_stop_khugepaged(void)
>  		kthread_stop(khugepaged_thread);
>  		khugepaged_thread = NULL;
>  	}
> +
>  	set_recommended_min_free_kbytes();

We are setting recommended min free kbytes each time somebody touches these
flags, maybe somebody somewhere relies on that and this change would break them?

So maybe we just want to do this each time anyway, but repress/rate limit
output?

> -fail:
> -	mutex_unlock(&khugepaged_mutex);
> -	return err;
> +	return 0;
>  }
>
>  void khugepaged_min_free_kbytes_update(void)
> --
>   Kiryl Shutsemau / Kirill A. Shutemov

Thanks, Lorenzo