Re: [PATCH 2/2] mm, memory_hotplug: provide a more generic restrictions for memory hotplug

[David Hildenbrand]

On 04.04.19 20:01, Oscar Salvador wrote:
> On Thu, Apr 04, 2019 at 04:57:03PM +0200, David Hildenbrand wrote:
> 
>>>  #ifdef CONFIG_MEMORY_HOTPLUG
>>> -int arch_add_memory(int nid, u64 start, u64 size, struct vmem_altmap *altmap,
>>> -		    bool want_memblock)
>>> +int arch_add_memory(int nid, u64 start, u64 size,
>>> +			struct mhp_restrictions *restrictions)
>>
>> Should the restrictions be marked const?
> 
> We could, but maybe some platforms want to override something later on
> depending on x or y configurations, so we could be more flexible here.
> 
>>> +/*
>>> + * Do we want sysfs memblock files created. This will allow userspace to online
>>> + * and offline memory explicitly. Lack of this bit means that the caller has to
>>> + * call move_pfn_range_to_zone to finish the initialization.
>>> + */
>>
>> I think you can be more precise here.
>>
>> "Create memory block devices for added pages. This is usually the case
>> for all system ram (and only system ram), as only this way memory can be
>> onlined/offlined by user space and kdump to correctly detect the new
>> memory using udev events."
>>
>> Maybe we should even go a step further and call this
>>
>> MHP_SYSTEM_RAM
>>
>> Because that is what it is right now.
> 
> I agree that that is nicer explanation, and I would not mind to add it.
> In the end, the more information and commented code the better.
> 
> But I am not really convinced by MHP_SYSTEM_RAM name, and I think we should stick
> with MHP_MEMBLOCK_API because it represents __what__ is that flag about and its
> function, e.g: create memory block devices.

This nicely aligns with the sub-section memory add support discussion.

MHP_MEMBLOCK_API immediately implies that

- memory is used as system ram. Memory can be onlined/offlined. Markers
  at sections indicate if the section is online/offline.
- memory has to follow certain restrictions (alignment + size multiple
  of memory block size)

IOW, if some ZONE_DEVICE memory would set this flag, very bad things
will happen. Especially, device memory with memory block devices should
also result in quite some issues (I remember it is checked somewhere).

System ram added without MHP_MEMBLOCK_API? What would happen? Memory can
never be onlined.

I feel like mixing these two interfaces - adding system memory vs.
adding device memory wasn't the best design choice. Lot of parameters to
set, but the some parameters are actually mutually exclusive. Especially
memory block devices are a difference.

Maybe having actual function cal variants like

__add_system_memory / arch_add_system_memory ...

and

__add_device_memory / arch_add_device_memory

would make things clearer. To me, it feels like we are trying to squeeze
too many different things into one function call path, allowing people
do do things that shouldn't be done.

Any opinions on the design/direction on these interfaces in general? I
don't see us moving away from memory block devices for system ram. But I
am seeing us moving towards sub-section hot-add support for anything not
system ram.

> 
>>> @@ -1102,6 +1102,7 @@ int __ref add_memory_resource(int nid, struct resource *res)
>>>  	u64 start, size;
>>>  	bool new_node = false;
>>>  	int ret;
>>> +	struct mhp_restrictions restrictions = {};
>>
>> I'd make this the very first variable.
>>
>> Also eventually
>>
>> struct mhp_restrictions restrictions = {
>> 	.restrictions = MHP_MEMBLOCK_API
>> };
> 
> It might be more right.
> Actually, that is the way we tend to pre-initialize fields in structs.
> 
> About the identation, I  am really puzzled, I checked my branch and I
> cannot see any space that should be a tab.
> Maybe it got screwed up when sending it.

It's not about spaces that should be tabs, rather about how many tabs to
use. But this is really just nit picking, because it usually directly
jumps into my eyes :) On vim with tabstop=8 it didn't look right.

> 
> Anyway, thanks for the feedback David ;-)
> 


-- 

Thanks,

David / dhildenb