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
