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

[Vlastimil Babka]

On 07/15/2016 09:48 AM, Mel Gorman wrote:
> 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.

Hmm really? At least some drivers do that in an easily grepable way:

drivers/tty/serial/dz.c:                        atomic_add(-1, &mux->map_guard);
drivers/tty/serial/sb1250-duart.c:                      atomic_add(-1, &duart->map_guard);
drivers/tty/serial/zs.c:                        atomic_add(-1, &scc->irq_guard);

And our own __mod_zone_page_state() can get both negative and positive
vales and boils down to atomic_long_add() (I assume the long variant wouldn't
be different in this aspect).

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

OK.

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

Mine's long gone, was thinking for the future newbies :)
 
> It can be pulled out and put past the "return false" at the end. It's
> just not necessarily any better.

I see...