linux-kernel - Re: [PATCH 34/34] mm, vmstat: remove zone and node double accounting by approximating retries

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: <20160715074859.GM9806@techsingularity.net>
Date:	Fri, 15 Jul 2016 08:48:59 +0100
From:	Mel Gorman <mgorman@...hsingularity.net>
To:	Vlastimil Babka <vbabka@...e.cz>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Linux-MM <linux-mm@...ck.org>, Rik van Riel <riel@...riel.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Minchan Kim <minchan@...nel.org>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 34/34] mm, vmstat: remove zone and node double accounting
 by approximating retries

On Thu, Jul 14, 2016 at 03:40:11PM +0200, Vlastimil Babka wrote:
> >@@ -4,6 +4,26 @@
> > #include <linux/huge_mm.h>
> > #include <linux/swap.h>
> >
> >+#ifdef CONFIG_HIGHMEM
> >+extern atomic_t highmem_file_pages;
> >+
> >+static inline void acct_highmem_file_pages(int zid, enum lru_list lru,
> >+							int nr_pages)
> >+{
> >+	if (is_highmem_idx(zid) && is_file_lru(lru)) {
> >+		if (nr_pages > 0)
> 
> This seems like a unnecessary branch, atomic_add should handle negative
> nr_pages just fine?
> 

On x86 it would but the interface makes no guarantees it'll handle
signed types properly on all architectures.

> >@@ -1456,14 +1461,27 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
> > 		unsigned long available;
> > 		enum compact_result compact_result;
> >
> >+		if (last_pgdat == zone->zone_pgdat)
> >+			continue;
> >+
> >+		/*
> >+		 * This over-estimates the number of pages available for
> >+		 * reclaim/compaction but walking the LRU would take too
> >+		 * long. The consequences are that compaction may retry
> >+		 * longer than it should for a zone-constrained allocation
> >+		 * request.
> 
> The comment above says that we don't retry zone-constrained at all. Is this
> an obsolete comment, or does it refer to the ZONE_NORMAL constraint? (as
> opposed to HIGHMEM, MOVABLE etc?).
> 

It can still over-estimate the amount of memory available if
ZONE_MOVABLE exists even if the request is not zone-constrained.

> >@@ -3454,6 +3455,15 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
> > 		return false;
> >
> > 	/*
> >+	 * Blindly retry lowmem allocation requests that are often ignored by
> >+	 * the OOM killer up to MAX_RECLAIM_RETRIES as we not have a reliable
> >+	 * and fast means of calculating reclaimable, dirty and writeback pages
> >+	 * in eligible zones.
> >+	 */
> >+	if (ac->high_zoneidx < ZONE_NORMAL)
> >+		goto out;
> 
> A goto inside two nested for cycles? Is there no hope for sanity? :(
> 

None, hand it in at the door.

It can be pulled out and put past the "return false" at the end. It's
just not necessarily any better.

-- 
Mel Gorman
SUSE Labs