linux-kernel - Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161011072605.GD31996@dhcp22.suse.cz>
Date:   Tue, 11 Oct 2016 09:26:06 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Minchan Kim <minchan@...nel.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Vlastimil Babka <vbabka@...e.cz>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Sangseok Lee <sangseok.lee@....com>
Subject: Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM

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.

> > > > 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.

>  		}
>  		spin_unlock_irqrestore(&zone->lock, flags);
>  	}
> +
> +	return ret;
>  }
>  
>  /* Remove an element from the buddy allocator from the fallback list */
-- 
Michal Hocko
SUSE Labs