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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



>> 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.

Based on the comments related to avoiding atomic allocation pages,
I would have thought that atomic variable had something to do with
this, it seems I misunderstood the meaning of the variable.

Thanks:)
________________________________________
发件人: Muchun Song <songmuchun@xxxxxxxxxxxxx>
发送时间: 2022年3月15日 21:29:27
收件人: 罗飞
抄送: Mike Kravetz; Andrew Morton; Linux Memory Management List; LKML
主题: Re: [PATCH] hugetlbfs: fix description about atomic allocation of vmemmap pages when free huge page

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.

Thanks.




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux