From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 86DB7C43460 for ; Fri, 14 May 2021 00:23:15 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id E68496143D for ; Fri, 14 May 2021 00:23:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E68496143D Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 16AE96B0036; Thu, 13 May 2021 20:23:14 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 11B886B006E; Thu, 13 May 2021 20:23:14 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F25006B0070; Thu, 13 May 2021 20:23:13 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0070.hostedemail.com [216.40.44.70]) by kanga.kvack.org (Postfix) with ESMTP id C2C5F6B0036 for ; Thu, 13 May 2021 20:23:13 -0400 (EDT) Received: from smtpin14.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 6A9031801E214 for ; Fri, 14 May 2021 00:23:13 +0000 (UTC) X-FDA: 78137937066.14.47B5CFA Received: from mail-pl1-f182.google.com (mail-pl1-f182.google.com [209.85.214.182]) by imf30.hostedemail.com (Postfix) with ESMTP id E8C50E0001AC for ; Fri, 14 May 2021 00:23:11 +0000 (UTC) Received: by mail-pl1-f182.google.com with SMTP id a11so15231144plh.3 for ; Thu, 13 May 2021 17:23:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=7M2VJY6FHhWqAc/4PJbGV/SNxyzRDLt47x/Is56SyL4=; b=XhJvN3eSjiyhi0yCEz6Xj5p90iLOnBn/5RpHpJ0zfBxTJuhFQzRL7XaOCuMMAYfL91 WoZG2cykWuKvpk6+yeZAwZt7oacaJowgzOvJl+080CjKnZPHsSDd3LHXjVAYVJr4UFpC GNrbQuh2i5OBTc75+gtXVbNve+FI/hJf6TVtWK11jYXeeGwc7fIf5swJgu8eAZqH0G0X L9VUVBCRSCnkm6Ay2su/nspuyR7Nmb3FoVForYKr8b3BXqDVlraqpwW66uF/mzs7lt9/ Um9JhacH0rqqJVcsYt6H6fa15DjKU5OveK/cq5xA9Eu6SAiK5LDpfKuGr9uEb8lFA582 61UQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=7M2VJY6FHhWqAc/4PJbGV/SNxyzRDLt47x/Is56SyL4=; b=C1n0x9QOAklXVCiKi7GVE1PwOZwVchMSnfc2vGPc2got75sDGtpXz5F81Wu/53lg7f AH2ZdKkqLi42DTj89H2kQ93X+IAjJ8he6ftFv/pGIsCNAHylyNKku+PCszZUbxDFVoc3 kYofZWizlE7L2WaZwujtFe86NjUXqQcpeA0HK4rJr7AGRJgbEZetjDhI3w49wW5Cj/+H 0kM6wi15/1wJeITN34En79gLZzmvzEw493BAk45LyGFvJ9NQjFcCWLxxwnt+uLxzYFHW 2TIMN82OO4xwyb5d5D5BLpcLUhZN4CbYpVVZn3GS6oleQPjEJppUFdDnQP9eILgUoGNF 7uqg== X-Gm-Message-State: AOAM530TiAjxlXDxS8E4KUzaCLVux0mq7vuYcnU1iAXu9InECsYirJoS QxfH0aOPBek167ZlOOHH/0IPPqB5snONVlW/AYX61Q== X-Google-Smtp-Source: ABdhPJwKuahZZpjrKaQ2vgQa72PIVHwa5eGmj3dhgowAYvPUNkxcDhSgLpKbduNC/8zh8cJxC3tykC3O1rP/ACwVRMY= X-Received: by 2002:a17:902:7c85:b029:ed:893d:ec7c with SMTP id y5-20020a1709027c85b02900ed893dec7cmr42018311pll.82.1620951791846; Thu, 13 May 2021 17:23:11 -0700 (PDT) MIME-Version: 1.0 References: <20210513234309.366727-1-almasrymina@google.com> <09dc0712-48e8-8ba2-f170-4c2febcfff83@oracle.com> In-Reply-To: <09dc0712-48e8-8ba2-f170-4c2febcfff83@oracle.com> From: Mina Almasry Date: Thu, 13 May 2021 17:23:00 -0700 Message-ID: Subject: Re: [PATCH] mm, hugetlb: fix resv_huge_pages underflow on UFFDIO_COPY To: Mike Kravetz Cc: Axel Rasmussen , Peter Xu , Linux-MM , Andrew Morton , open list Content-Type: text/plain; charset="UTF-8" Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=google.com header.s=20161025 header.b=XhJvN3eS; spf=pass (imf30.hostedemail.com: domain of almasrymina@google.com designates 209.85.214.182 as permitted sender) smtp.mailfrom=almasrymina@google.com; dmarc=pass (policy=reject) header.from=google.com X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: E8C50E0001AC X-Stat-Signature: c971ft5k5kcgwsc8k8r1jxfsutxeze4o X-HE-Tag: 1620951791-509488 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: On Thu, May 13, 2021 at 5:14 PM Mike Kravetz wrote: > > On 5/13/21 4:49 PM, Mina Almasry wrote: > > On Thu, May 13, 2021 at 4:43 PM Mina Almasry wrote: > >> > >> When hugetlb_mcopy_atomic_pte() is called with: > >> - mode==MCOPY_ATOMIC_NORMAL and, > >> - we already have a page in the page cache corresponding to the > >> associated address, > >> > >> We will allocate a huge page from the reserves, and then fail to insert it > >> into the cache and return -EEXIST. In this case, we need to return -EEXIST > >> without allocating a new page as the page already exists in the cache. > >> Allocating the extra page causes the resv_huge_pages to underflow temporarily > >> until the extra page is freed. > >> > >> To fix this we check if a page exists in the cache, and allocate it and > >> insert it in the cache immediately while holding the lock. After that we > >> copy the contents into the page. > >> > >> As a side effect of this, pages may exist in the cache for which the > >> copy failed and for these pages PageUptodate(page) == false. Modify code > >> that query the cache to handle this correctly. > >> > > > > To be honest, I'm not sure I've done this bit correctly. Please take a > > look and let me know what you think. It may be too overly complicated > > to have !PageUptodate() pages in the cache and ask the rest of the > > code to handle that edge case correctly, but I'm not sure how else to > > fix this issue. > > > > I think you just moved the underflow from hugetlb_mcopy_atomic_pte to > hugetlb_no_page. Why? > > Consider the case where there is only one reserve left and someone does > the MCOPY_ATOMIC_NORMAL for the address. We will allocate the page and > consume the reserve (reserve count == 0) and insert the page into the > cache. Now, if the copy_huge_page_from_user fails we must drop the > locks/fault mutex to do the copy. While locks are dropped, someone > faults on the address and ends up in hugetlb_no_page. The page is in > the cache but not up to date, so we go down the allocate new page path > and will decrement the reserve count again to cause underflow. > For what it's worth, I think I fixed the underflow with this patch, not moved it. I added a check in hugetlb_no_page() such that if we find a page in the cache with !PageUptodate(page), we will reuse that page instead of allocating a new one and decrementing the count again. Running the test with the WARN_ONCE_ON locally shows no warnings again. > How about this approach? I'll give it a shot for sure. FWIW on first glance it looks more complicated that what I have here, but my guess I'm not doing the !PageUptodate() handling correctly and that's why it seems this solution is simpler. I'll give it a shot though. > - Keep the check for hugetlbfs_pagecache_present in hugetlb_mcopy_atomic_pte > that you added. That will catch the race where the page was added to > the cache before entering the routine. > - With the above check in place, we only need to worry about the case > where copy_huge_page_from_user fails and we must drop locks. In this > case we: > - Free the page previously allocated. > - Allocate a 'temporary' huge page without consuming reserves. I'm > thinking of something similar to page migration. > - Drop the locks and let the copy_huge_page_from_user be done to the > temporary page. > - When reentering hugetlb_mcopy_atomic_pte after dropping locks (the > *pagep case) we need to once again check > hugetlbfs_pagecache_present. > - We then try to allocate the huge page which will consume the > reserve. If successful, copy contents of temporary page to newly > allocated page. Free temporary page. > > There may be issues with this, and I have not given it deep thought. It > does abuse the temporary huge page concept, but perhaps no more than > page migration. Things do slow down if the extra page allocation and > copy is required, but that would only be the case if copy_huge_page_from_user > needs to be done without locks. Not sure, but hoping that is rare. > -- > Mike Kravetz