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
