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

[David Hildenbrand]

On 17.04.19 15:31, Michal Hocko wrote:
> 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.

I agree, let's discuss the bigger plan I have in mind

1. arch_add_memory()/arch_remove_memory() don't deal with memory block
devices. add_memory()/remove_memory()/online_pages()/offline_pages() do.

2. add_memory()/remove_memory()/online_pages()/offline_pages()
- Only work on memory block device alignment/granularity
- Only work on single nodes.
- Only work on single zones.

3. mem->nid correctly indicates if
- Memory block devices belongs to single node / no node / multiple nodes
- Fast and reliable way to detect
remove_memory()/online_pages()/offline_pages() being called with
multiple nodes.

4. arch_remove_memory() and friends never fail. Removing of memory
always succeeds. This allows better error handling when adding of memory
fails. We will move some parts from CONFIG_MEMORY_HOTREMOVE to
CONFIG_MEMORY_HOTPLUG, so we can use them to clean up if adding of
memory fails.

5. Remove all accesses to struct_page from memory removal path. Pages
might never have been initialized, they should not be touched.

All other features I see on the horizon (vmemmap on added memory,
sub-section hot-add) would mainly be centered around
arch_add_memory()/arch_remove_memory(), which would not have to deal
with any special cases around memory block device memory.

Do you agree, do you have any other points/things in mind you consider
important?

-- 

Thanks,

David / dhildenb