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
