On 10/19/2015 06:57 AM, Hugh Dickins wrote:
> I don't know of any problem from the way it's used in our current tree,
> but there is one defect in page migration's custom put_new_page feature.
>
> An unused newpage is expected to be released with the put_new_page(),
> but there was one MIGRATEPAGE_SUCCESS (0) path which released it with
> putback_lru_page(): which can be very wrong for a custom pool.
I'm a bit confused. So there's no immediate bug to be fixed but there was one in
the mainline in the past? Or elsewhere?
> Fixed more easily by resetting put_new_page once it won't be needed,
> than by adding a further flag to modify the rc test.
What is "fixed" if there is no bug? :) Maybe "Further bugs would be
prevented..." or something?
> Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
I agree it's less error-prone after you patch, so:
Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
> ---
> mm/migrate.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> --- migrat.orig/mm/migrate.c 2015-10-18 17:53:17.579329434 -0700
> +++ migrat/mm/migrate.c 2015-10-18 17:53:20.159332371 -0700
> @@ -938,10 +938,11 @@ static ICE_noinline int unmap_and_move(n
> int force, enum migrate_mode mode,
> enum migrate_reason reason)
> {
> - int rc = 0;
> + int rc = MIGRATEPAGE_SUCCESS;
> int *result = NULL;
> - struct page *newpage = get_new_page(page, private, &result);
> + struct page *newpage;
>
> + newpage = get_new_page(page, private, &result);
> if (!newpage)
> return -ENOMEM;
>
> @@ -955,6 +956,8 @@ static ICE_noinline int unmap_and_move(n
> goto out;
>
> rc = __unmap_and_move(page, newpage, force, mode);
> + if (rc == MIGRATEPAGE_SUCCESS)
> + put_new_page = NULL;
>
> out:
> if (rc != -EAGAIN) {
> @@ -981,7 +984,7 @@ out:
> * it. Otherwise, putback_lru_page() will drop the reference grabbed
> * during isolation.
> */
> - if (rc != MIGRATEPAGE_SUCCESS && put_new_page) {
> + if (put_new_page) {
> ClearPageSwapBacked(newpage);
> put_new_page(newpage, private);
> } else if (unlikely(__is_movable_balloon_page(newpage))) {
> @@ -1022,7 +1025,7 @@ static int unmap_and_move_huge_page(new_
> struct page *hpage, int force,
> enum migrate_mode mode)
> {
> - int rc = 0;
> + int rc = -EAGAIN;
> int *result = NULL;
> int page_was_mapped = 0;
> struct page *new_hpage;
> @@ -1044,8 +1047,6 @@ static int unmap_and_move_huge_page(new_
> if (!new_hpage)
> return -ENOMEM;
>
> - rc = -EAGAIN;
> -
> if (!trylock_page(hpage)) {
> if (!force || mode != MIGRATE_SYNC)
> goto out;
> @@ -1070,8 +1071,10 @@ static int unmap_and_move_huge_page(new_
> if (anon_vma)
> put_anon_vma(anon_vma);
>
> - if (rc == MIGRATEPAGE_SUCCESS)
> + if (rc == MIGRATEPAGE_SUCCESS) {
> hugetlb_cgroup_migrate(hpage, new_hpage);
> + put_new_page = NULL;
> + }
>
> unlock_page(hpage);
> out:
> @@ -1083,7 +1086,7 @@ out:
> * it. Otherwise, put_page() will drop the reference grabbed during
> * isolation.
> */
> - if (rc != MIGRATEPAGE_SUCCESS && put_new_page)
> + if (put_new_page)
> put_new_page(new_hpage, private);
> else
> putback_active_hugepage(new_hpage);
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>