From patchwork Sun Nov 25 03:21:07 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hugh Dickins X-Patchwork-Id: 10696659 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 577311709 for ; Sun, 25 Nov 2018 03:21:24 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 38360296C5 for ; Sun, 25 Nov 2018 03:21:24 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 256FF29FAE; Sun, 25 Nov 2018 03:21:24 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.5 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE, USER_IN_DEF_DKIM_WL autolearn=ham version=3.3.1 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B2721296C5 for ; Sun, 25 Nov 2018 03:21:22 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 33DD56B39ED; Sat, 24 Nov 2018 22:21:20 -0500 (EST) Delivered-To: linux-mm-outgoing@kvack.org Received: by kanga.kvack.org (Postfix, from userid 40) id 2ED4E6B39EE; Sat, 24 Nov 2018 22:21:20 -0500 (EST) X-Original-To: int-list-linux-mm@kvack.org X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1B5BF6B39EF; Sat, 24 Nov 2018 22:21:20 -0500 (EST) X-Original-To: linux-mm@kvack.org X-Delivered-To: linux-mm@kvack.org Received: from mail-pl1-f200.google.com (mail-pl1-f200.google.com [209.85.214.200]) by kanga.kvack.org (Postfix) with ESMTP id CB2706B39ED for ; Sat, 24 Nov 2018 22:21:19 -0500 (EST) Received: by mail-pl1-f200.google.com with SMTP id x21-v6so11366040pln.10 for ; Sat, 24 Nov 2018 19:21:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:dkim-signature:date:from:to:cc:subject :message-id:user-agent:mime-version; bh=+8zCRZUBJK0Ya78Xou9gqwoebzB337SjzErezgTkTLU=; b=Oboxv+jqoRMopNP5qYV4UaLySOg8V+tD5c+GZKNu6ECu6iEWhUuQmrY+FyOACImjsT GgGFkc631JRwuZWaker0k6ogIs1qSPs3KmQf3BDI0Ow1LJcBAR13+Ui7SGPZ31Y2sfx2 /8qSszSuo4SppIeXlf+uK1xzH+7ly6d+8ClfgA0Ld9idOLvsz/82PJz0T6dOTx7b2O0J UqPiZQ+gUBl+EcIXkKGttotwCy0jt0IgEZZn/NqDigcMOEw2Sbd0cxK8XhSJOBuL8LT5 i2Oimd1vRbHxo9vOeUy10wfNlWMRxS3tedFXL2S3X6q1jOec3HkKOBIjKlWv73ec6Xlg pp4g== X-Gm-Message-State: AGRZ1gJjzq+KV2OXuJ4WvL5+h6RNyYQ1TEzclYygR6RWT+V8GNTslktX Eo2XfRtQpqSU5uwq+15e4Q2Rw7biY0N/la5Kb4e2ry48xetWHH/QBajb1lsDBxd44iOCuKEyvX9 tEFAe4HHc8wgSMO4s/SANFIFHbUPJNH3Ievr1Quo+9f4DUP0SbZIL+keUGGRoZbjJr41a7UJNLR lzK85EV+P4pfJ5/xyyDe1jtp0yI3KoEcvGnSwjQYoFPjp7XNGvMsdPJ2vioM+XZGjnGDyebzkeM cL1x3OElswR/FnSv4tpP/eD2xn5PYh/ytgCqyUdovRs84hd11Pcn5p3qsULJ4krTHuEplDdYfF/ qE6yubIrbGjgTlTVr4W8WtiLh7rleDLCbnnwGPh1100YdYF8V+MXZt6W9F1pPBxXQMt2jt1ff0/ O X-Received: by 2002:a63:4665:: with SMTP id v37mr20167027pgk.425.1543116079363; Sat, 24 Nov 2018 19:21:19 -0800 (PST) X-Received: by 2002:a63:4665:: with SMTP id v37mr20166981pgk.425.1543116078056; Sat, 24 Nov 2018 19:21:18 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1543116078; cv=none; d=google.com; s=arc-20160816; b=EVQZbaH7iFIhSRXtf2LFb2POq06p7/5iEVq+WoplX3TJaaRVOd/u2je0lfnbbk8NFb nDv4XqCej2DSlW0w2GfcmRptSrsAyHUR05MeOkwVW3yCNl9ovB6nlcxpIucbdSoIuz12 Uzjpi5NZrDRAj1g7Nr+G5HeyuENm/Iul+UdulZHdA6I2Da2cZs0SrbwThpYaoak7NM+E HLJgxfQPV24o+I7zo/8702kaOaDJrq4yYW5/5G77RZEdkLDw7J6LuDmIz08+EJg+UxT6 cYANPTFmLl0KquIZRbebPJoUIpHs328LRAm1bPqXbywi7m3nKMHMyDaa/2j1z9XXJO3B WfKw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=mime-version:user-agent:message-id:subject:cc:to:from:date :dkim-signature; bh=+8zCRZUBJK0Ya78Xou9gqwoebzB337SjzErezgTkTLU=; b=Gk71oSAxUT41wuQic5bptFPQmSR84MjSl92wifMFRgurUSGjYi9AgaY1jFdMna/T08 qOxWs/ypQ6Qg/XSzDICPxO1ukIOoJ94a+7Uqkml6W6Wgc3utOTXT/W+LufBkiqsvzErq Ztv2TCzAbKiZcNAWkotir2Epz2ToDz4JmshzgrR8PMpXDI6uuwyyuE3fdxMfuoSnax/X ZJRKG4rR15zw0PJZfAs3pwgYtQgi9k/1GcOTLMnu6Gm6/1Wz29Kq5RTxtQ9bR3zOsEru MZERSrjzVHJBMqx3rvGXVPD8YveCu8t+5TxTTcPSjJJgzXK4FgPFJryay2SJtlY2692S 7vyg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=ZESMPdoo; spf=pass (google.com: domain of hughd@google.com designates 209.85.220.65 as permitted sender) smtp.mailfrom=hughd@google.com; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from mail-sor-f65.google.com (mail-sor-f65.google.com. [209.85.220.65]) by mx.google.com with SMTPS id y73sor7767945pfd.54.2018.11.24.19.21.17 for (Google Transport Security); Sat, 24 Nov 2018 19:21:18 -0800 (PST) Received-SPF: pass (google.com: domain of hughd@google.com designates 209.85.220.65 as permitted sender) client-ip=209.85.220.65; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=ZESMPdoo; spf=pass (google.com: domain of hughd@google.com designates 209.85.220.65 as permitted sender) smtp.mailfrom=hughd@google.com; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:user-agent:mime-version; bh=+8zCRZUBJK0Ya78Xou9gqwoebzB337SjzErezgTkTLU=; b=ZESMPdoo1ygUxKtjfs6RzmgduvZklwXG71qo/4xpc1kL/F4uClANTVngUjQ+MSzt5q SIKh75Htfp1zXmc9sSSW1RYtHd43a05SmoLu9BPcN2BkbxtnLI45GNIHToQ1rVKdO2O+ hic/dKjWLpfCVnJj3XG08xwRNwGMFHJoK4Xq+R2Gd3VFOPOo29joAdUoqUk1FAlVWmUH lfJdUA8IswOLLNyMqCsnBSylFyDQTJWD4cwzBBbl7Q0JrRHF3ZcMGSx//tG61qmDOSOW IeC6qq8C7S8wO5y1l8vFdSXw7zDMYZtA3OVHURDAYMkfpdO1P8gekINH0Pgb1Nwo51an v8Qw== X-Google-Smtp-Source: AJdET5fevNVYBSmrPQXvy9EeeOxhUSpZmF8QZTxwwNAWgD5ic4rqsxwNF9xezMPZ8wJyrB8j/jfMnA== X-Received: by 2002:a62:75d1:: with SMTP id q200mr22316087pfc.254.1543116077199; Sat, 24 Nov 2018 19:21:17 -0800 (PST) Received: from [100.112.89.103] ([104.133.8.103]) by smtp.gmail.com with ESMTPSA id k14sm77117928pgs.52.2018.11.24.19.21.15 (version=TLS1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sat, 24 Nov 2018 19:21:16 -0800 (PST) Date: Sat, 24 Nov 2018 19:21:07 -0800 (PST) From: Hugh Dickins X-X-Sender: hugh@eggly.anvils To: Linus Torvalds cc: Andrew Morton , Baoquan He , Michal Hocko , Vlastimil Babka , Andrea Arcangeli , David Hildenbrand , Mel Gorman , David Herrmann , Tim Chen , Kan Liang , Andi Kleen , Davidlohr Bueso , Peter Zijlstra , Christoph Lameter , Nicholas Piggin , pifang@redhat.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: [PATCH] mm: put_and_wait_on_page_locked() while page is migrated Message-ID: User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: X-Virus-Scanned: ClamAV using ClamSMTP Waiting on a page migration entry has used wait_on_page_locked() all along since 2006: but you cannot safely wait_on_page_locked() without holding a reference to the page, and that extra reference is enough to make migrate_page_move_mapping() fail with -EAGAIN, when a racing task faults on the entry before migrate_page_move_mapping() gets there. And that failure is retried nine times, amplifying the pain when trying to migrate a popular page. With a single persistent faulter, migration sometimes succeeds; with two or three concurrent faulters, success becomes much less likely (and the more the page was mapped, the worse the overhead of unmapping and remapping it on each try). This is especially a problem for memory offlining, where the outer level retries forever (or until terminated from userspace), because a heavy refault workload can trigger an endless loop of migration failures. wait_on_page_locked() is the wrong tool for the job. David Herrmann (but was he the first?) noticed this issue in 2014: https://marc.info/?l=linux-mm&m=140110465608116&w=2 Tim Chen started a thread in August 2017 which appears relevant: https://marc.info/?l=linux-mm&m=150275941014915&w=2 where Kan Liang went on to implicate __migration_entry_wait(): https://marc.info/?l=linux-mm&m=150300268411980&w=2 and the thread ended up with the v4.14 commits: 2554db916586 ("sched/wait: Break up long wake list walk") 11a19c7b099f ("sched/wait: Introduce wakeup boomark in wake_up_page_bit") Baoquan He reported "Memory hotplug softlock issue" 14 November 2018: https://marc.info/?l=linux-mm&m=154217936431300&w=2 We have all assumed that it is essential to hold a page reference while waiting on a page lock: partly to guarantee that there is still a struct page when MEMORY_HOTREMOVE is configured, but also to protect against reuse of the struct page going to someone who then holds the page locked indefinitely, when the waiter can reasonably expect timely unlocking. But in fact, so long as wait_on_page_bit_common() does the put_page(), and is careful not to rely on struct page contents thereafter, there is no need to hold a reference to the page while waiting on it. That does mean that this case cannot go back through the loop: but that's fine for the page migration case, and even if used more widely, is limited by the "Stop walking if it's locked" optimization in wake_page_function(). Add interface put_and_wait_on_page_locked() to do this, using negative value of the lock arg to wait_on_page_bit_common() to implement it. No interruptible or killable variant needed yet, but they might follow: I have a vague notion that reporting -EINTR should take precedence over return from wait_on_page_bit_common() without knowing the page state, so arrange it accordingly - but that may be nothing but pedantic. __migration_entry_wait() still has to take a brief reference to the page, prior to calling put_and_wait_on_page_locked(): but now that it is dropped before waiting, the chance of impeding page migration is very much reduced. Should we perhaps disable preemption across this? shrink_page_list()'s __ClearPageLocked(): that was a surprise! This survived a lot of testing before that showed up. PageWaiters may have been set by wait_on_page_bit_common(), and the reference dropped, just before shrink_page_list() succeeds in freezing its last page reference: in such a case, unlock_page() must be used. Follow the suggestion from Michal Hocko, just revert a978d6f52106 ("mm: unlockless reclaim") now: that optimization predates PageWaiters, and won't buy much these days; but we can reinstate it for the !PageWaiters case if anyone notices. It does raise the question: should vmscan.c's is_page_cache_freeable() and __remove_mapping() now treat a PageWaiters page as if an extra reference were held? Perhaps, but I don't think it matters much, since shrink_page_list() already had to win its trylock_page(), so waiters are not very common there: I noticed no difference when trying the bigger change, and it's surely not needed while put_and_wait_on_page_locked() is only used for page migration. Reported-and-tested-by: Baoquan He Signed-off-by: Hugh Dickins Acked-by: Michal Hocko Reviewed-by: Andrea Arcangeli --- Linus, I'm addressing this patch to you because I see from Tim Chen's thread that it would interest you, and you were disappointed not to root cause the issue back then. I'm not pushing for you to fast-track this into 4.20-rc, but I expect Andrew will pick it up for mmotm, and thence linux-next. Or you may spot a terrible defect, but I hope not. Hugh include/linux/pagemap.h | 2 ++ mm/filemap.c | 60 ++++++++++++++++++++++++++++++++--------- mm/huge_memory.c | 6 ++--- mm/migrate.c | 12 +++------ mm/vmscan.c | 10 ++----- 5 files changed, 57 insertions(+), 33 deletions(-) diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index 226f96f0dee0..e2d7039af6a3 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -537,6 +537,8 @@ static inline int wait_on_page_locked_killable(struct page *page) return wait_on_page_bit_killable(compound_head(page), PG_locked); } +extern void put_and_wait_on_page_locked(struct page *page); + /* * Wait for a page to complete writeback */ diff --git a/mm/filemap.c b/mm/filemap.c index 81adec8ee02c..d645fbbf34d7 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -981,7 +981,14 @@ static int wake_page_function(wait_queue_entry_t *wait, unsigned mode, int sync, if (wait_page->bit_nr != key->bit_nr) return 0; - /* Stop walking if it's locked */ + /* + * Stop walking if it's locked. + * Is this safe if put_and_wait_on_page_locked() is in use? + * Yes: the waker must hold a reference to this page, and if PG_locked + * has now already been set by another task, that task must also hold + * a reference to the *same usage* of this page; so there is no need + * to walk on to wake even the put_and_wait_on_page_locked() callers. + */ if (test_bit(key->bit_nr, &key->page->flags)) return -1; @@ -1050,24 +1057,28 @@ static void wake_up_page(struct page *page, int bit) } static inline int wait_on_page_bit_common(wait_queue_head_t *q, - struct page *page, int bit_nr, int state, bool lock) + struct page *page, int bit_nr, int state, int lock) { struct wait_page_queue wait_page; wait_queue_entry_t *wait = &wait_page.wait; + bool bit_is_set; bool thrashing = false; + bool delayacct = false; unsigned long pflags; int ret = 0; if (bit_nr == PG_locked && !PageUptodate(page) && PageWorkingset(page)) { - if (!PageSwapBacked(page)) + if (!PageSwapBacked(page)) { delayacct_thrashing_start(); + delayacct = true; + } psi_memstall_enter(&pflags); thrashing = true; } init_wait(wait); - wait->flags = lock ? WQ_FLAG_EXCLUSIVE : 0; + wait->flags = lock > 0 ? WQ_FLAG_EXCLUSIVE : 0; wait->func = wake_page_function; wait_page.page = page; wait_page.bit_nr = bit_nr; @@ -1084,14 +1095,17 @@ static inline int wait_on_page_bit_common(wait_queue_head_t *q, spin_unlock_irq(&q->lock); - if (likely(test_bit(bit_nr, &page->flags))) { + bit_is_set = test_bit(bit_nr, &page->flags); + if (lock < 0) + put_page(page); + + if (likely(bit_is_set)) io_schedule(); - } - if (lock) { + if (lock > 0) { if (!test_and_set_bit_lock(bit_nr, &page->flags)) break; - } else { + } else if (lock == 0) { if (!test_bit(bit_nr, &page->flags)) break; } @@ -1100,12 +1114,23 @@ static inline int wait_on_page_bit_common(wait_queue_head_t *q, ret = -EINTR; break; } + + if (lock < 0) { + /* + * We can no longer safely access page->flags: + * even if CONFIG_MEMORY_HOTREMOVE is not enabled, + * there is a risk of waiting forever on a page reused + * for something that keeps it locked indefinitely. + * But best check for -EINTR above before breaking. + */ + break; + } } finish_wait(q, wait); if (thrashing) { - if (!PageSwapBacked(page)) + if (delayacct) delayacct_thrashing_end(); psi_memstall_leave(&pflags); } @@ -1124,17 +1149,26 @@ static inline int wait_on_page_bit_common(wait_queue_head_t *q, void wait_on_page_bit(struct page *page, int bit_nr) { wait_queue_head_t *q = page_waitqueue(page); - wait_on_page_bit_common(q, page, bit_nr, TASK_UNINTERRUPTIBLE, false); + wait_on_page_bit_common(q, page, bit_nr, TASK_UNINTERRUPTIBLE, 0); } EXPORT_SYMBOL(wait_on_page_bit); int wait_on_page_bit_killable(struct page *page, int bit_nr) { wait_queue_head_t *q = page_waitqueue(page); - return wait_on_page_bit_common(q, page, bit_nr, TASK_KILLABLE, false); + return wait_on_page_bit_common(q, page, bit_nr, TASK_KILLABLE, 0); } EXPORT_SYMBOL(wait_on_page_bit_killable); +void put_and_wait_on_page_locked(struct page *page) +{ + wait_queue_head_t *q; + + page = compound_head(page); + q = page_waitqueue(page); + wait_on_page_bit_common(q, page, PG_locked, TASK_UNINTERRUPTIBLE, -1); +} + /** * add_page_wait_queue - Add an arbitrary waiter to a page's wait queue * @page: Page defining the wait queue of interest @@ -1264,7 +1298,7 @@ void __lock_page(struct page *__page) { struct page *page = compound_head(__page); wait_queue_head_t *q = page_waitqueue(page); - wait_on_page_bit_common(q, page, PG_locked, TASK_UNINTERRUPTIBLE, true); + wait_on_page_bit_common(q, page, PG_locked, TASK_UNINTERRUPTIBLE, 1); } EXPORT_SYMBOL(__lock_page); @@ -1272,7 +1306,7 @@ int __lock_page_killable(struct page *__page) { struct page *page = compound_head(__page); wait_queue_head_t *q = page_waitqueue(page); - return wait_on_page_bit_common(q, page, PG_locked, TASK_KILLABLE, true); + return wait_on_page_bit_common(q, page, PG_locked, TASK_KILLABLE, 1); } EXPORT_SYMBOL_GPL(__lock_page_killable); diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 622cced74fd9..832ab11badc2 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -1501,8 +1501,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf, pmd_t pmd) if (!get_page_unless_zero(page)) goto out_unlock; spin_unlock(vmf->ptl); - wait_on_page_locked(page); - put_page(page); + put_and_wait_on_page_locked(page); goto out; } @@ -1538,8 +1537,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf, pmd_t pmd) if (!get_page_unless_zero(page)) goto out_unlock; spin_unlock(vmf->ptl); - wait_on_page_locked(page); - put_page(page); + put_and_wait_on_page_locked(page); goto out; } diff --git a/mm/migrate.c b/mm/migrate.c index f7e4bfdc13b7..acda06f99754 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -327,16 +327,13 @@ void __migration_entry_wait(struct mm_struct *mm, pte_t *ptep, /* * Once page cache replacement of page migration started, page_count - * *must* be zero. And, we don't want to call wait_on_page_locked() - * against a page without get_page(). - * So, we use get_page_unless_zero(), here. Even failed, page fault - * will occur again. + * is zero; but we must not call put_and_wait_on_page_locked() without + * a ref. Use get_page_unless_zero(), and just fault again if it fails. */ if (!get_page_unless_zero(page)) goto out; pte_unmap_unlock(ptep, ptl); - wait_on_page_locked(page); - put_page(page); + put_and_wait_on_page_locked(page); return; out: pte_unmap_unlock(ptep, ptl); @@ -370,8 +367,7 @@ void pmd_migration_entry_wait(struct mm_struct *mm, pmd_t *pmd) if (!get_page_unless_zero(page)) goto unlock; spin_unlock(ptl); - wait_on_page_locked(page); - put_page(page); + put_and_wait_on_page_locked(page); return; unlock: spin_unlock(ptl); diff --git a/mm/vmscan.c b/mm/vmscan.c index 62ac0c488624..9c50d90b9bc5 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -1456,14 +1456,8 @@ static unsigned long shrink_page_list(struct list_head *page_list, count_memcg_page_event(page, PGLAZYFREED); } else if (!mapping || !__remove_mapping(mapping, page, true)) goto keep_locked; - /* - * At this point, we have no other references and there is - * no way to pick any more up (removed from LRU, removed - * from pagecache). Can use non-atomic bitops now (and - * we obviously don't have to worry about waking up a process - * waiting on the page lock, because there are no references. - */ - __ClearPageLocked(page); + + unlock_page(page); free_it: nr_reclaimed++;