[PATCH v2] powerpc/pseries: Fix maximum memory value

[PATCH v2] powerpc/pseries: Fix maximum memory value

[Aravinda Prasad]

Calculating the maximum memory based on the number of lmbs
and lmb size does not account for the RMA region. Hence
use memory_hotplug_max(), which already accounts for the
RMA region, to fetch the maximum memory value. Thanks to
Nathan Lynch for suggesting the memory_hotplug_max()
function.

Fixes: 772b039fd9a7: ("powerpc/pseries: Export maximum memory value")
Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/lparcfg.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/lparcfg.c b/arch/powerpc/platforms/pseries/lparcfg.c
index e33e8bc..010a41f 100644
--- a/arch/powerpc/platforms/pseries/lparcfg.c
+++ b/arch/powerpc/platforms/pseries/lparcfg.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/hugetlb.h>
+#include <linux/memblock.h>
 #include <asm/lppaca.h>
 #include <asm/hvcall.h>
 #include <asm/firmware.h>
@@ -33,7 +34,6 @@
 #include <asm/vio.h>
 #include <asm/mmu.h>
 #include <asm/machdep.h>
-#include <asm/drmem.h>
 
 #include "pseries.h"
 
@@ -435,7 +435,7 @@ static void maxmem_data(struct seq_file *m)
 {
 	unsigned long maxmem = 0;
 
-	maxmem += drmem_info->n_lmbs * drmem_info->lmb_size;
+	maxmem += memory_hotplug_max();
 	maxmem += hugetlb_total_pages() * PAGE_SIZE;
 
 	seq_printf(m, "MaxMem=%ld\n", maxmem);

Re: [PATCH v2] powerpc/pseries: Fix maximum memory value

[Nathan Lynch]

Aravinda Prasad <aravinda@linux.vnet.ibm.com> writes:
> Calculating the maximum memory based on the number of lmbs
> and lmb size does not account for the RMA region. Hence
> use memory_hotplug_max(), which already accounts for the
> RMA region, to fetch the maximum memory value. Thanks to
> Nathan Lynch for suggesting the memory_hotplug_max()
> function.

Well, I hope I haven't led you astray... will it give you the desired
result on a kernel configured without memory hotplug support, booted in
an LPAR with some huge pages configured?

If so, then
Acked-by: Nathan Lynch <nathanl@linux.ibm.com>

It would likely help with review and future maintenance if the semantics
and intended use of the MaxMem field are made a little more
explicit. For example, is it supposed to include persistent memory?
Perhaps a follow-up patch could address this. Or maybe I'm overthinking
it.

Re: [PATCH v2] powerpc/pseries: Fix maximum memory value

[Naveen N. Rao]

Nathan Lynch wrote:
> Aravinda Prasad <aravinda@linux.vnet.ibm.com> writes:
>> Calculating the maximum memory based on the number of lmbs
>> and lmb size does not account for the RMA region. Hence
>> use memory_hotplug_max(), which already accounts for the
>> RMA region, to fetch the maximum memory value. Thanks to
>> Nathan Lynch for suggesting the memory_hotplug_max()
>> function.
> 
> Well, I hope I haven't led you astray... will it give you the desired
> result on a kernel configured without memory hotplug support, booted in
> an LPAR with some huge pages configured?
> 
> If so, then
> Acked-by: Nathan Lynch <nathanl@linux.ibm.com>
> 
> It would likely help with review and future maintenance if the semantics
> and intended use of the MaxMem field are made a little more
> explicit. For example, is it supposed to include persistent memory?
> Perhaps a follow-up patch could address this. Or maybe I'm overthinking
> it.

This was primarily aimed to replicate what AIX lparstat does and 
documentation (*) just says:

  Maximum Memory
      Maximum possible amount of Memory.

I think this mirrors the maximum memory value set in the LPAR profile, 
and this provides a way to obtain that value from within the LPAR.

This doesn't necessarily answer your question, but that's at least the 
reference.

(*) 
https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/com.ibm.aix.cmds3/lparstat.htm

- Naveen

Re: [PATCH v2] powerpc/pseries: Fix maximum memory value

[Nathan Lynch]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> Nathan Lynch wrote:
>> It would likely help with review and future maintenance if the semantics
>> and intended use of the MaxMem field are made a little more
>> explicit. For example, is it supposed to include persistent memory?
>> Perhaps a follow-up patch could address this. Or maybe I'm overthinking
>> it.
>
> This was primarily aimed to replicate what AIX lparstat does and 
> documentation (*) just says:
>
>   Maximum Memory
>       Maximum possible amount of Memory.
>
> I think this mirrors the maximum memory value set in the LPAR profile, 
> and this provides a way to obtain that value from within the LPAR.
>
> This doesn't necessarily answer your question, but that's at least the 
> reference.
>
> (*) 
> https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/com.ibm.aix.cmds3/lparstat.htm

Thanks for the reference. Consider my concern withdrawn.

Re: [PATCH v2] powerpc/pseries: Fix maximum memory value

[Michael Ellerman]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> Nathan Lynch wrote:
>> Aravinda Prasad <aravinda@linux.vnet.ibm.com> writes:
>>> Calculating the maximum memory based on the number of lmbs
>>> and lmb size does not account for the RMA region. Hence
>>> use memory_hotplug_max(), which already accounts for the
>>> RMA region, to fetch the maximum memory value. Thanks to
>>> Nathan Lynch for suggesting the memory_hotplug_max()
>>> function.
>> 
>> Well, I hope I haven't led you astray... will it give you the desired
>> result on a kernel configured without memory hotplug support, booted in
>> an LPAR with some huge pages configured?
>> 
>> If so, then
>> Acked-by: Nathan Lynch <nathanl@linux.ibm.com>
>> 
>> It would likely help with review and future maintenance if the semantics
>> and intended use of the MaxMem field are made a little more
>> explicit. For example, is it supposed to include persistent memory?
>> Perhaps a follow-up patch could address this. Or maybe I'm overthinking
>> it.
>
> This was primarily aimed to replicate what AIX lparstat does and 
> documentation (*) just says:
>
>   Maximum Memory
>       Maximum possible amount of Memory.
>
> I think this mirrors the maximum memory value set in the LPAR profile, 
> and this provides a way to obtain that value from within the LPAR.

But the doc string for memory_hotplug_max() says:

 * memory_hotplug_max - return max address of memory that may be added


ie. maximum *address* not maximum *amount*.

Possibly it turns out to be the same value, but that is just because you
have no holes in your layout.

So I don't think this patch is correct.

cheers

Re: [PATCH v2] powerpc/pseries: Fix maximum memory value

[Aravinda Prasad]

On Thursday 04 July 2019 04:43 PM, Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
>> Nathan Lynch wrote:
>>> Aravinda Prasad <aravinda@linux.vnet.ibm.com> writes:
>>>> Calculating the maximum memory based on the number of lmbs
>>>> and lmb size does not account for the RMA region. Hence
>>>> use memory_hotplug_max(), which already accounts for the
>>>> RMA region, to fetch the maximum memory value. Thanks to
>>>> Nathan Lynch for suggesting the memory_hotplug_max()
>>>> function.
>>>
>>> Well, I hope I haven't led you astray... will it give you the desired
>>> result on a kernel configured without memory hotplug support, booted in
>>> an LPAR with some huge pages configured?
>>>
>>> If so, then
>>> Acked-by: Nathan Lynch <nathanl@linux.ibm.com>
>>>
>>> It would likely help with review and future maintenance if the semantics
>>> and intended use of the MaxMem field are made a little more
>>> explicit. For example, is it supposed to include persistent memory?
>>> Perhaps a follow-up patch could address this. Or maybe I'm overthinking
>>> it.
>>
>> This was primarily aimed to replicate what AIX lparstat does and 
>> documentation (*) just says:
>>
>>   Maximum Memory
>>       Maximum possible amount of Memory.
>>
>> I think this mirrors the maximum memory value set in the LPAR profile, 
>> and this provides a way to obtain that value from within the LPAR.
> 
> But the doc string for memory_hotplug_max() says:
> 
>  * memory_hotplug_max - return max address of memory that may be added
> 
> 
> ie. maximum *address* not maximum *amount*.
> 
> Possibly it turns out to be the same value, but that is just because you
> have no holes in your layout.
> 
> So I don't think this patch is correct.

memory_hotplug_max (in one of the cases) is taking the value from
"ibm,lrdr-capacity" and according to PAPR:

PAPR section C.6.3.1 ibm,lrdr-capacity:

"The phys (of size #address-cells) communicates the maximum address in
bytes and therefore, the most memory that can be allocated to this
partition."

On other cases memory_hotplug_max() is calculating based on the number
of lmbs assigned to the partition, so should still give max mem value


> 
> cheers
> 

-- 
Regards,
Aravinda

Re: [PATCH v2] powerpc/pseries: Fix maximum memory value

[Aravinda Prasad]

On Friday 28 June 2019 10:57 PM, Nathan Lynch wrote:
> Aravinda Prasad <aravinda@linux.vnet.ibm.com> writes:
>> Calculating the maximum memory based on the number of lmbs
>> and lmb size does not account for the RMA region. Hence
>> use memory_hotplug_max(), which already accounts for the
>> RMA region, to fetch the maximum memory value. Thanks to
>> Nathan Lynch for suggesting the memory_hotplug_max()
>> function.
> 
> Well, I hope I haven't led you astray... will it give you the desired
> result on a kernel configured without memory hotplug support, booted in
> an LPAR with some huge pages configured?

Yes. I have tested the patch both with CONFIG_MEMORY_HOTPLUG set and not
set. It is working as expected.

Regards,
Aravinda

> 
> If so, then
> Acked-by: Nathan Lynch <nathanl@linux.ibm.com>
> 
> It would likely help with review and future maintenance if the semantics
> and intended use of the MaxMem field are made a little more
> explicit. For example, is it supposed to include persistent memory?
> Perhaps a follow-up patch could address this. Or maybe I'm overthinking
> it.
> 

-- 
Regards,
Aravinda