lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <1a45492e-3372-844b-93ca-143c0ef3fe6c@linux.alibaba.com>
Date:   Sun, 7 Nov 2021 17:08:07 +0800
From:   Baolin Wang <baolin.wang@...ux.alibaba.com>
To:     Zi Yan <ziy@...dia.com>
Cc:     akpm@...ux-foundation.org, rostedt@...dmis.org, mingo@...hat.com,
        shy828301@...il.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/3] mm: migrate: Fix the return value of
 migrate_pages()



On 2021/11/5 23:21, Zi Yan wrote:
> On 5 Nov 2021, at 6:17, Baolin Wang wrote:
> 
>> On 2021/11/4 23:33, Zi Yan wrote:
>>> On 3 Nov 2021, at 6:51, Baolin Wang wrote:
>>>
>>>> As Zi Yan pointed out, the syscall move_pages() can return a non-migrated
>>>> number larger than the number of pages the users tried to migrate, when a
>>>> THP page is failed to migrate. This is confusing for users.
>>>>
>>>> Since other migration scenarios do not care about the actual non-migrated
>>>> number of pages except the memory compaction migration which will fix in
>>>> following patch. Thus we can change the return value to return the number
>>>> of {normal page, THP, hugetlb} instead to avoid this issue, meanwhile we
>>>> should still keep the migration counters using the number of normal pages.
>>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@...ux.alibaba.com>
>>>> ---
>>>>    mm/migrate.c | 18 ++++++++++--------
>>>>    1 file changed, 10 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>> index a11e948..00b8922 100644
>>>> --- a/mm/migrate.c
>>>> +++ b/mm/migrate.c
>>>> @@ -1428,7 +1428,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>>>>     * @mode:		The migration mode that specifies the constraints for
>>>>     *			page migration, if any.
>>>>     * @reason:		The reason for page migration.
>>>> - * @ret_succeeded:	Set to the number of pages migrated successfully if
>>>> + * @ret_succeeded:	Set to the number of normal pages migrated successfully if
>>>>     *			the caller passes a non-NULL pointer.
>>>>     *
>>>>     * The function returns after 10 attempts or if no pages are movable any more
>>>> @@ -1436,7 +1436,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>>>>     * It is caller's responsibility to call putback_movable_pages() to return pages
>>>>     * to the LRU or free list only if ret != 0.
>>>>     *
>>>> - * Returns the number of pages that were not migrated, or an error code.
>>>> + * Returns the number of {normal page, THP} that were not migrated, or an error code.
>>>>     */
>>>>    int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    		free_page_t put_new_page, unsigned long private,
>>>> @@ -1445,6 +1445,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    	int retry = 1;
>>>>    	int thp_retry = 1;
>>>>    	int nr_failed = 0;
>>>> +	int nr_failed_pages = 0;
>>>>    	int nr_succeeded = 0;
>>>>    	int nr_thp_succeeded = 0;
>>>>    	int nr_thp_failed = 0;
>>>> @@ -1517,7 +1518,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    					}
>>>>
>>>>    					nr_thp_failed++;
>>>> -					nr_failed += nr_subpages;
>>>> +					nr_failed_pages += nr_subpages;
>>>>    					break;
>>>>    				}
>>>>
>>>> @@ -1537,7 +1538,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    					}
>>>>
>>>>    					nr_thp_failed++;
>>>> -					nr_failed += nr_subpages;
>>>> +					nr_failed_pages += nr_subpages;
>>>>    					goto out;
>>>>    				}
>>>>    				nr_failed++;
>>>> @@ -1566,7 +1567,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    				 */
>>>>    				if (is_thp) {
>>>>    					nr_thp_failed++;
>>>> -					nr_failed += nr_subpages;
>>>> +					nr_failed_pages += nr_subpages;
>>>>    					break;
>>>>    				}
>>>>    				nr_failed++;
>>>> @@ -1575,8 +1576,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    		}
>>>>    	}
>>>>    	nr_failed += retry + thp_retry;
>>>
>>> This line can probably go away, since we do not want to count retried pages.
>>
>> OK.
> 
> My bad, I misread the code. This should stay, since each -EAGIN does not
> increase nr_failed or nr_thp_failed and after the for loop, retry and thp_retry
> give the number of pages that fail to migrate after 10 retries.

Ah, right.

