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

Reply via email to