2017-07-03 20:06:09

by Victor Aoqui

[permalink] [raw]
Subject: [PATCH] powerpc/mm: Implemented default_hugepagesz verification for powerpc

Implemented default hugepage size verification (default_hugepagesz=)
in order to allow allocation of defined number of pages (hugepages=)
only for supported hugepage sizes.

Signed-off-by: Victor Aoqui <[email protected]>
---
arch/powerpc/mm/hugetlbpage.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index a4f33de..464e72e 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c
@@ -797,6 +797,21 @@ static int __init hugepage_setup_sz(char *str)
}
__setup("hugepagesz=", hugepage_setup_sz);

+static int __init default_hugepage_setup_sz(char *str)
+{
+ unsigned long long size;
+
+ size = memparse(str, &str);
+
+ if (add_huge_page_size(size) != 0) {
+ hugetlb_bad_size();
+ pr_err("Invalid default huge page size specified(%llu)\n", size);
+ }
+
+ return 1;
+}
+__setup("default_hugepagesz=", default_hugepage_setup_sz);
+
struct kmem_cache *hugepte_cache;
static int __init hugetlbpage_init(void)
{
--
1.8.3.1


2017-07-05 04:26:45

by Aneesh Kumar K.V

[permalink] [raw]
Subject: Re: [PATCH] powerpc/mm: Implemented default_hugepagesz verification for powerpc



On Tuesday 04 July 2017 01:35 AM, Victor Aoqui wrote:
> Implemented default hugepage size verification (default_hugepagesz=)
> in order to allow allocation of defined number of pages (hugepages=)
> only for supported hugepage sizes.
>
> Signed-off-by: Victor Aoqui <[email protected]>
> ---
> arch/powerpc/mm/hugetlbpage.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
> index a4f33de..464e72e 100644
> --- a/arch/powerpc/mm/hugetlbpage.c
> +++ b/arch/powerpc/mm/hugetlbpage.c
> @@ -797,6 +797,21 @@ static int __init hugepage_setup_sz(char *str)
> }
> __setup("hugepagesz=", hugepage_setup_sz);
>
> +static int __init default_hugepage_setup_sz(char *str)
> +{
> + unsigned long long size;
> +
> + size = memparse(str, &str);
> +
> + if (add_huge_page_size(size) != 0) {
> + hugetlb_bad_size();
> + pr_err("Invalid default huge page size specified(%llu)\n", size);
> + }
> +
> + return 1;
> +}
> +__setup("default_hugepagesz=", default_hugepage_setup_sz);

isn't that a behavior change in what we have now ? . Right now if size
specified is not supported, we fallback to HPAGE_SIZE.

mm/hugetlb.c

if (!size_to_hstate(default_hstate_size)) {
default_hstate_size = HPAGE_SIZE;
if (!size_to_hstate(default_hstate_size))
hugetlb_add_hstate(HUGETLB_PAGE_ORDER);
}


> +
> struct kmem_cache *hugepte_cache;
> static int __init hugetlbpage_init(void)
> {
>

Even if we want to do this, this should be done in generic code and
should not be powerpc specific

-aneesh

2017-07-05 04:31:46

by Anshuman Khandual

[permalink] [raw]
Subject: Re: [PATCH] powerpc/mm: Implemented default_hugepagesz verification for powerpc

On 07/04/2017 01:35 AM, Victor Aoqui wrote:
> Implemented default hugepage size verification (default_hugepagesz=)
> in order to allow allocation of defined number of pages (hugepages=)
> only for supported hugepage sizes.
>
> Signed-off-by: Victor Aoqui <[email protected]>
> ---
> arch/powerpc/mm/hugetlbpage.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
> index a4f33de..464e72e 100644
> --- a/arch/powerpc/mm/hugetlbpage.c
> +++ b/arch/powerpc/mm/hugetlbpage.c
> @@ -797,6 +797,21 @@ static int __init hugepage_setup_sz(char *str)
> }
> __setup("hugepagesz=", hugepage_setup_sz);
>
> +static int __init default_hugepage_setup_sz(char *str)

The function name should be hugetlb_default_size_setup in
sync with the generic function hugetlb_default_setup for the
same parameter default_hugepagesz.

