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

[Oscar Salvador]

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.

> > @@ -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.

Anyway, thanks for the feedback David ;-)

-- 
Oscar Salvador
SUSE L3