Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755725AbbGELic (ORCPT ); Sun, 5 Jul 2015 07:38:32 -0400 Received: from mail-la0-f49.google.com ([209.85.215.49]:35877 "EHLO mail-la0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755389AbbGELiW (ORCPT ); Sun, 5 Jul 2015 07:38:22 -0400 MIME-Version: 1.0 In-Reply-To: References: <1435312710-15108-1-git-send-email-gioh.kim@lge.com> <1435312710-15108-5-git-send-email-gioh.kim@lge.com> Date: Sat, 4 Jul 2015 22:00:26 +0300 Message-ID: Subject: Re: [RFCv2 4/5] mm/compaction: compaction calls generic migration From: Konstantin Khlebnikov To: Gioh Kim Cc: Jeff Layton , Bruce Fields , Vlastimil Babka , Joonsoo Kim , Al Viro , "Michael S. Tsirkin" , Minchan Kim , Rafael Aquini , linux-fsdevel , virtualization@lists.linux-foundation.org, Linux Kernel Mailing List , Linux API , "linux-mm@kvack.org" , Andrew Morton Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6481 Lines: 150 On Sat, Jul 4, 2015 at 9:13 PM, Konstantin Khlebnikov wrote: > On Fri, Jun 26, 2015 at 12:58 PM, Gioh Kim wrote: >> Compaction calls interfaces of driver page migration >> instead of calling balloon migration directly. >> >> Signed-off-by: Gioh Kim >> --- >> drivers/virtio/virtio_balloon.c | 1 + >> mm/compaction.c | 9 +++++---- >> mm/migrate.c | 21 ++++++++++++--------- >> 3 files changed, 18 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c >> index c49b553..5e5cbea 100644 >> --- a/drivers/virtio/virtio_balloon.c >> +++ b/drivers/virtio/virtio_balloon.c >> @@ -30,6 +30,7 @@ >> #include >> #include >> #include >> +#include >> >> /* >> * Balloon device works in 4K page units. So each page is pointed to by >> diff --git a/mm/compaction.c b/mm/compaction.c >> index 16e1b57..cc5ec81 100644 >> --- a/mm/compaction.c >> +++ b/mm/compaction.c >> @@ -14,7 +14,7 @@ >> #include >> #include >> #include >> -#include >> +#include >> #include >> #include >> #include "internal.h" >> @@ -714,12 +714,13 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, >> >> /* >> * Check may be lockless but that's ok as we recheck later. >> - * It's possible to migrate LRU pages and balloon pages >> + * It's possible to migrate LRU pages and driver pages >> * Skip any other type of page >> */ >> if (!PageLRU(page)) { >> - if (unlikely(balloon_page_movable(page))) { >> - if (balloon_page_isolate(page)) { >> + if (unlikely(driver_page_migratable(page))) { >> + if (page->mapping->a_ops->isolatepage(page, >> + isolate_mode)) { > > Dereferencing page->mapping isn't safe here. > Page could be "truncated" from mapping at any time. > > As you can see balloon_page_isolate() calls get_page_unless_zero, > trylock_page and only after that checks balloon_page_movable again. Page must be getted and locked before calling aops method, somethin like this: If (!PageLRU(page)) { if (PageBalloon(page) || PageMobile(page)) if (get_page_unless_zero(page)) if (try_lock(page)) if (page->mapping && page->mapping->a_ops->isolatepage) page->mapping->a_ops->isolate_page(page, ...) .... > > Existing code already does similar unsafe dereference in > __isolate_lru_page(): page->mapping->a_ops->migratepage > >> /* Successfully isolated */ >> goto isolate_success; >> } >> diff --git a/mm/migrate.c b/mm/migrate.c >> index 236ee25..a0bc1e4 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -35,7 +35,7 @@ >> #include >> #include >> #include >> -#include >> +#include >> #include >> >> #include >> @@ -76,7 +76,7 @@ int migrate_prep_local(void) >> * from where they were once taken off for compaction/migration. >> * >> * This function shall be used whenever the isolated pageset has been >> - * built from lru, balloon, hugetlbfs page. See isolate_migratepages_range() >> + * built from lru, driver, hugetlbfs page. See isolate_migratepages_range() >> * and isolate_huge_page(). >> */ >> void putback_movable_pages(struct list_head *l) >> @@ -92,8 +92,8 @@ void putback_movable_pages(struct list_head *l) >> list_del(&page->lru); >> dec_zone_page_state(page, NR_ISOLATED_ANON + >> page_is_file_cache(page)); >> - if (unlikely(isolated_balloon_page(page))) >> - balloon_page_putback(page); >> + if (unlikely(driver_page_migratable(page))) >> + page->mapping->a_ops->putbackpage(page); >> else >> putback_lru_page(page); >> } >> @@ -844,15 +844,18 @@ static int __unmap_and_move(struct page *page, struct page *newpage, >> } >> } >> >> - if (unlikely(isolated_balloon_page(page))) { >> + if (unlikely(driver_page_migratable(page))) { >> /* >> - * A ballooned page does not need any special attention from >> + * A driver page does not need any special attention from >> * physical to virtual reverse mapping procedures. >> * Skip any attempt to unmap PTEs or to remap swap cache, >> * in order to avoid burning cycles at rmap level, and perform >> * the page migration right away (proteced by page lock). >> */ >> - rc = balloon_page_migrate(newpage, page, mode); >> + rc = page->mapping->a_ops->migratepage(page->mapping, >> + newpage, >> + page, >> + mode); >> goto out_unlock; >> } >> >> @@ -962,8 +965,8 @@ out: >> if (rc != MIGRATEPAGE_SUCCESS && put_new_page) { >> ClearPageSwapBacked(newpage); >> put_new_page(newpage, private); >> - } else if (unlikely(__is_movable_balloon_page(newpage))) { >> - /* drop our reference, page already in the balloon */ >> + } else if (unlikely(driver_page_migratable(newpage))) { >> + /* drop our reference */ >> put_page(newpage); >> } else >> putback_lru_page(newpage); >> -- >> 1.9.1 >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/