mm/hugetlb: create hugetlb cgroup file in hugetlb_init

Build kernel with CONFIG_HUGETLBFS=y,CONFIG_HUGETLB_PAGE=y and CONFIG_CGROUP_HUGETLB=y, then specify hugepagesz=xx boot option, system will boot fail.

This failure is caused by following code path:
setup_hugepagesz
hugetlb_add_hstate
hugetlb_cgroup_file_init
cgroup_add_cftypes
kzalloc <--slab is *not available* yet

For this path, slab is not available yet, so memory allocated will be failed, and cause WARN_ON() in hugetlb_cgroup_file_init().

So I move hugetlb_cgroup_file_init() into hugetlb_init().

Signed-off-by Jianguo Wu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 include/linux/hugetlb_cgroup.h |    7 ++-----
 mm/hugetlb.c                   |   11 +----------
 mm/hugetlb_cgroup.c            |   23 +++++++++++++++++++++--
 3 files changed, 24 insertions(+), 17 deletions(-)

include/linux/hugetlb_cgroup.h +2 -5

index d73878c..5bb9c28 100644
@@ -62,7 +62,7 @@ extern void hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages,
 					 struct page *page);
 extern void hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 					   struct hugetlb_cgroup *h_cg);
-extern int hugetlb_cgroup_file_init(int idx) __init;
+extern void hugetlb_cgroup_file_init(void) __init;
 extern void hugetlb_cgroup_migrate(struct page *oldhpage,
 				   struct page *newhpage);
 

@@ -111,10 +111,7 @@ hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 	return;
 }
 
-static inline int __init hugetlb_cgroup_file_init(int idx)
-{
-	return 0;
-}
+static inline void __init hugetlb_cgroup_file_init() {}
 
 static inline void hugetlb_cgroup_migrate(struct page *oldhpage,
 					  struct page *newhpage)

mm/hugetlb.c +1 -10

index 1ef2cd4..a30da48 100644
@@ -1906,14 +1906,12 @@ static int __init hugetlb_init(void)
 		default_hstate.max_huge_pages = default_hstate_max_huge_pages;
 
 	hugetlb_init_hstates();
-
 	gather_bootmem_prealloc();
-
 	report_hugepages();
 
 	hugetlb_sysfs_init();
-
 	hugetlb_register_all_nodes();
+	hugetlb_cgroup_file_init();
 
 	return 0;
 }

@@ -1943,13 +1941,6 @@ void __init hugetlb_add_hstate(unsigned order)
 	h->next_nid_to_free = first_node(node_states[N_HIGH_MEMORY]);
 	snprintf(h->name, HSTATE_NAME_LEN, "hugepages-%lukB",
 					huge_page_size(h)/1024);
-	/*
-	 * Add cgroup control files only if the huge page consists
-	 * of more than two normal pages. This is because we use
-	 * page[2].lru.next for storing cgoup details.
-	 */
-	if (order >= HUGETLB_CGROUP_MIN_ORDER)
-		hugetlb_cgroup_file_init(hugetlb_max_hstate - 1);
 
 	parsed_hstate = h;
 }

mm/hugetlb_cgroup.c +21 -3

index a3f358f..284cb68 100644
@@ -340,7 +340,7 @@ static char *mem_fmt(char *buf, int size, unsigned long hsize)
 	return buf;
 }
 
-int __init hugetlb_cgroup_file_init(int idx)
+static void __init __hugetlb_cgroup_file_init(int idx)
 {
 	char buf[32];
 	struct cftype *cft;

@@ -382,7 +382,26 @@ int __init hugetlb_cgroup_file_init(int idx)
 
 	WARN_ON(cgroup_add_cftypes(&hugetlb_subsys, h->cgroup_files));
 
-	return 0;
+	return;
+}
+
+void __init hugetlb_cgroup_file_init()
+{
+	struct hstate *h;
+	int idx;
+
+	idx = 0;
+	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].lru.next for storing cgoup details.
+		 */
+		if (h->order >= HUGETLB_CGROUP_MIN_ORDER)
+			__hugetlb_cgroup_file_init(idx);
+
+		idx++;
+	}
 }
 
 /*
-- 1.7.1

Build kernel with CONFIG_HUGETLBFS=y,CONFIG_HUGETLB_PAGE=y and CONFIG_CGROUP_HUGETLB=y, then specify hugepagesz=xx boot option, system will boot fail.

This failure is caused by following code path:
setup_hugepagesz
hugetlb_add_hstate
hugetlb_cgroup_file_init
cgroup_add_cftypes
kzalloc <--slab is *not available* yet

For this path, slab is not available yet, so memory allocated will be failed, and cause WARN_ON() in hugetlb_cgroup_file_init().

So I move hugetlb_cgroup_file_init() into hugetlb_init().

Signed-off-by Jianguo Wu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 include/linux/hugetlb_cgroup.h |    7 ++-----
 mm/hugetlb.c                   |   11 +----------
 mm/hugetlb_cgroup.c            |   23 +++++++++++++++++++++--
 3 files changed, 24 insertions(+), 17 deletions(-)

include/linux/hugetlb_cgroup.h +2 -5

index d73878c..5bb9c28 100644
@@ -62,7 +62,7 @@ extern void hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages,
 					 struct page *page);
 extern void hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 					   struct hugetlb_cgroup *h_cg);
-extern int hugetlb_cgroup_file_init(int idx) __init;
+extern void hugetlb_cgroup_file_init(void) __init;
 extern void hugetlb_cgroup_migrate(struct page *oldhpage,
 				   struct page *newhpage);
 

@@ -111,10 +111,7 @@ hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 	return;
 }
 
-static inline int __init hugetlb_cgroup_file_init(int idx)
-{
-	return 0;
-}
+static inline void __init hugetlb_cgroup_file_init() {}
 
 static inline void hugetlb_cgroup_migrate(struct page *oldhpage,
 					  struct page *newhpage)

mm/hugetlb.c +1 -10

index 1ef2cd4..a30da48 100644
@@ -1906,14 +1906,12 @@ static int __init hugetlb_init(void)
 		default_hstate.max_huge_pages = default_hstate_max_huge_pages;
 
 	hugetlb_init_hstates();
-
 	gather_bootmem_prealloc();
-
 	report_hugepages();
 
 	hugetlb_sysfs_init();
-
 	hugetlb_register_all_nodes();
+	hugetlb_cgroup_file_init();
 
 	return 0;
 }

@@ -1943,13 +1941,6 @@ void __init hugetlb_add_hstate(unsigned order)
 	h->next_nid_to_free = first_node(node_states[N_HIGH_MEMORY]);
 	snprintf(h->name, HSTATE_NAME_LEN, "hugepages-%lukB",
 					huge_page_size(h)/1024);
-	/*
-	 * Add cgroup control files only if the huge page consists
-	 * of more than two normal pages. This is because we use
-	 * page[2].lru.next for storing cgoup details.
-	 */
-	if (order >= HUGETLB_CGROUP_MIN_ORDER)
-		hugetlb_cgroup_file_init(hugetlb_max_hstate - 1);
 
 	parsed_hstate = h;
 }

