Re: [PATCH v5 2/5] mm,compaction: Let isolate_migratepages_{range,block} return error codes
On Fri, Mar 19, 2021 at 11:14:25AM +0100, Vlastimil Babka wrote:
> No I meant this:
>
> --- 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 */
> + /*
> +* Acts as an in/out parameter to page isolation for migration.
> +* isolate_migratepages uses it as a search base.
> +* isolate_migratepages_block will update the value to the next pfn
> +* after the last isolated one.
> +*/
> + unsigned long migrate_pfn;
> unsigned long fast_start_pfn; /* a pfn to start linear scan from */
> struct zone *zone;
> unsigned long total_migrate_scanned;
Meh, silly me.
Ok, I will do it that way.
I am also for expanding some of the comments as I see that some explanations are
rather laconic, but I do not think such work fits in this patchset.
Since I happen to be checking compaction code due to other reasons, I shall
come back to this matter once I am done with this patchset.
--
Oscar Salvador
SUSE L3
Re: [PATCH v5 2/5] mm,compaction: Let isolate_migratepages_{range,block} return error codes
On 3/19/21 10:57 AM, Oscar Salvador wrote:
> On Thu, Mar 18, 2021 at 12:36:52PM +0100, Michal Hocko wrote:
>> Yeah, makes sense. I am not a fan of the above form of documentation.
>> Btw. maybe renaming the field would be even better, both from the
>> intention and review all existing users. I would go with pfn_iter or
>> something that wouldn't make it sound like migration specific.
>
> Just to be sure we are on the same page, you meant something like the
> following
> (wrt. comments):
>
> /*
> * compact_control is used to track pages being migrated and the free pages
> * they are being migrated to during memory compaction. The free_pfn starts
> * at the end of a zone and migrate_pfn begins at the start. Movable pages
> * are moved to the end of a zone during a compaction run and the run
> * completes when free_pfn <= migrate_pfn
> *
> * freepages: List of free pages to migrate to
> * migratepages:List of pages that need to be migrated
> * nr_freepages:Number of isolated free pages
> ...
> */
> struct compact_control {
> struct list_head freepages;
> ...
No I meant this:
--- 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 */
+ /*
+* Acts as an in/out parameter to page isolation for migration.
+* isolate_migratepages uses it as a search base.
+* isolate_migratepages_block will update the value to the next pfn
+* after the last isolated one.
+*/
+ unsigned long migrate_pfn;
unsigned long fast_start_pfn; /* a pfn to start linear scan from */
struct zone *zone;
unsigned long total_migrate_scanned;
> With the preface that I am not really familiar with compaction code:
>
> About renaming the variable to something else, I wouldn't do it.
> I see migrate_pfn being used in contexts where migration gets mentioned,
> e.g:
I also don't like the renaming much. "Migration" is important as this is about
pages to be migrated, and there's "free_pfn" field tracking scan for free pages
as
migration target. So the name can't be as generic as "pfn_iter".
> /*
> * Briefly search the free lists for a migration source that already has
> * some free pages to reduce the number of pages that need migration
> * before a pageblock is free.
> */
> fast_find_migrateblock(struct compact_control *cc)
> {
> ...
> unsigned long pfn = cc->migrate_pfn;
> }
>
> isolate_migratepages()
> /* Record where migration scanner will be restarted. */
>
>
> So, I would either stick with it, or add a new 'iter_pfn'/'next_pfn_scan'
> field if we feel the need to.
>
>
Re: [PATCH v5 2/5] mm,compaction: Let isolate_migratepages_{range,block} return error codes
On Thu, Mar 18, 2021 at 12:36:52PM +0100, Michal Hocko wrote:
> Yeah, makes sense. I am not a fan of the above form of documentation.
> Btw. maybe renaming the field would be even better, both from the
> intention and review all existing users. I would go with pfn_iter or
> something that wouldn't make it sound like migration specific.
Just to be sure we are on the same page, you meant something like the following
(wrt. comments):
/*
* compact_control is used to track pages being migrated and the free pages
* they are being migrated to during memory compaction. The free_pfn starts
* at the end of a zone and migrate_pfn begins at the start. Movable pages
* are moved to the end of a zone during a compaction run and the run
* completes when free_pfn <= migrate_pfn
*
* freepages: List of free pages to migrate to
* migratepages:List of pages that need to be migrated
* nr_freepages:Number of isolated free pages
...
*/
struct compact_control {
struct list_head freepages;
...
With the preface that I am not really familiar with compaction code:
About renaming the variable to something else, I wouldn't do it.
I see migrate_pfn being used in contexts where migration gets mentioned,
e.g:
/*
* Briefly search the free lists for a migration source that already has
* some free pages to reduce the number of pages that need migration
* before a pageblock is free.
*/
fast_find_migrateblock(struct compact_control *cc)
{
...
unsigned long pfn = cc->migrate_pfn;
}
isolate_migratepages()
/* Record where migration scanner will be restarted. */
So, I would either stick with it, or add a new 'iter_pfn'/'next_pfn_scan'
field if we feel the need to.
--
Oscar Salvador
SUSE L3
Re: [PATCH v5 2/5] mm,compaction: Let isolate_migratepages_{range,block} return error codes
On Thu 18-03-21 12:10:14, Vlastimil Babka wrote:
> On 3/18/21 11:22 AM, Michal Hocko wrote:
[...]
> > 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.
Yeah, makes sense. I am not a fan of the above form of documentation.
Btw. maybe renaming the field would be even better, both from the
intention and review all existing users. I would go with pfn_iter or
something that wouldn't make it sound like migration specific.
--
Michal Hocko
SUSE Labs
Re: [PATCH v5 2/5] mm,compaction: Let isolate_migratepages_{range,block} return error codes
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.
Re: [PATCH v5 2/5] mm,compaction: Let isolate_migratepages_{range,block} return error codes
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.
+*/
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."
--
Michal Hocko
SUSE Labs
Re: [PATCH v5 2/5] mm,compaction: Let isolate_migratepages_{range,block} return error codes
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? :)
Re: [PATCH v5 2/5] mm,compaction: Let isolate_migratepages_{range,block} return error codes
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.
>
> 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.
This deserves a big fat comment.
> > > @@ -810,6 +811,8 @@ isolate_migratepages_block(struct compact_control
> > > *cc, unsigned long low_pfn,
> > > unsigned long next_skip_pfn = 0;
> > > bool skip_updated = false;
> > >
> > > + cc->migrate_pfn = low_pfn;
> > > +
> > > /*
> > >* Ensure that there are not too many pages isolated from the LRU
> > >* list by either parallel reclaimers or compaction. If there are,
> > > @@ -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 -EINTR;
> > >
> > > /* async migration should just abort */
> > > if (cc->mode == MIGRATE_ASYNC)
> > > - return 0;
> > > + return -EINTR;
> >
> > EINTR for anything other than signal based bail out is really confusing.
>
> When coding that, I thought about using -1 for the first two checks, and keep
> -EINTR for the signal check, but isolate_migratepages_block only has two
> users:
No, do not mix error reporting with different semantic. Either make it
errno or return -1 for all failures if you do not care which error that
is. You do care and hence this patch so make that errno and above two
should simply EAGAIN as this is a congestion situation.
> - isolate_migratepages: Does not care about the return code other than pfn !=
> 0,
> and it does not pass the error down the chain.
> - isolate_migratepages_range: The error is passed down the chain, and !pfn is
> being
> treated as -EINTR:
>
> static int __alloc_contig_migrate_range(struct compact_control *cc,
> unsigned long start, unsigned long end)
> {
> ...
> ...
> pfn = isolate_migratepages_range(cc, pfn, end);
> if (!pfn) {
> ret = -EINTR;
> break;
> }
> ...
> }
>
> That is why I decided to stick with -EINTR.
I suspect this is only because there was not really a better way to tell
the failure so it went with EINTR which makes alloc_contig_range bail
out. The high level handling there is quite dubious as EAGAIN is already
possible from the page migration path and that shouldn't be a fatal
failure.
--
Michal Hocko
SUSE Labs
Re: [PATCH v5 2/5] mm,compaction: Let isolate_migratepages_{range,block} return error codes
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.
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.
> > @@ -810,6 +811,8 @@ isolate_migratepages_block(struct compact_control *cc,
> > unsigned long low_pfn,
> > unsigned long next_skip_pfn = 0;
> > bool skip_updated = false;
> >
> > + cc->migrate_pfn = low_pfn;
> > +
> > /*
> > * Ensure that there are not too many pages isolated from the LRU
> > * list by either parallel reclaimers or compaction. If there are,
> > @@ -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 -EINTR;
> >
> > /* async migration should just abort */
> > if (cc->mode == MIGRATE_ASYNC)
> > - return 0;
> > + return -EINTR;
>
> EINTR for anything other than signal based bail out is really confusing.
When coding that, I thought about using -1 for the first two checks, and keep
-EINTR for the signal check, but isolate_migratepages_block only has two users:
- isolate_migratepages: Does not care about the return code other than pfn != 0,
and it does not pass the error down the chain.
- isolate_migratepages_range: The error is passed down the chain, and !pfn is
being
treated as -EINTR:
static int __alloc_contig_migrate_range(struct compact_control *cc,
unsigned long start, unsigned long end)
{
...
...
pfn = isolate_migratepages_range(cc, pfn, end);
if (!pfn) {
ret = -EINTR;
break;
}
...
}
That is why I decided to stick with -EINTR.
--
Oscar Salvador
SUSE L3
Re: [PATCH v5 2/5] mm,compaction: Let isolate_migratepages_{range,block} return error codes
On Wed 17-03-21 12:12:48, 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.
Yes this is an improvement.
> 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.
> @@ -810,6 +811,8 @@ isolate_migratepages_block(struct compact_control *cc,
> unsigned long low_pfn,
> unsigned long next_skip_pfn = 0;
> bool skip_updated = false;
>
> + cc->migrate_pfn = low_pfn;
> +
> /*
>* Ensure that there are not too many pages isolated from the LRU
>* list by either parallel reclaimers or compaction. If there are,
> @@ -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 -EINTR;
>
> /* async migration should just abort */
> if (cc->mode == MIGRATE_ASYNC)
> - return 0;
> + return -EINTR;
EINTR for anything other than signal based bail out is really confusing.
--
Michal Hocko
SUSE Labs
