On 12.11.19 19:34, Alexander Duyck wrote: > 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 mm/page_isolation.c:set_migratetype_isolate() ... if (is_migrate_isolate_page(page)) goto out; ... -> Currently -EBUSY > pages are still considered "movable" since they are still buddy pages > aren't they? They are neither movable nor unmovable AFAIK. They are temporarily blocked. E.g., memory offlining currently returns -EBUSY if it cannot isolate the page range. alloc_contig_range() does the same. Imagine somebody allocating a gigantic page. You certainly cannot move the pages that are isolated while allocating the page. But you can signal to the caller to try again later. > > 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. Memory notifier? Balloon pages getting freed? No. The memory notifier is used for onlining/offlining, it is not involved here. I think what you mean is the "isolate notifier", which is only used by CMM on PPC. See https://lkml.org/lkml/2019/10/31/487, where I rip that notifier out. > >> 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. Any approach that temporarily blocks some free pages from getting allocated will essentially have this issue, no? I think one main design point to minimize false OOMs was to limit the number of pages we report at a time. Or what do you propose here in addition to 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. Yes. > > 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. Yes, swapping out the bitmap via RCU is also be a way to make memory hotplug work. I was also thinking about a different bitmap approach. Store for each section a bitmap. Use a meta bitmap with a bit for each section that contains pages to report. Sparse zones and memory hot(un)plug would not be a real issue anymore. One could go one step further and only have a bitmap with a bit for each section. Only remember that some (large) page was not reported in that section (e.g., after buddy merging). In the reporting thread, report all free pages within that section. You could end up reporting the same page a couple of times, but the question would be if this is relevant at all. One would have to prototype and measure that. Long story short, I am not 100% a fan of the current "bitmap per zone" approach but is is fairly simple to start with :) > >> 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? While alloc_contig_range() e.g., calls lru_add_drain_all(), I don't think isolation will. Where did you spot something like this in mm/page_isolation.c? > >> 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. Isolated pages cannot be isolated. This is tracked via the migratetype. > >> 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. I guess it is rare, there is a tiny slot between checking if the page is free and isolating it. Would have to see that in action. > >> 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. Now we have a clean MM interface to do that :) And yes, which data structure we're using becomes irrelevant. > > 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 If you have a page that is not PageBuddy() and not movable within ZONE_MOVABLE, has_unmovable_pages() will WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE). This can be triggered via memory offlining, when isolating the page range. If your approach does exactly that (clear PageBuddy() on a ZONE_MOVABLE), it would be a bug. The only safe way is to have the pageblock(s) isolated. > still having to use the scatterlist in order to hold the pages and track > what you will need to undo the isolation later. I think it is very neat and not complex at all. Page isolation is a nice feature we have in the kernel. :) It deserves some cleanups, though. -- Thanks, David / dhildenb
