On Tue, 2019-11-12 at 14:04 +0100, David Hildenbrand wrote: > > > > fact is it is still invasive, just to different parts of the mm subsystem. > > > > > > I'd love to see how it uses the page isolation framework, and only has a > > > single hook to queue pages. I don't like the way pages are pulled out of > > > the buddy in Niteshs approach currently. What you have is cleaner. > > > > I don't see how you could use the page isolation framework to pull out > > free pages. Is there a thread somewhere on the topic that I missed? > > It's basically only isolating pages while reporting them, and not > pulling them out of the buddy (IOW, you move the pages to the isolate > queues where nobody is allowed to touch them, and setting the > migratetype properly). This e.g., makes other user of page isolation > (e.g., memory offlining, alloc_contig_range()) play nicely with these > isolated pages. "somebody else just isolated them, please try again." How so? If I understand correctly there isn't anything that prevents you from isolating an already isolated page, is there? Last I knew isolated pages are still considered "movable" since they are still buddy pages aren't they? Also this seems like it would have other implications since isolating a page kicks of the memory notifier so as a result a balloon driver would then free the pages back out so that they could be isolated with the assumption the region is going offline. > start_isolate_page_range()/undo_isolate_page_range()/test_pages_isolated() > along with a lockless check if the page is free. Okay, that part I think I get. However doesn't all that logic more or less ignore the watermarks? It seems like you could cause an OOM if you don't have the necessary checks in place for that. > I think it should be something like this (ignoring different > migratetypes and such for now) > > 1. Test lockless if page is free: Not free? Done. So this should help to reduce the liklihood of races in the steps below. However it might also be useful if the code had some other check to see if it was done other than just making a pass through the bitmap. One thing I had brought up with Nitesh was the idea of maybe doing some sort of RCU bitmap type approach. Basically while we hold the zone lock we could swap out the old bitmap for a new one. We could probably even keep a counter at the start of the structure so that we could track how many bits are actually set there. Then it becomes less likely of having a race where you free a page and set the bit and the hinting thread tests and clears the bit but doesn't see the freed page since it is not synchronized. Otherwise your notification setup and reporting thread may need a few smp barriers added where necessary. > 2. start_isolate_page_range(): Busy? Rare race (with other isolate users Doesn't this have the side effect of draining all the percpu caches in order to make certain to flush the pages we isolated from there? > or with an allocation). Done. > 3. test_pages_isolated() So I have reviewed the code and I don't see how this could conflict with other callers isolating the pages. If anything it seems like if another thread has already isolated the pages you would end up getting a false positive, reporting the pages, and pulling them back out of isolation. > 3a. no? Rare race, page not free anymore. undo_isolate_page_range() I would hope it is rare. However for something like a max order page I could easily see a piece of it having been pulled out. I would think this case would be exceedingly expensive since you would have to put back any pages you had previous moved into isolation. > 3b. yes? Report, then undo_isolate_page_range() > > If we would run into performance issues with the current page isolation > implementation (esp. locking), I think there are some nice > cleanups/reworks possible of which all current users could benefit > (especially accross pageblocks). To me this feels a lot like what you had for this solution near the start. Only now instead of placing the pages into an array you are tracking a bitmap and then using that bitmap to populate the MIGRATE_ISOLATE lists. This sounds far more complex to me then it probably needs to be since just holding the pages with the buddy type cleared should be enough to make them temporarily unusable for other threads, and even in your case you are still having to use the scatterlist in order to hold the pages and track what you will need to undo the isolation later.
