[PATCH] powerpc/pseries: Auto online hotplugged memory

[PATCH] powerpc/pseries: Auto online hotplugged memory

[Nathan Fontenot]

Auto online hotplugged memory

A recent update (commit id 31bc3858ea3) to the core mm hotplug code
introduced the memhp_auto_online variable to allow for automatically
onlining memory that is added.

This patch update the pseries memory hotplug code to enable this so that
any memory DLPAR added to the system is automatically onlined. The code
to add the memory block for memory added from add_memory() is removed as
this is not needed, the memory_add code does this.

Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/hotplug-memory.c |   52 +++++------------------
 1 file changed, 11 insertions(+), 41 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 2ce1385..03f6169 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -533,50 +533,11 @@ static int dlpar_memory_remove_by_index(u32 drc_index, struct property *prop)
 
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
-static int dlpar_add_lmb_memory(struct of_drconf_cell *lmb)
+static int dlpar_add_lmb(struct of_drconf_cell *lmb)
 {
-	struct memory_block *mem_block;
 	unsigned long block_sz;
 	int nid, rc;
 
-	block_sz = memory_block_size_bytes();
-
-	/* Find the node id for this address */
-	nid = memory_add_physaddr_to_nid(lmb->base_addr);
-
-	/* Add the memory */
-	rc = add_memory(nid, lmb->base_addr, block_sz);
-	if (rc)
-		return rc;
-
-	/* Register this block of memory */
-	rc = memblock_add(lmb->base_addr, block_sz);
-	if (rc) {
-		remove_memory(nid, lmb->base_addr, block_sz);
-		return rc;
-	}
-
-	mem_block = lmb_to_memblock(lmb);
-	if (!mem_block) {
-		remove_memory(nid, lmb->base_addr, block_sz);
-		return -EINVAL;
-	}
-
-	rc = device_online(&mem_block->dev);
-	put_device(&mem_block->dev);
-	if (rc) {
-		remove_memory(nid, lmb->base_addr, block_sz);
-		return rc;
-	}
-
-	lmb->flags |= DRCONF_MEM_ASSIGNED;
-	return 0;
-}
-
-static int dlpar_add_lmb(struct of_drconf_cell *lmb)
-{
-	int rc;
-
 	if (lmb->flags & DRCONF_MEM_ASSIGNED)
 		return -EINVAL;
 
@@ -592,10 +553,19 @@ static int dlpar_add_lmb(struct of_drconf_cell *lmb)
 		return rc;
 	}
 
-	rc = dlpar_add_lmb_memory(lmb);
+	block_sz = memory_block_size_bytes();
+
+	/* Find the node id for this address */
+	nid = memory_add_physaddr_to_nid(lmb->base_addr);
+
+	/* Add and online memory */
+	memhp_auto_online = true;
+	rc = add_memory(nid, lmb->base_addr, block_sz);
 	if (rc) {
 		dlpar_remove_device_tree_lmb(lmb);
 		dlpar_release_drc(lmb->drc_index);
+	} else {
+		lmb->flags |= DRCONF_MEM_ASSIGNED;
 	}
 
 	return rc;

Re: [PATCH] powerpc/pseries: Auto online hotplugged memory

[Michael Ellerman]

On Mon, 2016-06-20 at 08:51 -0500, Nathan Fontenot wrote:

> Auto online hotplugged memory
> 
> A recent update (commit id 31bc3858ea3) to the core mm hotplug code
> introduced the memhp_auto_online variable to allow for automatically
> onlining memory that is added.
> 
> This patch update the pseries memory hotplug code to enable this so that
> any memory DLPAR added to the system is automatically onlined. The code
> to add the memory block for memory added from add_memory() is removed as
> this is not needed, the memory_add code does this.

Is this a bug fix, or just a cleanup?

cheers

Re: [PATCH] powerpc/pseries: Auto online hotplugged memory

[Nathan Fontenot]

On 06/20/2016 07:57 PM, Michael Ellerman wrote:
> On Mon, 2016-06-20 at 08:51 -0500, Nathan Fontenot wrote:
> 
>> Auto online hotplugged memory
>>
>> A recent update (commit id 31bc3858ea3) to the core mm hotplug code
>> introduced the memhp_auto_online variable to allow for automatically
>> onlining memory that is added.
>>
>> This patch update the pseries memory hotplug code to enable this so that
>> any memory DLPAR added to the system is automatically onlined. The code
>> to add the memory block for memory added from add_memory() is removed as
>> this is not needed, the memory_add code does this.
> 
> Is this a bug fix, or just a cleanup?
> 

Hmmm.. some cleanup and some new feature. The removal of the memblock_add()
call is a cleanup and the setting of the memhp_auto_online variable is
taking advantage of a feature I was not previously aware of.

None of this is a bug fix.

-Nathan

Re: [PATCH] powerpc/pseries: Auto online hotplugged memory

[Michael Ellerman]

