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