>>
>>>
>>>> +	nr_failed_pages += nr_failed;
>>>>    	nr_thp_failed += thp_retry;
>>>> -	rc = nr_failed;
>>>> +	rc = nr_failed + nr_thp_failed;
>>>>    out:
>>>>    	/*
>>>>    	 * Put the permanent failure page back to migration list, they
>>>> @@ -1585,11 +1587,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    	list_splice(&ret_pages, from);
>>>>
>>>>    	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
>>>> -	count_vm_events(PGMIGRATE_FAIL, nr_failed);
>>>> +	count_vm_events(PGMIGRATE_FAIL, nr_failed_pages);
>>>>    	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
>>>>    	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
>>>>    	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
>>>> -	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
>>>> +	trace_mm_migrate_pages(nr_succeeded, nr_failed_pages, nr_thp_succeeded,
>>>>    			       nr_thp_failed, nr_thp_split, mode, reason);
>>>>
>>>>    	if (!swapwrite)
>>>> -- 
>>>> 1.8.3.1
>>>
>>> Thank you for the patch!
>>>
>>> In general, this looks good to me. But like you said in other email, when a THP fails to
>>> migrate and gets split, the number of nr_failed will still be inflated by the number of
>>> failed subpage migrations. What I can think of is to split THPs to a separate list and
>>> stop increasing nr_failed when the pages from the new list is under migration. Let me
>>> know how it sounds to you.
>>
>> Thanks for your patch, but I think it does not cover all the cases.
>>
>> Firstly, what confuses me is that if we return 1 to users when failed to migrate 1 THP page, but actually we may have migrated some normal pages successfaully if the THP page is split. Anyway we can add some comments for migrate_pages() to explain it if this is acceptable.
> 
> Sure.
> 
>>
>> Another concern about your patch is that, if the first round migration all pages are migrated successfaully (nr_failed = 0), but if failed to migrate the subpages of the THP in the second round, we will still return 0 to users, which is incorrect. Further more, if the
> 
> Ah, I missed this. We should increase nr_thp_failed when THP is split.
> 
>> subpages of the THP are migrated partially in the second round, what the number of non-migrated should be returned?  Suppose multiple THP pages have been split?
> 
> Assuming users do not know/care what kinds of pages are in their migration list, we can
> just return 1 if a THP is split, no matter how many subpages are migrated successfully.
> If they do want to know the details, they probably can check the tracepoints.
OK.

> 
>>
>> Last concern is that, we will try to migrate subpages of the THP in the second round, but if some non-migrated pages are still remained in the 'from' list, we will do another redundant migration for these failed-migration pages and no failed counting for them. We can fix this issue by moving the remained non-migrated pages to 'ret_pages' list, which will put back to 'from' list when returning from this function.
>>
>> if (!list_empty(&thp_split_pages)) {
>> +	list_splice(from, &ret_pages);
>> 	list_splice(&thp_split_pages, from);
>> 	no_failed_counting = true;
>> 	goto thp_subpage_migration;
>> }
> 
> You are right. At this point, pages in the from list have been retried 10 times,
> no need to migrate them again.
> 
>>
>> The concern 2 is more complicated, or we can just use 'nr_thp_split' to return if some subpages are failed to be migrated to simplify the case, no matter how many subpages are failed?
>>
>> rc = nr_failed + nr_thp_failed + nr_thp_split;
> 
> Like I said above, I would just increase nr_thp_failed when a THP is split. Here is the patch
> I come up with based on your feedback above. This time I incorporated your patch above. Please
> let me know how it looks. Thanks.

Overall it looks good, but I found some issues as below when I did some 
testing, otherwise it looks good to me. I've folded this into my new 
patch set, please help to review again. Thanks.

> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 1852d787e6ab..a5fad22259a3 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1429,7 +1429,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>    * @mode:              The migration mode that specifies the constraints for
>    *                     page migration, if any.
>    * @reason:            The reason for page migration.
> - * @ret_succeeded:     Set to the number of pages migrated successfully if
> + * @ret_succeeded:     Set to the number of normal pages migrated successfully if
>    *                     the caller passes a non-NULL pointer.
>    *
>    * The function returns after 10 attempts or if no pages are movable any more
> @@ -1437,7 +1437,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>    * It is caller's responsibility to call putback_movable_pages() to return pages
>    * to the LRU or free list only if ret != 0.
>    *
> - * Returns the number of pages that were not migrated, or an error code.
> + * Returns the number of {normal page, THP} that were not migrated, or an error code.
>    */
>   int migrate_pages(struct list_head *from, new_page_t get_new_page,
>                  free_page_t put_new_page, unsigned long private,
> @@ -1446,6 +1446,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>          int retry = 1;
>          int thp_retry = 1;
>          int nr_failed = 0;
> +       int nr_failed_pages = 0;
>          int nr_succeeded = 0;
>          int nr_thp_succeeded = 0;
>          int nr_thp_failed = 0;
> @@ -1457,13 +1458,16 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>          int swapwrite = current->flags & PF_SWAPWRITE;
>          int rc, nr_subpages;
>          LIST_HEAD(ret_pages);
> +       LIST_HEAD(thp_split_pages);
>          bool nosplit = (reason == MR_NUMA_MISPLACED);
> +       bool dont_count_failed_subpage = false;
> 
>          trace_mm_migrate_pages_start(mode, reason);
> 
>          if (!swapwrite)
>                  current->flags |= PF_SWAPWRITE;
> 
> +thp_subpage_migration:
>          for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
>                  retry = 0;
>                  thp_retry = 0;
> @@ -1512,18 +1516,21 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>                          case -ENOSYS:
>                                  /* THP migration is unsupported */
>                                  if (is_thp) {
> -                                       if (!try_split_thp(page, &page2, from)) {
> +                                       nr_thp_failed++;
> +                                       if (!try_split_thp(page, &page2, &thp_split_pages)) {
>                                                  nr_thp_split++;
>                                                  goto retry;
>                                          }
> 
> -                                       nr_thp_failed++;
> -                                       nr_failed += nr_subpages;
> +                                       nr_failed_pages += nr_subpages;
>                                          break;
>                                  }
> 
>                                  /* Hugetlb migration is unsupported */
> -                               nr_failed++;
> +                               if (!dont_count_failed_subpage) {
> +                                       nr_failed++;
> +                                       nr_failed_pages++;
> +                               }

Shoule be:
	if (!dont_count_failed_subpage)
		nr_failed++;
	nr_failed_pages++;

Since we still need count the number of normal pages into 
"nr_failed_pages" in the second round.

>                                  break;
>                          case -ENOMEM:
>                                  /*
> @@ -1532,16 +1539,19 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>                                   * THP NUMA faulting doesn't split THP to retry.
>                                   */
>                                  if (is_thp && !nosplit) {
> -                                       if (!try_split_thp(page, &page2, from)) {
> +                                       nr_thp_failed++;
> +                                       if (!try_split_thp(page, &page2, &thp_split_pages)) {
>                                                  nr_thp_split++;
>                                                  goto retry;
>                                          }
> 
> -                                       nr_thp_failed++;
> -                                       nr_failed += nr_subpages;
> +                                       nr_failed_pages += nr_subpages;
>                                          goto out;
>                                  }
> -                               nr_failed++;
> +                               if (!dont_count_failed_subpage) {
> +                                       nr_failed++;
> +                                       nr_failed_pages++;
> +                               }

Ditto

>                                  goto out;
>                          case -EAGAIN:
>                                  if (is_thp) {
> @@ -1567,17 +1577,34 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>                                   */
>                                  if (is_thp) {
>                                          nr_thp_failed++;
> -                                       nr_failed += nr_subpages;
> +                                       nr_failed_pages += nr_subpages;
>                                          break;
>                                  }
> -                               nr_failed++;
> +                               if (!dont_count_failed_subpage) {
> +                                       nr_failed++;
> +                                       nr_failed_pages++;
> +                               }

Ditto.

>                                  break;
>                          }
>                  }
>          }
>          nr_failed += retry + thp_retry;
>          nr_thp_failed += thp_retry;
> -       rc = nr_failed;
> +       /*
> +        * try to migrate subpages of fail-to-migrate THPs, no nr_failed
> +        * counting in this round, since all subpages of a THP is counted as
> +        * 1 failure in the first round.
> +        */
> +       if (!list_empty(&thp_split_pages)) {
> +               /*
> +                * move non-migrated pages (after 10 retries) to ret_pages to
> +                * avoid migrating them again.
> +                */
> +               list_splice(from, &ret_pages);
> +               list_splice(&thp_split_pages, from);

should be:
	list_splice_init(from, &ret_pages);
	list_splice_init(&thp_split_pages, from);

Since the 'from' and  'thp_split_pages' will be reused, otherwise will 
cause dead-loop.

> +               dont_count_failed_subpage = true;

Should add 'retry = 1;' here, otherwise it will not try to migrate subpages.

> +               goto thp_subpage_migration;
> +       }
> +
> +       rc = nr_failed + nr_thp_failed;
>   out:
>          /*
>           * Put the permanent failure page back to migration list, they
> @@ -1586,11 +1613,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>          list_splice(&ret_pages, from);
> 
>          count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
> -       count_vm_events(PGMIGRATE_FAIL, nr_failed);
> +       count_vm_events(PGMIGRATE_FAIL, nr_failed_pages);
>          count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
>          count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
>          count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
> -       trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
> +       trace_mm_migrate_pages(nr_succeeded, nr_failed_pages, nr_thp_succeeded,
>                                 nr_thp_failed, nr_thp_split, mode, reason);
> 
>          if (!swapwrite)
> 
> --
> Best Regards,
> Yan, Zi
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