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 0C808C02180 for ; Mon, 13 Jan 2025 19:09:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8FBA26B00A4; Mon, 13 Jan 2025 14:09:40 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 8837D6B00A5; Mon, 13 Jan 2025 14:09:40 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6D63C6B00A6; Mon, 13 Jan 2025 14:09:40 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 4AB766B00A4 for ; Mon, 13 Jan 2025 14:09:40 -0500 (EST) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id ECFFFA02C0 for ; Mon, 13 Jan 2025 19:09:39 +0000 (UTC) X-FDA: 83003367678.29.85BB2FE Received: from mail-qt1-f179.google.com (mail-qt1-f179.google.com [209.85.160.179]) by imf18.hostedemail.com (Postfix) with ESMTP id 151FE1C000D for ; Mon, 13 Jan 2025 19:09:37 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=WZdYp0oF; spf=pass (imf18.hostedemail.com: domain of surenb@google.com designates 209.85.160.179 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=1736795378; 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=awAcP1PapTldB001pJ/Irl/POVAt+oNoWSzuce2g7Bo=; b=FxpACEPUz/z4VjCKXplYV0M10NtD/Lk1Pud9Ieadq8OAKeLDacjswRfRFL6ij/RSpEw17F TAvEvMD0Mr8m4bf7gTowKWrJ0jlem+zRXtzpfmzGD2wd3gPLc4zVrkfWRVltpSHsFSphj/ GcUnaIGsH1pKJ5jThC1Hbsnp4Hlt33Q= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736795378; a=rsa-sha256; cv=none; b=eIljRlA+B7/jWQJgqgUe4XmCeh2d9C8Xia/2F3R9aAhrss8CTtQU4eu3q9A2mv/cfAxE4q GRQU3VLkV/ZwZoYtJYpDtzIbXjaeqPuwY9fx8KIrt+kHGufl1iMdkKQmG+2LGNe/Z8Vqnu c3o05xXPXD8mtLjjpNRadPHj6q35XJE= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=WZdYp0oF; spf=pass (imf18.hostedemail.com: domain of surenb@google.com designates 209.85.160.179 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-qt1-f179.google.com with SMTP id d75a77b69052e-467896541e1so32111cf.0 for ; Mon, 13 Jan 2025 11:09:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1736795377; x=1737400177; 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=awAcP1PapTldB001pJ/Irl/POVAt+oNoWSzuce2g7Bo=; b=WZdYp0oFIsDNAuzKoNhl0l6JsdMgmooeVuEhGwKwvby6eLHOFI+09Pg31dID/mjpGI RuuYgfIYOC7SWTTYgFCLENnJ3y8uzjwY6bJ228KokrwoAmPBKEh+zDIwsbFI6oTGMkQn 8qs7KyPXJAorLZnJVHOfeQamnvKOmIGWMDV68alG1Lh+53LMqQMNLYpzEud2kspQhh4+ 4h04iXPj2TDZFpU6p3evBroA+r1aTu9wwxUI3BTYIMBx1YZH9WF1anjHLnhN24KfQop/ o+GQ5bEPTj4yHoX3UUcI6vxtv6YQ9rY9NVHucKx6CwTHYBKCF5posIxVc33T68qqC1YB QZZQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736795377; x=1737400177; 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=awAcP1PapTldB001pJ/Irl/POVAt+oNoWSzuce2g7Bo=; b=Oe9UVmCpQqzaTn28PIB3Fz4JOe5mcSPXx9s7Y8Dgk8UJgeqUz/1DzU0JKfft8nUJrB 2dVpYxfbSIC5NdHfi866yI9U2Jbkb1Ie+o1kPL7Q7w1a+fgqtk2/glvknHs7aCLcmPrc KI6Bdr8h894BmBcuakkvKuYzE9MXga7epepesfjsd3E5tifiToz+Y2Sk32FziRCQvrSH d395hiyq6nG1B0GmheqnubYMf9eB1uRwGXiB7r9eIKGPdpsXMNe0vtCwuEQ7Ni9t5pkA KnNxqqHv06XVbpoVgIIF6/0LbkNKUESm2wLKEU5rMq+6/FzzKyC61DZU3zhZLmrs7IYa EN4g== X-Forwarded-Encrypted: i=1; AJvYcCW6OvQb1CVupBXuzJ39mB7D31964MNDW7b5ec45jG0cHm69T6dOtwTCkZ/BT7MDkMrqQTneHd9g2w==@kvack.org X-Gm-Message-State: AOJu0Yw2ynWPa/47krIm+wELY1qH6c8ZDA0vLplahPZeFpx5MJ6JLFE5 QOdY8T5wIyIhgBUflyxdscwKXzn6z4gMITkr7tnn/GXn/FkDnqlWLZ1iEFZPIUR7bHQR6eWfVNJ fmX2axuWxsZeQeZTqnVBlI5XLVDh3zA7Sizq5 X-Gm-Gg: ASbGnctFERrdpJrTsxER0ixeZIkjvbkJ/NgqvTWL1rcXIE6cdhGvwYjuixWo+xcgXqD CVvKJmfLZa+OgLuU8Iie7SDPJFa5rsvjQjJiBbg== X-Google-Smtp-Source: AGHT+IHxSCwBZPKwoB9jmpHDH1aufJlnEZ9DADuU3/4fqu4oePhrEHgnJ2HBiqc2XzOGEAHQrgmaEuAHHJ8q9CNZDmA= X-Received: by 2002:ac8:5e4c:0:b0:465:c590:ed18 with SMTP id d75a77b69052e-46de99be855mr11101cf.9.1736795376763; Mon, 13 Jan 2025 11:09:36 -0800 (PST) MIME-Version: 1.0 References: <20250111042604.3230628-1-surenb@google.com> <20250111042604.3230628-5-surenb@google.com> <6e9329ba-8dad-423f-9741-e5447f85659f@lucifer.local> <640fee1d-e76b-4aca-8975-f6bd4f3279d9@lucifer.local> In-Reply-To: <640fee1d-e76b-4aca-8975-f6bd4f3279d9@lucifer.local> From: Suren Baghdasaryan Date: Mon, 13 Jan 2025 11:09:25 -0800 X-Gm-Features: AbW1kvb1k5p6OBd-FXsvft9mNCJV5mkViZk3XuQzBbtTuPaqGsMjh14nn6oWGkM Message-ID: Subject: Re: [PATCH v9 04/17] mm: introduce vma_iter_store_attached() to use with attached vmas To: Lorenzo Stoakes Cc: akpm@linux-foundation.org, peterz@infradead.org, willy@infradead.org, liam.howlett@oracle.com, david.laight.linux@gmail.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, lokeshgidra@google.com, minchan@google.com, jannh@google.com, shakeel.butt@linux.dev, souravpanda@google.com, pasha.tatashin@soleen.com, klarasmodin@gmail.com, richard.weiyang@gmail.com, corbet@lwn.net, linux-doc@vger.kernel.org, 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-Queue-Id: 151FE1C000D X-Rspam-User: X-Rspamd-Server: rspam07 X-Stat-Signature: 4uwryx3j7hcinyqygay3cnjrsbwemmrx X-HE-Tag: 1736795377-513443 X-HE-Meta: U2FsdGVkX195OPp8NIgzagGMqop+5GiuW6LAfvMCTq3XKouj3cqmq4QwRSm5C3C/cwG9btM73B4YhL7bz2bBwYRRfgcZyhJhKiqe1oS668MNy6IwMdLJZbit8cPvggFYuCIge6d8cAMdpue607He9K5254TrWtnfI2nl+FoZFj8Mmlr9wtLaLFZS/xMPdPM5AHkdOhMn7SmkKe7Ctb/XdNvjXDBz5UFEd0XGS/RGoY0zoWLkkycOK72Ge0P6SWsDcjL7z1NCHG9NRvHII54a83yloN+BOUXgej94Fx2kgYaWei/j3erz1OxFJ/2O/fforctc+TT2QBU4M6nhB11/ck120XtMefxOLzG4h8zfMaC4LJIEYEYaBkaVUladTB+EZ1cxUPukSS8+YnkgEvrAEM5XBgPiqT6BmmrwmTmJKdYkBQA51vvEqtk3ARHmZqSc82wWrwPErnuUMm5U7+JAxo4YcI30NT3caTpN33LutK/Bj4KqUf6fw8Z3PGuNujygn7n12Kg1Rk+239ITnSQeSlciv42IrURsEf5Dp7847BhOR8x/FqiSwxpwhI18hJcH3U9cNhAuApAt3bJcN02FXsYRZuy6/XD1O+XYEoSKOdIQfR8qLeEag3Y3SxLME2reAxWI+PbpcPclDEyWHhFo39Zmw3AlysQL7KJN7ve90d/AaU0Z8wPV3bjA9RVLsPLp63JH6ZUvSWdIlgRvPKgarpgsbEbT3WDDnqgP9V/CgySaLY9br5h1rHDtMS+BnWeTqLlHKdqs7D99Vi/Sz51QoTLvxRJjhqX1hxYiazQS7Gii1obxZ9e92CZ/aM0u1jbBi8FoC59MR4dh6ZIZJfcsu28vsX0a68a2KNhYy7x9KQzKufGxzTgSintl8oAHr1sfSC+zQPh1BUW/qWD46ni2sCVzbXGlCOloun3+gYgqAUxB8qyo/nUkVH1gqXnXnVZQzgn4KSZXFuA1SUsY1Rt jitV3y6D ouqN5EmP1+m9cEyeVQzRW30xcCkQGZVECzxTeySJAJJC4DxtHftoxHVXi2aO3i1u/4s+tMz0+6leGf5v8oyhc+Gc0NyN7BD/17+fkgSYkLbmk5V7wI+uqJ79fLK/AZi1H+wkYwuGEnfPGSK3rh+A0/E6Kh1O1miMcOqrC/znX5KXWcATnUXrtvjQ36ETOzup9ApKI1kFreC8uclmH18xzU07+cuPbP7tduvTFh0/xcT/5ygxdPiNlYhzsZO1GeAzjVkBjUngOHkBECTSRVw4/us+j9753YWBpzgZL54scsMx2xgVPUNtzNBnaw5XoIsKhtd+hVGsg5aZYrE8YRHFQBITID7ujsdyiaejknidv1Unqmzs= 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 Mon, Jan 13, 2025 at 8:48=E2=80=AFAM Lorenzo Stoakes wrote: > > On Mon, Jan 13, 2025 at 08:31:45AM -0800, Suren Baghdasaryan wrote: > > On Mon, Jan 13, 2025 at 3:58=E2=80=AFAM Lorenzo Stoakes > > wrote: > > > > > > On Fri, Jan 10, 2025 at 08:25:51PM -0800, Suren Baghdasaryan wrote: > > > > vma_iter_store() functions can be used both when adding a new vma a= nd > > > > when updating an existing one. However for existing ones we do not = need > > > > to mark them attached as they are already marked that way. Introduc= e > > > > vma_iter_store_attached() to be used with already attached vmas. > > > > > > OK I guess the intent of this is to reinstate the previously existing > > > asserts, only explicitly checking those places where we attach. > > > > No, the motivation is to prevern re-attaching an already attached vma > > or re-detaching an already detached vma for state consistency. I guess > > I should amend the description to make that clear. > > Sorry for noise, missed this reply. > > What I mean by this is, in a past iteration of this series I reviewed cod= e > where you did this but did _not_ differentiate between cases of new VMAs > vs. existing, which caused an assert in your series which I reported. > > So I"m saying - now you _are_ differentiating between the two cases. > > It's certainly worth belabouring the point of exactly what it is you are > trying to catch here, however! :) So yes please do add a little more to > commit msg that'd be great, thanks! Sure. How about: With vma->detached being a separate flag, double-marking a vmas as attached or detached is not an issue because the flag will simply be overwritten with the same value. However once we fold this flag into the refcount later in this series, re-attaching or re-detaching a vma becomes an issue since these operations will be incrementing/decrementing a refcount. Fix the places where we currently re-attaching a vma during vma update and add assertions in vma_mark_attached()/vma_mark_detached() to catch invalid usage. > > > > > > > > > I'm a little concerned that by doing this, somebody might simply invo= ke > > > this function without realising the implications. > > > > Well, in that case somebody should get an assertion. If > > vma_iter_store() is called against already attached vma, we get this > > assertion: > > > > vma_iter_store() > > vma_mark_attached() > > vma_assert_detached() > > > > If vma_iter_store_attached() is called against a detached vma, we get t= his one: > > > > vma_iter_store_attached() > > vma_assert_attached() > > > > Does that address your concern? > > > > > > > > Can we have something functional like > > > > > > vma_iter_store_new() and vma_iter_store_overwrite() > > > > Ok. A bit more churn but should not be too bad. > > > > > > > > ? > > > > > > I don't like us just leaving vma_iter_store() quietly making an assum= ption > > > that a caller doesn't necessarily realise. > > > > > > Also it's more greppable this way. > > > > > > I had a look through callers and it does seem you've snagged them all > > > correctly. > > > > > > > > > > > Signed-off-by: Suren Baghdasaryan > > > > Reviewed-by: Vlastimil Babka > > > > --- > > > > include/linux/mm.h | 12 ++++++++++++ > > > > mm/vma.c | 8 ++++---- > > > > mm/vma.h | 11 +++++++++-- > > > > 3 files changed, 25 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > > > index 2b322871da87..2f805f1a0176 100644 > > > > --- a/include/linux/mm.h > > > > +++ b/include/linux/mm.h > > > > @@ -821,6 +821,16 @@ static inline void vma_assert_locked(struct vm= _area_struct *vma) > > > > vma_assert_write_locked(vma); > > > > } > > > > > > > > +static inline void vma_assert_attached(struct vm_area_struct *vma) > > > > +{ > > > > + VM_BUG_ON_VMA(vma->detached, vma); > > > > +} > > > > + > > > > +static inline void vma_assert_detached(struct vm_area_struct *vma) > > > > +{ > > > > + VM_BUG_ON_VMA(!vma->detached, vma); > > > > +} > > > > + > > > > static inline void vma_mark_attached(struct vm_area_struct *vma) > > > > { > > > > vma->detached =3D false; > > > > @@ -866,6 +876,8 @@ static inline void vma_end_read(struct vm_area_= struct *vma) {} > > > > static inline void vma_start_write(struct vm_area_struct *vma) {} > > > > static inline void vma_assert_write_locked(struct vm_area_struct *= vma) > > > > { mmap_assert_write_locked(vma->vm_mm); } > > > > +static inline void vma_assert_attached(struct vm_area_struct *vma)= {} > > > > +static inline void vma_assert_detached(struct vm_area_struct *vma)= {} > > > > static inline void vma_mark_attached(struct vm_area_struct *vma) {= } > > > > static inline void vma_mark_detached(struct vm_area_struct *vma) {= } > > > > > > > > diff --git a/mm/vma.c b/mm/vma.c > > > > index d603494e69d7..b9cf552e120c 100644 > > > > --- a/mm/vma.c > > > > +++ b/mm/vma.c > > > > @@ -660,14 +660,14 @@ static int commit_merge(struct vma_merge_stru= ct *vmg, > > > > vma_set_range(vmg->vma, vmg->start, vmg->end, vmg->pgoff); > > > > > > > > if (expanded) > > > > - vma_iter_store(vmg->vmi, vmg->vma); > > > > + vma_iter_store_attached(vmg->vmi, vmg->vma); > > > > > > > > if (adj_start) { > > > > adjust->vm_start +=3D adj_start; > > > > adjust->vm_pgoff +=3D PHYS_PFN(adj_start); > > > > if (adj_start < 0) { > > > > WARN_ON(expanded); > > > > - vma_iter_store(vmg->vmi, adjust); > > > > + vma_iter_store_attached(vmg->vmi, adjust); > > > > } > > > > } > > > > > > I kind of feel this whole function (that yes, I added :>) though deri= ved > > > from existing logic) needs rework, as it's necessarily rather confusi= ng. > > > > > > But hey, that's on me :) > > > > > > But this does look right... OK see this as a note-to-self... > > > > > > > > > > > @@ -2845,7 +2845,7 @@ int expand_upwards(struct vm_area_struct *vma= , unsigned long address) > > > > anon_vma_interval_tree_pre_update_vma= (vma); > > > > vma->vm_end =3D address; > > > > /* Overwrite old entry in mtree. */ > > > > - vma_iter_store(&vmi, vma); > > > > + vma_iter_store_attached(&vmi, vma); > > > > anon_vma_interval_tree_post_update_vm= a(vma); > > > > > > > > perf_event_mmap(vma); > > > > @@ -2925,7 +2925,7 @@ int expand_downwards(struct vm_area_struct *v= ma, unsigned long address) > > > > vma->vm_start =3D address; > > > > vma->vm_pgoff -=3D grow; > > > > /* Overwrite old entry in mtree. */ > > > > - vma_iter_store(&vmi, vma); > > > > + vma_iter_store_attached(&vmi, vma); > > > > anon_vma_interval_tree_post_update_vm= a(vma); > > > > > > > > perf_event_mmap(vma); > > > > diff --git a/mm/vma.h b/mm/vma.h > > > > index 2a2668de8d2c..63dd38d5230c 100644 > > > > --- a/mm/vma.h > > > > +++ b/mm/vma.h > > > > @@ -365,9 +365,10 @@ static inline struct vm_area_struct *vma_iter_= load(struct vma_iterator *vmi) > > > > } > > > > > > > > /* Store a VMA with preallocated memory */ > > > > -static inline void vma_iter_store(struct vma_iterator *vmi, > > > > - struct vm_area_struct *vma) > > > > +static inline void vma_iter_store_attached(struct vma_iterator *vm= i, > > > > + struct vm_area_struct *vma= ) > > > > { > > > > + vma_assert_attached(vma); > > > > > > > > #if defined(CONFIG_DEBUG_VM_MAPLE_TREE) > > > > if (MAS_WARN_ON(&vmi->mas, vmi->mas.status !=3D ma_start && > > > > @@ -390,7 +391,13 @@ static inline void vma_iter_store(struct vma_i= terator *vmi, > > > > > > > > __mas_set_range(&vmi->mas, vma->vm_start, vma->vm_end - 1); > > > > mas_store_prealloc(&vmi->mas, vma); > > > > +} > > > > + > > > > +static inline void vma_iter_store(struct vma_iterator *vmi, > > > > + struct vm_area_struct *vma) > > > > +{ > > > > vma_mark_attached(vma); > > > > + vma_iter_store_attached(vmi, vma); > > > > } > > > > > > > > > > See comment at top, and we need some comments here to explain why we'= re > > > going to pains to do this. > > > > Ack. I'll amend the patch description to make that clear. > > > > > > > > What about mm/nommu.c? I guess these cases are always new VMAs. > > > > CONFIG_PER_VMA_LOCK depends on !CONFIG_NOMMU, so for nommu case all > > these attach/detach functions become NOPs. > > > > > > > > We probably definitely need to check this series in a nommu setup, ha= ve you > > > done this? As I can see this breaking things. Then again I suppose yo= u'd > > > have expected bots to moan by now... > > > > > > > static inline unsigned long vma_iter_addr(struct vma_iterator *vmi= ) > > > > -- > > > > 2.47.1.613.gc27f4b7a9f-goog > > > >