mm/hugetlb_cgroup.c +21 -3

index a3f358f..284cb68 100644
@@ -340,7 +340,7 @@ static char *mem_fmt(char *buf, int size, unsigned long hsize)
 	return buf;
 }
 
-int __init hugetlb_cgroup_file_init(int idx)
+static void __init __hugetlb_cgroup_file_init(int idx)
 {
 	char buf[32];
 	struct cftype *cft;

@@ -382,7 +382,26 @@ int __init hugetlb_cgroup_file_init(int idx)
 
 	WARN_ON(cgroup_add_cftypes(&hugetlb_subsys, h->cgroup_files));
 
-	return 0;
+	return;
+}
+
+void __init hugetlb_cgroup_file_init()
+{
+	struct hstate *h;
+	int idx;
+
+	idx = 0;
+	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].lru.next for storing cgoup details.
+		 */
+		if (h->order >= HUGETLB_CGROUP_MIN_ORDER)
+			__hugetlb_cgroup_file_init(idx);
+
+		idx++;
+	}
 }
 
 /*
-- 1.7.1

Quoted 11 lines · Jianguo Wu

Build kernel with CONFIG_HUGETLBFS=y,CONFIG_HUGETLB_PAGE=y and CONFIG_CGROUP_HUGETLB=y, then specify hugepagesz=xx boot option, system will boot fail.

This failure is caused by following code path:
setup_hugepagesz
hugetlb_add_hstate
hugetlb_cgroup_file_init
cgroup_add_cftypes
kzalloc <--slab is *not available* yet

Quoted 4 lines

For this path, slab is not available yet, so memory allocated will be failed, and cause WARN_ON() in hugetlb_cgroup_file_init().

So I move hugetlb_cgroup_file_init() into hugetlb_init().

I do not think this is a good idea. hugetlb_init is in __init section as well so what guarantees that the slab is initialized by then? Isn't this just a good ordering that makes this working?

Shouldn't this be rather placed in hugetlb_cgroup_create?

Quoted 107 lines

Signed-off-by Jianguo Wu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 include/linux/hugetlb_cgroup.h |    7 ++-----
 mm/hugetlb.c                   |   11 +----------
 mm/hugetlb_cgroup.c            |   23 +++++++++++++++++++++--
 3 files changed, 24 insertions(+), 17 deletions(-)

include/linux/hugetlb_cgroup.h +2 -5

index d73878c..5bb9c28 100644
@@ -62,7 +62,7 @@ extern void hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages,
 					 struct page *page);
 extern void hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 					   struct hugetlb_cgroup *h_cg);
-extern int hugetlb_cgroup_file_init(int idx) __init;
+extern void hugetlb_cgroup_file_init(void) __init;
 extern void hugetlb_cgroup_migrate(struct page *oldhpage,
 				   struct page *newhpage);
 

@@ -111,10 +111,7 @@ hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 	return;
 }
 
-static inline int __init hugetlb_cgroup_file_init(int idx)
-{
-	return 0;
-}
+static inline void __init hugetlb_cgroup_file_init() {}
 
 static inline void hugetlb_cgroup_migrate(struct page *oldhpage,
 					  struct page *newhpage)

mm/hugetlb.c +1 -10

index 1ef2cd4..a30da48 100644
@@ -1906,14 +1906,12 @@ static int __init hugetlb_init(void)
 		default_hstate.max_huge_pages = default_hstate_max_huge_pages;
 
 	hugetlb_init_hstates();
-
 	gather_bootmem_prealloc();
-
 	report_hugepages();
 
 	hugetlb_sysfs_init();
-
 	hugetlb_register_all_nodes();
+	hugetlb_cgroup_file_init();
 
 	return 0;
 }

@@ -1943,13 +1941,6 @@ void __init hugetlb_add_hstate(unsigned order)
 	h->next_nid_to_free = first_node(node_states[N_HIGH_MEMORY]);
 	snprintf(h->name, HSTATE_NAME_LEN, "hugepages-%lukB",
 					huge_page_size(h)/1024);
-	/*
-	 * Add cgroup control files only if the huge page consists
-	 * of more than two normal pages. This is because we use
-	 * page[2].lru.next for storing cgoup details.
-	 */
-	if (order >= HUGETLB_CGROUP_MIN_ORDER)
-		hugetlb_cgroup_file_init(hugetlb_max_hstate - 1);
 
 	parsed_hstate = h;
 }

