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 985DAC4332F for ; Thu, 3 Nov 2022 10:45:13 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 160C76B0071; Thu, 3 Nov 2022 06:45:13 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0EB1B6B0072; Thu, 3 Nov 2022 06:45:13 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EA6716B0073; Thu, 3 Nov 2022 06:45:12 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id D57146B0071 for ; Thu, 3 Nov 2022 06:45:12 -0400 (EDT) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 9FCA71A13F7 for ; Thu, 3 Nov 2022 10:45:12 +0000 (UTC) X-FDA: 80091798864.15.4BA29E1 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf16.hostedemail.com (Postfix) with ESMTP id 4034B180006 for ; Thu, 3 Nov 2022 10:45:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1667472311; h=from:from: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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=xeIVOx9sLvuNMPr0vxXdmyPkIr4MFSUz9ymTyQBULJ4=; b=BZaJ4L4J1zsSLxAy2VnitHSOgfMoT8AN8NHzdhjhM2urdnKAYPvOT+V5Tl1pllkEdiiimQ XU27kxGSrWDzRIswWfcgyCkY7XLPwXs1u0U3cC51muuxr/q9lwr8+5O/J6O+gU2HNUMtzd pyg/pgZfFKXJxJCdvB5W2DZ2gO2eiW0= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-452-_ckiVVu1PgOd8pYzycsTEw-1; Thu, 03 Nov 2022 06:45:10 -0400 X-MC-Unique: _ckiVVu1PgOd8pYzycsTEw-1 Received: by mail-wm1-f71.google.com with SMTP id 1-20020a05600c028100b003cf7833293cso2443675wmk.3 for ; Thu, 03 Nov 2022 03:45:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:subject:organization:from :references:cc:to:content-language:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=xeIVOx9sLvuNMPr0vxXdmyPkIr4MFSUz9ymTyQBULJ4=; b=RNpABhLwhhn6CluAVkJV8DHJQr/39udLAZRkxRcTeW/2NdeKzvd3MH0cD02dF8DuKq ejyUTUy517zFLY3ThLIhPRO7pdmDQle4ozVqHpIAKOhO/g/9v2L6R2j5rbWcQKV9Bu4L fpwmDAlKS6OjEWfDG+q0FD8y0u4i7IdWQbm5NN+VQklVqNgI99C9oDpak674nrY3VOjK c0iDjI1v6R0ZLRhupPpFwQIVthmljoPOy236ZkEpZpI8Nc9YjwWqrr9PxWDu8NSmFQjy Ch2Lk+K5Lh2e383KbBuY6dTwW39TWwd/0qhES66F1nMa8KhdVI4Z0x8Op21ClGNAeE/Z kQ4Q== X-Gm-Message-State: ACrzQf2gxyyVtNtB1OhH/E3aMLb5htUHPZl19hm9qVNwJHixhM+tVkz1 okziqdx/9dbBww1eUU84VQ+nXFNAXmRxcWMHC1x6ZUMjkKhkXYYkyXNhv+389VbQRj0sgvKBT7q N/GQYAdv6oZE= X-Received: by 2002:a05:6000:15c8:b0:236:812d:d3e5 with SMTP id y8-20020a05600015c800b00236812dd3e5mr18067970wry.303.1667472309210; Thu, 03 Nov 2022 03:45:09 -0700 (PDT) X-Google-Smtp-Source: AMsMyM5VbCnR40pmKtSUyhc42THXlkmWTKWgcmaV162v3S4FqpWxW9R5nd7ZlQjR0huei6YRk+ADnQ== X-Received: by 2002:a05:6000:15c8:b0:236:812d:d3e5 with SMTP id y8-20020a05600015c800b00236812dd3e5mr18067931wry.303.1667472308829; Thu, 03 Nov 2022 03:45:08 -0700 (PDT) Received: from ?IPV6:2003:cb:c707:a400:e2d7:3ee3:8d35:ac8? (p200300cbc707a400e2d73ee38d350ac8.dip0.t-ipconnect.de. [2003:cb:c707:a400:e2d7:3ee3:8d35:ac8]) by smtp.gmail.com with ESMTPSA id o38-20020a05600c512600b003cf4eac8e80sm1166998wms.23.2022.11.03.03.45.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 03 Nov 2022 03:45:08 -0700 (PDT) Message-ID: <37192084-fc78-2d51-3bcf-1454248dcc74@redhat.com> Date: Thu, 3 Nov 2022 11:45:07 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 To: Nadav Amit Cc: kernel list , Linux-MM , linuxppc-dev , Linus Torvalds , Andrew Morton , Mel Gorman , Dave Chinner , Peter Xu , Andrea Arcangeli , Hugh Dickins , Vlastimil Babka , Michael Ellerman , Nicholas Piggin , Mike Rapoport , Anshuman Khandual References: <20221102191209.289237-1-david@redhat.com> <20221102191209.289237-5-david@redhat.com> From: David Hildenbrand Organization: Red Hat Subject: Re: [PATCH v1 4/6] mm/autonuma: use can_change_(pte|pmd)_writable() to replace savedwrite In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1667472312; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=xeIVOx9sLvuNMPr0vxXdmyPkIr4MFSUz9ymTyQBULJ4=; b=7yCmuwdX/n99iEAOSvYMl26VMKDv2+Iv+Ixd83NErsZXY/xtt+XfKWa28q/RfaOnw0DFtS KhHPqwaa81JnlWb6Uiye0EQJchLcLxV/UQh97BK4gRwFztqbtUspTBX0XNOEhXnb+8fzci yyQzNARclkdBzt8LjRqUFKqt214CO0U= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=BZaJ4L4J; spf=pass (imf16.hostedemail.com: domain of david@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=david@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1667472312; a=rsa-sha256; cv=none; b=mZMp9KG4dmBux2B9cKjnf931uvTgz8e17OUv5KO9Ql425REV8HSmLCvaxFsx8HgTJMhX1N vmpWXVXg/Z1TtsiXGoSaBnliWWDTKTsQuuHgyPW06TXFlIIFRS3GnLlv8YT9IKMBzSeSv3 TQdgVyLxHPiwBtwi7N6XMsa2p/LmilI= X-Stat-Signature: heirksqdddfo15dms9s5yr3qxefkabfz X-Rspamd-Queue-Id: 4034B180006 X-Rspamd-Server: rspam06 X-Rspam-User: Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=BZaJ4L4J; spf=pass (imf16.hostedemail.com: domain of david@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=david@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-HE-Tag: 1667472312-398175 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 02.11.22 22:22, Nadav Amit wrote: > On Nov 2, 2022, at 12:12 PM, David Hildenbrand wrote: > >> !! External Email >> >> commit b191f9b106ea ("mm: numa: preserve PTE write permissions across a >> NUMA hinting fault") added remembering write permissions using ordinary >> pte_write() for PROT_NONE mapped pages to avoid write faults when >> remapping the page !PROT_NONE on NUMA hinting faults. >> > > [ snip ] > > Here’s a very shallow reviewed with some minor points... Appreciated. > >> --- >> include/linux/mm.h | 2 ++ >> mm/huge_memory.c | 28 +++++++++++++++++----------- >> mm/ksm.c | 9 ++++----- >> mm/memory.c | 19 ++++++++++++++++--- >> mm/mprotect.c | 7 ++----- >> 5 files changed, 41 insertions(+), 24 deletions(-) >> >> diff --git a/include/linux/mm.h b/include/linux/mm.h >> index 25ff9a14a777..a0deeece5e87 100644 >> --- a/include/linux/mm.h >> +++ b/include/linux/mm.h >> @@ -1975,6 +1975,8 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma, >> #define MM_CP_UFFD_WP_ALL (MM_CP_UFFD_WP | \ >> MM_CP_UFFD_WP_RESOLVE) >> >> +bool can_change_pte_writable(struct vm_area_struct *vma, unsigned long addr, >> + pte_t pte); > > It might not be customary, but how about marking it as __pure? Right, there is no a single use of __pure in the mm domain. > >> extern unsigned long change_protection(struct mmu_gather *tlb, >> struct vm_area_struct *vma, unsigned long start, >> unsigned long end, pgprot_t newprot, >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index 2ad68e91896a..45abd27d75a0 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -1462,8 +1462,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf) >> unsigned long haddr = vmf->address & HPAGE_PMD_MASK; >> int page_nid = NUMA_NO_NODE; >> int target_nid, last_cpupid = (-1 & LAST_CPUPID_MASK); >> - bool migrated = false; >> - bool was_writable = pmd_savedwrite(oldpmd); >> + bool try_change_writable, migrated = false; >> int flags = 0; >> >> vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd); >> @@ -1472,13 +1471,22 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf) >> goto out; >> } >> >> + /* See mprotect_fixup(). */ >> + if (vma->vm_flags & VM_SHARED) >> + try_change_writable = vma_wants_writenotify(vma, vma->vm_page_prot); >> + else >> + try_change_writable = !!(vma->vm_flags & VM_WRITE); > > Do you find it better to copy the code instead of extracting it to a > separate function? Yeah, you're right ;) usually the issue is coming up with a suitable name. Let me try. vma_wants_manual_writability_change() hmm ... > >> + >> pmd = pmd_modify(oldpmd, vma->vm_page_prot); >> page = vm_normal_page_pmd(vma, haddr, pmd); >> if (!page) >> goto out_map; >> >> /* See similar comment in do_numa_page for explanation */ >> - if (!was_writable) >> + if (try_change_writable && !pmd_write(pmd) && >> + can_change_pmd_writable(vma, vmf->address, pmd)) >> + pmd = pmd_mkwrite(pmd); >> + if (!pmd_write(pmd)) >> flags |= TNF_NO_GROUP; >> >> page_nid = page_to_nid(page); >> @@ -1523,8 +1531,12 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf) >> /* Restore the PMD */ >> pmd = pmd_modify(oldpmd, vma->vm_page_prot); >> pmd = pmd_mkyoung(pmd); >> - if (was_writable) >> + >> + /* Similar to mprotect() protection updates, avoid write faults. */ >> + if (try_change_writable && !pmd_write(pmd) && >> + can_change_pmd_writable(vma, vmf->address, pmd)) > > Why do I have a deja-vu? :) > > There must be a way to avoid the redundant code and specifically the call to > can_change_pmd_writable(), no? The issue is that as soon as we drop the page table lock, that information is stale. Especially, after we fail migration. So the following should work, however, if we fail migration we wouldn't map the page writable and would have to re-calculate: diff --git a/mm/memory.c b/mm/memory.c index c5599a9279b1..a997625641e4 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -4674,10 +4674,10 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf) struct vm_area_struct *vma = vmf->vma; struct page *page = NULL; int page_nid = NUMA_NO_NODE; + bool writable = false; int last_cpupid; int target_nid; pte_t pte, old_pte; - bool was_writable = pte_savedwrite(vmf->orig_pte); int flags = 0; /* @@ -4696,6 +4696,17 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf) old_pte = ptep_get(vmf->pte); pte = pte_modify(old_pte, vma->vm_page_prot); + /* + * Detect now whether the PTE is or can be writable. Note that this + * information is valid as long as we're holding the PT lock, so also on + * the remap path below. + */ + writable = pte_write(pte); + if (!writable && vma_wants_manual_writability_change(vma) && + can_change_pte_writable(vma, vmf->address, pte); + writable = true; + } + page = vm_normal_page(vma, vmf->address, pte); if (!page || is_zone_device_page(page)) goto out_map; @@ -4712,7 +4723,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf) * pte_dirty has unpredictable behaviour between PTE scan updates, * background writeback, dirty balancing and application behaviour. */ - if (!was_writable) + if (!writable) flags |= TNF_NO_GROUP; /* @@ -4738,6 +4749,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf) put_page(page); goto out_map; } + writable = false; pte_unmap_unlock(vmf->pte, vmf->ptl); /* Migrate to the requested node */ @@ -4767,7 +4779,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf) old_pte = ptep_modify_prot_start(vma, vmf->address, vmf->pte); pte = pte_modify(old_pte, vma->vm_page_prot); pte = pte_mkyoung(pte); - if (was_writable) + if (writable) pte = pte_mkwrite(pte); ptep_modify_prot_commit(vma, vmf->address, vmf->pte, old_pte, pte); update_mmu_cache(vma, vmf->address, vmf->pte); To me, the less error-prone approach is to re-calculate. [...] >> --- a/mm/ksm.c >> +++ b/mm/ksm.c >> @@ -1069,7 +1069,6 @@ static int write_protect_page(struct vm_area_struct *vma, struct page *page, >> >> anon_exclusive = PageAnonExclusive(page); >> if (pte_write(*pvmw.pte) || pte_dirty(*pvmw.pte) || >> - (pte_protnone(*pvmw.pte) && pte_savedwrite(*pvmw.pte)) || > > Not related to your code, but it does not make me comfortable that PTE’s > status bits (which are volatile) are not accessed in this manner. > > Especially since the PTE is later saved into orig_pte. It would feel safer > to do READ_ONCE(*pvmw.pte) and work on it (probably in a separate patch). I assume you are talking about the dirty bit. I don't immediately see how something could go wrong here, but I agree that it might look cleaner that way. Anyhow, independent of this series, so I'll leave that alone for now but add a note for the future. -- Thanks, David / dhildenb