| Message ID | 20230418191313.268131-15-hannes@cmpxchg.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: reliable huge page allocator | expand |
On Tue, Apr 18, 2023 at 03:13:01PM -0400, Johannes Weiner wrote: > The different branches for retry are unnecessarily complicated. There > is really only three outcomes: progress, skipped, failed. Also, the > retry counter only applies to loops that made progress, move it there. > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > --- > mm/page_alloc.c | 60 +++++++++++++++++-------------------------------- > 1 file changed, 20 insertions(+), 40 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index c3b7dc479936..18fa2bbba44b 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -4608,7 +4608,6 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > enum compact_priority *compact_priority, > int *compaction_retries) > { > - int max_retries = MAX_COMPACT_RETRIES; > int min_priority; > bool ret = false; > int retries = *compaction_retries; Think this breaks build because of trace_compact_retry and max_retries is declared in a different scope on the next hunk. Again, move this to a preparation series. I didn't actually think about this patch at all because I'm trying to reach the main purpose of the series and it's now late on a Friday so I'll probably fail or forget by Monday.
Johannes Weiner <hannes@cmpxchg.org> writes: > The different branches for retry are unnecessarily complicated. There > is really only three outcomes: progress, skipped, failed. Also, the > retry counter only applies to loops that made progress, move it there. > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > --- > mm/page_alloc.c | 60 +++++++++++++++++-------------------------------- > 1 file changed, 20 insertions(+), 40 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index c3b7dc479936..18fa2bbba44b 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -4608,7 +4608,6 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > enum compact_priority *compact_priority, > int *compaction_retries) > { > - int max_retries = MAX_COMPACT_RETRIES; > int min_priority; > bool ret = false; > int retries = *compaction_retries; > @@ -4621,19 +4620,27 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > return false; > > /* > - * Compaction managed to coalesce some page blocks, but the > - * allocation failed presumably due to a race. Retry some. > + * Compaction coalesced some page blocks, but the allocation > + * failed, presumably due to a race. Retry a few times. > */ > - if (compact_result == COMPACT_SUCCESS) > - (*compaction_retries)++; > + if (compact_result == COMPACT_SUCCESS) { > + int max_retries = MAX_COMPACT_RETRIES; > > - /* > - * All zones were scanned completely and still no result. It > - * doesn't really make much sense to retry except when the > - * failure could be caused by insufficient priority > - */ > - if (compact_result == COMPACT_COMPLETE) > - goto check_priority; > + /* > + * !costly requests are much more important than > + * __GFP_RETRY_MAYFAIL costly ones because they are de > + * facto nofail and invoke OOM killer to move on while > + * costly can fail and users are ready to cope with > + * that. 1/4 retries is rather arbitrary but we would > + * need much more detailed feedback from compaction to > + * make a better decision. > + */ > + if (order > PAGE_ALLOC_COSTLY_ORDER) > + max_retries /= 4; > + > + ret = ++(*compaction_retries) <= MAX_COMPACT_RETRIES; ~~~~~~~~~~~~~~~~~~~ Should be max_retries? Best Regards, Huang, Ying > + goto out; > + } > > /* > * Compaction was skipped due to a lack of free order-0 > @@ -4645,35 +4652,8 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > } > > /* > - * If compaction backed due to being deferred, due to > - * contended locks in async mode, or due to scanners meeting > - * after a partial scan, retry with increased priority. > - */ > - if (compact_result == COMPACT_DEFERRED || > - compact_result == COMPACT_CONTENDED || > - compact_result == COMPACT_PARTIAL_SKIPPED) > - goto check_priority; > - > - /* > - * !costly requests are much more important than __GFP_RETRY_MAYFAIL > - * costly ones because they are de facto nofail and invoke OOM > - * killer to move on while costly can fail and users are ready > - * to cope with that. 1/4 retries is rather arbitrary but we > - * would need much more detailed feedback from compaction to > - * make a better decision. > - */ > - if (order > PAGE_ALLOC_COSTLY_ORDER) > - max_retries /= 4; > - if (*compaction_retries <= max_retries) { > - ret = true; > - goto out; > - } > - > - /* > - * Make sure there are attempts at the highest priority if we exhausted > - * all retries or failed at the lower priorities. > + * Compaction failed. Retry with increasing priority. > */ > -check_priority: > min_priority = (order > PAGE_ALLOC_COSTLY_ORDER) ? > MIN_COMPACT_COSTLY_PRIORITY : MIN_COMPACT_PRIORITY;
On Tue, Apr 25, 2023 at 08:56:47AM +0800, Huang, Ying wrote: > Johannes Weiner <hannes@cmpxchg.org> writes: > > > The different branches for retry are unnecessarily complicated. There > > is really only three outcomes: progress, skipped, failed. Also, the > > retry counter only applies to loops that made progress, move it there. > > > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > > --- > > mm/page_alloc.c | 60 +++++++++++++++++-------------------------------- > > 1 file changed, 20 insertions(+), 40 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index c3b7dc479936..18fa2bbba44b 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -4608,7 +4608,6 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > > enum compact_priority *compact_priority, > > int *compaction_retries) > > { > > - int max_retries = MAX_COMPACT_RETRIES; > > int min_priority; > > bool ret = false; > > int retries = *compaction_retries; > > @@ -4621,19 +4620,27 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > > return false; > > > > /* > > - * Compaction managed to coalesce some page blocks, but the > > - * allocation failed presumably due to a race. Retry some. > > + * Compaction coalesced some page blocks, but the allocation > > + * failed, presumably due to a race. Retry a few times. > > */ > > - if (compact_result == COMPACT_SUCCESS) > > - (*compaction_retries)++; > > + if (compact_result == COMPACT_SUCCESS) { > > + int max_retries = MAX_COMPACT_RETRIES; > > > > - /* > > - * All zones were scanned completely and still no result. It > > - * doesn't really make much sense to retry except when the > > - * failure could be caused by insufficient priority > > - */ > > - if (compact_result == COMPACT_COMPLETE) > > - goto check_priority; > > + /* > > + * !costly requests are much more important than > > + * __GFP_RETRY_MAYFAIL costly ones because they are de > > + * facto nofail and invoke OOM killer to move on while > > + * costly can fail and users are ready to cope with > > + * that. 1/4 retries is rather arbitrary but we would > > + * need much more detailed feedback from compaction to > > + * make a better decision. > > + */ > > + if (order > PAGE_ALLOC_COSTLY_ORDER) > > + max_retries /= 4; > > + > > + ret = ++(*compaction_retries) <= MAX_COMPACT_RETRIES; > ~~~~~~~~~~~~~~~~~~~ > > Should be max_retries? Good catch. max_retries is deleted in a later patch, but this one should be fixed regardless. Thanks, I will correct it.
On Fri, Apr 21, 2023 at 03:36:54PM +0100, Mel Gorman wrote: > On Tue, Apr 18, 2023 at 03:13:01PM -0400, Johannes Weiner wrote: > > The different branches for retry are unnecessarily complicated. There > > is really only three outcomes: progress, skipped, failed. Also, the > > retry counter only applies to loops that made progress, move it there. > > > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > > --- > > mm/page_alloc.c | 60 +++++++++++++++++-------------------------------- > > 1 file changed, 20 insertions(+), 40 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index c3b7dc479936..18fa2bbba44b 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -4608,7 +4608,6 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > > enum compact_priority *compact_priority, > > int *compaction_retries) > > { > > - int max_retries = MAX_COMPACT_RETRIES; > > int min_priority; > > bool ret = false; > > int retries = *compaction_retries; > > Think this breaks build because of trace_compact_retry and max_retries is > declared in a different scope on the next hunk. Right you are. Will fix. Thanks!
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index c3b7dc479936..18fa2bbba44b 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -4608,7 +4608,6 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, enum compact_priority *compact_priority, int *compaction_retries) { - int max_retries = MAX_COMPACT_RETRIES; int min_priority; bool ret = false; int retries = *compaction_retries; @@ -4621,19 +4620,27 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, return false; /* - * Compaction managed to coalesce some page blocks, but the - * allocation failed presumably due to a race. Retry some. + * Compaction coalesced some page blocks, but the allocation + * failed, presumably due to a race. Retry a few times. */ - if (compact_result == COMPACT_SUCCESS) - (*compaction_retries)++; + if (compact_result == COMPACT_SUCCESS) { + int max_retries = MAX_COMPACT_RETRIES; - /* - * All zones were scanned completely and still no result. It - * doesn't really make much sense to retry except when the - * failure could be caused by insufficient priority - */ - if (compact_result == COMPACT_COMPLETE) - goto check_priority; + /* + * !costly requests are much more important than + * __GFP_RETRY_MAYFAIL costly ones because they are de + * facto nofail and invoke OOM killer to move on while + * costly can fail and users are ready to cope with + * that. 1/4 retries is rather arbitrary but we would + * need much more detailed feedback from compaction to + * make a better decision. + */ + if (order > PAGE_ALLOC_COSTLY_ORDER) + max_retries /= 4; + + ret = ++(*compaction_retries) <= MAX_COMPACT_RETRIES; + goto out; + } /* * Compaction was skipped due to a lack of free order-0 @@ -4645,35 +4652,8 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, } /* - * If compaction backed due to being deferred, due to - * contended locks in async mode, or due to scanners meeting - * after a partial scan, retry with increased priority. - */ - if (compact_result == COMPACT_DEFERRED || - compact_result == COMPACT_CONTENDED || - compact_result == COMPACT_PARTIAL_SKIPPED) - goto check_priority; - - /* - * !costly requests are much more important than __GFP_RETRY_MAYFAIL - * costly ones because they are de facto nofail and invoke OOM - * killer to move on while costly can fail and users are ready - * to cope with that. 1/4 retries is rather arbitrary but we - * would need much more detailed feedback from compaction to - * make a better decision. - */ - if (order > PAGE_ALLOC_COSTLY_ORDER) - max_retries /= 4; - if (*compaction_retries <= max_retries) { - ret = true; - goto out; - } - - /* - * Make sure there are attempts at the highest priority if we exhausted - * all retries or failed at the lower priorities. + * Compaction failed. Retry with increasing priority. */ -check_priority: min_priority = (order > PAGE_ALLOC_COSTLY_ORDER) ? MIN_COMPACT_COSTLY_PRIORITY : MIN_COMPACT_PRIORITY;
The different branches for retry are unnecessarily complicated. There is really only three outcomes: progress, skipped, failed. Also, the retry counter only applies to loops that made progress, move it there. Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> --- mm/page_alloc.c | 60 +++++++++++++++++-------------------------------- 1 file changed, 20 insertions(+), 40 deletions(-)