[PATCH] mm, hugetlb: remove HUGETLB_CGROUP_MIN_ORDER

[PATCH] mm, hugetlb: remove HUGETLB_CGROUP_MIN_ORDER

[Frank van der Linden]

Originally, hugetlb_cgroup was the only hugetlb user of tail page
structure fields. So, the code defined and checked against
HUGETLB_CGROUP_MIN_ORDER to make sure pages weren't too small
to use.

However, by now, tail page #2 is used to store hugetlb
hwpoison and subpool information as well. In other words,
without that tail page hugetlb doesn't work.

Acknowledge this fact by getting rid of HUGETLB_CGROUP_MIN_ORDER
and checks against it. Instead, just check for the minimum viable
page order at hstate creation time.

Signed-off-by: Frank van der Linden <fvdl@google.com>
---
 include/linux/hugetlb_cgroup.h | 11 -----------
 mm/hugetlb.c                   |  2 +-
 mm/hugetlb_cgroup.c            | 20 ++------------------
 3 files changed, 3 insertions(+), 30 deletions(-)

diff --git a/include/linux/hugetlb_cgroup.h b/include/linux/hugetlb_cgroup.h
index 3d82d91f49ac..e5d64b8b59c2 100644
--- a/include/linux/hugetlb_cgroup.h
+++ b/include/linux/hugetlb_cgroup.h
@@ -22,13 +22,6 @@ struct resv_map;
 struct file_region;
 
 #ifdef CONFIG_CGROUP_HUGETLB
