Message ID | cc48dc1e4db8c33289f168cf380ab3641f45f8ad.1666251624.git.baolin.wang@linux.alibaba.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp124623wrs; Thu, 20 Oct 2022 01:00:31 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6/di5ydE968N3xCFNn5CrwW/8g1VuK8qXbEU1uXa84D8j8BtETXUAnqep/XwB0cBwyOSWX X-Received: by 2002:a17:906:5dcc:b0:78d:e76a:ef23 with SMTP id p12-20020a1709065dcc00b0078de76aef23mr9618003ejv.317.1666252831748; Thu, 20 Oct 2022 01:00:31 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666252831; cv=none; d=google.com; s=arc-20160816; b=g7Ak4WeItbicMfHH3IG2DWoXtl58HH8JolJXNYVjY8E2eCfLL7V20lMJUN8Y37S5kE GosAWBSbq1zMxQUXF/ilXyWAZPQJeqzMBBUVPakSPIjYz/cl+dx+zsWUKSTKZPBJ1a64 ZAe/PdPm2GMbK45/zoaGxanX+73ozDnzWRYuIn2F5mACxhg0nX9QkR8jdT9gtqFQ1PjQ H2+lf+jlZc5aXYM7FAuvN9N+vbGxl/gw1AkorA5d6PtzY12d1vDHcmT0rcUzwMn9+aSK bLrsYnTTRt4Cq7Ntwgvn3eF3pQ2QSZQQpg7cqT/g7bs6ZTQYbypghvVLqi57EmQYKJtQ PRFg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:message-id:date:subject:cc:to:from; bh=NESju3q1gAbH3qzt3XpNCw/tm1i7c3v5l8OQbViFb0M=; b=mabAGtYmB7ltkZr9s84oJTdOgN0EtwxEsva/XhixFKOyUe4DwBrD/qUXbYDixyiTIS s9UyOvv82BEYxJ+VhuCJHxqoTDp+ue43BJ3rLZmDtvxHd8KO+nfU1k2qFUoJGAgpeGc6 jzGa3ACTTPII25QwvYUeTV1RoUP6q4QN99JioBEkiwIljawYf8w/Jby9rDL7PjtFPKff KhGjkRgA4uzeeTVQ4mX37OaWcp39Zy+MrI3aDXVGxalrM/ScYlh6F1KymbSfxp516qfm ZvksEMu7WbAJGUzSE/P5RQo0GRIGf7GMKlf2CUpAiBZipWPinQ1frFAMbxTkEvZQ9MOg +WIQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alibaba.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id sz15-20020a1709078b0f00b0078cffe5dcdesi12680353ejc.451.2022.10.20.01.00.05; Thu, 20 Oct 2022 01:00:31 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alibaba.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230481AbiJTHtV (ORCPT <rfc822;realc9580@gmail.com> + 99 others); Thu, 20 Oct 2022 03:49:21 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51664 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231149AbiJTHtR (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 20 Oct 2022 03:49:17 -0400 Received: from out30-56.freemail.mail.aliyun.com (out30-56.freemail.mail.aliyun.com [115.124.30.56]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0E6CC1757B3 for <linux-kernel@vger.kernel.org>; Thu, 20 Oct 2022 00:49:15 -0700 (PDT) X-Alimail-AntiSpam: AC=PASS;BC=-1|-1;BR=01201311R191e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=ay29a033018046050;MF=baolin.wang@linux.alibaba.com;NM=1;PH=DS;RN=9;SR=0;TI=SMTPD_---0VSeILu6_1666252152; Received: from localhost(mailfrom:baolin.wang@linux.alibaba.com fp:SMTPD_---0VSeILu6_1666252152) by smtp.aliyun-inc.com; Thu, 20 Oct 2022 15:49:13 +0800 From: Baolin Wang <baolin.wang@linux.alibaba.com> To: akpm@linux-foundation.org Cc: david@redhat.com, ying.huang@intel.com, ziy@nvidia.com, shy828301@gmail.com, baolin.wang@linux.alibaba.com, jingshan@linux.alibaba.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: [PATCH 1/2] mm: gup: Re-pin pages in case of trying several times to migrate Date: Thu, 20 Oct 2022 15:49:00 +0800 Message-Id: <cc48dc1e4db8c33289f168cf380ab3641f45f8ad.1666251624.git.baolin.wang@linux.alibaba.com> X-Mailer: git-send-email 1.8.3.1 X-Spam-Status: No, score=-9.9 required=5.0 tests=BAYES_00, ENV_AND_HDR_SPF_MATCH,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2, SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747192728993719132?= X-GMAIL-MSGID: =?utf-8?q?1747192728993719132?= |
Series |
[1/2] mm: gup: Re-pin pages in case of trying several times to migrate
|
|
Commit Message
Baolin Wang
Oct. 20, 2022, 7:49 a.m. UTC
The migrate_pages() will return the number of {normal page, THP, hugetlb}
that were not migrated, or an error code. That means it can still return
the number of failure count, though the pages have been migrated
successfully with several times re-try.
So we should not use the return value of migrate_pages() to determin
if there are pages are failed to migrate. Instead we can validate the
'movable_page_list' to see if there are pages remained in the list,
which are failed to migrate. That can mitigate the failure of longterm
pinning.
Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
mm/gup.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
Comments
Baolin Wang <baolin.wang@linux.alibaba.com> writes: > The migrate_pages() will return the number of {normal page, THP, hugetlb} > that were not migrated, or an error code. That means it can still return > the number of failure count, though the pages have been migrated > successfully with several times re-try. If my understanding were correct, if pages are migrated successfully after several times re-tries, the return value will be 0. There's one possibility for migrate_pages() to return non-zero but all pages are migrated. That is, when THP is split and all subpages are migrated successfully. > So we should not use the return value of migrate_pages() to determin > if there are pages are failed to migrate. Instead we can validate the > 'movable_page_list' to see if there are pages remained in the list, > which are failed to migrate. That can mitigate the failure of longterm > pinning. Another choice is to use a special return value for split THP + success migration. But I'm fine to use list_empty(return_pages). > Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> > --- > mm/gup.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 5182aba..bd8cfcd 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1914,9 +1914,10 @@ static int migrate_longterm_unpinnable_pages( > .gfp_mask = GFP_USER | __GFP_NOWARN, > }; > > - if (migrate_pages(movable_page_list, alloc_migration_target, > - NULL, (unsigned long)&mtc, MIGRATE_SYNC, > - MR_LONGTERM_PIN, NULL)) { > + ret = migrate_pages(movable_page_list, alloc_migration_target, > + NULL, (unsigned long)&mtc, MIGRATE_SYNC, > + MR_LONGTERM_PIN, NULL); > + if (ret < 0 || !list_empty(movable_page_list)) { It seems that !list_empty() is sufficient here. > ret = -ENOMEM; Why change the error code? I don't think it's a good idea to do that. > goto err; > } Best Regards, Huang, Ying
On 10/20/2022 4:15 PM, Huang, Ying wrote: > Baolin Wang <baolin.wang@linux.alibaba.com> writes: > >> The migrate_pages() will return the number of {normal page, THP, hugetlb} >> that were not migrated, or an error code. That means it can still return >> the number of failure count, though the pages have been migrated >> successfully with several times re-try. > > If my understanding were correct, if pages are migrated successfully > after several times re-tries, the return value will be 0. There's one > possibility for migrate_pages() to return non-zero but all pages are > migrated. That is, when THP is split and all subpages are migrated > successfully. Yeah, that's the case I tested. Thanks for pointing out. I'll re-write my incorrect commit message next time. > >> So we should not use the return value of migrate_pages() to determin >> if there are pages are failed to migrate. Instead we can validate the >> 'movable_page_list' to see if there are pages remained in the list, >> which are failed to migrate. That can mitigate the failure of longterm >> pinning. > > Another choice is to use a special return value for split THP + success > migration. But I'm fine to use list_empty(return_pages). OK. Using list_empty(return_pages) looks more simple. > >> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >> --- >> mm/gup.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/mm/gup.c b/mm/gup.c >> index 5182aba..bd8cfcd 100644 >> --- a/mm/gup.c >> +++ b/mm/gup.c >> @@ -1914,9 +1914,10 @@ static int migrate_longterm_unpinnable_pages( >> .gfp_mask = GFP_USER | __GFP_NOWARN, >> }; >> >> - if (migrate_pages(movable_page_list, alloc_migration_target, >> - NULL, (unsigned long)&mtc, MIGRATE_SYNC, >> - MR_LONGTERM_PIN, NULL)) { >> + ret = migrate_pages(movable_page_list, alloc_migration_target, >> + NULL, (unsigned long)&mtc, MIGRATE_SYNC, >> + MR_LONGTERM_PIN, NULL); >> + if (ret < 0 || !list_empty(movable_page_list)) { > > It seems that !list_empty() is sufficient here. OK. Drop the 'ret < 0' >> ret = -ENOMEM; > > Why change the error code? I don't think it's a good idea to do that. The GUP need a -errno for failure or partial success when migration, and we can not return the number of pages failed to migrate. So returning -ENOMEM seems suitable for both cases?
Baolin Wang <baolin.wang@linux.alibaba.com> writes: > On 10/20/2022 4:15 PM, Huang, Ying wrote: >> Baolin Wang <baolin.wang@linux.alibaba.com> writes: >> >>> The migrate_pages() will return the number of {normal page, THP, hugetlb} >>> that were not migrated, or an error code. That means it can still return >>> the number of failure count, though the pages have been migrated >>> successfully with several times re-try. >> If my understanding were correct, if pages are migrated successfully >> after several times re-tries, the return value will be 0. There's one >> possibility for migrate_pages() to return non-zero but all pages are >> migrated. That is, when THP is split and all subpages are migrated >> successfully. > > Yeah, that's the case I tested. Thanks for pointing out. I'll re-write my > incorrect commit message next time. This is confusing to me. So users of move_page() will see an unsuccessful migration even when all subpages were migrated? Seems like we should fix the return code of migrate_pages() for this case where all subpages were successfully migrated. >> >>> So we should not use the return value of migrate_pages() to determin >>> if there are pages are failed to migrate. Instead we can validate the >>> 'movable_page_list' to see if there are pages remained in the list, >>> which are failed to migrate. That can mitigate the failure of longterm >>> pinning. >> Another choice is to use a special return value for split THP + success >> migration. But I'm fine to use list_empty(return_pages). > > OK. Using list_empty(return_pages) looks more simple. > >> >>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >>> --- >>> mm/gup.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/mm/gup.c b/mm/gup.c >>> index 5182aba..bd8cfcd 100644 >>> --- a/mm/gup.c >>> +++ b/mm/gup.c >>> @@ -1914,9 +1914,10 @@ static int migrate_longterm_unpinnable_pages( >>> .gfp_mask = GFP_USER | __GFP_NOWARN, >>> }; >>> - if (migrate_pages(movable_page_list, alloc_migration_target, >>> - NULL, (unsigned long)&mtc, MIGRATE_SYNC, >>> - MR_LONGTERM_PIN, NULL)) { >>> + ret = migrate_pages(movable_page_list, alloc_migration_target, >>> + NULL, (unsigned long)&mtc, MIGRATE_SYNC, >>> + MR_LONGTERM_PIN, NULL); >>> + if (ret < 0 || !list_empty(movable_page_list)) { >> It seems that !list_empty() is sufficient here. > > OK. Drop the 'ret < 0' > >>> ret = -ENOMEM; >> Why change the error code? I don't think it's a good idea to do that. > > The GUP need a -errno for failure or partial success when migration, and we can > not return the number of pages failed to migrate. So returning -ENOMEM seems > suitable for both cases? Seem reasonable to me. migrate_pages() might return -EAGAIN which would cause everything to be re-pinned and tried again which is not what you want here. See the comment at the start of check_and_migrate_movable_pages().
Alistair Popple <apopple@nvidia.com> writes: > Baolin Wang <baolin.wang@linux.alibaba.com> writes: > >> On 10/20/2022 4:15 PM, Huang, Ying wrote: >>> Baolin Wang <baolin.wang@linux.alibaba.com> writes: >>> >>>> The migrate_pages() will return the number of {normal page, THP, hugetlb} >>>> that were not migrated, or an error code. That means it can still return >>>> the number of failure count, though the pages have been migrated >>>> successfully with several times re-try. >>> If my understanding were correct, if pages are migrated successfully >>> after several times re-tries, the return value will be 0. There's one >>> possibility for migrate_pages() to return non-zero but all pages are >>> migrated. That is, when THP is split and all subpages are migrated >>> successfully. >> >> Yeah, that's the case I tested. Thanks for pointing out. I'll re-write my >> incorrect commit message next time. > > This is confusing to me. So users of move_page() will see an > unsuccessful migration even when all subpages were migrated? Seems like > we should fix the return code of migrate_pages() for this case where all > subpages were successfully migrated. > >>> >>>> So we should not use the return value of migrate_pages() to determin >>>> if there are pages are failed to migrate. Instead we can validate the >>>> 'movable_page_list' to see if there are pages remained in the list, >>>> which are failed to migrate. That can mitigate the failure of longterm >>>> pinning. >>> Another choice is to use a special return value for split THP + success >>> migration. But I'm fine to use list_empty(return_pages). >> >> OK. Using list_empty(return_pages) looks more simple. >> >>> >>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >>>> --- >>>> mm/gup.c | 7 ++++--- >>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/mm/gup.c b/mm/gup.c >>>> index 5182aba..bd8cfcd 100644 >>>> --- a/mm/gup.c >>>> +++ b/mm/gup.c >>>> @@ -1914,9 +1914,10 @@ static int migrate_longterm_unpinnable_pages( >>>> .gfp_mask = GFP_USER | __GFP_NOWARN, >>>> }; >>>> - if (migrate_pages(movable_page_list, alloc_migration_target, >>>> - NULL, (unsigned long)&mtc, MIGRATE_SYNC, >>>> - MR_LONGTERM_PIN, NULL)) { >>>> + ret = migrate_pages(movable_page_list, alloc_migration_target, >>>> + NULL, (unsigned long)&mtc, MIGRATE_SYNC, >>>> + MR_LONGTERM_PIN, NULL); >>>> + if (ret < 0 || !list_empty(movable_page_list)) { >>> It seems that !list_empty() is sufficient here. >> >> OK. Drop the 'ret < 0' >> >>>> ret = -ENOMEM; >>> Why change the error code? I don't think it's a good idea to do that. >> >> The GUP need a -errno for failure or partial success when migration, and we can >> not return the number of pages failed to migrate. So returning -ENOMEM seems >> suitable for both cases? > > Seem reasonable to me. migrate_pages() might return -EAGAIN which would > cause everything to be re-pinned and tried again which is not what you > want here. See the comment at the start of > check_and_migrate_movable_pages(). Yes. You are right. The error code of migrate_pages() isn't good for caller here. Best Regards, Huang, Ying
On 10/20/2022 7:43 PM, Alistair Popple wrote: > > Baolin Wang <baolin.wang@linux.alibaba.com> writes: > >> On 10/20/2022 4:15 PM, Huang, Ying wrote: >>> Baolin Wang <baolin.wang@linux.alibaba.com> writes: >>> >>>> The migrate_pages() will return the number of {normal page, THP, hugetlb} >>>> that were not migrated, or an error code. That means it can still return >>>> the number of failure count, though the pages have been migrated >>>> successfully with several times re-try. >>> If my understanding were correct, if pages are migrated successfully >>> after several times re-tries, the return value will be 0. There's one >>> possibility for migrate_pages() to return non-zero but all pages are >>> migrated. That is, when THP is split and all subpages are migrated >>> successfully. >> >> Yeah, that's the case I tested. Thanks for pointing out. I'll re-write my >> incorrect commit message next time. > > This is confusing to me. So users of move_page() will see an > unsuccessful migration even when all subpages were migrated? Seems like Yes. > we should fix the return code of migrate_pages() for this case where all > subpages were successfully migrated. After more investigation, some other callers will also check the return value to see of all pages are migrated successfully. So yes, I will change the return value in migrate_pages() to fix this issue for all callers like you and Ying suggested. Thanks. >>>> So we should not use the return value of migrate_pages() to determin >>>> if there are pages are failed to migrate. Instead we can validate the >>>> 'movable_page_list' to see if there are pages remained in the list, >>>> which are failed to migrate. That can mitigate the failure of longterm >>>> pinning. >>> Another choice is to use a special return value for split THP + success >>> migration. But I'm fine to use list_empty(return_pages). >> >> OK. Using list_empty(return_pages) looks more simple. >> >>> >>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >>>> --- >>>> mm/gup.c | 7 ++++--- >>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/mm/gup.c b/mm/gup.c >>>> index 5182aba..bd8cfcd 100644 >>>> --- a/mm/gup.c >>>> +++ b/mm/gup.c >>>> @@ -1914,9 +1914,10 @@ static int migrate_longterm_unpinnable_pages( >>>> .gfp_mask = GFP_USER | __GFP_NOWARN, >>>> }; >>>> - if (migrate_pages(movable_page_list, alloc_migration_target, >>>> - NULL, (unsigned long)&mtc, MIGRATE_SYNC, >>>> - MR_LONGTERM_PIN, NULL)) { >>>> + ret = migrate_pages(movable_page_list, alloc_migration_target, >>>> + NULL, (unsigned long)&mtc, MIGRATE_SYNC, >>>> + MR_LONGTERM_PIN, NULL); >>>> + if (ret < 0 || !list_empty(movable_page_list)) { >>> It seems that !list_empty() is sufficient here. >> >> OK. Drop the 'ret < 0' >> >>>> ret = -ENOMEM; >>> Why change the error code? I don't think it's a good idea to do that. >> >> The GUP need a -errno for failure or partial success when migration, and we can >> not return the number of pages failed to migrate. So returning -ENOMEM seems >> suitable for both cases? > > Seem reasonable to me. migrate_pages() might return -EAGAIN which would > cause everything to be re-pinned and tried again which is not what you > want here. See the comment at the start of > check_and_migrate_movable_pages().
diff --git a/mm/gup.c b/mm/gup.c index 5182aba..bd8cfcd 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1914,9 +1914,10 @@ static int migrate_longterm_unpinnable_pages( .gfp_mask = GFP_USER | __GFP_NOWARN, }; - if (migrate_pages(movable_page_list, alloc_migration_target, - NULL, (unsigned long)&mtc, MIGRATE_SYNC, - MR_LONGTERM_PIN, NULL)) { + ret = migrate_pages(movable_page_list, alloc_migration_target, + NULL, (unsigned long)&mtc, MIGRATE_SYNC, + MR_LONGTERM_PIN, NULL); + if (ret < 0 || !list_empty(movable_page_list)) { ret = -ENOMEM; goto err; }