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

[Peter Xu <peterx@redhat.com>]

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?

Thanks,

-- 
Peter Xu