Re: [PATCH v5 2/5] mm,compaction: Let isolate_migratepages_{range,block} return error codes
From: Vlastimil Babka <hidden>
Date: 2021-03-18 11:11:16
Also in:
lkml
On 3/18/21 11:22 AM, Michal Hocko wrote:
quoted hunk ↗ jump to hunk
On Thu 18-03-21 10:50:38, Vlastimil Babka wrote:quoted
On 3/17/21 3:59 PM, Michal Hocko wrote:quoted
On Wed 17-03-21 15:38:35, Oscar Salvador wrote:quoted
On Wed, Mar 17, 2021 at 03:12:29PM +0100, Michal Hocko wrote:quoted
quoted
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.quoted
quoted
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.quoted
This deserves a big fat comment.Comment where, saying what? :)E.g. something like the followingdiff --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.