mm/hugetlb_cgroup.c +21 -2

index a3f358f..284cb68 100644
@@ -340,7 +340,7 @@ static char *mem_fmt(char *buf, int size, unsigned long hsize)
 	return buf;
 }
 
-int __init hugetlb_cgroup_file_init(int idx)
+static void __init __hugetlb_cgroup_file_init(int idx)
 {
 	char buf[32];
 	struct cftype *cft;

@@ -382,7 +382,26 @@ int __init hugetlb_cgroup_file_init(int idx)
 
 	WARN_ON(cgroup_add_cftypes(&hugetlb_subsys, h->cgroup_files));
 
-	return 0;
+	return;
+}
+
+void __init hugetlb_cgroup_file_init()
+{
+	struct hstate *h;
+	int idx;
+
+	idx = 0;
+	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].lru.next for storing cgoup details.
+		 */
+		if (h->order >= HUGETLB_CGROUP_MIN_ORDER)
+			__hugetlb_cgroup_file_init(idx);
+
+		idx++;
+	}
 }
 

mm/hugetlb_cgroup.c int __init hugetlb_cgroup_file_init(int idx)

Quoted 3 lines

mm/hugetlb_cgroup.c +0 -1

 /*
-- 1.7.1

Quoted 11 lines · Jianguo Wu

Build kernel with CONFIG_HUGETLBFS=y,CONFIG_HUGETLB_PAGE=y and CONFIG_CGROUP_HUGETLB=y, then specify hugepagesz=xx boot option, system will boot fail.

This failure is caused by following code path:
setup_hugepagesz
hugetlb_add_hstate
hugetlb_cgroup_file_init
cgroup_add_cftypes
kzalloc <--slab is *not available* yet

Quoted 4 lines

For this path, slab is not available yet, so memory allocated will be failed, and cause WARN_ON() in hugetlb_cgroup_file_init().

So I move hugetlb_cgroup_file_init() into hugetlb_init().

I do not think this is a good idea. hugetlb_init is in __init section as well so what guarantees that the slab is initialized by then? Isn't this just a good ordering that makes this working?

Shouldn't this be rather placed in hugetlb_cgroup_create?

Quoted 110 lines

Signed-off-by Jianguo Wu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 include/linux/hugetlb_cgroup.h |    7 ++-----
 mm/hugetlb.c                   |   11 +----------
 mm/hugetlb_cgroup.c            |   23 +++++++++++++++++++++--
 3 files changed, 24 insertions(+), 17 deletions(-)

include/linux/hugetlb_cgroup.h +2 -5

index d73878c..5bb9c28 100644
@@ -62,7 +62,7 @@ extern void hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages,
 					 struct page *page);
 extern void hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 					   struct hugetlb_cgroup *h_cg);
-extern int hugetlb_cgroup_file_init(int idx) __init;
+extern void hugetlb_cgroup_file_init(void) __init;
 extern void hugetlb_cgroup_migrate(struct page *oldhpage,
 				   struct page *newhpage);
 

@@ -111,10 +111,7 @@ hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 	return;
 }
 
-static inline int __init hugetlb_cgroup_file_init(int idx)
-{
-	return 0;
-}
+static inline void __init hugetlb_cgroup_file_init() {}
 
 static inline void hugetlb_cgroup_migrate(struct page *oldhpage,
 					  struct page *newhpage)

mm/hugetlb.c +1 -10

index 1ef2cd4..a30da48 100644
@@ -1906,14 +1906,12 @@ static int __init hugetlb_init(void)
 		default_hstate.max_huge_pages = default_hstate_max_huge_pages;
 
 	hugetlb_init_hstates();
-
 	gather_bootmem_prealloc();
-
 	report_hugepages();
 
 	hugetlb_sysfs_init();
-
 	hugetlb_register_all_nodes();
+	hugetlb_cgroup_file_init();
 
 	return 0;
 }

@@ -1943,13 +1941,6 @@ void __init hugetlb_add_hstate(unsigned order)
 	h->next_nid_to_free = first_node(node_states[N_HIGH_MEMORY]);
 	snprintf(h->name, HSTATE_NAME_LEN, "hugepages-%lukB",
 					huge_page_size(h)/1024);
-	/*
-	 * Add cgroup control files only if the huge page consists
-	 * of more than two normal pages. This is because we use
-	 * page[2].lru.next for storing cgoup details.
-	 */
-	if (order >= HUGETLB_CGROUP_MIN_ORDER)
-		hugetlb_cgroup_file_init(hugetlb_max_hstate - 1);
 
 	parsed_hstate = h;
 }

mm/hugetlb_cgroup.c +21 -3

index a3f358f..284cb68 100644
@@ -340,7 +340,7 @@ static char *mem_fmt(char *buf, int size, unsigned long hsize)
 	return buf;
 }
 
