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

[David Hildenbrand <david@redhat.com>]

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.

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.

-- 

Thanks,

David / dhildenb