-/*
- * Minimum page order trackable by hugetlb cgroup.
- * At least 3 pages are necessary for all the tracking information.
- * The second tail page contains all of the hugetlb-specific fields.
- */
-#define HUGETLB_CGROUP_MIN_ORDER order_base_2(__NR_USED_SUBPAGE)
-
 enum hugetlb_memory_event {
 	HUGETLB_MAX,
 	HUGETLB_NR_MEMORY_EVENTS,
@@ -68,8 +61,6 @@ static inline struct hugetlb_cgroup *
 __hugetlb_cgroup_from_folio(struct folio *folio, bool rsvd)
 {
 	VM_BUG_ON_FOLIO(!folio_test_hugetlb(folio), folio);
-	if (folio_order(folio) < HUGETLB_CGROUP_MIN_ORDER)
-		return NULL;
 	if (rsvd)
 		return folio->_hugetlb_cgroup_rsvd;
 	else
@@ -91,8 +82,6 @@ static inline void __set_hugetlb_cgroup(struct folio *folio,
 				       struct hugetlb_cgroup *h_cg, bool rsvd)
 {
 	VM_BUG_ON_FOLIO(!folio_test_hugetlb(folio), folio);
-	if (folio_order(folio) < HUGETLB_CGROUP_MIN_ORDER)
-		return;
 	if (rsvd)
 		folio->_hugetlb_cgroup_rsvd = h_cg;
 	else
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 52d26072dfda..7c3e479abb68 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4282,7 +4282,7 @@ void __init hugetlb_add_hstate(unsigned int order)
 		return;
 	}
 	BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
-	BUG_ON(order == 0);
+	BUG_ON(order < order_base_2(__NR_USED_SUBPAGE));
 	h = &hstates[hugetlb_max_hstate++];
 	mutex_init(&h->resize_lock);
 	h->order = order;
diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
index dedd2edb076e..aa4486bd3904 100644
--- a/mm/hugetlb_cgroup.c
+++ b/mm/hugetlb_cgroup.c
@@ -262,12 +262,6 @@ static int __hugetlb_cgroup_charge_cgroup(int idx, unsigned long nr_pages,
 
 	if (hugetlb_cgroup_disabled())
 		goto done;
-	/*
-	 * We don't charge any cgroup if the compound page have less
-	 * than 3 pages.
-	 */
-	if (huge_page_order(&hstates[idx]) < HUGETLB_CGROUP_MIN_ORDER)
-		goto done;
 again:
 	rcu_read_lock();
 	h_cg = hugetlb_cgroup_from_task(current);
@@ -397,9 +391,6 @@ static void __hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 	if (hugetlb_cgroup_disabled() || !h_cg)
 		return;
 
-	if (huge_page_order(&hstates[idx]) < HUGETLB_CGROUP_MIN_ORDER)
-		return;
-
 	page_counter_uncharge(__hugetlb_cgroup_counter_from_cgroup(h_cg, idx,
 								   rsvd),
 			      nr_pages);
@@ -869,15 +860,8 @@ void __init hugetlb_cgroup_file_init(void)
 {
 	struct hstate *h;
 
-	for_each_hstate(h) {
-		/*
-		 * Add cgroup control files only if the huge page consists
-		 * of more than two normal pages. This is because we use
-		 * page[2].private for storing cgroup details.
-		 */
-		if (huge_page_order(h) >= HUGETLB_CGROUP_MIN_ORDER)
-			__hugetlb_cgroup_file_init(hstate_index(h));
-	}
+	for_each_hstate(h)
+		__hugetlb_cgroup_file_init(hstate_index(h));
 }
 
 /*
-- 
2.42.0.582.g8ccd20d70d-goog

Re: [PATCH] mm, hugetlb: remove HUGETLB_CGROUP_MIN_ORDER

[Mike Kravetz]

On 10/04/23 15:32, Frank van der Linden wrote:
> Originally, hugetlb_cgroup was the only hugetlb user of tail page
> structure fields. So, the code defined and checked against
> HUGETLB_CGROUP_MIN_ORDER to make sure pages weren't too small
> to use.
> 
> However, by now, tail page #2 is used to store hugetlb
> hwpoison and subpool information as well. In other words,
> without that tail page hugetlb doesn't work.

When I first read this, I thought we might be exposed today.  But, I see
that currently order must be > 0 so we are covered.

> Acknowledge this fact by getting rid of HUGETLB_CGROUP_MIN_ORDER
> and checks against it. Instead, just check for the minimum viable
> page order at hstate creation time.

IIUC, we do lose the ability to run with an order 1 hstate.  Correct?
The minimum must now be 2.
I do not think is worth worrying about.  And, the code checking for the
VERY unlikely case where order could be big enough to to be valid, but
too small for cgroups was strange.

I think this is a nice simplification.

> Signed-off-by: Frank van der Linden <fvdl@google.com>
> ---
>  include/linux/hugetlb_cgroup.h | 11 -----------
>  mm/hugetlb.c                   |  2 +-
>  mm/hugetlb_cgroup.c            | 20 ++------------------
>  3 files changed, 3 insertions(+), 30 deletions(-)

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
-- 
Mike Kravetz

Re: [PATCH] mm, hugetlb: remove HUGETLB_CGROUP_MIN_ORDER

[Frank van der Linden]

On Thu, Oct 5, 2023 at 5:53 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> On 10/04/23 15:32, Frank van der Linden wrote:
> > Originally, hugetlb_cgroup was the only hugetlb user of tail page
> > structure fields. So, the code defined and checked against
> > HUGETLB_CGROUP_MIN_ORDER to make sure pages weren't too small
> > to use.
> >
> > However, by now, tail page #2 is used to store hugetlb
> > hwpoison and subpool information as well. In other words,
> > without that tail page hugetlb doesn't work.
>
> When I first read this, I thought we might be exposed today.  But, I see
> that currently order must be > 0 so we are covered.
>
> > Acknowledge this fact by getting rid of HUGETLB_CGROUP_MIN_ORDER
> > and checks against it. Instead, just check for the minimum viable
> > page order at hstate creation time.
>
> IIUC, we do lose the ability to run with an order 1 hstate.  Correct?
> The minimum must now be 2.
> I do not think is worth worrying about.  And, the code checking for the
> VERY unlikely case where order could be big enough to to be valid, but
> too small for cgroups was strange.


Theoretically, order 1 hstate (without hugetlb cgroup) used to be
possible, since the subpool pointer was in the 1st tail page. But
dad6a5eb55 ("mm,hugetlb: use folio fields in second tail page")  moved
everything to fields in the 2nd tail page, so that hasn't worked for a
bit. I guess it would have let you create it, but things would have
gone south pretty quickly..
>
>
> I think this is a nice simplification.


Thanks for the review,

- Frank