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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aa500c4fb8e641e38b888325b2798989@unicloud.com>
Date:   Wed, 16 Mar 2022 01:40:24 +0000
From:   罗飞 <luofei@...cloud.com>
To:     Muchun Song <songmuchun@...edance.com>
CC:     Mike Kravetz <mike.kravetz@...cle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Memory Management List <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: 答复: [PATCH] hugetlbfs: fix description about atomic allocation of vmemmap pages when free huge page

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

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@...edance.com>
发送时间: 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@...cloud.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@...cloud.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.