> >> 2) I really dislike having to scatter section online checks all over the >> place in page ext code. Once there is a difference between active vs. >> stale page ext data things get a bit messy and error prone. This is >> already ugly enough in our generic memmap handling code IMHO. > > They should represent a free page in any case so even they are stall > they shouldn't be really dangerous, right? Good question. The use-after-free tells me that there could at least be something accessing page_ext data after offlining right now. As long as it's only unsynchronized read access, we should be fine. > >> 3) Having on-demand allocations, such as KASAN or page ext from the >> memory online notifier is at least currently cleaner, because we don't >> have to handle each and every subsystem that hooks into that during the >> core memory hotadd/remove phase, which primarily only setups the >> vmemmap, direct map and memory block devices. > > Cannot this hook into __add_pages which is the real implementation of > the arch independent way to allocate vmemmap. Or at the sparsemem level > because we do not (and very likely won't) support memory hotplug on > any other memory model. As __add_pages() is also called from mm/memremap.c where we don't want that metadata, we'd have to special-case (would need a new parameter I guess). > >> Personally, I think what we have in this patch is quite nice and clean. >> But I won't object if it can be similarly done in a clean way from >> hot(un)plug code. > > Well, if the scheme can be done without synchronize_rcu for each section > which can backfire and if the scheme doesn't add too much complexity to > achieve that then sure I won't object. I just do not get why page_ext > should have a different allocation lifetime expectancy than a real page. > Quite confusing if you ask me. In contrast to memmap, people actually test for zero pointers here. If you ask me the memmap access is ugly enough and I don't really enjoy other metadata following that pattern of "stale and suddenly removed". Here seems to be an easy way to do it in a clean way. But yes, if the synchronize_rcu turns out problematic, we'd either have to optimize or move the allcoation/free phase. -- Thanks, David / dhildenb