-int __init hugetlb_cgroup_file_init(int idx)
+static void __init __hugetlb_cgroup_file_init(int idx)
 {
 	char buf[32];
 	struct cftype *cft;

@@ -382,7 +382,26 @@ int __init hugetlb_cgroup_file_init(int idx)
 
 	WARN_ON(cgroup_add_cftypes(&hugetlb_subsys, h->cgroup_files));
 
-	return 0;
+	return;
+}
+
+void __init hugetlb_cgroup_file_init()
+{
+	struct hstate *h;
+	int idx;
+
+	idx = 0;
+	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].lru.next for storing cgoup details.
+		 */
+		if (h->order >= HUGETLB_CGROUP_MIN_ORDER)
+			__hugetlb_cgroup_file_init(idx);
+
+		idx++;
+	}
 }
 
 /*
-- 1.7.1

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Jianguo Wu

Build kernel with CONFIG_HUGETLBFS=y,CONFIG_HUGETLB_PAGE=y and CONFIG_CGROUP_HUGETLB=y, then specify hugepagesz=xx boot option, system will boot fail.

This failure is caused by following code path:
setup_hugepagesz
hugetlb_add_hstate
hugetlb_cgroup_file_init
cgroup_add_cftypes
kzalloc <--slab is *not available* yet

Quoted 8 lines

Quoted 4 lines

For this path, slab is not available yet, so memory allocated will be failed, and cause WARN_ON() in hugetlb_cgroup_file_init().

So I move hugetlb_cgroup_file_init() into hugetlb_init().

I do not think this is a good idea. hugetlb_init is in __init section as well so what guarantees that the slab is initialized by then? Isn't this just a good ordering that makes this working?

Hi Michal,

__initcall functions will be called in
start_kernel()

	rest_init()  // -> slab is already
		kernel_init()
			kernel_init_freeable()
				do_basic_setup()
					do_initcalls()

and setup_hugepagesz() will be called in
start_kernel()

	parse_early_param()  // -> before mm_init() -> kmem_cache_init()

Is this right?

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Jianguo Wu

Build kernel with CONFIG_HUGETLBFS=y,CONFIG_HUGETLB_PAGE=y and CONFIG_CGROUP_HUGETLB=y, then specify hugepagesz=xx boot option, system will boot fail.

This failure is caused by following code path:
setup_hugepagesz
hugetlb_add_hstate
hugetlb_cgroup_file_init
cgroup_add_cftypes
kzalloc <--slab is *not available* yet

Quoted 8 lines

Quoted 4 lines

For this path, slab is not available yet, so memory allocated will be failed, and cause WARN_ON() in hugetlb_cgroup_file_init().

So I move hugetlb_cgroup_file_init() into hugetlb_init().

I do not think this is a good idea. hugetlb_init is in __init section as well so what guarantees that the slab is initialized by then? Isn't this just a good ordering that makes this working?

Hi Michal,

__initcall functions will be called in
start_kernel()

	rest_init()  // -> slab is already
		kernel_init()
			kernel_init_freeable()
				do_basic_setup()
					do_initcalls()

and setup_hugepagesz() will be called in
start_kernel()

	parse_early_param()  // -> before mm_init() -> kmem_cache_init()

Is this right?

On Wed 12-12-12 18:44:13, Xishi Qiu wrote:

Quoted 10 lines

Hi Michal,

__initcall functions will be called in
start_kernel()

	rest_init()  // -> slab is already
		kernel_init()
			kernel_init_freeable()
				do_basic_setup()
					do_initcalls()

Quoted 5 lines

and setup_hugepagesz() will be called in
start_kernel()

	parse_early_param()  // -> before mm_init() -> kmem_cache_init()

Is this right?

Yes this is right. I just noticed that kmem_cache_init_late is an __init function as well and didn't realize it is called directly. Sorry about the confusion.
Anyway I still think it would be a better idea to move the call into the hugetlb_cgroup_create callback where it is more logical IMO but now that I'm looking at other controllers (blk and kmem.tcp) they all do this from init calls as well. So it doesn't make sense to have hugetlb behave differently.

So

Acked-by Michal Hocko <[email protected]>

On Wed 12-12-12 18:44:13, Xishi Qiu wrote:

Quoted 10 lines

Hi Michal,

__initcall functions will be called in
start_kernel()

	rest_init()  // -> slab is already
		kernel_init()
			kernel_init_freeable()
				do_basic_setup()
					do_initcalls()

Quoted 5 lines

and setup_hugepagesz() will be called in
start_kernel()

	parse_early_param()  // -> before mm_init() -> kmem_cache_init()

Is this right?

Yes this is right. I just noticed that kmem_cache_init_late is an __init function as well and didn't realize it is called directly. Sorry about the confusion.
Anyway I still think it would be a better idea to move the call into the hugetlb_cgroup_create callback where it is more logical IMO but now that I'm looking at other controllers (blk and kmem.tcp) they all do this from init calls as well. So it doesn't make sense to have hugetlb behave differently.

So

Acked-by Michal Hocko <[email protected]>

Build kernel with CONFIG_HUGETLBFS=y,CONFIG_HUGETLB_PAGE=y and CONFIG_CGROUP_HUGETLB=y, then specify hugepagesz=xx boot option, system will boot fail.

This failure is caused by following code path:
setup_hugepagesz
hugetlb_add_hstate
hugetlb_cgroup_file_init
cgroup_add_cftypes
kzalloc <--slab is *not available* yet

For this path, slab is not available yet, so memory allocated will be failed, and cause WARN_ON() in hugetlb_cgroup_file_init().

So I move hugetlb_cgroup_file_init() into hugetlb_init().

Signed-off-by Jianguo Wu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 include/linux/hugetlb_cgroup.h |    7 ++-----
 mm/hugetlb.c                   |   11 +----------
 mm/hugetlb_cgroup.c            |   23 +++++++++++++++++++++--
 3 files changed, 24 insertions(+), 17 deletions(-)

include/linux/hugetlb_cgroup.h +2 -5

index d73878c..5bb9c28 100644
@@ -62,7 +62,7 @@ extern void hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages,
 					 struct page *page);
 extern void hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 					   struct hugetlb_cgroup *h_cg);
-extern int hugetlb_cgroup_file_init(int idx) __init;
+extern void hugetlb_cgroup_file_init(void) __init;
 extern void hugetlb_cgroup_migrate(struct page *oldhpage,
 				   struct page *newhpage);
 

@@ -111,10 +111,7 @@ hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 	return;
 }
 
-static inline int __init hugetlb_cgroup_file_init(int idx)
-{
-	return 0;
-}
+static inline void __init hugetlb_cgroup_file_init() {}
 
 static inline void hugetlb_cgroup_migrate(struct page *oldhpage,
 					  struct page *newhpage)

mm/hugetlb.c +1 -10

index 1ef2cd4..a30da48 100644
@@ -1906,14 +1906,12 @@ static int __init hugetlb_init(void)
 		default_hstate.max_huge_pages = default_hstate_max_huge_pages;
 
 	hugetlb_init_hstates();
-
 	gather_bootmem_prealloc();
-
 	report_hugepages();
 
 	hugetlb_sysfs_init();
-
 	hugetlb_register_all_nodes();
+	hugetlb_cgroup_file_init();
 
 	return 0;
 }

@@ -1943,13 +1941,6 @@ void __init hugetlb_add_hstate(unsigned order)
 	h->next_nid_to_free = first_node(node_states[N_HIGH_MEMORY]);
 	snprintf(h->name, HSTATE_NAME_LEN, "hugepages-%lukB",
 					huge_page_size(h)/1024);
-	/*
-	 * Add cgroup control files only if the huge page consists
-	 * of more than two normal pages. This is because we use
-	 * page[2].lru.next for storing cgoup details.
-	 */
-	if (order >= HUGETLB_CGROUP_MIN_ORDER)
-		hugetlb_cgroup_file_init(hugetlb_max_hstate - 1);
 
 	parsed_hstate = h;
 }

