On Tue, Oct 11, 2016 at 09:26:06AM +0200, Michal Hocko wrote:
> On Tue 11-10-16 16:09:45, Minchan Kim wrote:
> > On Tue, Oct 11, 2016 at 08:50:48AM +0200, Michal Hocko wrote:
> > > On Tue 11-10-16 14:01:41, Minchan Kim wrote:
> [...]
> > > > Also, your patch makes retry loop greater than MAX_RECLAIM_RETRIES
> > > > if unreserve_highatomic_pageblock returns true. Theoretically,
> > > > it would make live lock. You might argue it's *really really* rare
> > > > but I don't want to add such subtle thing.
> > > > Maybe, we could drain when no_progress_loops == MAX_RECLAIM_RETRIES.
> > >
> > > What would be the scenario when we would really livelock here? How can
> > > we have unreserve_highatomic_pageblock returning true for ever?
> >
> > Other context freeing highorder page/reallocating repeatedly while
> > a process stucked direct reclaim is looping with should_reclaim_retry.
>
> If we unreserve those pages then we should converge to OOM. Btw. this
> can happen even without highmem reserves. Heavy short lived allocations
> might keep us looping at the lowest priority. They are just too unlikely
> to care about.
Indeed.
>
> > > > > aggressive to me. If we just do one at the time we have a chance to
> > > > > keep some reserves if the OOM situation is really ephemeral.
> > > > >
> > > > > Does this patch work in your usecase?
> > > >
> > > > I didn't test but I guess it works but it has problems I mentioned
> > > > above.
> > >
> > > Please do not make this too over complicated and be practical. I do not
> > > really want to dismiss your usecase but I am really not convinced that
> > > such a "perfectly fit into all memory" situations are sustainable and
> > > justify to make the whole code more complex. I agree that we can at
> > > least try to do something to release those reserves but let's do it
> > > as simple as possible.
> >
> > If you think it's too complicated, how about this?
>
> Definitely better than the original patch. Little bit too aggressive
> because we could really go with one block at the time. But this is a
> minor thing and easily fixable...
>
> > @@ -2154,12 +2156,24 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
> > * may increase.
> > */
> > set_pageblock_migratetype(page, ac->migratetype);
> > - move_freepages_block(zone, page, ac->migratetype);
> > - spin_unlock_irqrestore(&zone->lock, flags);
> > - return;
> > + ret = move_freepages_block(zone, page,
> > + ac->migratetype);
> > + /*
> > + * By race with page freeing functions, !highatomic
> > + * pageblocks can have free pages in highatomic free
> > + * list so if drain is true, try to unreserve every
> > + * free pages in highatomic free list without bailing
> > + * out.
> > + */
> > + if (!drain) {
>
> if (ret)
> > + spin_unlock_irqrestore(&zone->lock, flags);
> > + return ret;
> > + }
>
> arguably this would work better also for !drain case which currently
> tries to unreserve but in case of the race it would do nothing.
I thought it but I was afraid if you say again it's over complicated.
I will do it with your SOB in next spin.
Thanks, Michal.
>
> > }
> > spin_unlock_irqrestore(&zone->lock, flags);
> > }
> > +
> > + return ret;
> > }
> >
> > /* Remove an element from the buddy allocator from the fallback list */
> --
> Michal Hocko
> SUSE Labs
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>