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 87971D462C3 for ; Wed, 13 Nov 2024 15:30:33 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id EC7E08D0007; Wed, 13 Nov 2024 10:30:32 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E50C58D0001; Wed, 13 Nov 2024 10:30:32 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CA3178D0007; Wed, 13 Nov 2024 10:30:32 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id A09738D0001 for ; Wed, 13 Nov 2024 10:30:32 -0500 (EST) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 481191C632A for ; Wed, 13 Nov 2024 15:30:32 +0000 (UTC) X-FDA: 82781457402.23.AA7DD03 Received: from mail-qt1-f177.google.com (mail-qt1-f177.google.com [209.85.160.177]) by imf09.hostedemail.com (Postfix) with ESMTP id 5954C140026 for ; Wed, 13 Nov 2024 15:30:00 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="cSuW2d/D"; spf=pass (imf09.hostedemail.com: domain of surenb@google.com designates 209.85.160.177 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1731511636; 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=5BbbffFEdpPTuuw0SdeJQjv4E7gg7EUGlV/WAkAH+3E=; b=MedCI0yJkozijAHdlUhheK3yQK+DIvr3ZYdUgdiaDcXII1avEOyzeokFjlQ+7zpobIgIZ7 R18gzz4PZd5R8FfZJMdB7unqnQlbJC599FXPsyrcCzxog5YIzDpkXYbSzl+go0r5uDQUjD tM7dWiXZ/OwZOT1rFvXvf/laVBJiZ9I= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="cSuW2d/D"; spf=pass (imf09.hostedemail.com: domain of surenb@google.com designates 209.85.160.177 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1731511636; a=rsa-sha256; cv=none; b=6eBw30ECrQo3HGmOQiemPMDkTjmFSrd1FULZ3YVVB0v8DDUJNN8tdS00gnf10gFKL06gYD cDJUAARROthrpVnw3BE46iKH52v82by+d7snrD10xrbmd/ijjNKUNckKpjs2x2hdFgSrp8 iE7xFpdMNXTazzw5cucUaTLnI6oiOko= Received: by mail-qt1-f177.google.com with SMTP id d75a77b69052e-4608dddaa35so313461cf.0 for ; Wed, 13 Nov 2024 07:30:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1731511829; x=1732116629; 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=5BbbffFEdpPTuuw0SdeJQjv4E7gg7EUGlV/WAkAH+3E=; b=cSuW2d/DLQych4Snuij2wfbgUc+/AbsupCx/QMORFnLQtIDXnlwLHxTlrnl6r318iV 3N6DxpukAj9andIREVoha9cxhhZaiCt9ZjGx3J3DWXEcgVv87IgsoKFfP9c8jTe3JINu dQZT+wtxzgARPAcZfgC0m2eTYqkK4JKsAvSFMTMoIuVAGa2hKtbkGufJWvkj4BMAd7oM Dfz3sZxNqeiQYPALWM/mARf392/C9i826OZ6ff8rHv0Ln1LeHWv3aaiZsmFVcyQsufl8 Wh/SJtK54t+YIRjGGCBE7aqDxM3+IodctYhZ8CWjUnRyzvkfEs4WzH0tzcEaH9i+mtB1 2pOg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731511829; x=1732116629; 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=5BbbffFEdpPTuuw0SdeJQjv4E7gg7EUGlV/WAkAH+3E=; b=herDJUq0BRpnaJrqZW/cdls+71yzHoYbW6rrnQgdQb/h2six52HbMGw8+kksg0U1Oh MgZbNCgBSHABDYY/CcXBnuQrUQ1qW35ZzEO0Fj5OpANZyXMHJxUcnvncXY+B2yHlMom8 3ZC1QK2icnxfyCaQwweuxpe7UNIY0WLDhFPUudnbwb1fzeLZjbf3QQTdg2K3jakJelbq KqU+6CgACTg674679qx0NvmtX5qqPTO4Qmjuose32rDKeoN9GYiAmRVDkp9p9AasPGXh sohNAp0Pt5rJD8UMCgAN83lqKHVg4zXy0lGkHDvxkKVpUA18Iq2p1Pl37VHfPQ0iVOMq EXMw== X-Forwarded-Encrypted: i=1; AJvYcCVy4rsZHuvaYuX0m7hXpmiE2SvODlxThrsMmOUg6qq7gZDFIkVmpO6uvSg+bG4/0tC5BSMhcq0YAw==@kvack.org X-Gm-Message-State: AOJu0YxSBHXYYHNELhpDz91xeplNj6iCL3RTbPxlGNckLipHnvkGbStG zQ+Yb6GgKDckIlfWi0uemT1OiPYM8s4CgjVhuMxKACNSFjiD+Yg99/SlXcMny+/0ZH/6zjdrHlz YNAFFaaTj7JM7X4YlcMyyX1olfmMi7N4fkBQb X-Gm-Gg: ASbGncu9N5VsGBw5Yrvi1VMbpj+BIO74J44VQ0Q3ToNX2cEt3dCZPlmvWHos1rkW3/j ziC56VoGm5iwylDpM4RrOapqr+be9ntY= X-Google-Smtp-Source: AGHT+IGz40KmLuNbxr1emImpXj3jJtsfK43R22qhvzAF8QhGBtb3EYXrWEmJ3so6kTbs7jEjbk5bchWT0D8Dx0lKbXY= X-Received: by 2002:a05:622a:47c6:b0:461:679f:f1ba with SMTP id d75a77b69052e-4634bca4e56mr3610881cf.20.1731511828953; Wed, 13 Nov 2024 07:30:28 -0800 (PST) MIME-Version: 1.0 References: <20241112194635.444146-1-surenb@google.com> <20241112194635.444146-2-surenb@google.com> <16442026-5237-4dcd-9343-99a242ddb7a7@lucifer.local> In-Reply-To: <16442026-5237-4dcd-9343-99a242ddb7a7@lucifer.local> From: Suren Baghdasaryan Date: Wed, 13 Nov 2024 07:30:17 -0800 Message-ID: Subject: Re: [PATCH v2 1/5] mm: introduce vma_start_read_locked{_nested} helpers To: Lorenzo Stoakes Cc: akpm@linux-foundation.org, willy@infradead.org, liam.howlett@oracle.com, mhocko@suse.com, vbabka@suse.cz, hannes@cmpxchg.org, mjguzik@gmail.com, oliver.sang@intel.com, mgorman@techsingularity.net, david@redhat.com, peterx@redhat.com, oleg@redhat.com, dave@stgolabs.net, paulmck@kernel.org, brauner@kernel.org, dhowells@redhat.com, hdanton@sina.com, hughd@google.com, minchan@google.com, jannh@google.com, shakeel.butt@linux.dev, souravpanda@google.com, pasha.tatashin@soleen.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@android.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 5954C140026 X-Stat-Signature: pfzdb9axnabuskd97nm8yytpmnngqd6i X-Rspam-User: X-HE-Tag: 1731511800-451225 X-HE-Meta: U2FsdGVkX1+Ay9WMoFmk8MzF0Fvrd/HZVZeJBs1LgLsGk/TEaIL+rEwx1D3eEyIprAP1D0JVc5avBAiRkB5krADFUOon55nv/Wu9B3ILMbniCPJ6AvMTdhl2854nPFJPiqf2utZrCrt1Ojv7U/+d/7KM/0Sn9cVjUZPeCBori0M1xuUDVbn8e22AHRn5fil/BTvHBJOSSFijDRRJpTs+URmTfNAgzznh2gzAyVT9xhp0/JvLxwuRiPI6i32dSCam0Q1zbi0dgY1yvVhvOfE/J+4GuRJik9whHzo5SWfcfhVL9ckBi2PhKzoRjsrglZZet2kFzqaUT5Ti6ujkGmt8hv50SKQDtHTgYEUtCnhL7wWQh9pGD9DefejhIZy30ubpiLO6BBDkWyj/jLvUARoDtnWdNc7oqZmjoUV7gKvaxDZWPIrmu0McK6Mwt33i4840e8cYqIN6m2c0ZWaAabuPcCWCPaS9ccjqdcEHDgHgcpLBJcjQ0MMk70T8GnzAEawYieWpgQ50QidNGGabOw05Z/Jl9W+uMdIyTBa7Dy6gssPkCmjsfB7rMFN2SzjCPXTDzx2+Hz4O5BRx6Y61Ejdb/1uG0K+wa6zFpFM1m6GCw2yuTyvgTyRcJbb/qu6IeLuqFAQtY56zWUkCX+5DlVSlOR0eMywwWzDXLw8uDcUHudqW8KsQqzi4+jtyXn58COKlroCbP3WA1kICeVR50irtoaTltKBWF9MTbQfYqhnNdpRvl977epSc+hC7QJ64vnDRDA7Zx89AmlTGkubNNo0s3TF4Kk6yZCjawmEdGajBPk2y/TcASQX4NZpsyTTweJNDrX/oDU3RFr5zZGK6z7s2fX/Bk3KhGC1Qd6ZYXvSC0nLRaHzPReOgXEnwxIJtV1msdCUW2xm0/BUjfCvGeobjf/DKUZPmqprAZHYFLL4LtxvO5cRn57Mok2r7ZtOoqlB9gOyHmoYQuY/eUe9kVpk vgGROiJD EYFbETIvVzJcNMGXYmoychh8F/TA+QilUQ+3YAhi8485Eu9onDDB8Sx54Nu42W1Lj9b4pZCmCFfvofr33ug2NfKcF0jLEMHzM0MhE3rpcEj/Cdqx28UolyJ7V3b5RVmNx6dz+S5e501M4QhhNQhhB/9SFDQp8HMvmL8yy34knC0AsFd0cxVoOPMIEv/doZIUbU2OGXQat9+0DFfHj1vanuKKYWkrXvjMXUGqb2r5FnjUjTGwv2XKjFK/P5j1HO+Q2se2Frhw2q6ESZ90NVqRHF7du4z9k5r0nYd3o 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 Wed, Nov 13, 2024 at 6:10=E2=80=AFAM Lorenzo Stoakes wrote: > > On Tue, Nov 12, 2024 at 11:46:31AM -0800, Suren Baghdasaryan wrote: > > Introduce helper functions which can be used to read-lock a VMA when > > holding mmap_lock for read. Replace direct accesses to vma->vm_lock > > with these new helpers. > > > > Signed-off-by: Suren Baghdasaryan > > --- > > include/linux/mm.h | 20 ++++++++++++++++++++ > > mm/userfaultfd.c | 14 ++++++-------- > > 2 files changed, 26 insertions(+), 8 deletions(-) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index fecd47239fa9..01ce619f3d17 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -722,6 +722,26 @@ static inline bool vma_start_read(struct vm_area_s= truct *vma) > > return true; > > } > > > > +/* > > + * Use only while holding mmap_read_lock which guarantees that nobody = can lock > > + * the vma for write (vma_start_write()) from under us. > > + */ > > +static inline void vma_start_read_locked_nested(struct vm_area_struct = *vma, int subclass) > > +{ > > + mmap_assert_locked(vma->vm_mm); > > + down_read_nested(&vma->vm_lock->lock, subclass); > > +} > > + > > +/* > > + * Use only while holding mmap_read_lock which guarantees that nobody = can lock > > + * the vma for write (vma_start_write()) from under us. > > + */ > > +static inline void vma_start_read_locked(struct vm_area_struct *vma) > > +{ > > + mmap_assert_locked(vma->vm_mm); > > + down_read(&vma->vm_lock->lock); > > +} > > Hm, but why would we want to VMA read lock under mmap read lock again? mm= ap > read lock will exclude VMA writers so it seems sort of redundant to do > this, is it because callers expect to then release the mmap read lock > afterwards or something? Yes, that's the pattern used there. They kinda downgrade from mmap to vma lock to make it more local. > > It feels like a quite specialist case that maybe is worth adding an > additional comment to because to me it seems entirely redundant - the who= le > point of the VMA locks is to be able to avoid having to take an mmap lock > on read. > > Something like 'while the intent of VMA read locks is to avoid having to > take mmap locks at all, there exist certain circumstances in which we wou= ld > need to hold the mmap read to begin with, and these helpers provide that > functionality'. Ok, I'll try documenting these functions better. > > Also, I guess naming is hard, but _locked here strikes me as confusing as > I'd read this as if the locked refer to the VMA lock rather than the mmap > lock. > > It also speaks to the need for some additional comment so nobody walks aw= ay > thinking they _must_ take a VMA read lock _as well as_ an mmap read lock > before reading from a VMA. > > Again, naming, hard :>) I'm open to suggestions. > > > + > > static inline void vma_end_read(struct vm_area_struct *vma) > > { > > rcu_read_lock(); /* keeps vma alive till the end of up_read */ > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > > index 60a0be33766f..55019c11b5a8 100644 > > --- a/mm/userfaultfd.c > > +++ b/mm/userfaultfd.c > > @@ -86,13 +86,11 @@ static struct vm_area_struct *uffd_lock_vma(struct = mm_struct *mm, > > vma =3D find_vma_and_prepare_anon(mm, address); > > if (!IS_ERR(vma)) { > > /* > > + * While holding mmap_lock we can't fail > > * We cannot use vma_start_read() as it may fail due to > > - * false locked (see comment in vma_start_read()). We > > - * can avoid that by directly locking vm_lock under > > - * mmap_lock, which guarantees that nobody can lock the > > - * vma for write (vma_start_write()) under us. > > + * false locked (see comment in vma_start_read()). > > Nit but 'as it may fail due to false locked' reads strangely. replace with "overflow"? > > > */ > > - down_read(&vma->vm_lock->lock); > > + vma_start_read_locked(vma); > > Do we even need this while gross 'this is an exception to the rule' comme= nt now > we have new helpers? > > Isn't it somewhat self-documenting now and uffd is no longer a special > snowflake? Ack. > > > > } > > > > mmap_read_unlock(mm); > > @@ -1480,10 +1478,10 @@ static int uffd_move_lock(struct mm_struct *mm, > > * See comment in uffd_lock_vma() as to why not using > > * vma_start_read() here. > > */ > > - down_read(&(*dst_vmap)->vm_lock->lock); > > + vma_start_read_locked(*dst_vmap); > > if (*dst_vmap !=3D *src_vmap) > > - down_read_nested(&(*src_vmap)->vm_lock->lock, > > - SINGLE_DEPTH_NESTING); > > + vma_start_read_locked_nested(*src_vmap, > > + SINGLE_DEPTH_NESTING); > > } > > mmap_read_unlock(mm); > > return err; > > -- > > 2.47.0.277.g8800431eea-goog > >