Re: [PATCH v5 1/5] mm,page_alloc: Bail out earlier on -ENOMEM in alloc_contig_migrate_range
From: Michal Hocko <mhocko@suse.com>
Date: 2021-03-17 14:06:40
Also in:
lkml
On Wed 17-03-21 12:12:47, Oscar Salvador wrote:
Currently, __alloc_contig_migrate_range can generate -EINTR, -ENOMEM or -EBUSY, and report them down the chain. The problem is that when migrate_pages() reports -ENOMEM, we keep going till we exhaust all the try-attempts (5 at the moment) instead of bailing out. migrate_pages() bails out right away on -ENOMEM because it is considered a fatal error. Do the same here instead of keep going and retrying.
I suspect this is not really a real life problem, right? The allocation would be more costly in the end but this is to be expected under a heavy memory pressure. That being said, bailing out early makes sense to me. But now that you've made me look into the migrate_pages excellent error state reporting I suspect we have a bug here. Note the "Returns the number of pages that were not migrated, or an error code." but I do not see putback_movable_pages for ret > 0 so it seems we might leak some pages. That aside. Now looking at other callers of migrate_pages most of them do not care about the number of failed pages. The only one which cares is migrate_pages syscall (do_migrate_pages). I think it would be much more reasonable to have migrate_pages (kernel function) return error or 0 and make the only caller which cares to count number of failed pages (e.g. by returning the number of pages from putback_movable_pages).
Signed-off-by: Oscar Salvador <osalvador@suse.de> Acked-by: Vlastimil Babka <redacted> Reviewed-by: David Hildenbrand <redacted>
The patch itself looks reasonable but make sure to mention this is mere cosmetic change unless there is a real problem fixed by this. Acked-by: Michal Hocko <mhocko@suse.com>
quoted hunk ↗ jump to hunk
--- mm/page_alloc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)diff --git a/mm/page_alloc.c b/mm/page_alloc.c index cfc72873961d..a4f67063b85f 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c@@ -8481,7 +8481,7 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, } tries = 0; } else if (++tries == 5) { - ret = ret < 0 ? ret : -EBUSY; + ret = -EBUSY; break; }@@ -8491,6 +8491,12 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, ret = migrate_pages(&cc->migratepages, alloc_migration_target, NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE); + /* + * On -ENOMEM, migrate_pages() bails out right away. It is pointless + * to retry again over this error, so do the same here. + */ + if (ret == -ENOMEM) + break; } if (ret < 0) { putback_movable_pages(&cc->migratepages);-- 2.16.3
-- Michal Hocko SUSE Labs