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

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

On Wed, 2019-11-13 at 00:10 +0100, David Hildenbrand wrote:
> > > > > 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?
> > 
> > If you take a look at __isolate_free_page it was performing a check to see
> > if pulling the page would place us below the minimum watermark for pages.
> > Odds are you should probably look at somehow incorporating that into the
> > solution before you pull the page. I have updated my approach to check for
> 
> Ah, now I see what you mean. Makes sense!
> 
> > the low watermark with the full capacity of MAX_ORDER - 1 pages before I
> > start reporting, and then I am using __isolate_free_page which will check
> > the minimum watermark to make sure I don't cross that.
> 
> Yeah, you probably want to check the watermark before doing any 
> reporting - I assume.
> 
> > > > > 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.
> > 
> > I had thought about that too. The only problem is that the section has to
> > be power of 2 sized and I don't know if we want to be increasing the size
> 
> ... are there sections that are not a power of 2? x86_64: 128MB, s390x: 
> 256MB, ...

No, what I meant was the mem_section structure. It has a hard requirement
about being power of 2 aligned and is already 16 bytes, or 32 with page
extensions enabled. There is room for a pad in the page extension case so
maybe you could squeeze in something there.

> It does not really make sense to have sections that are not a power of 
> two, thinking about page tables ... I would really be interested where 
> something like that is possible.

Sorry for the confusion on that.

> > 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?
> > 
> > On the end of set_migratetype_isolate(). The last thing it does is call
> > drain_all_pages.
> 
> Ahh, missed that, thanks. Yeah, one could probably make the 
> configurable, because for that use case, where we already expect a free 
> page, we don't need that.

I suppose but that gets back into adding complexity as we now have to
special case isolation to work with page reporting.

<snip>

> > pages in the region are isolated since as you pointed out you get an EBUSY
> > when you attempt to isolate a page that is already isolated and as such
> > removal will fail won't it?
> 
> Right now, yes.
> 
> (we should rework that code either way to return -EAGAIN in that case 
> and let memory offlining try again automatically. But we have to rework 
> the -EAGAIN vs. -EBUSY handling in memory offlining code at one point 
> either way, I discussed that partially with Michal recently. There is a 
> lot of cleaning up to do.)

So it sounds like a cleanup/rewrite of some of the isolation code will be
needed to really get it doing what you want.

Actually I wonder if we couldn't look at something like the
free_reported_page function I did and instead split it up so that it could
be used as an inverse of __isolate_free_page. Maybe something like a
__free_isolated_page.

> > > > 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.
> > 
> > We can agree to disagree. At this point you are talking about adding bits
> > for sections and pages, and in the meantime I am working with zones and
> > pages. I believe finding free space in the section may be much more tricky
> > than finding it in the zone or page has been. Now that I am rid of the
> > list manipulators my approach may soon surpass the bitmap one in terms of
> > being less intrusive/complex.. :-)
> 
> I am definitely interested to see that approach :) Good to see that the 
> whole discussion in this big thread turned out to be productive.

Yeah, when I started working on the patch split Mel wanted I kind of
realized I was optimizing for the shuffle case which really shouldn't be
an optimization target. I think I just got focused on it as it was in the
way of some of the initial changes I needed to make to handle the
notifier.