Received-SPF: pass (google.com: domain of linux-kernel+bounces-55850-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From: Muchun Song <muchun.song@linux.dev>
Precedence: bulk
Mime-Version: 1.0
Subject: Re: [PATCH v3] mm: hugetlb: improve the handling of hugetlb allocation failure for freed or in-use hugetlb
Date: Wed, 7 Feb 2024 10:24:52 +0800
Message-Id: <CB00D336-84AE-412C-ACA4-254EDD3C2279@linux.dev>
References: <62890fd60b1ecd5bf1cdc476c973f60fe37aa0cb.1707181934.git.baolin.wang@linux.alibaba.com>
Cc: akpm@linux-foundation.org, osalvador@suse.de, david@redhat.com,
 mhocko@kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org
In-Reply-To: <62890fd60b1ecd5bf1cdc476c973f60fe37aa0cb.1707181934.git.baolin.wang@linux.alibaba.com>
To: Baolin Wang <baolin.wang@linux.alibaba.com>


> On Feb 6, 2024, at 11:08, Baolin Wang <baolin.wang@linux.alibaba.com> wrot=
e:
>=20
> =EF=BB=BFalloc_and_dissolve_hugetlb_folio() preallocates a new hugetlb pag=
e before
> it takes hugetlb_lock. In 3 out of 4 cases the page is not really used and=

> therefore the newly allocated page is just freed right away. This is
> wasteful and it might cause pre-mature failures in those cases.
>=20
> Address that by moving the allocation down to the only case (hugetlb
> page is really in the free pages pool). We need to drop hugetlb_lock
> to do so and therefore need to recheck the page state after regaining
> it.
>=20
> The patch is more of a cleanup than an actual fix to an existing
> problem. There are no known reports about pre-mature failures.
>=20
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Reviewed-by: Muchun Song <muchun.song@linux.dev>

Thanks

> ---
> Changes from v2=EF=BC=9B
> - Update the commit message suggested by Michal.
> - Remove unnecessary comments.
> Changes from v1:
> - Update the suject line per Muchun.
> - Move the allocation into the free hugetlb handling branch per Michal.
> ---
> mm/hugetlb.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>=20
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 9d996fe4ecd9..a05507a2143f 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3031,21 +3031,9 @@ static int alloc_and_dissolve_hugetlb_folio(struct h=
state *h,
> {
>    gfp_t gfp_mask =3D htlb_alloc_mask(h) | __GFP_THISNODE;
>    int nid =3D folio_nid(old_folio);
> -    struct folio *new_folio;
> +    struct folio *new_folio =3D NULL;
>    int ret =3D 0;
>=20
> -    /*
> -     * Before dissolving the folio, we need to allocate a new one for the=

> -     * pool to remain stable.  Here, we allocate the folio and 'prep' it
> -     * by doing everything but actually updating counters and adding to
> -     * the pool.  This simplifies and let us do most of the processing
> -     * under the lock.
> -     */
> -    new_folio =3D alloc_buddy_hugetlb_folio(h, gfp_mask, nid, NULL, NULL)=
;
> -    if (!new_folio)
> -        return -ENOMEM;
> -    __prep_new_hugetlb_folio(h, new_folio);
> -
> retry:
>    spin_lock_irq(&hugetlb_lock);
>    if (!folio_test_hugetlb(old_folio)) {
> @@ -3075,6 +3063,16 @@ static int alloc_and_dissolve_hugetlb_folio(struct h=
state *h,
>        cond_resched();
>        goto retry;
>    } else {
> +        if (!new_folio) {
> +            spin_unlock_irq(&hugetlb_lock);
> +            new_folio =3D alloc_buddy_hugetlb_folio(h, gfp_mask, nid,
> +                                  NULL, NULL);
> +            if (!new_folio)
> +                return -ENOMEM;
> +            __prep_new_hugetlb_folio(h, new_folio);
> +            goto retry;
> +        }
> +
>        /*
>         * Ok, old_folio is still a genuine free hugepage. Remove it from
>         * the freelist and decrease the counters. These will be
> @@ -3102,9 +3100,11 @@ static int alloc_and_dissolve_hugetlb_folio(struct h=
state *h,
>=20
> free_new:
>    spin_unlock_irq(&hugetlb_lock);
> -    /* Folio has a zero ref count, but needs a ref to be freed */
> -    folio_ref_unfreeze(new_folio, 1);
> -    update_and_free_hugetlb_folio(h, new_folio, false);
> +    if (new_folio) {
> +        /* Folio has a zero ref count, but needs a ref to be freed */
> +        folio_ref_unfreeze(new_folio, 1);
> +        update_and_free_hugetlb_folio(h, new_folio, false);
> +    }
>=20
>    return ret;
> }
> --=20
> 2.39.3
>=20