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 203C0C4345F for ; Tue, 16 Apr 2024 02:36:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7F8A46B0087; Mon, 15 Apr 2024 22:36:22 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7A8C86B0088; Mon, 15 Apr 2024 22:36:22 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6491D6B0089; Mon, 15 Apr 2024 22:36:22 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 47B486B0087 for ; Mon, 15 Apr 2024 22:36:22 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id EE354A08D2 for ; Tue, 16 Apr 2024 02:36:21 +0000 (UTC) X-FDA: 82013830962.05.A2071EB Received: from mail-vk1-f176.google.com (mail-vk1-f176.google.com [209.85.221.176]) by imf15.hostedemail.com (Postfix) with ESMTP id 33FF7A000D for ; Tue, 16 Apr 2024 02:36:20 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="HThDRVU/"; spf=pass (imf15.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.176 as permitted sender) smtp.mailfrom=21cnbao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713234980; a=rsa-sha256; cv=none; b=DEERimppcWlj09IQz0dckN4B5rcAki3yVCfhUcuf0IKOEHWhZWJV9MzJDwMlp7t4AhkNV3 BWy1zqeAjH0gGrOW4Ftg8XvSurFu+5L7pz6LNHwdZfg/ehZKpUHmc/RD/MWpC69al39P6M QTD/nMxcHS+29RJ7ZNsHtWW2s//qJsQ= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="HThDRVU/"; spf=pass (imf15.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.176 as permitted sender) smtp.mailfrom=21cnbao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713234980; 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=Zq4U0Tp6kyjk4hccTjIpN9kg7kEYO/hCNjlkdetKms0=; b=CNOIGMCi6jQKRqEzK2N79ZfwnTZ8fWu3mN/mEv/VN6tP33yIKPaOGnfxYL284/Ek0pe2GS xcPcsKdJZlwVNbwP48o4SiUBygvmMCvaJfTf8fHiiwj4OXLp9wZaEekrNUkrgU23PiDv2A DdogRogermb/CZyEq5p1LnpMB079v6k= Received: by mail-vk1-f176.google.com with SMTP id 71dfb90a1353d-4dacb2ad01dso1690739e0c.3 for ; Mon, 15 Apr 2024 19:36:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1713234979; x=1713839779; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=Zq4U0Tp6kyjk4hccTjIpN9kg7kEYO/hCNjlkdetKms0=; b=HThDRVU/OTd8qXKbeZb2MLh0lxfHHj2sThF6zCYZVdnDV1XBBkcx8WX/ex5YihzE0g p47bj4LTc7T7QJa1SQ+ad8du3p59u+J1/Eky05vgaz6KGVtHIOxwoq01Y497z6I6EseA jfMmG3Xhwz4JX3+RLXLUSe35s+85LIgTUJhZPxqsoYdiPbTXKpU29SZ+mHWNQM05hQrl xzknbtlzn81HEI4GaT/zsQ65vQyJdcvC/Be8P4KgfYaiAv2+skVA28yQpeJtzpnwQkqV 0uE2aA/AHcnR/7T204qjDDhwBFVN9cJyGbUvcK9J4Wb0KKxQuqHrkDIasQI2J17puLxq KWgg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713234979; x=1713839779; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Zq4U0Tp6kyjk4hccTjIpN9kg7kEYO/hCNjlkdetKms0=; b=QCAxpSIB1wATOv4ZLLNwL7gffNMXdJLb03X6sIiI8I60+nmySx0dz2Quv5wUvqZ895 ldLTjXJ+O9zO2syY+WUGaoYbR/2MXk/ZIuzww7943O/FN1UrsxldF0XfFZRL/L7alkHO VvXqLofTPPmTE8PiCtTx5KNOWXsONIijbbn1C4MDFV7WBpdoPOdophSMRnPkpI+FYWak iS8f7DB5SiYn9dqfJ8qpL4HgBqsKj3W+F0OAYnoKBpiAMgqQU/bnU9JA6RuflXiZ6Lt4 7HaxfXvhFZZL5Ap1rOxCKW4qjOAbnSBJfEkYPpEa7Krxuhbf2GWarJ0svKQ6s1tJCwCM WoBg== X-Forwarded-Encrypted: i=1; AJvYcCUxStb3uUxfW9OLi2iGdM6bKwZxLfVTvMVMepCRqG843jiwMBEh23oPZCmGooKyNVMXieuJLPIrmc1j8skCTUEu+dY= X-Gm-Message-State: AOJu0YxCprJjaFkIL5TRTB4gYKijV98Rp6vsyoY/B7ir6QZ17JzmpzYx WZheACSl1bt/cLZ7pUgyxb7x+icWPWbDNHK62lvzPYJLrtHuGlr16yO+2CMllRI0VRNCZAofp9h Xl4NAjqYOmT9BipzCYdq9Azfd9zo= X-Google-Smtp-Source: AGHT+IEp3nz2cb2A/wAMV4V2iVSMB5XIwdoWzJxBFCKJc0T4AW2hkoolYglVM+sLPe+0iIb/mwjdTPSIKv7T9Z06+u0= X-Received: by 2002:a05:6122:d8e:b0:4d8:74cb:e3c2 with SMTP id bc14-20020a0561220d8e00b004d874cbe3c2mr9379297vkb.9.1713234979213; Mon, 15 Apr 2024 19:36:19 -0700 (PDT) MIME-Version: 1.0 References: <20240409082631.187483-1-21cnbao@gmail.com> <20240409082631.187483-5-21cnbao@gmail.com> <87frvn2f89.fsf@yhuang6-desk2.ccr.corp.intel.com> <8734rm2gdj.fsf@yhuang6-desk2.ccr.corp.intel.com> In-Reply-To: <8734rm2gdj.fsf@yhuang6-desk2.ccr.corp.intel.com> From: Barry Song <21cnbao@gmail.com> Date: Tue, 16 Apr 2024 14:36:08 +1200 Message-ID: Subject: Re: [PATCH v2 4/5] mm: swap: entirely map large folios found in swapcache To: "Huang, Ying" Cc: Khalid Aziz , akpm@linux-foundation.org, linux-mm@kvack.org, baolin.wang@linux.alibaba.com, chrisl@kernel.org, david@redhat.com, hanchuanhua@oppo.com, hannes@cmpxchg.org, hughd@google.com, kasong@tencent.com, ryan.roberts@arm.com, surenb@google.com, v-songbaohua@oppo.com, willy@infradead.org, xiang@kernel.org, yosryahmed@google.com, yuzhao@google.com, ziy@nvidia.com, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 33FF7A000D X-Stat-Signature: 9dz14nu63g55zi9dmcehaa5t6wwztxst X-Rspam-User: X-HE-Tag: 1713234980-574167 X-HE-Meta: U2FsdGVkX1/q7Y6fE1XK1K5yKKdkgiBVzp5I9yOoEdh5qCawz/xaidi2fO/gjcBX+UT7DypbHlrW2nawSnxiWkylVRhp9fNhEYMZCUTZ5tbPqgDRXwxUP7/7GtMNLdCZhrQppx/1MVUa3RBgUHYVCror0y4H29gXiYFrMoWffRXa2gtasiPtO6QEbnXjmBkJCNoN+EJGrfr9ZyCHe8bs4Cyx9iRUN8fNkF5rCPcZvKm2fvydmjbmfMIJyT/OCnGH7xEgzqhaNsQlw92szmf5Sn1/IwshIeS0cDxGNlnKug2ar86zeNBLX+pYfts3bbeudrF+YZquQVSQeFNkGP7AYHdqACTcsg5FNfYhEfgn2pDfeoH1vFREji5DKmD+Vip7OjLvz6RrPg9IcOnyGwyZPr0L/naA8k1qd+bxeqP116yw5GuJPhuOemGu7RpPHGGjFgSnxmeKDw91ChZJT9z5smCS9zw5103ETElb4pkAuhfMH7R8NNAcbvE8WJo6R/EsFNGlLavO9yQ+IBMOG57cvwSDFga0uuVMOW+yR48ncSmmu/Nx1eP7QDQleHgY/K0SDFyxkLvLEDQzakZbfdMBf1AlQP8TEFyOE5XA2FCj9jgzyVEWtk8dcSUW6vtSCaM7qHmzNiV92IeIbrUJE2p4SX/yvG4Xg+ErVl5Xk2qJnDhi08Im9UMRV8Z6+6Uvm5dJZKX653Jm68ovS3jH1U8JtXHjuUI+79mG6EeSf3Oc746Tc0wXQT9cv0aoXHwVfEkkStdXNuOEwgqxWKuwROPLLMzTHRwImehpdtuisOBLbCG51LJ6b4a9h8+d3AHjSsNJMlal5kJ0xxVuKnlgrlzla3mlDmjuaPWkNmTvNw9yuMLvQlqK0J0OnTH0R6GKQObPL1UF49PS3DoOEfzzBR65Jpa9E1aoE4A2ZOVRNLYZcPNPOI1tdyztTjdnWLF6D/V3suwtvoXx8XIRLuXz4WC jAVwcQ9g ifUY4FcJJOSmXYLP01MW0hFtjRCS9omH2KUILfE8VugDPPeLRl7tR/reVWOV1kCg3dw0o2NgEyvaSFgovAj2tETWO89zGx5fnvsEWMN51sImF6qhl/TtNj7cYh8WX5wwA5fSfPBmVs6UWcAvjEftRp1SgE8jtg1EUrwOE/oVwxQJl1tFn7wr+t2ih8V1oxQyfpWZ6zOlcpp1i8NPNNV91XTk8XCoDcCKGI5rSa6GKXeU0T7/enLMWq+qGc5DJYe5Vk+3HL6lq0Yljl4I5XGQaYmiGFhDgDIwRFa8HYiI662Qw5zA0K/vVToS6f8XCRx+Hs8HDg17JZzIOzWl1bYza/u5pA6VyUnzhPx0OJyk/vi2mzgEi9o4ckEq++DlDrM1/uYNFOcTyJtpw8BEJFD87xSrEHA== 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: List-Subscribe: List-Unsubscribe: On Tue, Apr 16, 2024 at 2:27=E2=80=AFPM Huang, Ying = wrote: > > > Added Khalid for arch_do_swap_page(). > > Barry Song <21cnbao@gmail.com> writes: > > > On Mon, Apr 15, 2024 at 8:39=E2=80=AFPM Huang, Ying wrote: > >> > >> Barry Song <21cnbao@gmail.com> writes: > > [snip] > > >> > >> > + bool any_swap_shared =3D false; > >> > > >> > if (!pte_unmap_same(vmf)) > >> > goto out; > >> > @@ -4137,6 +4141,35 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > >> > */ > >> > vmf->pte =3D pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->ad= dress, > >> > &vmf->ptl); > >> > >> We should move pte check here. That is, > >> > >> if (unlikely(!vmf->pte || !pte_same(ptep_get(vmf->pte), vmf->o= rig_pte))) > >> goto out_nomap; > >> > >> This will simplify the situation for large folio. > > > > the plan is moving the whole code block > > > > if (start_pte && folio_test_large(folio) && folio_test_swapcache(folio)= ) > > > > after > > if (unlikely(!folio_test_uptodate(folio))) { > > ret =3D VM_FAULT_SIGBUS; > > goto out_nomap; > > } > > > > though we couldn't be !folio_test_uptodate(folio)) for hitting > > swapcache but it seems > > logically better for future use. > > LGTM, Thanks! > > >> > >> > + > >> > + /* We hit large folios in swapcache */ > >> > >> The comments seems unnecessary because the code tells that already. > >> > >> > + if (start_pte && folio_test_large(folio) && folio_test_swapcac= he(folio)) { > >> > + int nr =3D folio_nr_pages(folio); > >> > + int idx =3D folio_page_idx(folio, page); > >> > + unsigned long folio_start =3D vmf->address - idx * PAG= E_SIZE; > >> > + unsigned long folio_end =3D folio_start + nr * PAGE_SI= ZE; > >> > + pte_t *folio_ptep; > >> > + pte_t folio_pte; > >> > + > >> > + if (unlikely(folio_start < max(vmf->address & PMD_MASK= , vma->vm_start))) > >> > + goto check_pte; > >> > + if (unlikely(folio_end > pmd_addr_end(vmf->address, vm= a->vm_end))) > >> > + goto check_pte; > >> > + > >> > + folio_ptep =3D vmf->pte - idx; > >> > + folio_pte =3D ptep_get(folio_ptep); > >> > >> It's better to construct pte based on fault PTE via generalizing > >> pte_next_swp_offset() (may be pte_move_swp_offset()). Then we can fin= d > >> inconsistent PTEs quicker. > > > > it seems your point is getting the pte of page0 by pte_next_swp_offset(= ) > > unfortunately pte_next_swp_offset can't go back. on the other hand, > > we have to check the real pte value of the 0nd entry right now because > > swap_pte_batch() only really reads pte from the 1st entry. it assumes > > pte argument is the real value for the 0nd pte entry. > > > > static inline int swap_pte_batch(pte_t *start_ptep, int max_nr, pte_t p= te) > > { > > pte_t expected_pte =3D pte_next_swp_offset(pte); > > const pte_t *end_ptep =3D start_ptep + max_nr; > > pte_t *ptep =3D start_ptep + 1; > > > > VM_WARN_ON(max_nr < 1); > > VM_WARN_ON(!is_swap_pte(pte)); > > VM_WARN_ON(non_swap_entry(pte_to_swp_entry(pte))); > > > > while (ptep < end_ptep) { > > pte =3D ptep_get(ptep); > > > > if (!pte_same(pte, expected_pte)) > > break; > > > > expected_pte =3D pte_next_swp_offset(expected_pte); > > ptep++; > > } > > > > return ptep - start_ptep; > > } > > Yes. You are right. > > But we may check whether the pte of page0 is same as "vmf->orig_pte - > folio_page_idx()" (fake code). right, that is why we are reading and checking PTE0 before calling swap_pte_batch() right now. folio_ptep =3D vmf->pte - idx; folio_pte =3D ptep_get(folio_ptep); if (!is_swap_pte(folio_pte) || non_swap_entry(pte_to_swp_entry(folio_pte)= ) || swap_pte_batch(folio_ptep, nr, folio_pte, &any_swap_shared) !=3D nr) goto check_pte; So, if I understand correctly, you're proposing that we should directly che= ck PTE0 in swap_pte_batch(). Personally, I don't have any objections to this i= dea. However, I'd also like to hear the feedback from Ryan and David :-) > > You need to check the pte of page 0 anyway. > > >> > >> > + if (!is_swap_pte(folio_pte) || non_swap_entry(pte_to_s= wp_entry(folio_pte)) || > >> > + swap_pte_batch(folio_ptep, nr, folio_pte, &any_swa= p_shared) !=3D nr) > >> > + goto check_pte; > >> > + > >> > + start_address =3D folio_start; > >> > + start_pte =3D folio_ptep; > >> > + nr_pages =3D nr; > >> > + entry =3D folio->swap; > >> > + page =3D &folio->page; > >> > + } > >> > + > >> > +check_pte: > >> > if (unlikely(!vmf->pte || !pte_same(ptep_get(vmf->pte), vmf->o= rig_pte))) > >> > goto out_nomap; > >> > > >> > @@ -4190,6 +4223,10 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > >> > */ > >> > exclusive =3D false; > >> > } > >> > + > >> > + /* Reuse the whole large folio iff all entries are exc= lusive */ > >> > + if (nr_pages > 1 && any_swap_shared) > >> > + exclusive =3D false; > >> > } > >> > > >> > /* > >> > @@ -4204,12 +4241,14 @@ vm_fault_t do_swap_page(struct vm_fault *vmf= ) > >> > * We're already holding a reference on the page but haven't m= apped it > >> > * yet. > >> > */ > >> > - swap_free(entry); > >> > + swap_free_nr(entry, nr_pages); > >> > if (should_try_to_free_swap(folio, vma, vmf->flags)) > >> > folio_free_swap(folio); > >> > > >> > - inc_mm_counter(vma->vm_mm, MM_ANONPAGES); > >> > - dec_mm_counter(vma->vm_mm, MM_SWAPENTS); > >> > + folio_ref_add(folio, nr_pages - 1); > >> > + add_mm_counter(vma->vm_mm, MM_ANONPAGES, nr_pages); > >> > + add_mm_counter(vma->vm_mm, MM_SWAPENTS, -nr_pages); > >> > + > >> > pte =3D mk_pte(page, vma->vm_page_prot); > >> > > >> > /* > >> > @@ -4219,33 +4258,34 @@ vm_fault_t do_swap_page(struct vm_fault *vmf= ) > >> > * exclusivity. > >> > */ > >> > if (!folio_test_ksm(folio) && > >> > - (exclusive || folio_ref_count(folio) =3D=3D 1)) { > >> > + (exclusive || (folio_ref_count(folio) =3D=3D nr_pages && > >> > + folio_nr_pages(folio) =3D=3D nr_pages))) { > >> > if (vmf->flags & FAULT_FLAG_WRITE) { > >> > pte =3D maybe_mkwrite(pte_mkdirty(pte), vma); > >> > vmf->flags &=3D ~FAULT_FLAG_WRITE; > >> > } > >> > rmap_flags |=3D RMAP_EXCLUSIVE; > >> > } > >> > - flush_icache_page(vma, page); > >> > + flush_icache_pages(vma, page, nr_pages); > >> > if (pte_swp_soft_dirty(vmf->orig_pte)) > >> > pte =3D pte_mksoft_dirty(pte); > >> > if (pte_swp_uffd_wp(vmf->orig_pte)) > >> > pte =3D pte_mkuffd_wp(pte); > >> > - vmf->orig_pte =3D pte; > >> > > >> > /* ksm created a completely new copy */ > >> > if (unlikely(folio !=3D swapcache && swapcache)) { > >> > - folio_add_new_anon_rmap(folio, vma, vmf->address); > >> > + folio_add_new_anon_rmap(folio, vma, start_address); > >> > folio_add_lru_vma(folio, vma); > >> > } else { > >> > - folio_add_anon_rmap_pte(folio, page, vma, vmf->address= , > >> > - rmap_flags); > >> > + folio_add_anon_rmap_ptes(folio, page, nr_pages, vma, s= tart_address, > >> > + rmap_flags); > >> > } > >> > > >> > VM_BUG_ON(!folio_test_anon(folio) || > >> > (pte_write(pte) && !PageAnonExclusive(page))); > >> > - set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte); > >> > - arch_do_swap_page(vma->vm_mm, vma, vmf->address, pte, vmf->ori= g_pte); > >> > + set_ptes(vma->vm_mm, start_address, start_pte, pte, nr_pages); > >> > + vmf->orig_pte =3D ptep_get(vmf->pte); > >> > + arch_do_swap_page(vma->vm_mm, vma, start_address, pte, pte); > >> > >> Do we need to call arch_do_swap_page() for each subpage? IIUC, the > >> corresponding arch_unmap_one() will be called for each subpage. > > > > i actually thought about this very carefully, right now, the only one w= ho > > needs this is sparc and it doesn't support THP_SWAPOUT at all. and > > there is no proof doing restoration one by one won't really break sparc= . > > so i'd like to defer this to when sparc really needs THP_SWAPOUT. > > Let's ask SPARC developer (Cced) for this. > > IMHO, even if we cannot get help, we need to change code with our > understanding instead of deferring it. ok. Thanks for Ccing sparc developers. > > > on the other hand, it seems really bad we have both > > arch_swap_restore - for this, arm64 has moved to using folio > > and > > arch_do_swap_page > > > > we should somehow unify them later if sparc wants THP_SWPOUT. > > > >> > >> > folio_unlock(folio); > >> > if (folio !=3D swapcache && swapcache) { > >> > @@ -4269,7 +4309,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > >> > } > >> > > >> > /* No need to invalidate - it was non-present before */ > >> > - update_mmu_cache_range(vmf, vma, vmf->address, vmf->pte, 1); > >> > + update_mmu_cache_range(vmf, vma, start_address, start_pte, nr_= pages); > >> > unlock: > >> > if (vmf->pte) > >> > pte_unmap_unlock(vmf->pte, vmf->ptl); > > -- > Best Regards, > Huang, Ying Thanks Barry