答复: [PATCH] hugetlbfs: fix description about atomic allocation of vmemmap pages when free huge page

Commit Message

luofei March 16, 2022, 2:26 a.m. UTC

>>>
>>> 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 <[e-mail]>
>>> ---
>>>  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

The comments made it clearer for me. I will revise the patch. Thanks :)

Patch

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