On Mon, 2016-06-20 at 21:14 -0500, Nathan Fontenot wrote:
> On 06/20/2016 07:57 PM, Michael Ellerman wrote:
> > On Mon, 2016-06-20 at 08:51 -0500, Nathan Fontenot wrote:
> > 
> > > Auto online hotplugged memory
> > > 
> > > A recent update (commit id 31bc3858ea3) to the core mm hotplug code
> > > introduced the memhp_auto_online variable to allow for automatically
> > > onlining memory that is added.
> > > 
> > > This patch update the pseries memory hotplug code to enable this so that
> > > any memory DLPAR added to the system is automatically onlined. The code
> > > to add the memory block for memory added from add_memory() is removed as
> > > this is not needed, the memory_add code does this.
> > 
> > Is this a bug fix, or just a cleanup?
> 
> Hmmm.. some cleanup and some new feature. The removal of the memblock_add()
> call is a cleanup and the setting of the memhp_auto_online variable is
> taking advantage of a feature I was not previously aware of.

OK. Looking at usage of memhp_auto_online it's not clear to me that you're
supposed to be setting it in arch code.

eg. if I build my kernel with CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE=n, I will
expect it to not be onlined by default.

Similarly if I boot with memhp_default_state=offline on the kernel command line.

But this patch would then mean it is onlined by default. So that seems kind of
confusing for users.

I think instead we should be merging the bulk of this patch, but without the
forced assignment to memhp_auto_online?

cheers

Re: [PATCH] powerpc/pseries: Auto online hotplugged memory

[Nathan Fontenot]

On 06/24/2016 12:35 AM, Michael Ellerman wrote:
> On Mon, 2016-06-20 at 21:14 -0500, Nathan Fontenot wrote:
>> On 06/20/2016 07:57 PM, Michael Ellerman wrote:
>>> On Mon, 2016-06-20 at 08:51 -0500, Nathan Fontenot wrote:
>>>
>>>> Auto online hotplugged memory
>>>>
>>>> A recent update (commit id 31bc3858ea3) to the core mm hotplug code
>>>> introduced the memhp_auto_online variable to allow for automatically
>>>> onlining memory that is added.
>>>>
>>>> This patch update the pseries memory hotplug code to enable this so that
>>>> any memory DLPAR added to the system is automatically onlined. The code
>>>> to add the memory block for memory added from add_memory() is removed as
>>>> this is not needed, the memory_add code does this.
>>>
>>> Is this a bug fix, or just a cleanup?
>>
>> Hmmm.. some cleanup and some new feature. The removal of the memblock_add()
>> call is a cleanup and the setting of the memhp_auto_online variable is
>> taking advantage of a feature I was not previously aware of.
> 
> OK. Looking at usage of memhp_auto_online it's not clear to me that you're
> supposed to be setting it in arch code.
> 
> eg. if I build my kernel with CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE=n, I will
> expect it to not be onlined by default.
> 
> Similarly if I boot with memhp_default_state=offline on the kernel command line.
> 
> But this patch would then mean it is onlined by default. So that seems kind of
> confusing for users.

Yes, but we already online memory when it is DLPAR added to the system. This has
always been the default behavior for pseries. I was using the memhp_auto_online
setting so that the memory will be added as part of the memory_add call instead
of having to online the memory as a separate step in the pseries code.

In other words, I am not changing the existing behavior of pseries code. 

Perhaps something a bit different? I could save and restore the memhp_auto_online
setting across the call to memory_add, or perhpas ther should be an
add_memory_and_online() variant of the add_memory() call that always onlines memory.

> 
> I think instead we should be merging the bulk of this patch, but without the
> forced assignment to memhp_auto_online?

The bulk of the patch revolves around the setting of the memhp_auto_online setting
and the bits of code we can remove by using this. The other part is removing
the call to memblock_add() which I can send another patch to do once we have
resolved the memhp_auto_online questions.

-Nathan

Re: [PATCH] powerpc/pseries: Auto online hotplugged memory

[Michael Ellerman]