> +{
> + unsigned long long size;
> +
> + size = memparse(str, &str);
> +
> + if (add_huge_page_size(size) != 0) {

I am little bit confused here. Do we always follow another
'hugepages=' element after 'default_hugepagesz' ? If not,
then we dont have to do 'add_huge_page_size'. But then
that function checks for valid huge page sizes and skips
adding hstate if its already added. So I guess it okay.

> + hugetlb_bad_size();
> + pr_err("Invalid default huge page size specified(%llu)\n", size);

Error message should have 'ppc' some where to indicate that
the arch rejected the size not core MM.

2017-07-05 16:03:39

by Victor Aoqui

[permalink] [raw]
Subject: Re: [PATCH] powerpc/mm: Implemented default_hugepagesz verification for powerpc

Em 2017-07-05 01:26, Aneesh Kumar K.V escreveu:
> On Tuesday 04 July 2017 01:35 AM, Victor Aoqui wrote:
>> Implemented default hugepage size verification (default_hugepagesz=)
>> in order to allow allocation of defined number of pages (hugepages=)
>> only for supported hugepage sizes.
>>
>> Signed-off-by: Victor Aoqui <[email protected]>
>> ---
>> arch/powerpc/mm/hugetlbpage.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/hugetlbpage.c
>> b/arch/powerpc/mm/hugetlbpage.c
>> index a4f33de..464e72e 100644
>> --- a/arch/powerpc/mm/hugetlbpage.c
>> +++ b/arch/powerpc/mm/hugetlbpage.c
>> @@ -797,6 +797,21 @@ static int __init hugepage_setup_sz(char *str)
>> }
>> __setup("hugepagesz=", hugepage_setup_sz);
>>
>> +static int __init default_hugepage_setup_sz(char *str)
>> +{
>> + unsigned long long size;
>> +
>> + size = memparse(str, &str);
>> +
>> + if (add_huge_page_size(size) != 0) {
>> + hugetlb_bad_size();
>> + pr_err("Invalid default huge page size
>> specified(%llu)\n", size);
>> + }
>> +
>> + return 1;
>> +}
>> +__setup("default_hugepagesz=", default_hugepage_setup_sz);
>
> isn't that a behavior change in what we have now ? . Right now if size
> specified is not supported, we fallback to HPAGE_SIZE.

Yes, it is. However, is this a correct behavior? If we specify an
unsupported value, for example default_hugepagesz=1M and hugepages=1000,
1M will be ignored and 1000 pages of 16M (arch default) will be
allocated. This could lead to non-expected out of of memory/performance
issue.

>
> mm/hugetlb.c
>
> if (!size_to_hstate(default_hstate_size)) {
> default_hstate_size = HPAGE_SIZE;
> if (!size_to_hstate(default_hstate_size))
> hugetlb_add_hstate(HUGETLB_PAGE_ORDER);
> }
>
>
>> +
>> struct kmem_cache *hugepte_cache;
>> static int __init hugetlbpage_init(void)
>> {
>>
>
> Even if we want to do this, this should be done in generic code and
> should not be powerpc specific
>

The verification of supported powerpc hugepage size (hugepagesz=) is
being performed on add_huge_page_size(), which is currently defined in
arch/powerpc/mm/hugetlbpage.c. I think it makes more sense to implement
default_hugepagesz= verification on arch/powerpc, don't you think?

> -aneesh

2017-07-05 16:09:24

by Victor Aoqui

[permalink] [raw]
Subject: Re: [PATCH] powerpc/mm: Implemented default_hugepagesz verification for powerpc

Em 2017-07-05 01:31, Anshuman Khandual escreveu:
> On 07/04/2017 01:35 AM, Victor Aoqui wrote:
>> Implemented default hugepage size verification (default_hugepagesz=)
>> in order to allow allocation of defined number of pages (hugepages=)
>> only for supported hugepage sizes.
>>
>> Signed-off-by: Victor Aoqui <[email protected]>
>> ---
>> arch/powerpc/mm/hugetlbpage.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/hugetlbpage.c
>> b/arch/powerpc/mm/hugetlbpage.c
>> index a4f33de..464e72e 100644
>> --- a/arch/powerpc/mm/hugetlbpage.c
>> +++ b/arch/powerpc/mm/hugetlbpage.c
>> @@ -797,6 +797,21 @@ static int __init hugepage_setup_sz(char *str)
>> }
>> __setup("hugepagesz=", hugepage_setup_sz);
>>
>> +static int __init default_hugepage_setup_sz(char *str)
>
> The function name should be hugetlb_default_size_setup in
> sync with the generic function hugetlb_default_setup for the
> same parameter default_hugepagesz.
>

Yes, makes sense to me.

>> +{
>> + unsigned long long size;
>> +
>> + size = memparse(str, &str);
>> +
>> + if (add_huge_page_size(size) != 0) {
>
> I am little bit confused here. Do we always follow another
> 'hugepages=' element after 'default_hugepagesz' ? If not,
> then we dont have to do 'add_huge_page_size'. But then
> that function checks for valid huge page sizes and skips
> adding hstate if its already added. So I guess it okay.
>

'default_hugepagesz=' is not always followed by 'hugepages=', but if we
specify 'hugepages=' along with 'default_hugepagesz=' it will try to
allocate the hugepage size specified. If the size is not supported by
hardware, it will try to allocate the number of pages specified with the
default hugepage size of the arch, which is not the desired behavior. So
calling add_huge_page_size would verify if the hugepage size is
supported and in case it's not, hugepages will not be allocated.

>> + hugetlb_bad_size();
>> + pr_err("Invalid default huge page size
>> specified(%llu)\n", size);
>
> Error message should have 'ppc' some where to indicate that
> the arch rejected the size not core MM.

2017-07-12 15:14:32

by Victor Aoqui

[permalink] [raw]
Subject: Re: [PATCH] powerpc/mm: Implemented default_hugepagesz verification for powerpc

Em 2017-07-05 13:03, victora escreveu:
> Em 2017-07-05 01:26, Aneesh Kumar K.V escreveu:
>> On Tuesday 04 July 2017 01:35 AM, Victor Aoqui wrote:
>>> Implemented default hugepage size verification (default_hugepagesz=)
>>> in order to allow allocation of defined number of pages (hugepages=)
>>> only for supported hugepage sizes.
>>>
>>> Signed-off-by: Victor Aoqui <[email protected]>
>>> ---
>>> arch/powerpc/mm/hugetlbpage.c | 15 +++++++++++++++
>>> 1 file changed, 15 insertions(+)
>>>
>>> diff --git a/arch/powerpc/mm/hugetlbpage.c
>>> b/arch/powerpc/mm/hugetlbpage.c
>>> index a4f33de..464e72e 100644
>>> --- a/arch/powerpc/mm/hugetlbpage.c
>>> +++ b/arch/powerpc/mm/hugetlbpage.c
>>> @@ -797,6 +797,21 @@ static int __init hugepage_setup_sz(char *str)
>>> }
>>> __setup("hugepagesz=", hugepage_setup_sz);
>>>
>>> +static int __init default_hugepage_setup_sz(char *str)
>>> +{
>>> + unsigned long long size;
>>> +
>>> + size = memparse(str, &str);
>>> +
>>> + if (add_huge_page_size(size) != 0) {
>>> + hugetlb_bad_size();
>>> + pr_err("Invalid default huge page size
>>> specified(%llu)\n", size);
>>> + }
>>> +
>>> + return 1;
>>> +}
>>> +__setup("default_hugepagesz=", default_hugepage_setup_sz);
>>
>> isn't that a behavior change in what we have now ? . Right now if size
>> specified is not supported, we fallback to HPAGE_SIZE.
>
> Yes, it is. However, is this a correct behavior? If we specify an
> unsupported value, for example default_hugepagesz=1M and
> hugepages=1000, 1M will be ignored and 1000 pages of 16M (arch
> default) will be allocated. This could lead to non-expected out of of
> memory/performance issue.
>
>>
>> mm/hugetlb.c
>>
>> if (!size_to_hstate(default_hstate_size)) {
>> default_hstate_size = HPAGE_SIZE;
>> if (!size_to_hstate(default_hstate_size))
>> hugetlb_add_hstate(HUGETLB_PAGE_ORDER);
>> }
>>
>>
>>> +
>>> struct kmem_cache *hugepte_cache;
>>> static int __init hugetlbpage_init(void)
>>> {
>>>
>>
>> Even if we want to do this, this should be done in generic code and
>> should not be powerpc specific
>>
>
> The verification of supported powerpc hugepage size (hugepagesz=) is
> being performed on add_huge_page_size(), which is currently defined in
> arch/powerpc/mm/hugetlbpage.c. I think it makes more sense to
> implement default_hugepagesz= verification on arch/powerpc, don't you
> think?
>
>> -aneesh

Hi Aneesh,

Did you have time to review the comments above?

Thanks
Victor