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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1F505C77B7A for ; Wed, 31 May 2023 00:38:43 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8F3F86B0072; Tue, 30 May 2023 20:38:42 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 87B3A6B0074; Tue, 30 May 2023 20:38:42 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6F53D900002; Tue, 30 May 2023 20:38:42 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 580E96B0072 for ; Tue, 30 May 2023 20:38:42 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 2D55C1A037F for ; Wed, 31 May 2023 00:38:42 +0000 (UTC) X-FDA: 80848689684.26.6298E15 Received: from mail-yw1-f173.google.com (mail-yw1-f173.google.com [209.85.128.173]) by imf29.hostedemail.com (Postfix) with ESMTP id 5748B120010 for ; Wed, 31 May 2023 00:38:40 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=s4psAFfQ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf29.hostedemail.com: domain of hughd@google.com designates 209.85.128.173 as permitted sender) smtp.mailfrom=hughd@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1685493520; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=ciIag9ppNam6Ikfax7r++Kew/N1dQoncU50To4G8bq4=; b=7dtD8kryBoslPEliZG+QteMMhRoJDuVOCGUdYKzTJY49g7d2IHP4kPBVp61fSa8QRGAlOe 9Okr/284lNB8GRereN80ImqlxHd8ZHn/0Q29eOKIcn+VXpf+40FtTTwjpGaqoZlSzqxuWT sY/0+zSE9b8zYylgh7huDuPt0Je/8kA= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=s4psAFfQ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf29.hostedemail.com: domain of hughd@google.com designates 209.85.128.173 as permitted sender) smtp.mailfrom=hughd@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1685493520; a=rsa-sha256; cv=none; b=P/3bSv4cBKZG/RrYh627c6Qfc8lPUOSl4m0bMsjUD2xbzCY8LetfndkwQrchRez4wuyWp8 0jgfnlPKezjNbZfjIF4aN2HboieEkMNiz8bRG/+HVukQydTHUcZ96qQfm/Wq1FmVf4/PqN 2MaqOCmTtW/NOtPT5eGy/zTxYx7YKiI= Received: by mail-yw1-f173.google.com with SMTP id 00721157ae682-565de553de1so6382707b3.0 for ; Tue, 30 May 2023 17:38:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1685493519; x=1688085519; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:from:to:cc:subject:date:message-id:reply-to; bh=ciIag9ppNam6Ikfax7r++Kew/N1dQoncU50To4G8bq4=; b=s4psAFfQL/Xvpx6CG11TvqCPK3dBE+CtAbPvc1LP/Vp6cCIPwmqOUxi+gGLKdymwVr KBXGfyRCyEb33I+9q2LBQlgDePc3a05l6O33H7YqyysB9hf1XyENU9qWBuoJKw9EUYlM pEmFeg9cJw7sTQQkmXYP686QUBSsQmfaa7kHqU5iVgb8czbTZTSlhAs0FZ/Obh5PJtKM KqgNPZ3zv/vf4lueLkpjpWZJ2r9PyPbJ09lE/jw8yqBmW8+Z8g93HIyjbo+Kh8JZVRr3 WEinxSHBsLsBK74sDXvaF9Wg6I0DN+6VHv7QBBKIYxpiJFFSywoCIJ9FPC0ocitPQl4O hpNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685493519; x=1688085519; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=ciIag9ppNam6Ikfax7r++Kew/N1dQoncU50To4G8bq4=; b=HU6kolGK3mZB0iFIE4ChSvmCFE6XDGlWWo1FfaAdJ5AUWvg00ofpUdI2/nfiCjhKKv 6ME6kb+2kHfvex8Pfp9DC71gPybnOiwj8GkZaoYXUtsz0Det5o403qgvHMvVO6atrTrR RpzsdfAYs9Cv2qhK++1TSKIdkeniOJHZS/aAhb8JFKAT2Zbke9/drTjYGIhe6l6Aj7Vk rKCPQAQl/MCkwzWcZHzZZrrYsLqP6mqYGyRXdd7yT60i6649SeR6z6k9fwPRj9dLQK43 T83CbkBwOU/W24Rzbt2ZfGBHWqkGxLFSqwoChSwQCc5qwyeYnI2LpXcc3m9AUXUYKckZ tIFw== X-Gm-Message-State: AC+VfDyRg5DwwN/scpN1wLa1QO0PH7uBfDVdisuz4LyzXj5tiq52YWmm DFQDHkCVctnSwtISJi110YALrw== X-Google-Smtp-Source: ACHHUZ6oAzWH6paUG3LosxgrJ++supK0+JI2vStYiabgmf/uhHM/SefufdWqFEJ0cjjU81mHFbbIJA== X-Received: by 2002:a0d:f905:0:b0:568:e69a:1178 with SMTP id j5-20020a0df905000000b00568e69a1178mr1764241ywf.5.1685493519139; Tue, 30 May 2023 17:38:39 -0700 (PDT) Received: from ripple.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id a7-20020a818a07000000b00555c30ec361sm4859512ywg.143.2023.05.30.17.38.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 30 May 2023 17:38:35 -0700 (PDT) Date: Tue, 30 May 2023 17:38:25 -0700 (PDT) From: Hugh Dickins X-X-Sender: hugh@ripple.attlocal.net To: Peter Xu cc: Hugh Dickins , Andrew Morton , Mike Kravetz , Mike Rapoport , "Kirill A. Shutemov" , Matthew Wilcox , David Hildenbrand , Suren Baghdasaryan , Qi Zheng , Yang Shi , Mel Gorman , Peter Zijlstra , Will Deacon , Yu Zhao , Alistair Popple , Ralph Campbell , Ira Weiny , Steven Price , SeongJae Park , Naoya Horiguchi , Christophe Leroy , Zack Rusin , Jason Gunthorpe , Axel Rasmussen , Anshuman Khandual , Pasha Tatashin , Miaohe Lin , Minchan Kim , Christoph Hellwig , Song Liu , Thomas Hellstrom , Russell King , "David S. Miller" , Michael Ellerman , "Aneesh Kumar K.V" , Heiko Carstens , Christian Borntraeger , Claudio Imbrenda , Alexander Gordeev , Jann Horn , linux-arm-kernel@lists.infradead.org, sparclinux@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH 09/12] mm/khugepaged: retract_page_tables() without mmap or vma lock In-Reply-To: Message-ID: <48c679e9-8eed-568a-1de1-c57e315c693c@google.com> References: <35e983f5-7ed3-b310-d949-9ae8b130cdab@google.com> <2e9996fa-d238-e7c-1194-834a2bd1f60@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 5748B120010 X-Stat-Signature: awudb4eynweqrjn1i1w5aue1ej3ck9jm X-HE-Tag: 1685493520-131591 X-HE-Meta: U2FsdGVkX18BgJLwKsBcTJTEQ4xj3FjKIgFBNCS6YxyLnblgvslV9lAEosrA6F5I86kOpSbu/gMtaePFfraN1VtEFj3ooB4FwxnHTclwGVhHtqE+QKB1A/4RG8jWAYe00wXVJQb10qCpCRCr24PES1Ra2UL3FAvKoUpsmHaDnN1hmMNiWIh0MAqrD+pskbP0xQkCF21z8kkUursWO61LEdDmCoHFT1K2M5x4a43vM5SQ49AVt9N5Ua4HQGKpMtz1OKsU01JxqagR3rzSjp5siWvLfsNeIgnJZRmdzfqFgaQkN2jXKuZYuz1Ma3Tcc6y3bS2Am+P3Val1+KlAkqLW9PA/oezd6QRd0LviffQnsNXjzrxHiyJ2J9MPVVDOaA0z1LnHOJx3cSAD1+XHhWDU1HPkZhIuRvZnCLhYbprFU2XgZWqs51fSkJvWYYz7+FIXpT9FyXmV13cTp5jYPwnKIGOVT+u6ftS5M3ibeqZPsPXae6fmc8a2gGXCORu30m42awvOPJLt3AYs6ZwGJPdDrrlPpMgRrufVUgMYR6M2Hoq7sEilolBh1OEbs40Snl26v/upgO6Xdy0z/VvyuGmcjuqQPGIpPX+3k63PG4r9/NDA+ed9IJzbPUfQlvtomJWqTPzH5n3OmG/kCGgP8/XqnZl9TT3jqDkJV0/TQirkPlo+MUS8yWDyXeMw03gKG7nL7O9s1eC6LfGRnbpkabArjjY+TM7Pj0yYG1oNXh/dDv/ZVZw7SUU0P6KGRABo5jQDYzi9NxcJbl4zhXr3PFPxaEibvMMv7j2iHh3d+HcHhsZGBjDsiUvW0x94HaKnKQSMRfAwO9GH04OkL4uycty32RB0X/fGio4GZ/Dy78j5im5Q4QVE0JyQ3RquoP8UDt06kBsw3bpstakVgLimAah4I0/blGNqsOWk+r+JCoqwKv0h1RiQDf8KcsLtft/grfVDNicpWy2lYEBLmYLVKTJ qvy5Oytu uHp9TYXA/VcbYbnPYWojMtx+xctLYPfRCIXPSjcg4Owh1Wmmluk5RXO53O5XyLNvcAjKKyxmqyvQXTM0hKnYzNzrfb2G734aK8PxZ1YBAMhJ9V90NxfKeqDfiE5LtfadnOLJFRBD3dKCcLFlLsMPG0OIvXw3HGu+FDfhAEU1TFtlvUP0uLL0Oz+al3+7DanU1Rz2VSvUT19Th44BfOCvHi1jARHvYivCDJEQ/17eRoIk8xFWBFBXHRSMdk3odItL/KXNw8mklLJltsrrXgYox8MuJjg7qtSotdbBaRP6evMNoHLa6ywmPQdf1+c2/OXcYHPJN/ZXERDNOptaTVC2qjmXiTxkp7wrD92g+wZDNAFqTn6KMzNP4XlckaTjwMUX4ATmtAeGmhaPRxG+TDOoIzL9872Njishccd7H 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: Thanks for looking, Peter: I was well aware of you dropping several hints that you wanted to see what's intended before passing judgment on earlier series, and I preferred to get on with showing this series, than go into detail in responses to you there - thanks for your patience :) On Mon, 29 May 2023, Peter Xu wrote: > On Sun, May 28, 2023 at 11:25:15PM -0700, Hugh Dickins wrote: ... > > @@ -1748,123 +1747,73 @@ static void khugepaged_collapse_pte_mapped_thps(struct khugepaged_mm_slot *mm_sl > > mmap_write_unlock(mm); > > } > > > > -static int retract_page_tables(struct address_space *mapping, pgoff_t pgoff, > > - struct mm_struct *target_mm, > > - unsigned long target_addr, struct page *hpage, > > - struct collapse_control *cc) > > +static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) > > { > > struct vm_area_struct *vma; > > - int target_result = SCAN_FAIL; > > > > - i_mmap_lock_write(mapping); > > + i_mmap_lock_read(mapping); > > vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) { > > - int result = SCAN_FAIL; > > - struct mm_struct *mm = NULL; > > - unsigned long addr = 0; > > - pmd_t *pmd; > > - bool is_target = false; > > + struct mm_struct *mm; > > + unsigned long addr; > > + pmd_t *pmd, pgt_pmd; > > + spinlock_t *pml; > > + spinlock_t *ptl; > > > > /* > > * Check vma->anon_vma to exclude MAP_PRIVATE mappings that > > - * got written to. These VMAs are likely not worth investing > > - * mmap_write_lock(mm) as PMD-mapping is likely to be split > > - * later. > > + * got written to. These VMAs are likely not worth removing > > + * page tables from, as PMD-mapping is likely to be split later. > > * > > - * Note that vma->anon_vma check is racy: it can be set up after > > - * the check but before we took mmap_lock by the fault path. > > - * But page lock would prevent establishing any new ptes of the > > - * page, so we are safe. > > - * > > - * An alternative would be drop the check, but check that page > > - * table is clear before calling pmdp_collapse_flush() under > > - * ptl. It has higher chance to recover THP for the VMA, but > > - * has higher cost too. It would also probably require locking > > - * the anon_vma. > > + * Note that vma->anon_vma check is racy: it can be set after > > + * the check, but page locks (with XA_RETRY_ENTRYs in holes) > > + * prevented establishing new ptes of the page. So we are safe > > + * to remove page table below, without even checking it's empty. > > */ > > - if (READ_ONCE(vma->anon_vma)) { > > - result = SCAN_PAGE_ANON; > > - goto next; > > - } > > + if (READ_ONCE(vma->anon_vma)) > > + continue; > > Not directly related to current patch, but I just realized there seems to > have similar issue as what ab0c3f1251b4 wanted to fix. > > IIUC any shmem vma that used to have uprobe/bp installed will have anon_vma > set here, then does it mean that any vma used to get debugged will never be > able to merge into a thp (with either madvise or khugepaged)? > > I think it'll only make a difference when the page cache is not huge yet > when bp was uninstalled, but then it becomes a thp candidate somehow. Even > if so, I think the anon_vma should still be there. > > Did I miss something, or maybe that's not even a problem? Finding vma->anon_vma set would discourage retract_page_tables() from doing its business with that previously uprobed area; but it does not stop collapse_pte_mapped_thp() (which uprobes unregister calls directly) from dealing with it, and MADV_COLLAPSE works on anon_vma'ed areas too. It's just a heuristic in retract_page_tables(), when it chooses to skip the anon_vma'ed areas as often not worth bothering with. As to vma merges: I haven't actually checked since the maple tree and other rewrites of vma merging, but previously one vma with anon_vma set could be merged with adjacent vma before or after without anon_vma set - the anon_vma comparison is not just equality of anon_vma, but allows NULL too - so the anon_vma will still be there, but extends to cover the wider extent. Right, I find is_mergeable_anon_vma() still following that rule. (And once vmas are merged, so that the whole of the huge page falls within a single vma, khugepaged can consider it, and do collapse_pte_mapped_thp() on it - before or after 11/12 I think.) As to whether it would even be a problem: generally no, the vma is supposed just to be an internal representation, and so long as the code resists proliferating them unnecessarily, occasional failures to merge should not matter. The one place that forever sticks in my mind as mattering (perhaps there are others I'm unaware of, but I'd call them bugs) is mremap(): which is sufficiently awkward and bug-prone already, that nobody ever had the courage to make it independent of vma boundaries; but ideally, it's mremap() that we should fix. But I may have written three answers, yet still missed your point. ... > > + > > + mm = vma->vm_mm; > > + if (find_pmd_or_thp_or_none(mm, addr, &pmd) != SCAN_SUCCEED) > > + continue; > > + > > + if (hpage_collapse_test_exit(mm)) > > + continue; > > + /* > > + * When a vma is registered with uffd-wp, we cannot recycle > > + * the page table because there may be pte markers installed. > > + * Other vmas can still have the same file mapped hugely, but > > + * skip this one: it will always be mapped in small page size > > + * for uffd-wp registered ranges. > > + * > > + * What if VM_UFFD_WP is set a moment after this check? No > > + * problem, huge page lock is still held, stopping new mappings > > + * of page which might then get replaced by pte markers: only > > + * existing markers need to be protected here. (We could check > > + * after getting ptl below, but this comment distracting there!) > > + */ > > + if (userfaultfd_wp(vma)) > > + continue; > > IIUC here with the new code we only hold (1) hpage lock, and (2) > i_mmap_lock read. Then could it possible that right after checking this > and found !UFFD_WP, but then someone quickly (1) register uffd-wp on this > vma, then UFFDIO_WRITEPROTECT to install some pte markers, before below > pgtable locks taken? > > The thing is installation of pte markers may not need either of the locks > iiuc.. > > Would taking mmap read lock help in this case? Isn't my comment above it a good enough answer? If I misunderstand the uffd-wp pte marker ("If"? certainly I don't understand it well enough, but I may or may not be too wrong about it here), and actually it can spring up in places where the page has not even been mapped yet, then I'd *much* rather just move that check down into the pte_locked area, than involve mmap read lock (which, though easier to acquire than its write lock, would I think take us back to square 1 in terms of needing trylock); but I did prefer not to have a big uffd-wp comment distracting from the code flow there. I expect now, that if I follow up UFFDIO_WRITEPROTECT, I shall indeed find it inserting pte markers where the page has not even been mapped yet. A "Yes" from you will save me looking, but probably I shall have to move that check down (oh well, the comment will be smaller there). Thanks, Hugh