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
>
