From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 29718E706FF for ; Thu, 21 Sep 2023 09:48:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 94A8E6B017F; Thu, 21 Sep 2023 05:47:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8FA176B01A3; Thu, 21 Sep 2023 05:47:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7E8EE6B01F0; Thu, 21 Sep 2023 05:47:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 6E9836B017F for ; Thu, 21 Sep 2023 05:47:59 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 32392C08B5 for ; Thu, 21 Sep 2023 09:47:59 +0000 (UTC) X-FDA: 81260128278.06.450BB67 Received: from out-230.mta1.migadu.com (out-230.mta1.migadu.com [95.215.58.230]) by imf03.hostedemail.com (Postfix) with ESMTP id 2D57120004 for ; Thu, 21 Sep 2023 09:47:56 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=en4DqbMQ; spf=pass (imf03.hostedemail.com: domain of muchun.song@linux.dev designates 95.215.58.230 as permitted sender) smtp.mailfrom=muchun.song@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1695289677; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=lRGTp/5qJIknHZ71d8tJbT3IXF9aiIEKzgyfD/pboKE=; b=zIEm6W6hd48vOU/kTvzOqoQNmNbuoGDUMKLgF+xYw11mfcKnzVkEajKBL1PGHT9v/crW1H g0JCR9sSLFcPEZUHo+1+XAQrTERY34aCRqN26pfN4uEcC5F2lL6OJvwM+PI/DL0Gtfxqwn 8r57Q8nom8nUJd9/FJRnt5afPsQ+YGA= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=en4DqbMQ; spf=pass (imf03.hostedemail.com: domain of muchun.song@linux.dev designates 95.215.58.230 as permitted sender) smtp.mailfrom=muchun.song@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1695289677; a=rsa-sha256; cv=none; b=OaBF1dQdOgVhsaadm7c8TwOKfP0re4bwKzAOazFSkVf6Yy1lrVFLQqI95EfwaEdF9SbFI8 ZgAuTJzLG/SwMcKPV7FsQUa1Hn++CAgsrBjxjl7NjsXGH7H6Rh0RyEOFks1jB4cEqANg53 DNy2q6OZx9qE7dfnj35ZD4p4nJhhS94= Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1695289674; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=lRGTp/5qJIknHZ71d8tJbT3IXF9aiIEKzgyfD/pboKE=; b=en4DqbMQwPfJtW83Nzb/AXJqQyugvKh1t2q7VtDGiVkwqjkqGM5/h7dhYp6qWFVKOwnMg9 jF7CPHZH2htwbjo5SF0Xohe1V9T5ubXKrr+iRup81yr/dJx13QDVZlgtokyQ05Uz5S0INI h5s5gLkRqeK7vFamuwX7afRJU1f29Sg= Date: Thu, 21 Sep 2023 17:47:46 +0800 MIME-Version: 1.0 Subject: Re: [PATCH v4 4/8] hugetlb: perform vmemmap restoration on a list of pages X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Muchun Song To: Mike Kravetz Cc: Linux-MM , LKML , Muchun Song , Joao Martins , Oscar Salvador , David Hildenbrand , Miaohe Lin , David Rientjes , Anshuman Khandual , Naoya Horiguchi , Barry Song <21cnbao@gmail.com>, Michal Hocko , Matthew Wilcox , Xiongchun Duan , Andrew Morton References: <20230918230202.254631-1-mike.kravetz@oracle.com> <20230918230202.254631-5-mike.kravetz@oracle.com> <20230919205756.GB425719@monkey> <2FDB2018-74AE-4514-9B43-01664A8E5DBF@linux.dev> <20230921011223.GC4065@monkey> <306da2a1-0dd4-e858-930f-211947a466d2@linux.dev> In-Reply-To: <306da2a1-0dd4-e858-930f-211947a466d2@linux.dev> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-Rspamd-Queue-Id: 2D57120004 X-Rspam-User: X-Stat-Signature: szqk6eg76is9e19zw53fs176rtxsq8ys X-Rspamd-Server: rspam01 X-HE-Tag: 1695289676-288133 X-HE-Meta: U2FsdGVkX18z+lGqQiHJWV+XWzdQ9n0dl03Jh76t5BJkoz2s80w02tNDRtilyRh1d271pPZ6XwMIdWUIJNs8+mD0AopWd2UzfyXV9IJxTowV/T2F3zvDY5QLOeOAyVPGyQerd7e2bXEu7f6narWss9cZzbMc/FLWoDluPF73YRzy5rTE0tcr5sUOrxFVo6f16Bo1E8cABHdK+JSloUIKmIBuFS2NL4iz1U8LEx9qf8h8Yv6WcyORUVHERGElOe6e8eIZ1kO24ZFFVnAsRSgaN+gtKuCpmUP3oBOKLyxl0bN38pXRcAb3EkdwM18TiQeHWGySmJ2ambWns5oDjP4cB7BPI0LWyxtdV6Cb47/5tItpvA1TPB+fcqe0KexEHUnAnr8JlEqd7rvKh34HeCg3zT5d4odbz7lk8B+jLUbEoQY1RcTy27uh6MtRUgxR8z2Tr9BSR89omDkRMJqV1dzHv3h7RuxvDRGmKm2p7Uqp6mRBDKiE/46Fbv5NNZ3GF93HSpdmWRZbtM9esoDUC3EzT7UHgQljR1squ9/uu/SN5p+V47awhHqHEmR4Yf58P3VRSqPa1g8Pb+ArIDm7l0zRe+8bRp3qdLXBizgL+Zt2FztgKnX3C/5sHgxqtbpGkSMGN4v1B/3PQ/SKtbp+zM/QrrqmTj8XSSksH13N3zjDopqZinYGD/Bn/n0U3eOm+zrjpkyp4EINXMmdLzdndolIWnIwc0H9du/g1hziXOJuanKPMnCs09BrFaXnE+YIhDeVS0nMbM9sgMrrrlHLrohU7tDgIlkSpCa+WJyEPH58i2GkbEeD7HeOB/dUTFMwZWQxSB9UOX7Rn742hfEveg4ceWhEoksOd3yuMoDjz6NEiDl9oSTXMak7lLSzoFlkU0EqAhQ0ZYEqae9QxH7dLY5buI7N4V51xihq/62q9XZoVV1e8VbpxK5lXBZovvYRSUJI1DlbFDOLvODJ6XTBIaJ IHOutF/Y yeabZinlPTQX3dzJIsWnikaVfzbMXZSarvo79AMAaK6ZcZe7jW0l5L50rbn6iZUZry+tNJPegBm8dKwH84QFwZwFkPaD3PmD5blCL8qHvMVeDuPutH0pRJgcXlg== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 2023/9/21 17:31, Muchun Song wrote: > > > On 2023/9/21 09:12, Mike Kravetz wrote: >> On 09/20/23 11:03, Muchun Song wrote: >>>> On Sep 20, 2023, at 10:56, Muchun Song wrote: >>>>> On Sep 20, 2023, at 04:57, Mike Kravetz >>>>> wrote: >>>>> On 09/19/23 17:52, Muchun Song wrote: >>>>>> On 2023/9/19 07:01, Mike Kravetz wrote: >>>>>> >>>>>> I still think we should free a non-optimized HugeTLB page if we >>>>>> encounter an OOM situation instead of continue to restore >>>>>> vemmmap pages. Restoring vmemmmap pages will only aggravate >>>>>> the OOM situation. The suitable appraoch is to free a non-optimized >>>>>> HugeTLB page to satisfy our allocation of vmemmap pages, what's >>>>>> your opinion, Mike? >>>>> I agree. >>>>> >>>>> As you mentioned previously, this may complicate this code path a >>>>> bit. >>>>> I will rewrite to make this happen. >>>> Maybe we could introduced two list passed to >>>> update_and_free_pages_bulk (this >>>> will be easy for the callers of it), one is for non-optimized huge >>>> page, >>>> another is optimized one. In update_and_free_pages_bulk, we could >>>> first >>>> free those non-optimized huge page, and then restore vemmmap pages for >>>> those optimized ones, in which case, the code could be simple. >>>> hugetlb_vmemmap_restore_folios() dose not need to add complexity, >>>> which >>>> still continue to restore vmemmap pages and will stop once we >>>> encounter >>>> an OOM situation. >> I am not sure if passing in optimized and non-optimized lists to >> update_and_free_pages_bulk will help much.  IIUC, it will almost always >> be the case where only one list has entries.  Is that mostly accurate? > > I think you are right. It will be less helpful since most of > pages will be not optimized when HVO is enabled. Sorry, correction: **not** should be deleted. > >>> BTW, maybe we should try again iff there are some non-optimized huge >>> page >>> whose vmemmap pages are restored successfully previously and could >>> be freed >>> first, then continue to restore the vmemmap pages of the remaining >>> huge pages. >>> I think the retry code could be done in update_and_free_pages_bulk() >>> as well. >> I came up with a new routine to handle these ENOMEM returns from >> hugetlb_vmemmap_restore_folios.  I 'think' it handles these situations. >> Here is an updated version of this patch.  Sorry, diff makes it a bit >> hard to read. >> >>  From b13bdccb01730f995191944769f87d0725c289ad Mon Sep 17 00:00:00 2001 >> From: Mike Kravetz >> Date: Sun, 10 Sep 2023 16:14:50 -0700 >> Subject: [PATCH] hugetlb: perform vmemmap restoration on a list of pages >> >> The routine update_and_free_pages_bulk already performs vmemmap >> restoration on the list of hugetlb pages in a separate step.  In >> preparation for more functionality to be added in this step, create a >> new routine hugetlb_vmemmap_restore_folios() that will restore >> vmemmap for a list of folios. >> >> This new routine must provide sufficient feedback about errors and >> actual restoration performed so that update_and_free_pages_bulk can >> perform optimally. >> >> Special care must be taken when encountering a ENOMEM error from >> hugetlb_vmemmap_restore_folios.  We want to continue making as much >> forward progress as possible.  A new routine bulk_vmemmap_restore_enomem >> handles this specific situation. >> >> Signed-off-by: Mike Kravetz >> --- >>   mm/hugetlb.c         | 83 ++++++++++++++++++++++++++++++++++---------- >>   mm/hugetlb_vmemmap.c | 39 +++++++++++++++++++++ >>   mm/hugetlb_vmemmap.h | 11 ++++++ >>   3 files changed, 115 insertions(+), 18 deletions(-) >> >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >> index 70fedf8682c4..52abe56cf38a 100644 >> --- a/mm/hugetlb.c >> +++ b/mm/hugetlb.c >> @@ -1834,38 +1834,85 @@ static void >> update_and_free_hugetlb_folio(struct hstate *h, struct folio *folio, >>           schedule_work(&free_hpage_work); >>   } >>   -static void update_and_free_pages_bulk(struct hstate *h, struct >> list_head *list) >> +static void bulk_vmemmap_restore_enomem(struct hstate *h, >> +                        struct list_head *list, >> +                        unsigned long restored) >>   { >>       struct folio *folio, *t_folio; >> -    bool clear_dtor = false; >>   -    /* >> -     * First allocate required vmemmmap (if necessary) for all >> folios on >> -     * list.  If vmemmap can not be allocated, we can not free folio to >> -     * lower level allocator, so add back as hugetlb surplus page. >> -     * add_hugetlb_folio() removes the page from THIS list. >> -     * Use clear_dtor to note if vmemmap was successfully allocated for >> -     * ANY page on the list. >> -     */ >> -    list_for_each_entry_safe(folio, t_folio, list, lru) { >> -        if (folio_test_hugetlb_vmemmap_optimized(folio)) { >> +    if (restored) { >> +        /* >> +         * On ENOMEM error, free any restored hugetlb pages so that >> +         * restore of the entire list can be retried. >> +         * The idea is that by freeing hugetlb pages with vmemmap >> +         * (those previously restored) we will free up memory so that >> +         * we can allocate vmemmap for more hugetlb pages. >> +         * We must examine and possibly free EVERY hugetlb page on list >> +         * in order to call hugetlb_vmemmap_restore_folios again. >> +         * This is not optimal, but is an error case that should not >> +         * happen frequently. >> +         */ >> +        list_for_each_entry_safe(folio, t_folio, list, lru) >> +            if (!folio_test_hugetlb_vmemmap_optimized(folio)) { >> +                list_del(&folio->lru); >> +                spin_lock_irq(&hugetlb_lock); >> +                __clear_hugetlb_destructor(h, folio); >> +                spin_unlock_irq(&hugetlb_lock); >> +                update_and_free_hugetlb_folio(h, folio, false); >> +                cond_resched(); >> +            } >> +    } else { >> +        /* >> +         * In the case where vmemmap was not restored for ANY folios, >> +         * we loop through them trying to restore individually in the >> +         * hope that someone elsewhere may free enough memory. >> +         * If unable to restore a page, the hugetlb page is made a >> +         * surplus page and removed from the list. >> +         * If are able to restore vmemmap for one hugetlb page, we free >> +         * it and quit processing the list to retry the bulk operation. >> +         */ >> +        list_for_each_entry_safe(folio, t_folio, list, lru) >>               if (hugetlb_vmemmap_restore(h, &folio->page)) { >>                   spin_lock_irq(&hugetlb_lock); >>                   add_hugetlb_folio(h, folio, true); >>                   spin_unlock_irq(&hugetlb_lock); >> -            } else >> -                clear_dtor = true; >> -        } >> +            } else { >> +                list_del(&folio->lru); >> +                spin_lock_irq(&hugetlb_lock); >> +                __clear_hugetlb_destructor(h, folio); >> +                spin_unlock_irq(&hugetlb_lock); >> +                update_and_free_hugetlb_folio(h, folio, false); >> +                break; >> +            } >>       } >> +} >> + >> +static void update_and_free_pages_bulk(struct hstate *h, struct >> list_head *list) >> +{ >> +    int ret; >> +    unsigned long restored; >> +    struct folio *folio, *t_folio; >>         /* >> -     * If vmemmmap allocation was performed on any folio above, take >> lock >> -     * to clear destructor of all folios on list.  This avoids the >> need to >> +     * First allocate required vmemmmap (if necessary) for all folios. >> +     * Carefully handle ENOMEM errors and free up any available hugetlb >> +     * pages in order to make forward progress. >> +     */ >> +retry: >> +    ret = hugetlb_vmemmap_restore_folios(h, list, &restored); >> +    if (ret == -ENOMEM) { >> +        bulk_vmemmap_restore_enomem(h, list, restored); >> +        goto retry; >> +    } >> + >> +    /* >> +     * If vmemmmap allocation was performed on ANY folio , take lock to >> +     * clear destructor of all folios on list.  This avoids the need to >>        * lock/unlock for each individual folio. >>        * The assumption is vmemmap allocation was performed on all or >> none >>        * of the folios on the list.  This is true expect in VERY rare >> cases. >>        */ >> -    if (clear_dtor) { >> +    if (restored) { >>           spin_lock_irq(&hugetlb_lock); >>           list_for_each_entry(folio, list, lru) >>               __clear_hugetlb_destructor(h, folio); >> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c >> index 4558b814ffab..cc91edbfb68b 100644 >> --- a/mm/hugetlb_vmemmap.c >> +++ b/mm/hugetlb_vmemmap.c >> @@ -480,6 +480,45 @@ int hugetlb_vmemmap_restore(const struct hstate >> *h, struct page *head) >>       return ret; >>   } >>   +/** >> + * hugetlb_vmemmap_restore_folios - restore vmemmap for every folio >> on the list. >> + * @h:        struct hstate. >> + * @folio_list:    list of folios. >> + * @restored:    Set to number of folios for which vmemmap was restored >> + *        successfully if caller passes a non-NULL pointer. >> + * >> + * Return: %0 if vmemmap exists for all folios on the list.  If an >> error is >> + *        encountered restoring vmemmap for ANY folio, an error code >> + *        will be returned to the caller.  It is then the >> responsibility >> + *        of the caller to check the hugetlb vmemmap optimized flag of >> + *        each folio to determine if vmemmap was actually restored. >> + *        Note that processing is stopped when first error is >> encountered. >> + */ >> +int hugetlb_vmemmap_restore_folios(const struct hstate *h, >> +                    struct list_head *folio_list, >> +                    unsigned long *restored) > > How about changing parameter of @restored to a list_head type which > returns the non-optimized (previously) or vmemmap-restored-sucessful > huge pages? > In which case, the caller could traverse this returned list to free > them first like you have implemented in bulk_vmemmap_restore_enomem(), > it will be more efficient. The meaning of returned value should also > be changed accordingly since update_and_free_pages_bulk() wants to > whether there is a vmemmap-optimized huge page being restored sucessfully > to determine if it should clear hugetlb flag. So > hugetlb_vmemmap_restore_folios() > could return how many huge pages being restored successful, if a negative > number is returned meaning there is some error in the process of > restoring > of vmemmap. > > Thanks. > >> +{ >> +    unsigned long num_restored; >> +    struct folio *folio; >> +    int ret = 0; >> + >> +    num_restored = 0; >> +    list_for_each_entry(folio, folio_list, lru) { >> +        if (folio_test_hugetlb_vmemmap_optimized(folio)) { >> +            ret = hugetlb_vmemmap_restore(h, &folio->page); >> +            if (ret) >> +                goto out; >> +            else >> +                num_restored++; >> +        } >> +    } >> + >> +out: >> +    if (*restored) >> +        *restored = num_restored; >> +    return ret; >> +} >> + >>   /* Return true iff a HugeTLB whose vmemmap should and can be >> optimized. */ >>   static bool vmemmap_should_optimize(const struct hstate *h, const >> struct page *head) >>   { >> diff --git a/mm/hugetlb_vmemmap.h b/mm/hugetlb_vmemmap.h >> index c512e388dbb4..bb58453c3cc0 100644 >> --- a/mm/hugetlb_vmemmap.h >> +++ b/mm/hugetlb_vmemmap.h >> @@ -19,6 +19,8 @@ >>     #ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP >>   int hugetlb_vmemmap_restore(const struct hstate *h, struct page >> *head); >> +int hugetlb_vmemmap_restore_folios(const struct hstate *h, >> +            struct list_head *folio_list, unsigned long *restored); >>   void hugetlb_vmemmap_optimize(const struct hstate *h, struct page >> *head); >>   void hugetlb_vmemmap_optimize_folios(struct hstate *h, struct >> list_head *folio_list); >>   @@ -45,6 +47,15 @@ static inline int hugetlb_vmemmap_restore(const >> struct hstate *h, struct page *h >>       return 0; >>   } >>   +static inline int hugetlb_vmemmap_restore_folios(const struct >> hstate *h, >> +                    struct list_head *folio_list, >> +                    unsigned long *restored) >> +{ >> +    if (restored) >> +        *restored = 0; >> +    return 0; >> +} >> + >>   static inline void hugetlb_vmemmap_optimize(const struct hstate *h, >> struct page *head) >>   { >>   } >