On Mon, 2016-06-27 at 09:41 -0500, Nathan Fontenot wrote:
> On 06/24/2016 12:35 AM, Michael Ellerman wrote:
> > On Mon, 2016-06-20 at 21:14 -0500, Nathan Fontenot wrote:
> > > On 06/20/2016 07:57 PM, Michael Ellerman wrote:
> > > > On Mon, 2016-06-20 at 08:51 -0500, Nathan Fontenot wrote:
> > > > 
> > > > > Auto online hotplugged memory
> > > > > 
> > > > > A recent update (commit id 31bc3858ea3) to the core mm hotplug code
> > > > > introduced the memhp_auto_online variable to allow for automatically
> > > > > onlining memory that is added.
> > > > > 
> > > > > This patch update the pseries memory hotplug code to enable this so that
> > > > > any memory DLPAR added to the system is automatically onlined. The code
> > > > > to add the memory block for memory added from add_memory() is removed as
> > > > > this is not needed, the memory_add code does this.
> > > > 
> > > > Is this a bug fix, or just a cleanup?
> > > 
> > > Hmmm.. some cleanup and some new feature. The removal of the memblock_add()
> > > call is a cleanup and the setting of the memhp_auto_online variable is
> > > taking advantage of a feature I was not previously aware of.
> > 
> > OK. Looking at usage of memhp_auto_online it's not clear to me that you're
> > supposed to be setting it in arch code.
> > 
> > eg. if I build my kernel with CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE=n, I will
> > expect it to not be onlined by default.
> > 
> > Similarly if I boot with memhp_default_state=offline on the kernel command line.
> > 
> > But this patch would then mean it is onlined by default. So that seems kind of
> > confusing for users.
> 
> Yes, but we already online memory when it is DLPAR added to the system. This has
> always been the default behavior for pseries. I was using the memhp_auto_online
> setting so that the memory will be added as part of the memory_add call instead
> of having to online the memory as a separate step in the pseries code.
> 
> In other words, I am not changing the existing behavior of pseries code. 

Yep, that's good.

What's not good is giving the user an option and then ignoring it.

> Perhaps something a bit different? I could save and restore the memhp_auto_online
> setting across the call to memory_add, or perhpas ther should be an
> add_memory_and_online() variant of the add_memory() call that always onlines memory.
 
No I think we should just honor the setting.

To retain the existing behavour on pseries we just set CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE=y
in the pseries defconfig.

That way users can control the behaviour, which is good, and all the existing
documentation applies equally to powerpc as other arches. If we do something
different, like force onlining it, then we need to go and update all the docs to
say "memhp_auto_online - ignored on powerpc".

cheers

Re: [PATCH] powerpc/pseries: Auto online hotplugged memory

[Nathan Fontenot]

On 06/27/2016 10:46 PM, Michael Ellerman wrote:
> On Mon, 2016-06-27 at 09:41 -0500, Nathan Fontenot wrote:
>> On 06/24/2016 12:35 AM, Michael Ellerman wrote:
>>> On Mon, 2016-06-20 at 21:14 -0500, Nathan Fontenot wrote:
>>>> On 06/20/2016 07:57 PM, Michael Ellerman wrote:
>>>>> On Mon, 2016-06-20 at 08:51 -0500, Nathan Fontenot wrote:
>>>>>
>>>>>> Auto online hotplugged memory
>>>>>>
>>>>>> A recent update (commit id 31bc3858ea3) to the core mm hotplug code
>>>>>> introduced the memhp_auto_online variable to allow for automatically
>>>>>> onlining memory that is added.
>>>>>>
>>>>>> This patch update the pseries memory hotplug code to enable this so that
>>>>>> any memory DLPAR added to the system is automatically onlined. The code
>>>>>> to add the memory block for memory added from add_memory() is removed as
>>>>>> this is not needed, the memory_add code does this.
>>>>>
>>>>> Is this a bug fix, or just a cleanup?
>>>>
>>>> Hmmm.. some cleanup and some new feature. The removal of the memblock_add()
>>>> call is a cleanup and the setting of the memhp_auto_online variable is
>>>> taking advantage of a feature I was not previously aware of.
>>>
>>> OK. Looking at usage of memhp_auto_online it's not clear to me that you're
>>> supposed to be setting it in arch code.
>>>
>>> eg. if I build my kernel with CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE=n, I will
>>> expect it to not be onlined by default.
>>>
>>> Similarly if I boot with memhp_default_state=offline on the kernel command line.
>>>
>>> But this patch would then mean it is onlined by default. So that seems kind of
>>> confusing for users.
>>
>> Yes, but we already online memory when it is DLPAR added to the system. This has
>> always been the default behavior for pseries. I was using the memhp_auto_online
>> setting so that the memory will be added as part of the memory_add call instead
>> of having to online the memory as a separate step in the pseries code.
>>
>> In other words, I am not changing the existing behavior of pseries code. 
> 
> Yep, that's good.
> 
> What's not good is giving the user an option and then ignoring it.

True, I just trying to preserve existing behavior.

> 
>> Perhaps something a bit different? I could save and restore the memhp_auto_online
>> setting across the call to memory_add, or perhpas ther should be an
>> add_memory_and_online() variant of the add_memory() call that always onlines memory.
>  
> No I think we should just honor the setting.
> 
> To retain the existing behavour on pseries we just set CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE=y
> in the pseries defconfig.
> 
> That way users can control the behaviour, which is good, and all the existing
> documentation applies equally to powerpc as other arches. If we do something
> different, like force onlining it, then we need to go and update all the docs to
> say "memhp_auto_online - ignored on powerpc".
> 

Sounds good, I'll send a new patch to set the config option for pseries and update
the dlpar memory add code accordingly.

Thanks,
-Nathan