mm/hugetlb_cgroup.c +8 -2

index a3f358f..284cb68 100644
@@ -340,7 +340,7 @@ static char *mem_fmt(char *buf, int size, unsigned long hsize)
 	return buf;
 }
 
-int __init hugetlb_cgroup_file_init(int idx)
+static void __init __hugetlb_cgroup_file_init(int idx)
 {
 	char buf[32];
 	struct cftype *cft;

@@ -382,7 +382,26 @@ int __init hugetlb_cgroup_file_init(int idx)
 
 	WARN_ON(cgroup_add_cftypes(&hugetlb_subsys, h->cgroup_files));
 
-	return 0;
+	return;
+}
+
+void __init hugetlb_cgroup_file_init()
+{
+	struct hstate *h;
+	int idx;

Build kernel with CONFIG_HUGETLBFS=y,CONFIG_HUGETLB_PAGE=y and CONFIG_CGROUP_HUGETLB=y, then specify hugepagesz=xx boot option, system will boot fail.

This failure is caused by following code path:
setup_hugepagesz
hugetlb_add_hstate
hugetlb_cgroup_file_init
cgroup_add_cftypes
kzalloc <--slab is *not available* yet

For this path, slab is not available yet, so memory allocated will be failed, and cause WARN_ON() in hugetlb_cgroup_file_init().

So I move hugetlb_cgroup_file_init() into hugetlb_init().

Signed-off-by Jianguo Wu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 include/linux/hugetlb_cgroup.h |    7 ++-----
 mm/hugetlb.c                   |   11 +----------
 mm/hugetlb_cgroup.c            |   23 +++++++++++++++++++++--
 3 files changed, 24 insertions(+), 17 deletions(-)

include/linux/hugetlb_cgroup.h +2 -5

index d73878c..5bb9c28 100644
@@ -62,7 +62,7 @@ extern void hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages,
 					 struct page *page);
 extern void hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 					   struct hugetlb_cgroup *h_cg);
-extern int hugetlb_cgroup_file_init(int idx) __init;
+extern void hugetlb_cgroup_file_init(void) __init;
 extern void hugetlb_cgroup_migrate(struct page *oldhpage,
 				   struct page *newhpage);
 

@@ -111,10 +111,7 @@ hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 	return;
 }
 
-static inline int __init hugetlb_cgroup_file_init(int idx)
-{
-	return 0;
-}
+static inline void __init hugetlb_cgroup_file_init() {}
 
 static inline void hugetlb_cgroup_migrate(struct page *oldhpage,
 					  struct page *newhpage)

mm/hugetlb.c +1 -10

index 1ef2cd4..a30da48 100644
@@ -1906,14 +1906,12 @@ static int __init hugetlb_init(void)
 		default_hstate.max_huge_pages = default_hstate_max_huge_pages;
 
 	hugetlb_init_hstates();
-
 	gather_bootmem_prealloc();
