Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220315042355.362810-1-luofei@unicloud.com>
In-Reply-To: <20220315042355.362810-1-luofei@unicloud.com>
From:   Muchun Song <songmuchun@bytedance.com>
Date:   Tue, 15 Mar 2022 21:29:27 +0800
Message-ID: <CAMZfGtWjnhZLVmRD0BSpMbAWr_vD5BCj5s0ARfNHpHeAAGWYjA@mail.gmail.com>
Subject: Re: [PATCH] hugetlbfs: fix description about atomic allocation of
 vmemmap pages when free huge page
To:     luofei <luofei@unicloud.com>
Cc:     Mike Kravetz <mike.kravetz@oracle.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux Memory Management List <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Mar 15, 2022 at 12:24 PM luofei <luofei@unicloud.com> wrote:
>
> No matter what context update_and_free_page() is called in,
> the flag for allocating the vmemmap page is fixed
> (GFP_KERNEL | __GFP_NORETRY | __GFP_THISNODE), and no atomic
> allocation is involved, so the description of atomicity here
> is somewhat inappropriate.
>
> and the atomic parameter naming of update_and_free_page() is
> somewhat misleading.
>
> Signed-off-by: luofei <luofei@unicloud.com>
> ---
>  mm/hugetlb.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index f8ca7cca3c1a..239ef82b7897 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1570,8 +1570,8 @@ static void __update_and_free_page(struct hstate *h, struct page *page)
>
>  /*
>   * As update_and_free_page() can be called under any context, so we cannot
> - * use GFP_KERNEL to allocate vmemmap pages. However, we can defer the
> - * actual freeing in a workqueue to prevent from using GFP_ATOMIC to allocate
> + * use GFP_ATOMIC to allocate vmemmap pages. However, we can defer the
> + * actual freeing in a workqueue to prevent waits caused by allocating
>   * the vmemmap pages.
>   *
>   * free_hpage_workfn() locklessly retrieves the linked list of pages to be
> @@ -1617,16 +1617,14 @@ static inline void flush_free_hpage_work(struct hstate *h)
>  }
>
>  static void update_and_free_page(struct hstate *h, struct page *page,
> -                                bool atomic)
> +                                bool delay)

Hi luofei,

At least, I don't agree with this change.  The "atomic" means if the
caller is under atomic context instead of whether using atomic
GFP_MASK.  The "delay" seems to tell the caller that it can undelay
the allocation even if it is under atomic context (actually, it has no
choice).  But "atomic" can indicate the user is being asked to tell us
if it is under atomic context.

Thanks.