lkml.org 
[lkml]   [2019]   [Apr]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] mm, memory_hotplug: provide a more generic restrictions for memory hotplug
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

\
 
 \ /
  Last update: 2019-04-04 20:02    [W:0.049 / U:5.660 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site