Re: [PATCH v6 01/15] mm: khugepaged: don't carry huge page to the next loop for !CONFIG_NUMA

[Zach O'Keefe <zokeefe@google.com>]

On Jun 29 16:49, Peter Xu wrote:
> On Fri, Jun 03, 2022 at 05:39:50PM -0700, Zach O'Keefe wrote:
> > -static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
> > +/* Sleep for the first alloc fail, break the loop for the second fail */
> > +static bool alloc_fail_should_sleep(struct page **hpage, bool *wait)
> >  {
> >  	if (IS_ERR(*hpage)) {
> >  		if (!*wait)
> > -			return false;
> > +			return true;
> >  
> >  		*wait = false;
> >  		*hpage = NULL;
> >  		khugepaged_alloc_sleep();
> > -	} else if (*hpage) {
> > -		put_page(*hpage);
> > -		*hpage = NULL;
> >  	}
> > -
> > -	return true;
> > +	return false;
> >  }
> 
> One nitpick here:
> 
> It's weird to me to sleep in a function called XXX_should_sleep(), we'd
> normally expect to sleep only if it returns true.
> 
> Meanwhile, would this be a very good chance to unwrap this function already
> to remove the "bool*" reference, which looks not pretty?  Something like:
> 
> ---8<---
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 16be62d493cd..807c10cd0816 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2235,9 +2235,6 @@ static void khugepaged_do_scan(void)
>         lru_add_drain_all();
>  
>         while (progress < pages) {
> -               if (!khugepaged_prealloc_page(&hpage, &wait))
> -                       break;
> -
>                 cond_resched();
>  
>                 if (unlikely(kthread_should_stop() || try_to_freeze()))
> @@ -2253,6 +2250,18 @@ static void khugepaged_do_scan(void)
>                 else
>                         progress = pages;
>                 spin_unlock(&khugepaged_mm_lock);
> +
> +               if (IS_ERR(*hpage)) {
> +                       /*
> +                        * If fail to allocate the first time, try to sleep
> +                        * for a while.  When hit again, cancel the scan.
> +                        */
> +                       if (!wait)
> +                               break;
> +                       wait = false;
> +                       *hpage = NULL;
> +                       khugepaged_alloc_sleep();
> +               }
>         }
> ---8<---
> 
> Would this look slightly better?

Hey Peter,

Thanks for taking the time to review. I think open coding this looks good. One
small detail is that if we move this to the end of the loop, we'll need to check
that progress < pages still before sleeping - else we run the risk of doing an
alloc sleep and a scan sleep.

But I'll let Yang make the call since it's his patch - he's just been kind
enough to donate it for this cause :)

Thanks,
Zach

> Thanks,
> 
> -- 
> Peter Xu
>