On Wed, Mar 16, 2022 at 5:16 AM Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
>
> On 3/15/22 06:29, Muchun Song wrote:
> > On Tue, Mar 15, 2022 at 12:24 PM luofei <luofei@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >> ---
> >> 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.
>
> There may be some confusion since GFP_ATOMIC is mentioned in the comments
> and GFP_ATOMIC is not used in the allocation of vmemmap pages. IIRC,
> the use of GFP_ATOMIC was discussed at one time but dismissed because of
> undesired side effects such as dipping into "atomic reserves".
>
> How about an update to the comments as follows (sorry mailer may mess up
> formatting)?
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index f8ca7cca3c1a..6a4d27e24b21 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1569,10 +1569,12 @@ 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
> - * the vmemmap pages.
> + * Freeing hugetlb pages in done in update_and_free_page(). When freeing a
> + * hugetlb page, vmemmap pages may need to be allocated. The routine
> + * alloc_huge_page_vmemmap() can possibly sleep as it uses GFP_KERNEL.
> + * However, update_and_free_page() can be called under any context. To
> + * avoid the possibility of sleeping in a context where sleeping is not
> + * allowed, defer the actual freeing in a workqueue where sleeping is allowed.
> *
> * free_hpage_workfn() locklessly retrieves the linked list of pages to be
> * freed and frees them one-by-one. As the page->mapping pointer is going
> @@ -1616,6 +1618,10 @@ static inline void flush_free_hpage_work(struct hstate *h)
> flush_work(&free_hpage_work);
> }
>
> +/*
> + * atomic == true indicates called from a context where sleeping is
> + * not allowed.
> + */
> static void update_and_free_page(struct hstate *h, struct page *page,
> bool atomic)
> {
> @@ -1625,7 +1631,8 @@ static void update_and_free_page(struct hstate *h, struct page *page,
> }
>
> /*
> - * Defer freeing to avoid using GFP_ATOMIC to allocate vmemmap pages.
> + * Defer freeing to avoid possible sleeping when allocating
> + * vmemmap pages.
> *
> * Only call schedule_work() if hpage_freelist is previously
> * empty. Otherwise, schedule_work() had been called but the workfn
>
LGTM. Thanks Mike.
