Re: [PATCH 2/2] mm: rename page dtor functions to {compound,huge,transhuge}_page__dtor

Hi Khandual,

quoted

 						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.

It is because of name conflict. I think you already get it per below comments.
I will describe it in commit message.

quoted

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

quoted

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

Sure.

quoted

 	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.

echo!

quoted

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

will do it. Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Thanks,
Changbin Du