[PATCH] powerpc/mm/nvdimm: Add an informative message if we fail to allocate altmap block

[PATCH] powerpc/mm/nvdimm: Add an informative message if we fail to allocate altmap block

[Aneesh Kumar K.V]

Allocation from altmap area can fail based on vmemmap page size used. Add kernel
info message to indicate the failure. That allows the user to identify whether they
are really using persistent memory reserved space for per-page metadata.

The message looks like:
[  136.587212] altmap block allocation failed, falling back to system memory

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 arch/powerpc/mm/init_64.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
index a4e17a979e45..57c0573650dc 100644
--- a/arch/powerpc/mm/init_64.c
+++ b/arch/powerpc/mm/init_64.c
@@ -194,8 +194,12 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
 		 * fail due to alignment issues when using 16MB hugepages, so
 		 * fall back to system memory if the altmap allocation fail.
 		 */
-		if (altmap)
+		if (altmap) {
 			p = altmap_alloc_block_buf(page_size, altmap);
+			if (!p)
+				pr_info("altmap block allocation failed, " \
+					"falling back to system memory");
+		}
 		if (!p)
 			p = vmemmap_alloc_block_buf(page_size, node);
 		if (!p)
-- 
2.21.0

Re: [PATCH] powerpc/mm/nvdimm: Add an informative message if we fail to allocate altmap block

[Oliver O'Halloran]

On Sat, Jun 29, 2019 at 5:39 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> Allocation from altmap area can fail based on vmemmap page size used. Add kernel
> info message to indicate the failure. That allows the user to identify whether they
> are really using persistent memory reserved space for per-page metadata.
>
> The message looks like:
> [  136.587212] altmap block allocation failed, falling back to system memory
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  arch/powerpc/mm/init_64.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
> index a4e17a979e45..57c0573650dc 100644
> --- a/arch/powerpc/mm/init_64.c
> +++ b/arch/powerpc/mm/init_64.c
> @@ -194,8 +194,12 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>                  * fail due to alignment issues when using 16MB hugepages, so
>                  * fall back to system memory if the altmap allocation fail.
>                  */
> -               if (altmap)
> +               if (altmap) {
>                         p = altmap_alloc_block_buf(page_size, altmap);
> +                       if (!p)

> +                               pr_info("altmap block allocation failed, " \
> +                                       "falling back to system memory");

I think this is kind of misleading. If you're mapping a large amount
of memory you can have most of the vmemmap backing allocated from the
altmap and one extra block allocated from normal memory. E.g. If you
have 32MB of altmap space, one 16MB block will be allocated from the
altmap, but the 2nd 16MB block is probably unusable due to the
reserved pages at the start of the altmap. Maybe this should be a
pr_debug() so it's only printed along with the "vmemmap_populate ..."
message above?

Also, isn't kernel style to keep printf()s, even long ones, on one line?

Oliver

Re: [PATCH] powerpc/mm/nvdimm: Add an informative message if we fail to allocate altmap block

[Aneesh Kumar K.V]

"Oliver O'Halloran" <oohall@gmail.com> writes:

> On Sat, Jun 29, 2019 at 5:39 PM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> Allocation from altmap area can fail based on vmemmap page size used. Add kernel
>> info message to indicate the failure. That allows the user to identify whether they
>> are really using persistent memory reserved space for per-page metadata.
>>
>> The message looks like:
>> [  136.587212] altmap block allocation failed, falling back to system memory
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>  arch/powerpc/mm/init_64.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
>> index a4e17a979e45..57c0573650dc 100644
>> --- a/arch/powerpc/mm/init_64.c
>> +++ b/arch/powerpc/mm/init_64.c
>> @@ -194,8 +194,12 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>>                  * fail due to alignment issues when using 16MB hugepages, so
>>                  * fall back to system memory if the altmap allocation fail.
>>                  */
>> -               if (altmap)
>> +               if (altmap) {
>>                         p = altmap_alloc_block_buf(page_size, altmap);
>> +                       if (!p)
>
>> +                               pr_info("altmap block allocation failed, " \
>> +                                       "falling back to system memory");
>
> I think this is kind of misleading. If you're mapping a large amount
> of memory you can have most of the vmemmap backing allocated from the
> altmap and one extra block allocated from normal memory. E.g. If you
> have 32MB of altmap space, one 16MB block will be allocated from the
> altmap, but the 2nd 16MB block is probably unusable due to the
> reserved pages at the start of the altmap. Maybe this should be a
> pr_debug() so it's only printed along with the "vmemmap_populate ..."
> message above?

Will switch to pr_debug. What I really wanted was an indication of which
pfn device failed to allocate  per page meata data in the device. But
we really don't have device details here and we don't end up calling
this function if there is already a 16MB mapping in DRAM for this area.

>
> Also, isn't kernel style to keep printf()s, even long ones, on one line?

I was not sure. It do print to kernel log in one line.

-aneesh

Re: [PATCH] powerpc/mm/nvdimm: Add an informative message if we fail to allocate altmap block

[Michael Ellerman]

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> "Oliver O'Halloran" <oohall@gmail.com> writes:
>
>> On Sat, Jun 29, 2019 at 5:39 PM Aneesh Kumar K.V
>> <aneesh.kumar@linux.ibm.com> wrote:
>>>
>>> Allocation from altmap area can fail based on vmemmap page size used. Add kernel
>>> info message to indicate the failure. That allows the user to identify whether they
>>> are really using persistent memory reserved space for per-page metadata.
>>>
>>> The message looks like:
>>> [  136.587212] altmap block allocation failed, falling back to system memory
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> ---
>>>  arch/powerpc/mm/init_64.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
>>> index a4e17a979e45..57c0573650dc 100644
>>> --- a/arch/powerpc/mm/init_64.c
>>> +++ b/arch/powerpc/mm/init_64.c
>>> @@ -194,8 +194,12 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>>>                  * fail due to alignment issues when using 16MB hugepages, so
>>>                  * fall back to system memory if the altmap allocation fail.
>>>                  */
>>> -               if (altmap)
>>> +               if (altmap) {
>>>                         p = altmap_alloc_block_buf(page_size, altmap);
>>> +                       if (!p)
>>
>>> +                               pr_info("altmap block allocation failed, " \
>>> +                                       "falling back to system memory");
>>
>> I think this is kind of misleading. If you're mapping a large amount
>> of memory you can have most of the vmemmap backing allocated from the
>> altmap and one extra block allocated from normal memory. E.g. If you
>> have 32MB of altmap space, one 16MB block will be allocated from the
>> altmap, but the 2nd 16MB block is probably unusable due to the
>> reserved pages at the start of the altmap. Maybe this should be a
>> pr_debug() so it's only printed along with the "vmemmap_populate ..."
>> message above?
>
> Will switch to pr_debug. What I really wanted was an indication of which
> pfn device failed to allocate  per page meata data in the device. But
> we really don't have device details here and we don't end up calling
> this function if there is already a 16MB mapping in DRAM for this area.
>
>>
>> Also, isn't kernel style to keep printf()s, even long ones, on one line?
>
> I was not sure. It do print to kernel log in one line.

It should be one line so it's easy to grep for the error string.

cheers