| 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
| ||
|
Message-Id: <4911ff99-ac77-0344-8696-a15ca9f3e763@linux.vnet.ibm.com>
Date: Tue, 17 Oct 2017 14:06:58 +0530
From: Anshuman Khandual <khandual@...ux.vnet.ibm.com>
To: changbin.du@...el.com, akpm@...ux-foundation.org, corbet@....net,
hughd@...gle.com
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mm@...ck.org
Subject: Re: [PATCH 2/2] mm: rename page dtor functions to
{compound,huge,transhuge}_page__dtor
On 10/16/2017 02:49 PM, changbin.du@...el.com wrote:
> From: Changbin Du <changbin.du@...el.com>
>
> The current name free_{huge,transhuge}_page are paired with
> alloc_{huge,transhuge}_page functions, but the actual page free
> function is still free_page() which will indirectly call
> free_{huge,transhuge}_page. So this patch removes this confusion
> by renaming all the compound page dtors.
>
> Signed-off-by: Changbin Du <changbin.du@...el.com>
> ---
> Documentation/vm/hugetlbfs_reserv.txt | 4 ++--
> include/linux/huge_mm.h | 2 +-
> include/linux/hugetlb.h | 2 +-
> include/linux/mm.h | 8 ++++----
> mm/huge_memory.c | 4 ++--
> mm/hugetlb.c | 14 +++++++-------
> mm/page_alloc.c | 10 +++++-----
> mm/swap.c | 2 +-
> mm/userfaultfd.c | 2 +-
> 9 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/Documentation/vm/hugetlbfs_reserv.txt b/Documentation/vm/hugetlbfs_reserv.txt
> index 9aca09a..b3ffa3e 100644
> --- a/Documentation/vm/hugetlbfs_reserv.txt
> +++ b/Documentation/vm/hugetlbfs_reserv.txt
> @@ -238,7 +238,7 @@ to the global reservation count (resv_huge_pages).
>
> Freeing Huge Pages
> ------------------
> -Huge page freeing is performed by the routine free_huge_page(). This routine
> +Huge page freeing is performed by the routine huge_page_dtor(). This routine
> is the destructor for hugetlbfs compound pages. As a result, it is only
> passed a pointer to the page struct. When a huge page is freed, reservation
> accounting may need to be performed. This would be the case if the page was
> @@ -468,7 +468,7 @@ However, there are several instances where errors are encountered after a huge
> page is allocated but before it is instantiated. In this case, the page
> allocation has consumed the reservation and made the appropriate subpool,
> reservation map and global count adjustments. If the page is freed at this
> -time (before instantiation and clearing of PagePrivate), then free_huge_page
> +time (before instantiation and clearing of PagePrivate), then huge_page_dtor
> will increment the global reservation count. However, the reservation map
> indicates the reservation was consumed. This resulting inconsistent state
> will cause the 'leak' of a reserved huge page. The global reserve count will
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index 1dd2c33..40ae3058 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -130,7 +130,7 @@ extern unsigned long thp_get_unmapped_area(struct file *filp,
> unsigned long addr, unsigned long len, unsigned long pgoff,
> unsigned long flags);
>
> -extern void free_transhuge_page(struct page *page);
> +extern void transhuge_page_dtor(struct page *page);
>
> struct page *alloc_transhuge_page_vma(gfp_t gfp_mask,
> struct vm_area_struct *vma, unsigned long addr);
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 8bbbd37..24492c5 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -118,7 +118,7 @@ long hugetlb_unreserve_pages(struct inode *inode, long start, long end,
> long freed);
> bool isolate_huge_page(struct page *page, struct list_head *list);
> void putback_active_hugepage(struct page *page);
> -void free_huge_page(struct page *page);
> +void huge_page_dtor(struct page *page);
> void hugetlb_fix_reserve_counts(struct inode *inode);
> extern struct mutex *hugetlb_fault_mutex_table;
> u32 hugetlb_fault_mutex_hash(struct hstate *h, struct mm_struct *mm,
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 065d99d..adfa906 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -616,7 +616,7 @@ void split_page(struct page *page, unsigned int order);
> * prototype for that function and accessor functions.
> * These are _only_ valid on the head of a compound page.
> */
> -typedef void compound_page_dtor(struct page *);
> +typedef void compound_page_dtor_t(struct page *);
Why changing this ? I understand _t kind of specifies it more
like a type def but this patch is just to rename the compound
page destructor functions. Not sure we should change datatype
here as well in this patch.
>
> /* Keep the enum in sync with compound_page_dtors array in mm/page_alloc.c */
> enum compound_dtor_id {
> @@ -630,7 +630,7 @@ enum compound_dtor_id {
> #endif
> NR_COMPOUND_DTORS,
> };
> -extern compound_page_dtor * const compound_page_dtors[];
> +extern compound_page_dtor_t * const compound_page_dtors[];
>
> static inline void set_compound_page_dtor(struct page *page,
> enum compound_dtor_id compound_dtor)
> @@ -639,7 +639,7 @@ static inline void set_compound_page_dtor(struct page *page,
> page[1].compound_dtor = compound_dtor;
> }
>
> -static inline compound_page_dtor *get_compound_page_dtor(struct page *page)
> +static inline compound_page_dtor_t *get_compound_page_dtor(struct page *page)
Which is adding these kind of changes to the patch without
having a corresponding description in the commit message.
> {
> VM_BUG_ON_PAGE(page[1].compound_dtor >= NR_COMPOUND_DTORS, page);
> return compound_page_dtors[page[1].compound_dtor];
> @@ -657,7 +657,7 @@ static inline void set_compound_order(struct page *page, unsigned int order)
> page[1].compound_order = order;
> }
>
> -void free_compound_page(struct page *page);
> +void compound_page_dtor(struct page *page);
>
> #ifdef CONFIG_MMU
> /*
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index e267488..a01125b 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2717,7 +2717,7 @@ fail: if (mapping)
> return ret;
> }
>
> -void free_transhuge_page(struct page *page)
> +void transhuge_page_dtor(struct page *page)
> {
> struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
> unsigned long flags;
> @@ -2728,7 +2728,7 @@ void free_transhuge_page(struct page *page)
> list_del(page_deferred_list(page));
> }
> spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
> - free_compound_page(page);
> + compound_page_dtor(page);
> }
>
> void deferred_split_huge_page(struct page *page)
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 424b0ef..1af2c4e7 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1250,7 +1250,7 @@ static void clear_page_huge_active(struct page *page)
> ClearPagePrivate(&page[1]);
> }
>
> -void free_huge_page(struct page *page)
> +void huge_page_dtor(struct page *page)
> {
> /*
> * Can't pass hstate in here because it is called from the
> @@ -1363,7 +1363,7 @@ int PageHeadHuge(struct page *page_head)
> if (!PageHead(page_head))
> return 0;
>
> - return get_compound_page_dtor(page_head) == free_huge_page;
> + return get_compound_page_dtor(page_head) == huge_page_dtor;
> }
>
> pgoff_t __basepage_index(struct page *page)
> @@ -1932,11 +1932,11 @@ static long vma_add_reservation(struct hstate *h,
> * specific error paths, a huge page was allocated (via alloc_huge_page)
> * and is about to be freed. If a reservation for the page existed,
> * alloc_huge_page would have consumed the reservation and set PagePrivate
> - * in the newly allocated page. When the page is freed via free_huge_page,
> + * in the newly allocated page. When the page is freed via huge_page_dtor,
> * the global reservation count will be incremented if PagePrivate is set.
> - * However, free_huge_page can not adjust the reserve map. Adjust the
> + * However, huge_page_dtor can not adjust the reserve map. Adjust the
> * reserve map here to be consistent with global reserve count adjustments
> - * to be made by free_huge_page.
> + * to be made by huge_page_dtor.
> */
> static void restore_reserve_on_error(struct hstate *h,
> struct vm_area_struct *vma, unsigned long address,
> @@ -1950,7 +1950,7 @@ static void restore_reserve_on_error(struct hstate *h,
> * Rare out of memory condition in reserve map
> * manipulation. Clear PagePrivate so that
> * global reserve count will not be incremented
> - * by free_huge_page. This will make it appear
> + * by huge_page_dtor. This will make it appear
> * as though the reservation for this page was
> * consumed. This may prevent the task from
> * faulting in the page at a later time. This
> @@ -2304,7 +2304,7 @@ static unsigned long set_max_huge_pages(struct hstate *h, unsigned long count,
> while (count > persistent_huge_pages(h)) {
> /*
> * If this allocation races such that we no longer need the
> - * page, free_huge_page will handle it by freeing the page
> + * page, huge_page_dtor will handle it by freeing the page
> * and reducing the surplus.
> */
> spin_unlock(&hugetlb_lock);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 77e4d3c..b31205c 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -248,14 +248,14 @@ char * const migratetype_names[MIGRATE_TYPES] = {
> #endif
> };
>
> -compound_page_dtor * const compound_page_dtors[] = {
> +compound_page_dtor_t * const compound_page_dtors[] = {
Adding this chunk as well.
> NULL,
> - free_compound_page,
> + compound_page_dtor,
> #ifdef CONFIG_HUGETLB_PAGE
> - free_huge_page,
> + huge_page_dtor,
> #endif
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> - free_transhuge_page,
> + transhuge_page_dtor,
> #endif
> };
Having *dtor* in the destructor functions for the huge pages
(all of them) actually makes sense. It wont be confused with
a lot other free_* functions and some of them dealing with
THP/HugeTLB as well.
>
> @@ -586,7 +586,7 @@ static void bad_page(struct page *page, const char *reason,
> * This usage means that zero-order pages may not be compound.
> */
>
> -void free_compound_page(struct page *page)
> +void compound_page_dtor(struct page *page)
> {
> __free_pages_ok(page, compound_order(page));
> }
> diff --git a/mm/swap.c b/mm/swap.c
> index a77d68f..8f98caf 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -81,7 +81,7 @@ static void __put_single_page(struct page *page)
>
> static void __put_compound_page(struct page *page)
> {
> - compound_page_dtor *dtor;
> + compound_page_dtor_t *dtor;
If the typedef change needs to be retained then the commit message
must include a line.
Powered by blists - more mailing lists