Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
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 > > Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
On 2012/12/13 1:05, Aneesh Kumar K.V wrote:
> Jianguo Wu 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
>> Signed-off-by: Jiang Liu
>> ---
>> 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(_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
>
>> +
>> +idx++;
>> +}
>> }
>>
>> /*
>> -- 1.7.1
>
> -anesh
>
>
> .
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
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 > > Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
Jianguo Wu 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
> Signed-off-by: Jiang Liu
> ---
> 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(_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
> +
> + idx++;
> + }
> }
>
> /*
> -- 1.7.1
-anesh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
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 Ohh, and this deserves to be backported to stable (since 3.6). -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
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 Thanks! -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
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
>> Signed-off-by: Jiang Liu
>> ---
>> 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(_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
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
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
> Signed-off-by: Jiang Liu
> ---
> 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(_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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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
Signed-off-by: Jiang Liu
---
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(_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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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 wujian...@huawei.com
Signed-off-by: Jiang Liu jiang@huawei.com
---
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
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 wujian...@huawei.com
Signed-off-by: Jiang Liu jiang@huawei.com
---
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
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 wujian...@huawei.com
Signed-off-by: Jiang Liu jiang@huawei.com
---
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
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 mho...@suse.cz Thanks! -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
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 mho...@suse.cz Ohh, and this deserves to be backported to stable (since 3.6). -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
Jianguo Wu wujian...@huawei.com 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 wujian...@huawei.com
Signed-off-by: Jiang Liu jiang@huawei.com
---
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.ku...@linux.vnet.ibm.com
+
+ idx++;
+ }
}
/*
-- 1.7.1
-anesh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
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 mho...@suse.cz Thanks! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
On 2012/12/13 1:05, Aneesh Kumar K.V wrote:
Jianguo Wu wujian...@huawei.com 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 wujian...@huawei.com
Signed-off-by: Jiang Liu jiang@huawei.com
---
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.ku...@linux.vnet.ibm.com
+
+idx++;
+}
}
/*
-- 1.7.1
-anesh
.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/hugetlb: create hugetlb cgroup file in hugetlb_init
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 mho...@suse.cz Thanks! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