-
 	report_hugepages();
 
 	hugetlb_sysfs_init();
-
 	hugetlb_register_all_nodes();
+	hugetlb_cgroup_file_init();
 
 	return 0;
 }

@@ -1943,13 +1941,6 @@ void __init hugetlb_add_hstate(unsigned order)
 	h->next_nid_to_free = first_node(node_states[N_HIGH_MEMORY]);
 	snprintf(h->name, HSTATE_NAME_LEN, "hugepages-%lukB",
 					huge_page_size(h)/1024);
-	/*
-	 * Add cgroup control files only if the huge page consists
-	 * of more than two normal pages. This is because we use
-	 * page[2].lru.next for storing cgoup details.
-	 */
-	if (order >= HUGETLB_CGROUP_MIN_ORDER)
-		hugetlb_cgroup_file_init(hugetlb_max_hstate - 1);
 
 	parsed_hstate = h;
 }

mm/hugetlb_cgroup.c +8 -2

index a3f358f..284cb68 100644
@@ -340,7 +340,7 @@ static char *mem_fmt(char *buf, int size, unsigned long hsize)
 	return buf;
 }
 
-int __init hugetlb_cgroup_file_init(int idx)
+static void __init __hugetlb_cgroup_file_init(int idx)
 {
 	char buf[32];
 	struct cftype *cft;

@@ -382,7 +382,26 @@ int __init hugetlb_cgroup_file_init(int idx)
 
 	WARN_ON(cgroup_add_cftypes(&hugetlb_subsys, h->cgroup_files));
 
-	return 0;
+	return;
+}
+
+void __init hugetlb_cgroup_file_init()
+{
+	struct hstate *h;
+	int idx;

Quoted 37 lines · Xishi Qiu

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Jianguo Wu

Build kernel with CONFIG_HUGETLBFS=y,CONFIG_HUGETLB_PAGE=y and CONFIG_CGROUP_HUGETLB=y, then specify hugepagesz=xx boot option, system will boot fail.

This failure is caused by following code path:
setup_hugepagesz
hugetlb_add_hstate
hugetlb_cgroup_file_init
cgroup_add_cftypes
kzalloc <--slab is *not available* yet

Quoted 8 lines

Quoted 4 lines

For this path, slab is not available yet, so memory allocated will be failed, and cause WARN_ON() in hugetlb_cgroup_file_init().

So I move hugetlb_cgroup_file_init() into hugetlb_init().

I do not think this is a good idea. hugetlb_init is in __init section as well so what guarantees that the slab is initialized by then? Isn't this just a good ordering that makes this working?

Hi Michal,

__initcall functions will be called in
start_kernel()

	rest_init()  // -> slab is already
		kernel_init()
			kernel_init_freeable()
				do_basic_setup()
					do_initcalls()

and setup_hugepagesz() will be called in
start_kernel()

	parse_early_param()  // -> before mm_init() -> kmem_cache_init()

Quoted 1 line

Is this right?

Yes this is right. I just noticed that kmem_cache_init_late is an __init function as well and didn't realize it is called directly. Sorry about the confusion.
Anyway I still think it would be a better idea to move the call into the hugetlb_cgroup_create callback where it is more logical IMO but now that

I'm looking at other controllers (blk and kmem.tcp) they all do this from init calls as well. So it doesn't make sense to have hugetlb behave differently.

So

Acked-by Michal Hocko <[email protected]>

Thanks!

Quoted 37 lines · Xishi Qiu

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Jianguo Wu

Build kernel with CONFIG_HUGETLBFS=y,CONFIG_HUGETLB_PAGE=y and CONFIG_CGROUP_HUGETLB=y, then specify hugepagesz=xx boot option, system will boot fail.

This failure is caused by following code path:
setup_hugepagesz
hugetlb_add_hstate
hugetlb_cgroup_file_init
cgroup_add_cftypes
kzalloc <--slab is *not available* yet

Quoted 8 lines

Quoted 4 lines

For this path, slab is not available yet, so memory allocated will be failed, and cause WARN_ON() in hugetlb_cgroup_file_init().

So I move hugetlb_cgroup_file_init() into hugetlb_init().

I do not think this is a good idea. hugetlb_init is in __init section as well so what guarantees that the slab is initialized by then? Isn't this just a good ordering that makes this working?

Hi Michal,

__initcall functions will be called in
start_kernel()

	rest_init()  // -> slab is already
		kernel_init()
			kernel_init_freeable()
				do_basic_setup()
					do_initcalls()

and setup_hugepagesz() will be called in
start_kernel()

	parse_early_param()  // -> before mm_init() -> kmem_cache_init()

Quoted 1 line

Is this right?

Yes this is right. I just noticed that kmem_cache_init_late is an __init function as well and didn't realize it is called directly. Sorry about the confusion.
Anyway I still think it would be a better idea to move the call into the hugetlb_cgroup_create callback where it is more logical IMO but now that

I'm looking at other controllers (blk and kmem.tcp) they all do this from init calls as well. So it doesn't make sense to have hugetlb behave differently.

So

Acked-by Michal Hocko <[email protected]>

Thanks!

Quoted 108 lines · Jianguo Wu

Build kernel with CONFIG_HUGETLBFS=y,CONFIG_HUGETLB_PAGE=y and CONFIG_CGROUP_HUGETLB=y, then specify hugepagesz=xx boot option, system will boot fail.

This failure is caused by following code path:
setup_hugepagesz
hugetlb_add_hstate
hugetlb_cgroup_file_init
cgroup_add_cftypes
kzalloc <--slab is *not available* yet

For this path, slab is not available yet, so memory allocated will be failed, and cause WARN_ON() in hugetlb_cgroup_file_init().

So I move hugetlb_cgroup_file_init() into hugetlb_init().

Signed-off-by Jianguo Wu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 include/linux/hugetlb_cgroup.h |    7 ++-----
 mm/hugetlb.c                   |   11 +----------
 mm/hugetlb_cgroup.c            |   23 +++++++++++++++++++++--
 3 files changed, 24 insertions(+), 17 deletions(-)

include/linux/hugetlb_cgroup.h +2 -5

index d73878c..5bb9c28 100644
@@ -62,7 +62,7 @@ extern void hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages,
 					 struct page *page);
 extern void hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 					   struct hugetlb_cgroup *h_cg);
