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

[Nathan Fontenot <nfont@linux.vnet.ibm.com>]

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