linux-kernel - Re: [PATCH v7 2/7] mm,compaction: Let isolate_migratepages

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: <2628d20e-3304-1a0c-5246-cfc2320cce8b@redhat.com>
Date:   Wed, 14 Apr 2021 13:54:17 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Oscar Salvador <osalvador@...e.de>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     Mike Kravetz <mike.kravetz@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Michal Hocko <mhocko@...nel.org>,
        Muchun Song <songmuchun@...edance.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/7] mm,compaction: Let
 isolate_migratepages_{range,block} return error codes

On 13.04.21 12:47, Oscar Salvador wrote:
> Currently, isolate_migratepages_{range,block} and their callers use
> a pfn == 0 vs pfn != 0 scheme to let the caller know whether there was
> any error during isolation.
> This does not work as soon as we need to start reporting different error
> codes and make sure we pass them down the chain, so they are properly
> interpreted by functions like e.g: alloc_contig_range.
> 
> Let us rework isolate_migratepages_{range,block} so we can report error
> codes.
> Since isolate_migratepages_block will stop returning the next pfn to be
> scanned, we reuse the cc->migrate_pfn field to keep track of that.
> 
> Signed-off-by: Oscar Salvador <osalvador@...e.de>
> Acked-by: Vlastimil Babka <vbabka@...e.cz>
> ---
>   mm/compaction.c | 54 +++++++++++++++++++++++++++---------------------------
>   mm/internal.h   | 10 ++++++++--
>   mm/page_alloc.c |  7 +++----
>   3 files changed, 38 insertions(+), 33 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 8c5028bfbd56..eeba4668c22c 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -787,15 +787,15 @@ static bool too_many_isolated(pg_data_t *pgdat)
>    *
>    * Isolate all pages that can be migrated from the range specified by
>    * [low_pfn, end_pfn). The range is expected to be within same pageblock.
> - * Returns zero if there is a fatal signal pending, otherwise PFN of the
> - * first page that was not scanned (which may be both less, equal to or more
> - * than end_pfn).
> + * Returns errno, like -EAGAIN or -EINTR in case e.g signal pending or congestion,
> + * or 0.
> + * cc->migrate_pfn will contain the next pfn to scan (which may be both less,
> + * equal to or more that end_pfn).

I failed to parse the last sentence -- e.g., using "both" and then 
listing three options. Also, s/than/than/? Can we simplify to

"cc->migrate_pfn will contain the next pfn to scan"

>    *
>    * The pages are isolated on cc->migratepages list (not required to be empty),
> - * and cc->nr_migratepages is updated accordingly. The cc->migrate_pfn field
> - * is neither read nor updated.
> + * and cc->nr_migratepages is updated accordingly.
>    */
> -static unsigned long
> +static int
>   isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>   			unsigned long end_pfn, isolate_mode_t isolate_mode)
>   {
> @@ -809,6 +809,9 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>   	bool skip_on_failure = false;
>   	unsigned long next_skip_pfn = 0;
>   	bool skip_updated = false;
> +	int ret = 0;
> +
> +	cc->migrate_pfn = low_pfn;
>   
>   	/*
>   	 * Ensure that there are not too many pages isolated from the LRU
> @@ -818,16 +821,16 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>   	while (unlikely(too_many_isolated(pgdat))) {
>   		/* stop isolation if there are still pages not migrated */
>   		if (cc->nr_migratepages)
> -			return 0;
> +			return -EAGAIN;
>   
>   		/* async migration should just abort */
>   		if (cc->mode == MIGRATE_ASYNC)
> -			return 0;
> +			return -EAGAIN;
>   
>   		congestion_wait(BLK_RW_ASYNC, HZ/10);
>   
>   		if (fatal_signal_pending(current))
> -			return 0;
> +			return -EINTR;
>   	}
>   
>   	cond_resched();
> @@ -875,8 +878,8 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>   
>   			if (fatal_signal_pending(current)) {
>   				cc->contended = true;
> +				ret = -EINTR;
>   
> -				low_pfn = 0;
>   				goto fatal_pending;
>   			}
>   
> @@ -1130,7 +1133,9 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>   	if (nr_isolated)
>   		count_compact_events(COMPACTISOLATED, nr_isolated);
>   
> -	return low_pfn;
> +	cc->migrate_pfn = low_pfn;
> +
> +	return ret;
>   }
>   
>   /**
> @@ -1139,15 +1144,15 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>    * @start_pfn: The first PFN to start isolating.
>    * @end_pfn:   The one-past-last PFN.
>    *
> - * Returns zero if isolation fails fatally due to e.g. pending signal.
> - * Otherwise, function returns one-past-the-last PFN of isolated page
> - * (which may be greater than end_pfn if end fell in a middle of a THP page).
> + * Returns errno, like -EAGAIN or -EINTR in case e.g signal pending or congestion,

errno is usually positive.

> + * or 0.

I'd be more specific here. Something like

"
Returns 0 if isolation succeeded. Returns -EINTR if a fatal signal is 
pending. Returns -EAGAIN when contended and the caller should retry.

In any case, cc->migrate_pfn is set to one-past-the-last PFN that has 
been isolated.
"

-- 
Thanks,

David / dhildenb