-extern int hugetlb_cgroup_file_init(int idx) __init;
+extern void hugetlb_cgroup_file_init(void) __init;
 extern void hugetlb_cgroup_migrate(struct page *oldhpage,
 				   struct page *newhpage);
 

@@ -111,10 +111,7 @@ hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 	return;
 }
 
-static inline int __init hugetlb_cgroup_file_init(int idx)
-{
-	return 0;
-}
+static inline void __init hugetlb_cgroup_file_init() {}
 
 static inline void hugetlb_cgroup_migrate(struct page *oldhpage,
 					  struct page *newhpage)

mm/hugetlb.c +1 -10

index 1ef2cd4..a30da48 100644
@@ -1906,14 +1906,12 @@ static int __init hugetlb_init(void)
 		default_hstate.max_huge_pages = default_hstate_max_huge_pages;
 
 	hugetlb_init_hstates();
-
 	gather_bootmem_prealloc();
-
 	report_hugepages();
 
 	hugetlb_sysfs_init();
-
 	hugetlb_register_all_nodes();
+	hugetlb_cgroup_file_init();
 
 	return 0;
 }

@@ -1943,13 +1941,6 @@ void __init hugetlb_add_hstate(unsigned order)
 	h->next_nid_to_free = first_node(node_states[N_HIGH_MEMORY]);
 	snprintf(h->name, HSTATE_NAME_LEN, "hugepages-%lukB",
 					huge_page_size(h)/1024);
-	/*
-	 * Add cgroup control files only if the huge page consists
-	 * of more than two normal pages. This is because we use
-	 * page[2].lru.next for storing cgoup details.
-	 */
-	if (order >= HUGETLB_CGROUP_MIN_ORDER)
-		hugetlb_cgroup_file_init(hugetlb_max_hstate - 1);
 
 	parsed_hstate = h;
 }

mm/hugetlb_cgroup.c +8 -2

index a3f358f..284cb68 100644
@@ -340,7 +340,7 @@ static char *mem_fmt(char *buf, int size, unsigned long hsize)
 	return buf;
 }
 
-int __init hugetlb_cgroup_file_init(int idx)
+static void __init __hugetlb_cgroup_file_init(int idx)
 {
 	char buf[32];
 	struct cftype *cft;

@@ -382,7 +382,26 @@ int __init hugetlb_cgroup_file_init(int idx)
 
 	WARN_ON(cgroup_add_cftypes(&hugetlb_subsys, h->cgroup_files));
 
-	return 0;
+	return;
+}
+
+void __init hugetlb_cgroup_file_init()
+{
+	struct hstate *h;
+	int idx;

mm/hugetlb_cgroup.c int __init hugetlb_cgroup_file_init(int idx) · line 401

Quoted 10 lines

mm/hugetlb_cgroup.c +10 -0

+
+	idx = 0;
+	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].lru.next for storing cgoup details.
+		 */
+		if (h->order >= HUGETLB_CGROUP_MIN_ORDER)
+			__hugetlb_cgroup_file_init(idx);

Is it better to say ?

if (huge_page_order(h) >= HUGETLB_CGROUP_MIN_ORDER)
__hugetlb_cgroup_file_init(hstate_index(h));

It should be ok both case.

Reviewed-by Aneesh Kumar K.V <[email protected]>

mm/hugetlb_cgroup.c int __init hugetlb_cgroup_file_init(int idx)

Quoted 7 lines

mm/hugetlb_cgroup.c +3 -1

