Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init

Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init

[Michal Hocko]

On Wed 12-12-12 16:25:59, Jianguo Wu wrote:
> 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().

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?

> Signed-off-by: Jianguo Wu <wujianguo-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jiang Liu <jiang.liu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  include/linux/hugetlb_cgroup.h |    7 ++-----
>  mm/hugetlb.c                   |   11 +----------
>  mm/hugetlb_cgroup.c            |   23 +++++++++++++++++++++--
>  3 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/hugetlb_cgroup.h b/include/linux/hugetlb_cgroup.h
> index d73878c..5bb9c28 100644
> --- a/include/linux/hugetlb_cgroup.h
> +++ b/include/linux/hugetlb_cgroup.h
> @@ -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)
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 1ef2cd4..a30da48 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -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;
>  }
> diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
> index a3f358f..284cb68 100644
> --- a/mm/hugetlb_cgroup.c
> +++ b/mm/hugetlb_cgroup.c
> @@ -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
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init

[Aneesh Kumar K.V]

Jianguo Wu <wujianguo-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> writes:

> 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 <wujianguo-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jiang Liu <jiang.liu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  include/linux/hugetlb_cgroup.h |    7 ++-----
>  mm/hugetlb.c                   |   11 +----------
>  mm/hugetlb_cgroup.c            |   23 +++++++++++++++++++++--
>  3 files changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/hugetlb_cgroup.h b/include/linux/hugetlb_cgroup.h
> index d73878c..5bb9c28 100644
> --- a/include/linux/hugetlb_cgroup.h
> +++ b/include/linux/hugetlb_cgroup.h
> @@ -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)
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 1ef2cd4..a30da48 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -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;
>  }
> diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
> index a3f358f..284cb68 100644
> --- a/mm/hugetlb_cgroup.c
> +++ b/mm/hugetlb_cgroup.c
> @@ -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);

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 <aneesh.kumar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

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

-anesh

Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init

[Xishi Qiu]

On 2012/12/12 18:19, Michal Hocko wrote:

> On Wed 12-12-12 16:25:59, Jianguo Wu wrote:
>> 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().
> 
> 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?

Thanks
Xishi Qiu

> Shouldn't this be rather placed in hugetlb_cgroup_create?
> 
>> Signed-off-by: Jianguo Wu <wujianguo-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Jiang Liu <jiang.liu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>  include/linux/hugetlb_cgroup.h |    7 ++-----
>>  mm/hugetlb.c                   |   11 +----------
>>  mm/hugetlb_cgroup.c            |   23 +++++++++++++++++++++--
>>  3 files changed, 24 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/linux/hugetlb_cgroup.h b/include/linux/hugetlb_cgroup.h
>> index d73878c..5bb9c28 100644
>> --- a/include/linux/hugetlb_cgroup.h
>> +++ b/include/linux/hugetlb_cgroup.h
>> @@ -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)
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 1ef2cd4..a30da48 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -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;
>>  }
>> diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
>> index a3f358f..284cb68 100644
>> --- a/mm/hugetlb_cgroup.c
>> +++ b/mm/hugetlb_cgroup.c
>> @@ -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
>>
> 

Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init

[Michal Hocko]

On Wed 12-12-12 18:44:13, Xishi Qiu wrote:
> On 2012/12/12 18:19, Michal Hocko wrote:
> 
> > On Wed 12-12-12 16:25:59, Jianguo Wu wrote:
> >> 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().
> > 
> > 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?

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 <mhocko-AlSwsSmVLrQ@public.gmane.org>

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init

[Michal Hocko]

On Wed 12-12-12 12:23:29, Michal Hocko wrote:
> On Wed 12-12-12 18:44:13, Xishi Qiu wrote:
[...]
> > 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?
> 
> 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 <mhocko-AlSwsSmVLrQ@public.gmane.org>

Ohh, and this deserves to be backported to stable (since 3.6).
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init

[Jianguo Wu]

On 2012/12/12 19:23, Michal Hocko wrote:

> On Wed 12-12-12 18:44:13, Xishi Qiu wrote:
>> On 2012/12/12 18:19, Michal Hocko wrote:
>>
>>> On Wed 12-12-12 16:25:59, Jianguo Wu wrote:
>>>> 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().
>>>
>>> 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?
> 
> 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

Hi Michal,

Thanks for your review and comments:).
hugetlb_cgroup_create is a callback of hugetlb_subsys,
and hugetlb_cgroup_file_init add h->cgroup_files to hugetlb_subsys,
so we cann't move hugetlb_cgroup_file_init into hugetlb_cgroup_create, right?

Thanks,
Jianguo wu

> 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 <mhocko-AlSwsSmVLrQ@public.gmane.org>
> 
> Thanks!

Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init

[Simon Jeons]

On Wed, 2012-12-12 at 12:23 +0100, Michal Hocko wrote:
> On Wed 12-12-12 18:44:13, Xishi Qiu wrote:
> > On 2012/12/12 18:19, Michal Hocko wrote:
> > 
> > > On Wed 12-12-12 16:25:59, Jianguo Wu wrote:
> > >> 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().
> > > 
> > > 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?
> 
> 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.

Which callback in cgroup_subsys should hugetlb_cgroup_create associated?
Currently, there is no such callback.  

> 
> So
> Acked-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
> 
> Thanks!

Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init

[Jianguo Wu]

On 2012/12/13 1:05, Aneesh Kumar K.V wrote:

> Jianguo Wu <wujianguo-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> writes:
> 
>> 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 <wujianguo-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Jiang Liu <jiang.liu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>  include/linux/hugetlb_cgroup.h |    7 ++-----
>>  mm/hugetlb.c                   |   11 +----------
>>  mm/hugetlb_cgroup.c            |   23 +++++++++++++++++++++--
>>  3 files changed, 24 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/linux/hugetlb_cgroup.h b/include/linux/hugetlb_cgroup.h
>> index d73878c..5bb9c28 100644
>> --- a/include/linux/hugetlb_cgroup.h
>> +++ b/include/linux/hugetlb_cgroup.h
>> @@ -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)
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 1ef2cd4..a30da48 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -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;
>>  }
>> diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
>> index a3f358f..284cb68 100644
>> --- a/mm/hugetlb_cgroup.c
>> +++ b/mm/hugetlb_cgroup.c
>> @@ -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);
> 
> Is it better to say ?
> 
>              if (huge_page_order(h) >= HUGETLB_CGROUP_MIN_ORDER)
>                           __hugetlb_cgroup_file_init(hstate_index(h));

Hi Aneesh,

Thanks for your review and suggestion, this is better.

Thanks,
Jianguo Wu

> 
> It should be ok both case.
> 
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
>> +
>> +		idx++;
>> +	}
>>  }
>>
>>  /*
>> -- 1.7.1
> 
> -anesh
> 
> 
> .
> 

[PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init

[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 <wujianguo-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jiang Liu <jiang.liu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 include/linux/hugetlb_cgroup.h |    7 ++-----
 mm/hugetlb.c                   |   11 +----------
 mm/hugetlb_cgroup.c            |   23 +++++++++++++++++++++--
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/include/linux/hugetlb_cgroup.h b/include/linux/hugetlb_cgroup.h
index d73878c..5bb9c28 100644
--- a/include/linux/hugetlb_cgroup.h
+++ b/include/linux/hugetlb_cgroup.h
@@ -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)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 1ef2cd4..a30da48 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -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;
 }
diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
index a3f358f..284cb68 100644
--- a/mm/hugetlb_cgroup.c
+++ b/mm/hugetlb_cgroup.c
@@ -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