Re: + mm-introduce-reported-pages.patch added to -mm tree

[Alexander Duyck <alexander.h.duyck@linux.intel.com>]

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.