On 28.07.22 11:53, Charan Teja Kalla wrote: > Thanks David for the inputs!! > > On 7/27/2022 10:59 PM, David Hildenbrand wrote: >>> Fix those paths where offline races with page_ext access by maintaining >>> synchronization with rcu lock and is achieved in 3 steps: >>> 1) Invalidate all the page_ext's of the sections of a memory block by >>> storing a flag in the LSB of mem_section->page_ext. >>> >>> 2) Wait till all the existing readers to finish working with the >>> ->page_ext's with synchronize_rcu(). Any parallel process that starts >>> after this call will not get page_ext, through lookup_page_ext(), for >>> the block parallel offline operation is being performed. >>> >>> 3) Now safely free all sections ->page_ext's of the block on which >>> offline operation is being performed. >>> >>> Thanks to David Hildenbrand for his views/suggestions on the initial >>> discussion[1] and Pavan kondeti for various inputs on this patch. >>> >>> FAQ's: >>> Q) Should page_ext_[get|put]() needs to be used for every page_ext >>> access? >>> A) NO, the synchronization is really not needed in all the paths of >>> accessing page_ext. One case is where extra refcount is taken on a >>> page for which memory block, this pages falls into, offline operation is >>> being performed. This extra refcount makes the offline operation not to >>> succeed hence the freeing of page_ext. Another case is where the page >>> is already being freed and we do reset its page_owner. >>> >>> Some examples where the rcu_lock is not taken while accessing the >>> page_ext are: >>> 1) In migration (where we also migrate the page_owner information), we >>> take the extra refcount on the source and destination pages and then >>> start the migration. This extra refcount makes the test_pages_isolated() >>> function to fail thus retry the offline operation. >>> >>> 2) In free_pages_prepare(), we do reset the page_owner(through page_ext) >>> which again doesn't need the protection to access because the page is >>> already freeing (through only one path). >>> >>> So, users need not to use page_ext_[get|put]() when they are sure that >>> extra refcount is taken on a page preventing the offline operation. >>> >>> Q) Why can't the page_ext is freed in the hot_remove path, where memmap >>> is also freed ? >>> >>> A) As per David's answers, there are many reasons and a few are: >>> 1) Discussions had happened in the past to eventually also use rcu >>> protection for handling pfn_to_online_page(). So doing it cleanly here >>> is certainly an improvement. >>> >>> 2) It's not good 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. >>> >>> 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. >>> >>> [1] https://lore.kernel.org/linux-mm/59edde13-4167-8550-86f0-11fc67882107@xxxxxxxxxxx/ >>> >> I guess if we care about the synchronize_rcu() we could go crazy with >> temporary allocations for data-to-free + call_rcu(). > > IMO, single synchronize_rcu() call overhead shouldn't be cared > especially if the memory offline operation it self is expected to > complete in seconds. On the Snapdragon system, I can see the lowest it > can complete in 3-4secs for a complete memory block of size 512M. And > agree that this time depends on lot of other factors too but wanted to > raise a point that it is really not a path where tiny optimizations > should be strictly considered. __Please help in correcting me If I am > really downplaying the scenario here__. I agree that we should optimize only if we find this to be an issue. > > But then I moved to single synchronize_rcu() just to avoid any visible > effects that can cause by multiple synchronize_rcu() for a single memory > block with lot of sections. Makes sense. > > Having said that, I am open to go for call_rcu() and infact it will be a > much simple change where I can do the freeing of page_ext in the > __free_page_ext() itself which is called for every section there by > avoid the extra tracking flag PAGE_EXT_INVALID. > ........... > WRITE_ONCE(ms->page_ext, NULL); > call_rcu(rcu_head, fun); // Free in fun() > ............. > > Or your opinion is to use call_rcu () only once in place of > synchronize_rcu() after invalidating all the page_ext's of memory block? Yeah, that would be an option. And if you fail to allocate a temporary buffer to hold the data-to-free (structure containing rcu_head), the slower fallback path would be synchronize_rcu(). But again, I'm also not sure if we have to optimize here right now. -- Thanks, David / dhildenb