+
+		idx++;
+	}
 }
 
 /*
-- 1.7.1

-anesh

.

Quoted 108 lines · Jianguo Wu

Build kernel with CONFIG_HUGETLBFS=y,CONFIG_HUGETLB_PAGE=y and CONFIG_CGROUP_HUGETLB=y, then specify hugepagesz=xx boot option, system will boot fail.

This failure is caused by following code path:
setup_hugepagesz
hugetlb_add_hstate
hugetlb_cgroup_file_init
cgroup_add_cftypes
kzalloc <--slab is *not available* yet

For this path, slab is not available yet, so memory allocated will be failed, and cause WARN_ON() in hugetlb_cgroup_file_init().

So I move hugetlb_cgroup_file_init() into hugetlb_init().

Signed-off-by Jianguo Wu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 include/linux/hugetlb_cgroup.h |    7 ++-----
 mm/hugetlb.c                   |   11 +----------
 mm/hugetlb_cgroup.c            |   23 +++++++++++++++++++++--
 3 files changed, 24 insertions(+), 17 deletions(-)

include/linux/hugetlb_cgroup.h +2 -5

index d73878c..5bb9c28 100644
@@ -62,7 +62,7 @@ extern void hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages,
 					 struct page *page);
 extern void hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 					   struct hugetlb_cgroup *h_cg);
-extern int hugetlb_cgroup_file_init(int idx) __init;
+extern void hugetlb_cgroup_file_init(void) __init;
 extern void hugetlb_cgroup_migrate(struct page *oldhpage,
 				   struct page *newhpage);
 

@@ -111,10 +111,7 @@ hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
 	return;
 }
 
-static inline int __init hugetlb_cgroup_file_init(int idx)
-{
-	return 0;
-}
+static inline void __init hugetlb_cgroup_file_init() {}
 
 static inline void hugetlb_cgroup_migrate(struct page *oldhpage,
 					  struct page *newhpage)

mm/hugetlb.c +1 -10

index 1ef2cd4..a30da48 100644
@@ -1906,14 +1906,12 @@ static int __init hugetlb_init(void)
 		default_hstate.max_huge_pages = default_hstate_max_huge_pages;
 
 	hugetlb_init_hstates();
-
 	gather_bootmem_prealloc();
-
 	report_hugepages();
 
 	hugetlb_sysfs_init();
-
 	hugetlb_register_all_nodes();
+	hugetlb_cgroup_file_init();
 
 	return 0;
 }

@@ -1943,13 +1941,6 @@ void __init hugetlb_add_hstate(unsigned order)
 	h->next_nid_to_free = first_node(node_states[N_HIGH_MEMORY]);
 	snprintf(h->name, HSTATE_NAME_LEN, "hugepages-%lukB",
 					huge_page_size(h)/1024);
-	/*
-	 * Add cgroup control files only if the huge page consists
-	 * of more than two normal pages. This is because we use
-	 * page[2].lru.next for storing cgoup details.
-	 */
-	if (order >= HUGETLB_CGROUP_MIN_ORDER)
-		hugetlb_cgroup_file_init(hugetlb_max_hstate - 1);
 
 	parsed_hstate = h;
 }

mm/hugetlb_cgroup.c +8 -2

index a3f358f..284cb68 100644
@@ -340,7 +340,7 @@ static char *mem_fmt(char *buf, int size, unsigned long hsize)
 	return buf;
 }
 
-int __init hugetlb_cgroup_file_init(int idx)
+static void __init __hugetlb_cgroup_file_init(int idx)
 {
 	char buf[32];
 	struct cftype *cft;

@@ -382,7 +382,26 @@ int __init hugetlb_cgroup_file_init(int idx)
 
 	WARN_ON(cgroup_add_cftypes(&hugetlb_subsys, h->cgroup_files));
 
-	return 0;
+	return;
+}
+
+void __init hugetlb_cgroup_file_init()
+{
+	struct hstate *h;
+	int idx;

mm/hugetlb_cgroup.c int __init hugetlb_cgroup_file_init(int idx) · line 401

Quoted 10 lines

mm/hugetlb_cgroup.c +10 -0

+
+	idx = 0;
+	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].lru.next for storing cgoup details.
+		 */
+		if (h->order >= HUGETLB_CGROUP_MIN_ORDER)
+			__hugetlb_cgroup_file_init(idx);

Is it better to say ?

if (huge_page_order(h) >= HUGETLB_CGROUP_MIN_ORDER)
__hugetlb_cgroup_file_init(hstate_index(h));

It should be ok both case.

Reviewed-by Aneesh Kumar K.V <[email protected]>

mm/hugetlb_cgroup.c int __init hugetlb_cgroup_file_init(int idx)

Quoted 7 lines

mm/hugetlb_cgroup.c +3 -1

+
+		idx++;
+	}
 }
 
 /*
-- 1.7.1

-anesh

.

Quoted 33 lines · Xishi Qiu

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Jianguo Wu

Build kernel with CONFIG_HUGETLBFS=y,CONFIG_HUGETLB_PAGE=y and CONFIG_CGROUP_HUGETLB=y, then specify hugepagesz=xx boot option, system will boot fail.

This failure is caused by following code path:
setup_hugepagesz
hugetlb_add_hstate
hugetlb_cgroup_file_init
cgroup_add_cftypes
kzalloc <--slab is *not available* yet

Quoted 8 lines

Quoted 4 lines

For this path, slab is not available yet, so memory allocated will be failed, and cause WARN_ON() in hugetlb_cgroup_file_init().

So I move hugetlb_cgroup_file_init() into hugetlb_init().

I do not think this is a good idea. hugetlb_init is in __init section as well so what guarantees that the slab is initialized by then? Isn't this just a good ordering that makes this working?

Hi Michal,

__initcall functions will be called in
start_kernel()

	rest_init()  // -> slab is already
		kernel_init()
			kernel_init_freeable()
				do_basic_setup()
					do_initcalls()

Quoted 5 lines

and setup_hugepagesz() will be called in
start_kernel()

	parse_early_param()  // -> before mm_init() -> kmem_cache_init()

Is this right?

Yes this is right. I just noticed that kmem_cache_init_late is an __init function as well and didn't realize it is called directly. Sorry about the confusion.
Anyway I still think it would be a better idea to move the call into the hugetlb_cgroup_create callback where it is more logical IMO but now that I'm looking at other controllers (blk and kmem.tcp) they all do this from init calls as well. So it doesn't make sense to have hugetlb behave differently.

So

Acked-by Michal Hocko <[email protected]>

Thanks!