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 BF4B7EB64D9 for ; Tue, 27 Jun 2023 16:07:04 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2D1728E0001; Tue, 27 Jun 2023 12:07:04 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 25AE08D0001; Tue, 27 Jun 2023 12:07:04 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0D4CA8E0001; Tue, 27 Jun 2023 12:07:04 -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 EEFE78D0001 for ; Tue, 27 Jun 2023 12:07:03 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id BC4F4C0A20 for ; Tue, 27 Jun 2023 16:07:03 +0000 (UTC) X-FDA: 80949006726.21.8F3EEF9 Received: from mail-pf1-f173.google.com (mail-pf1-f173.google.com [209.85.210.173]) by imf21.hostedemail.com (Postfix) with ESMTP id 722191C0276 for ; Tue, 27 Jun 2023 16:05:45 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=GjHZ1FPL; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf21.hostedemail.com: domain of surenb@google.com designates 209.85.210.173 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1687881945; 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=q26ddN6EaiG+6pUfPDCx1Slx/RD6peq9xPMgzmc1DZ0=; b=atZYe9z93rr6fRP8DWM7RmmBefeLIwv7ioYI/gxYuhqLeb3+nyL9w76HoqyBfKqr9VgkrU SKXr7GU0kn7coMMwDzP187Ftn5I10v7e3czQk7eI8vx6Bi1MDdE8H4F/OJmV61NkmRSlTu 5GBhx2JoYAzEPaEd1LDOWZOc98ml9dU= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=GjHZ1FPL; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf21.hostedemail.com: domain of surenb@google.com designates 209.85.210.173 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1687881945; a=rsa-sha256; cv=none; b=KyqVQ+uieoSHlt/D8GamJkkAiUrFOIBjexgSvg5/lM+vEKmuXaikJaZxDYaqOI/dN5Tn+r tnc/ajEewV58eOi9LOu6CUsQl15YUZn2mBsWb1Q/BQnUhlV9giO8HoHQYF9zUvnKnOlUH1 X2Qnv18bAlx7hP++Hb/C+v0TUEU+3oA= Received: by mail-pf1-f173.google.com with SMTP id d2e1a72fcca58-666eef03ebdso2604962b3a.1 for ; Tue, 27 Jun 2023 09:05:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1687881944; x=1690473944; 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=q26ddN6EaiG+6pUfPDCx1Slx/RD6peq9xPMgzmc1DZ0=; b=GjHZ1FPL4Uf5eOs8kydNy5ZtRjtHSLXq4QEouhq/SzL/NIX/VYDhx2RV5DZs3iVInm 3l/6uAlkL6DIwiurz1xhs1cnC2fUZnsGFoxg6boimRtdXBRh1ObuA2j9AjK6dmkiu87G DlrF/l3ezbzmGpdqLW8zujMOqsy/0bCV0BgEhxViprPgs0TZT3u7T2Mfv/J8SkJULIKq LYeEKM5YertQUMrf4P2rcVHxDM7XOLvPqR3ci14/Z+TLxf9yoZlotgXdnKuooHOZ1Uaw 89dzVGLYaJtIPecoAmy6jYXmQBIPSQwHj3O6s9HoVghYcej8/RP0Cwy2VvRmCpd8TrRE IQ1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687881944; x=1690473944; 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=q26ddN6EaiG+6pUfPDCx1Slx/RD6peq9xPMgzmc1DZ0=; b=cgOCs36hLUnuZl5WRGMZT6Psp3cbBeYzkNi9j1B0h7UFBt9RRHEbm0yAwJsZhjO7qC TwiSSx3kTxi92obCeq1in2ev9AZzxQIUcV3UaM5akrJrfbQ1R2ATrQiDmlA2n1uBi8Qo 2YggBBHEmtnXZFfJGIlONFKPpb/6R7z/pl163vu+5K0bxNzQt/Quc7YSX1E7IO8f+W2c nUJ9sZ+TRinYyL+r2W+5GBs7M+Wg+m1e6yjjAQNBrnquwPvWh97CCkNlHym80euWHP0A 33Wocj3v2q/LBc0zscd19z5jiItrnjD6Kfsa52UB5Gy1yUGfacfa0rD+ncbbcqfJ1L0D jAdw== X-Gm-Message-State: AC+VfDwA3QwU4ACZiMUGT5EExKrpgSAsfPkrxCVnzUsL9Kqb607vCXAC q3u47ogQGfk/3Au8bCHPtWHYIvRWb9yT1XdZEJtQVA== X-Google-Smtp-Source: ACHHUZ7qNIqz25NqhtZvximqJ6qWhiZHpcocQ+RiSDrTYZkluHctdAXLi6rX/cQ8We3t7qgOBbM6dTOpUrhhLyaiYk0= X-Received: by 2002:a05:6a20:4286:b0:125:9d7f:3c03 with SMTP id o6-20020a056a20428600b001259d7f3c03mr11721657pzj.23.1687881943704; Tue, 27 Jun 2023 09:05:43 -0700 (PDT) MIME-Version: 1.0 References: <20230627042321.1763765-1-surenb@google.com> <20230627042321.1763765-7-surenb@google.com> In-Reply-To: From: Suren Baghdasaryan Date: Tue, 27 Jun 2023 09:05:32 -0700 Message-ID: Subject: Re: [PATCH v3 6/8] mm: handle swap page faults under per-VMA lock To: Peter Xu Cc: akpm@linux-foundation.org, willy@infradead.org, hannes@cmpxchg.org, mhocko@suse.com, josef@toxicpanda.com, jack@suse.cz, ldufour@linux.ibm.com, laurent.dufour@fr.ibm.com, michel@lespinasse.org, liam.howlett@oracle.com, jglisse@google.com, vbabka@suse.cz, minchan@google.com, dave@stgolabs.net, punit.agrawal@bytedance.com, lstoakes@gmail.com, hdanton@sina.com, apopple@nvidia.com, ying.huang@intel.com, david@redhat.com, yuzhao@google.com, dhowells@redhat.com, hughd@google.com, viro@zeniv.linux.org.uk, brauner@kernel.org, pasha.tatashin@soleen.com, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@android.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: rtbk8w8djn9pfq8b16zpbn7x6oayxr6y X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 722191C0276 X-HE-Tag: 1687881945-835082 X-HE-Meta: U2FsdGVkX19wVaEznCLBtkx1MqKjtF6rl5ldOpwlgYj7TuA6Fxj5FORNrOvj9bQ7nrMQ0WQxKN9vqgps1hF+P4Yyuo/5OluiZnlvif+LgWfHP9gs+EFln05IQePR8tqmuob/2tJ8sDIpxQCeI8Uzj2Q4jvj8uNssuvgxeosCjxfTaBM8MfY1NmOgutM+p8CK802VwFjOs0Q5tuKya+IM/007Myjfx0yV/zVs8gtkF79EqmwIP30ZJ98BkFH9JkGc2LSdVQKdy2QQ731rSr3uWfNkcYh8oDoKaKH9VHAjvn5GXoaKurdUv9z9G36PL2sqPh6uixJOCBG5HxB17AG8rh5E2JXtAjmOew7+y9gdg3p1CBukcWBQVVyR2SztaumPTBDWdrRZEpY0miksLAOcj9fPx9v4wSSlfBioIbbNHPW20VfbJ6EjL1OcplEtXlNDczZKknYDqDqHYFxlp8MzTHICj5CT5vNuKdTOlOpzit1zEiTlyfhM03ZOhDvjQlrDkPzhelSwvTHnd+ZZi9EEBJe54Wkb5lvvai+ctZAcFOh2Y3gzEWB36CHvsdg2am8l2iY7Yon4n3+aA1nLoI1/0f0kfKiWYYESgQKw1incDxQE/nidZgkQ51Wn+xwRt0iBb76AFeg5WLlweOXkq0ebcxOUgyo1BLSIXZui1mAij6PkopiJ/d34N6wcXK8oSwHOnGb7W77mR0lKotYd4i0uMiAPda7lHf4Inqc1Pp5D6M5q5LFiMNcC5IdRgsMOwGLP+OQvM0kqzaI36wVVHmDAgkbk1aGUgz86H3W7OGo2aYnNLSjL/ioCSAixeZrUITZmzJz6cGzkkNTASo16Sk6oj3QKHa4yCChm1n0SmvGRSHMvHVHu7FnPhtw3pjXZ3wtPufIPAuZiGv1Vk9XY1AE53j0AEHqTlPXhUz76ZzD8YVQ0+4QXcQEzYs0A5yoTFflAmvgyTABRBLRWgWVulUQ UI4dXdW1 oO9A2Zss+gSElcvrcobhCTBgojyi4+EhN1OHh1vr0IgJX0IlrM1bQolx3n69Ncp5B/NmS9VLS6Lp92RDhruVpRoRa0c8SEpenDUVps6GBxZqKu3qnOUN24jt6y4zFrA2lX/7kYTScd+bef2M9/ZF0LMwXyaKeaG+RD0Eeoy3GrOIYi9LkYVGNJEdyrsd1TCeKGMCE2PGpeKuOMTi79Y+Nt/78eQn+2ZN1XvBhDfrPaN0gHt6nqosRFDJVLDsLAdH8TgwgWq4GfhKWfIJwTtKjm5TWTQ== 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 Tue, Jun 27, 2023 at 8:41=E2=80=AFAM Peter Xu wrote: > > On Mon, Jun 26, 2023 at 09:23:19PM -0700, Suren Baghdasaryan wrote: > > When page fault is handled under per-VMA lock protection, all swap page > > faults are retried with mmap_lock because folio_lock_fault (formerly > > known as folio_lock_or_retry) had to drop and reacquire mmap_lock > > if folio could not be immediately locked. > > Follow the same pattern as mmap_lock to drop per-VMA lock when waiting > > for folio in folio_lock_fault and retrying once folio is available. > > With this obstacle removed, enable do_swap_page to operate under > > per-VMA lock protection. Drivers implementing ops->migrate_to_ram might > > still rely on mmap_lock, therefore we have to fall back to mmap_lock in > > that particular case. > > Note that the only time do_swap_page calls synchronous swap_readpage > > is when SWP_SYNCHRONOUS_IO is set, which is only set for > > QUEUE_FLAG_SYNCHRONOUS devices: brd, zram and nvdimms (both btt and > > pmem). Therefore we don't sleep in this path, and there's no need to > > drop the mmap or per-VMA lock. > > > > Signed-off-by: Suren Baghdasaryan > > --- > > mm/filemap.c | 24 ++++++++++++++++-------- > > mm/memory.c | 21 ++++++++++++++------- > > 2 files changed, 30 insertions(+), 15 deletions(-) > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > index 8ad06d69895b..683f11f244cd 100644 > > --- a/mm/filemap.c > > +++ b/mm/filemap.c > > @@ -1703,12 +1703,14 @@ static int __folio_lock_async(struct folio *fol= io, struct wait_page_queue *wait) > > * Return values: > > * 0 - folio is locked. > > * VM_FAULT_RETRY - folio is not locked. > > - * mmap_lock has been released (mmap_read_unlock(), unless flags h= ad both > > - * FAULT_FLAG_ALLOW_RETRY and FAULT_FLAG_RETRY_NOWAIT set, in > > - * which case mmap_lock is still held. > > + * FAULT_FLAG_LOCK_DROPPED bit in vmf flags will be set if mmap_lo= ck or > > This "FAULT_FLAG_LOCK_DROPPED" should belong to that patch when introduce= d. > But again I still think this flag as a whole with that patch is not neede= d > and should be dropped, unless I miss something important.. > > > + * per-VMA lock got dropped. mmap_lock/per-VMA lock is dropped whe= n > > + * function fails to lock the folio, unless flags had both > > + * FAULT_FLAG_ALLOW_RETRY and FAULT_FLAG_RETRY_NOWAIT set, in whic= h case > > + * the lock is still held. > > * > > * If neither ALLOW_RETRY nor KILLABLE are set, will always return 0 > > - * with the folio locked and the mmap_lock unperturbed. > > + * with the folio locked and the mmap_lock/per-VMA lock unperturbed. > > */ > > vm_fault_t __folio_lock_fault(struct folio *folio, struct vm_fault *vm= f) > > { > > @@ -1716,13 +1718,16 @@ vm_fault_t __folio_lock_fault(struct folio *fol= io, struct vm_fault *vmf) > > > > if (fault_flag_allow_retry_first(vmf->flags)) { > > /* > > - * CAUTION! In this case, mmap_lock is not released > > - * even though return VM_FAULT_RETRY. > > + * CAUTION! In this case, mmap_lock/per-VMA lock is not > > + * released even though returning VM_FAULT_RETRY. > > */ > > if (vmf->flags & FAULT_FLAG_RETRY_NOWAIT) > > return VM_FAULT_RETRY; > > > > - mmap_read_unlock(mm); > > + if (vmf->flags & FAULT_FLAG_VMA_LOCK) > > + vma_end_read(vmf->vma); > > + else > > + mmap_read_unlock(mm); > > vmf->flags |=3D FAULT_FLAG_LOCK_DROPPED; > > if (vmf->flags & FAULT_FLAG_KILLABLE) > > folio_wait_locked_killable(folio); > > @@ -1735,7 +1740,10 @@ vm_fault_t __folio_lock_fault(struct folio *foli= o, struct vm_fault *vmf) > > > > ret =3D __folio_lock_killable(folio); > > if (ret) { > > - mmap_read_unlock(mm); > > + if (vmf->flags & FAULT_FLAG_VMA_LOCK) > > + vma_end_read(vmf->vma); > > + else > > + mmap_read_unlock(mm); > > vmf->flags |=3D FAULT_FLAG_LOCK_DROPPED; > > return VM_FAULT_RETRY; > > } > > diff --git a/mm/memory.c b/mm/memory.c > > index 3c2acafcd7b6..5caaa4c66ea2 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > > @@ -3712,11 +3712,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > if (!pte_unmap_same(vmf)) > > goto out; > > > > - if (vmf->flags & FAULT_FLAG_VMA_LOCK) { > > So if with my imagination, here we'll already have the vma_read_end() and > this patch will remove it, which makes sense. Then... > > > - ret =3D VM_FAULT_RETRY; > > - goto out; > > - } > > - > > entry =3D pte_to_swp_entry(vmf->orig_pte); > > if (unlikely(non_swap_entry(entry))) { > > if (is_migration_entry(entry)) { > > @@ -3726,6 +3721,15 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > vmf->page =3D pfn_swap_entry_to_page(entry); > > ret =3D remove_device_exclusive_entry(vmf); > > } else if (is_device_private_entry(entry)) { > > + if (vmf->flags & FAULT_FLAG_VMA_LOCK) { > > + /* > > + * migrate_to_ram is not yet ready to ope= rate > > + * under VMA lock. > > + */ > > ... here we probably just do vma_read_end(), then... > > > + ret |=3D VM_FAULT_RETRY; > > + goto out; > > + } > > + > > vmf->page =3D pfn_swap_entry_to_page(entry); > > vmf->pte =3D pte_offset_map_lock(vma->vm_mm, vmf-= >pmd, > > vmf->address, &vmf->ptl); > > @@ -5089,9 +5093,12 @@ static vm_fault_t __handle_mm_fault(struct vm_ar= ea_struct *vma, > > /* > > * In case of VM_FAULT_RETRY or VM_FAULT_COMPLETED we mig= ht > > * be still holding per-VMA lock to keep the vma stable a= s long > > - * as possible. Drop it before returning. > > + * as possible. In this situation vmf.flags has > > + * FAULT_FLAG_VMA_LOCK set and FAULT_FLAG_LOCK_DROPPED un= set. > > + * Drop the lock before returning when this happens. > > */ > > - if (vmf.flags & FAULT_FLAG_VMA_LOCK) > > + if ((vmf.flags & (FAULT_FLAG_VMA_LOCK | FAULT_FLAG_LOCK_D= ROPPED)) =3D=3D > > + FAULT_FLAG_VMA_LOCK) > > vma_end_read(vma); > > This whole chunk should have been dropped altogether with my comment in > previous patch, iiuc, and it should be no-op anyway for swap case. For t= he > real "waiting for page lock during swapin" phase we should always 100% > release the vma lock in folio_lock_or_retry() - just like mmap lock. Yep, we drop FAULT_FLAG_LOCK_DROPPED, release vma lock when we return RETRY and that makes all this unnecessary. I just need to make sure we do not access VMA after we drop its lock since we will be releasing it now earlier than before. > > Thanks, > > > } > > return ret; > > -- > > 2.41.0.178.g377b9f9a00-goog > > > > -- > Peter Xu >