Re: [PATCH v1 1/4] mm/memory_hotplug: Release memory resource after arch_remove_memory()

[Michal Hocko]

On Wed 17-04-19 15:24:47, David Hildenbrand wrote:
> On 17.04.19 15:12, Michal Hocko wrote:
> > On Tue 09-04-19 12:01:45, David Hildenbrand wrote:
> >> __add_pages() doesn't add the memory resource, so __remove_pages()
> >> shouldn't remove it. Let's factor it out. Especially as it is a special
> >> case for memory used as system memory, added via add_memory() and
> >> friends.
> >>
> >> We now remove the resource after removing the sections instead of doing
> >> it the other way around. I don't think this change is problematic.
> >>
> >> add_memory()
> >> 	register memory resource
> >> 	arch_add_memory()
> >>
> >> remove_memory
> >> 	arch_remove_memory()
> >> 	release memory resource
> >>
> >> While at it, explain why we ignore errors and that it only happeny if
> >> we remove memory in a different granularity as we added it.
> > 
> > OK, I agree that the symmetry is good in general and it certainly makes
> > sense here as well. But does it make sense to pick up this particular
> > part without larger considerations of add vs. remove apis? I have a
> > strong feeling this wouldn't be the only thing to care about. In other
> > words does this help future changes or it is more likely to cause more
> > code conflicts with other features being developed right now?
> 
> I am planning to
> 
> 1. factor out memory block device handling, so features like sub-section
> add/remove are easier to add internally. Move it to the user that wants
> it. Clean up all the mess we have right now due to supporting memory
> block devices that span several sections.
> 
> 2. Make sure that any arch_add_pages() and friends clean up properly if
> they fail instead of indicating failure but leaving some partially added
> memory lying around.
> 
> 3. Clean up node handling regarding to memory hotplug/unplug. Especially
> don't allow to offline/remove memory spanning several nodes etc.

Yes, this all sounds sane to me.

> IOW, in order to properly clean up memory block device handling and
> prepare for more changes people are interested in (e.g. sub-section add
> of device memory), this is the right thing to do. The other parts are
> bigger changes.

This would be really valuable to have in the cover. Beause there is so
much to clean up in this mess but making random small cleanups without a
larger plan tends to step on others toes more than being useful.
-- 
Michal Hocko
SUSE Labs