Re: [PATCH v5 2/5] mm,compaction: Let isolate_migratepages_{range,block} return error codes

[Vlastimil Babka <vbabka@suse.cz>]

On 3/18/21 11:22 AM, Michal Hocko wrote:
> On Thu 18-03-21 10:50:38, Vlastimil Babka wrote:
>> On 3/17/21 3:59 PM, Michal Hocko wrote:
>> > On Wed 17-03-21 15:38:35, Oscar Salvador wrote:
>> >> On Wed, Mar 17, 2021 at 03:12:29PM +0100, Michal Hocko wrote:
>> >> > > 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.
>> >> > 
>> >> > This looks hakish and I cannot really tell that users of cc->migrate_pfn
>> >> > work as intended.
>> 
>> We did check those in detail. Of course it's possible to overlook something...
>> 
>> The alloc_contig_range user never cared about cc->migrate_pfn. compaction
>> (isolate_migratepages() -> isolate_migratepages_block()) did, and
>> isolate_migratepages_block() returned the pfn only to be assigned to
>> cc->migrate_pfn in isolate_migratepages(). I think it's now better that
>> isolate_migratepages_block() sets it.
>> 
>> >> When discussing this with Vlastimil, I came up with the idea of adding a new
>> >> field in compact_control struct, e.g: next_pfn_scan to keep track of the next
>> >> pfn to be scanned.
>> >> 
>> >> But Vlastimil made me realize that since cc->migrate_pfn points to that aleady,
>> >> so we do not need any extra field.
>> 
>> Yes, the first patch had at asome point:
>> 
>> 	/* Record where migration scanner will be restarted. */
>> 	cc->migrate_pfn = cc->the_new_field;
>> 
>> Which was a clear sign that the new field is unnecessary.
>> 
>> > This deserves a big fat comment.
>> 
>> Comment where, saying what? :)
> 
> E.g. something like the following
> diff --git a/mm/internal.h b/mm/internal.h
> index 1432feec62df..6c5a9066adf0 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -225,7 +225,13 @@ struct compact_control {
>  	unsigned int nr_freepages;	/* Number of isolated free pages */
>  	unsigned int nr_migratepages;	/* Number of pages to migrate */
>  	unsigned long free_pfn;		/* isolate_freepages search base */
> -	unsigned long migrate_pfn;	/* isolate_migratepages search base */
> +	unsigned long migrate_pfn;	/* Acts as an in/out parameter to page
> +					 * isolation.
> +					 * isolate_migratepages uses it as a search base.
> +					 * isolate_migratepages_block will update the
> +					 * value the next pfn after the last isolated
> +					 * one.
> +					 */

Fair enough. I would even stop pretending we might cram something useful in the
rest of the line, and move all the comments to blocks before the variables.
There might be more of them that would deserve more thorough description.

>  	unsigned long fast_start_pfn;	/* a pfn to start linear scan from */
>  	struct zone *zone;
>  	unsigned long total_migrate_scanned;
> 
> Btw isolate_migratepages_block still has this comment which needs
> updating
> "The cc->migrate_pfn field is neither read nor updated."

Good catch.