On 17.07.19 09:38, Oscar Salvador wrote: > On Tue, Jul 16, 2019 at 07:28:54PM -0700, Dan Williams wrote: >> This makes it more clear that the problem is with the "start_pfn == >> pfn" check relative to subsections, but it does not clarify why it >> needs to clear pfn_valid() before calling shrink_zone_span(). >> Sections were not invalidated prior to shrink_zone_span() in the >> pre-subsection implementation and it seems all we need is to keep the >> same semantic. I.e. skip the range that is currently being removed: > > Yes, as I said in my reply to Aneesh, that is the other way I thought > when fixing it. > The reason I went this way is because it seemed more reasonable and > natural to me that pfn_valid() would just return the next active > sub-section. > > I just though that we could leverage the fact that we can deactivate > a sub-section before scanning for the next one. > > On a second thought, the changes do not outweight the case, being the first > fix enough and less intrusive, so I will send a v2 with that instead. > > I'd also like to note that we should strive for making all zone-related changes when offlining in the future, not when removing memory. So ideally, any core changes we perform from now, should make that step (IOW implementing that) easier, not harder. Of course, BUGs have to be fixed. The rough idea would be to also mark ZONE_DEVICE sections as ONLINE (or rather rename it to "ACTIVE" to generalize). For each section we would then have - pfn_valid(): We have a valid "struct page" / memmap - pfn_present(): We have actually added that memory via an oficial interface to mm (e.g., arch_add_memory() ) - pfn_online() / (or pfn_active()): Memory is active (online in "buddy"- speak, or memory that was moved to the ZONE_DEVICE zone) When resizing the zones (e.g., when offlining memory), we would then search for pfn_online(), not pfn_present(). In addition to move_pfn_range_to_zone(), we would have remove_pfn_range_from_zone(), called during offline_pages() / by devmem/hmm/pmem code before removing. (I started to look into this, but I don't have any patches yet) -- Thanks, David / dhildenb
