linux-kernel - Re: [RFC PATCH 1/3] mm, reclaim: make should_continue

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: <295a37b1-8257-9b4a-b586-9a4990cc9d35@suse.cz>
Date:   Wed, 31 Jul 2019 13:08:44 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Hillf Danton <hdanton@...a.com>, Mel Gorman <mgorman@...e.de>,
        Mike Kravetz <mike.kravetz@...cle.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Michal Hocko <mhocko@...nel.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [RFC PATCH 1/3] mm, reclaim: make should_continue_reclaim perform
 dryrun detection

On 7/26/19 9:40 AM, Hillf Danton wrote:
> 
> On Thu, 25 Jul 2019 08:05:55 +0000 (UTC) Mel Gorman wrote:
>>
>> Agreed that the description could do with improvement. However, it
>> makes sense that if compaction reports it can make progress that it is
>> unnecessary to continue reclaiming.
> 
> Thanks Mike and Mel.
> 
> Hillf
> ---8<---
> From: Hillf Danton <hdanton@...a.com>
> Subject: [RFC PATCH 1/3] mm, reclaim: make should_continue_reclaim perform dryrun detection
> 
> Address the issue of should_continue_reclaim continuing true too often
> for __GFP_RETRY_MAYFAIL attempts when !nr_reclaimed and nr_scanned.
> This could happen during hugetlb page allocation causing stalls for
> minutes or hours.
> 
> We can stop reclaiming pages if compaction reports it can make a progress.
> A code reshuffle is needed to do that. And it has side-effects, however,
> with allocation latencies in other cases but that would come at the cost
> of potential premature reclaim which has consequences of itself.

I don't really understand that paragraph, did Mel meant it like this?

> We can also bail out of reclaiming pages if we know that there are not
> enough inactive lru pages left to satisfy the costly allocation.
> 
> We can give up reclaiming pages too if we see dryrun occur, with the
> certainty of plenty of inactive pages. IOW with dryrun detected, we are
> sure we have reclaimed as many pages as we could.
> 
> Cc: Mike Kravetz <mike.kravetz@...cle.com>
> Cc: Mel Gorman <mgorman@...e.de>
> Cc: Michal Hocko <mhocko@...nel.org>
> Cc: Vlastimil Babka <vbabka@...e.cz>
> Cc: Johannes Weiner <hannes@...xchg.org>
> Signed-off-by: Hillf Danton <hdanton@...a.com>

I agree this is an improvement overall, but perhaps the patch does too
many things at once. The reshuffle is one thing and makes sense. The
change of the last return condition could perhaps be separate. Also
AFAICS the ultimate result is that when nr_reclaimed == 0, the function
will now always return false. Which makes the initial test for
__GFP_RETRY_MAYFAIL and the comments there misleading. There will no
longer be a full LRU scan guaranteed - as long as the scanned LRU chunk
yields no reclaimed page, we abort.

> ---
>  mm/vmscan.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index f4fd02a..484b6b1 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2673,18 +2673,6 @@ static inline bool should_continue_reclaim(struct pglist_data *pgdat,
>  			return false;
>  	}
>  
> -	/*
> -	 * If we have not reclaimed enough pages for compaction and the
> -	 * inactive lists are large enough, continue reclaiming
> -	 */
> -	pages_for_compaction = compact_gap(sc->order);
> -	inactive_lru_pages = node_page_state(pgdat, NR_INACTIVE_FILE);
> -	if (get_nr_swap_pages() > 0)
> -		inactive_lru_pages += node_page_state(pgdat, NR_INACTIVE_ANON);
> -	if (sc->nr_reclaimed < pages_for_compaction &&
> -			inactive_lru_pages > pages_for_compaction)
> -		return true;
> -
>  	/* If compaction would go ahead or the allocation would succeed, stop */
>  	for (z = 0; z <= sc->reclaim_idx; z++) {
>  		struct zone *zone = &pgdat->node_zones[z];
> @@ -2700,7 +2688,21 @@ static inline bool should_continue_reclaim(struct pglist_data *pgdat,
>  			;
>  		}
>  	}
> -	return true;
> +
> +	/*
> +	 * If we have not reclaimed enough pages for compaction and the
> +	 * inactive lists are large enough, continue reclaiming
> +	 */
> +	pages_for_compaction = compact_gap(sc->order);
> +	inactive_lru_pages = node_page_state(pgdat, NR_INACTIVE_FILE);
> +	if (get_nr_swap_pages() > 0)
> +		inactive_lru_pages += node_page_state(pgdat, NR_INACTIVE_ANON);
> +
> +	return inactive_lru_pages > pages_for_compaction &&
> +		/*
> +		 * avoid dryrun with plenty of inactive pages
> +		 */
> +		nr_scanned && nr_reclaimed;
>  }
>  
>  static bool pgdat_memcg_congested(pg_data_t *pgdat, struct mem_cgroup *memcg)
> --
>