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 3F92DC001DE for ; Mon, 31 Jul 2023 20:39:13 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B5D8A2800A6; Mon, 31 Jul 2023 16:39:12 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B0CE328007A; Mon, 31 Jul 2023 16:39:12 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9D4AB2800A6; Mon, 31 Jul 2023 16:39:12 -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 8E8C028007A for ; Mon, 31 Jul 2023 16:39:12 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 59F358012E for ; Mon, 31 Jul 2023 20:39:12 +0000 (UTC) X-FDA: 81073071744.14.A3A902D Received: from mail-yb1-f173.google.com (mail-yb1-f173.google.com [209.85.219.173]) by imf29.hostedemail.com (Postfix) with ESMTP id 2510B120014 for ; Mon, 31 Jul 2023 20:39:09 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=5MbruZmo; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf29.hostedemail.com: domain of surenb@google.com designates 209.85.219.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=1690835950; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to: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=LlRYd4YMaajQ0xXPSbk9pEmLduWsiZdsNXzqgUFeIsA=; b=PaODNYZ2eCO7uKaDmUeQTMvHpwox82qQW8UeNIkPIY9yMhwpVI86Zo12yTGgJTN31ro+Bw BBziRHzzaRG87MFrgZFXgYVu6aAs3MYfmCgckPIZJT3w3flTO3I+etpLY1/WrrZk9p978O Fav6g74YUIVt97L0ntxMxTbD/CZiRm8= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=5MbruZmo; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf29.hostedemail.com: domain of surenb@google.com designates 209.85.219.173 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1690835950; a=rsa-sha256; cv=none; b=8ea/n52lvVNoiXFdThb5ulzkF05v5KOHK81qN/AmzowlBBGXyRHKFGOeH5aUis29rcK1ph HvFOaX8jBJyR4R3LVgtr46m6saLZUzoVWmsSCjp7S9nMQq48DPpcEvY7U2u8P3z/Sfpg/n 1pF0EMGihBTsN2KvX7VeKUkpntH1shI= Received: by mail-yb1-f173.google.com with SMTP id 3f1490d57ef6-c4cb4919bb9so5114279276.3 for ; Mon, 31 Jul 2023 13:39:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1690835949; x=1691440749; h=content-transfer-encoding:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=LlRYd4YMaajQ0xXPSbk9pEmLduWsiZdsNXzqgUFeIsA=; b=5MbruZmoSbLb/zevKa240hssKFM2XzUyqhw7ssvcHHJn4mIGQrY5W6LzNX3jVU5Yek 6G7+IOxVUZJW3+htvGee1hd+VBFSESt2reHypW5zUrGo7OFYCblkvY9QojFjK5SHEx8Y WA7f4kY2MyQwkXeeqQPzVGwzDk5puAdNQm+9zuvD0IUsGvOKBXEiBObLg3S59mQPwd1k 9/+BXGy+1X4GJqMGmP7sEPJiEnEbgg0Lwf4N4PqgpCev5KPsGeLie9kacRGpmuzN/IbE 0k/CzLV0QLp+vN7jSB/6RAP8c6/UgU8jNUr629eyRG0iiQqCwZPPF/gcslcx2gjMhI7C H8hg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690835949; x=1691440749; h=content-transfer-encoding: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=LlRYd4YMaajQ0xXPSbk9pEmLduWsiZdsNXzqgUFeIsA=; b=BQ7DqXV0NRPDS41mh+ObZWDxtjE77TMPmqzNaQSzdOjPs8d67KZPnTA8EkTFTioInF 7tM+cU7YWXv9EA8Ai0hOPjlV90LYyttlXEYfcvddjcW9vbTecwEVdy2E4ctBGgJFiMmw weFsRZKMfZJ5kUJsXAb10PUF47v4D5b0vVnwba62gpoTO5PSWtoX/nZMKoLbggYnnH8E 93OBI3Qegd+O/DLA3waugH/P/wZaKk3PLk4XV3qgP0R52KflTlIJ0UpB78n8V1i9krnf 48CXr9+XcaYgeJoBoBXvXlGlUxnXZC7R+BkqgsEQNYxXibRILu9jJ9cZG8psav99QYB5 P5tw== X-Gm-Message-State: ABy/qLboKOIIID2IbmeLmKH09ZtHa6QN3ySPKxzM6XegbL64ZXKuvwdp 7r8ysxA1vUv4oKacQW4lBeUBOH6SW8s8u90CC1R8jA== X-Google-Smtp-Source: APBJJlFu2UEwyyq7d3CDSqXwCOLCd4McJ8AzkctZiKIZQEhb6BKbLMy0406YWzEI58g3jYog+ildUQfmSA0lq2oxJm0= X-Received: by 2002:a5b:750:0:b0:d0c:fd53:aaae with SMTP id s16-20020a5b0750000000b00d0cfd53aaaemr10065125ybq.2.1690835949388; Mon, 31 Jul 2023 13:39:09 -0700 (PDT) MIME-Version: 1.0 References: <20230731171233.1098105-7-surenb@google.com> <20230731203032.z66gjqv5p4y662zo@revolver> In-Reply-To: <20230731203032.z66gjqv5p4y662zo@revolver> From: Suren Baghdasaryan Date: Mon, 31 Jul 2023 13:38:58 -0700 Message-ID: Subject: Re: [PATCH 6/6] mm: move vma locking out of vma_prepare To: "Liam R. Howlett" , Suren Baghdasaryan , akpm@linux-foundation.org, torvalds@linux-foundation.org, jannh@google.com, willy@infradead.org, david@redhat.com, peterx@redhat.com, ldufour@linux.ibm.com, vbabka@suse.cz, michel@lespinasse.org, jglisse@google.com, mhocko@suse.com, hannes@cmpxchg.org, dave@stgolabs.net, hughd@google.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, stable@vger.kernel.org, Lorenzo Stoakes , Linus Torvalds Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 2510B120014 X-Stat-Signature: 86oh6qdsmck5odaqmfe5ysc69sxe1rws X-Rspam-User: X-HE-Tag: 1690835949-777394 X-HE-Meta: U2FsdGVkX1/dDFCmbnaiG212+PC1o4bOvQDlmebnTJMDd2PhAy3AONckqEyKooEpxWduUKhpMwy+IPMmGc3jdfNXZ61o2qOoDFGIdNwazPmV46ZP1tYEQRpn9fBdZdOQ73Q6Df38ntYo9R+42P7pzvNZI+BFPs2QzZamT6XI7cfKdMbwKsDegxtfMDpJAdnzlRe2HvgzIqGjMKE5BK+cAgiSItZLCCYSTGe/gCpF7Qc3aXQg3UflJmAP8FiquHbOMhVzWU36MHsnAU1dNHH81XQs/zRxlgOJBxx82EBQPT2VCgOhhcaYJI6wVa1IOOExcXoyqU3eyAl7M0NMgt0vPTe9M3TwSwfiSerh86uVxqOUPncz3WL0RlvC6axNpVVF8lQjCNspYC1iOIxTvIy7ffx20EEeMyl/tzGvwHBttZPfPseNBSbptnELhiBmXgMfYbQBzVLsrF85mEfQfqmNeUMkZgzyUrhuRceauC7rzbaEfectseJRAzXG7dnQvbTzxqVd90kr9Tz9k/m3bxxBhMvLRp4Ii4CgGgLl/OFPi0enO5/4UT7ajZ8XclInCDX5OM4n3ZcbeeAE3sOeiQ4xes23z7BKpHV97KXVrcDFPNeswTKsWxF3GlhC5LZq8kvMwYGtvoA2CSETls19YkrlsivuvwThD5igyAXDQKo7RnhiUcIwCoQb0uCq18279putAtbDC7uS3pVrpvvAS4wxgByI89lIB8WFdyo++5m9IXKmps0QdlbZzZduawpjaWXYWY7oZuZVSefo0KG0F0ErMmKZH1ys1TxcNnyUPbXRgcmD4m9zPhVw0JnVHi7RdHlJcKTTzGa/2H9xmAkTOal5D/7MLzWjzFNwnQkImIbkvPUZZWFjG/sf9ksGJ0SYwCHVSnmYxkr0oCDQiNiYYPGliOGiiu68QYEMDc1PNz7QrSRR1ydO0rfw0d4RxigxqM5IXgryYLjsdhATavZoBDb 3MAc/5bT IqR2frb2TYUaHc29WLnx9b9edZIuxi0TqaUbrDTmiN/rW6xnjd6mbBRad5Q9Y7jNvrL21KSUUI9FjdjUbKkyAFQ7FCPMQDBWc3aQrT7hpHW7LoDmB28cwCg4tDIMN3LVEG06tk4nMm7IiMkij6JvN6CRC/OvmutJWobnp/WM6PvRLB3x1u2qmUbKzYuFbuPn8G4kPZMYK5omVCzmaYbOKOgN57dWZm9ZAGgKkMVnxt3GiEwDgBj3QcMGkD9zdcEKs36fMe3Rsr0l2HLKvQZ1gPDY67ZVLZyyuj+AroRoaSpSTIMqgrKIQJdZ8a0vVTHFvURAhl1MiQSnh5oziRoInJTQgL/liuY2kmPz+Uv7SQxYWvKD5hx2NqjL9bvjwaTh9C46w 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 Mon, Jul 31, 2023 at 1:30=E2=80=AFPM Liam R. Howlett wrote: > > Adding Lorenzo since this also touches vma_merge() again.. > > * Suren Baghdasaryan [230731 13:12]: > > vma_prepare() is currently the central place where vmas are being locke= d > > before vma_complete() applies changes to them. While this is convenient= , > > it also obscures vma locking and makes it hard to follow the locking ru= les. > > Move vma locking out of vma_prepare() and take vma locks explicitly at = the > > locations where vmas are being modified. > > I get the idea of locking closer to the edits, but vma_merge() is very > hard to follow. It was worse when it was two functions and much larger, > but adding this into vma_merge() is difficult to validate. > > We still set vma =3D in places, so that adds to the > difficulty of ensuring the end result is all VMAs that will be > modified/removed have been locked...and the 'locking rules' are also > obscured. > > It's also annoying that this doesn't fully allow you to follow the > locking anyways. dup_anon_vma() still locks internally, with good > reason, but it's still not clear that the VMA is locked when looking at > this. > > That being said, I did go through each case and it looks like it locks > the correct VMA(s) to me. Thanks! Yeah, it took me some time to ensure the locking there is correct. If you see a better alternative to make the locking more obvious I'm open to suggestions. I accept that locking in vma_merge() is not easy to follow. > > Reviewed-by: Liam R. Howlett > > > > > Suggested-by: Linus Torvalds > > Signed-off-by: Suren Baghdasaryan > > --- > > mm/mmap.c | 26 ++++++++++++++++---------- > > 1 file changed, 16 insertions(+), 10 deletions(-) > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > index 850a39dee075..e59d83cb1d7a 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -476,16 +476,6 @@ static inline void init_vma_prep(struct vma_prepar= e *vp, > > */ > > static inline void vma_prepare(struct vma_prepare *vp) > > { > > - vma_start_write(vp->vma); > > - if (vp->adj_next) > > - vma_start_write(vp->adj_next); > > - if (vp->insert) > > - vma_start_write(vp->insert); > > - if (vp->remove) > > - vma_start_write(vp->remove); > > - if (vp->remove2) > > - vma_start_write(vp->remove2); > > - > > if (vp->file) { > > uprobe_munmap(vp->vma, vp->vma->vm_start, vp->vma->vm_end= ); > > > > @@ -650,6 +640,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_= area_struct *vma, > > bool remove_next =3D false; > > struct vma_prepare vp; > > > > + vma_start_write(vma); > > if (next && (vma !=3D next) && (end =3D=3D next->vm_end)) { > > int ret; > > > > @@ -657,6 +648,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_= area_struct *vma, > > ret =3D dup_anon_vma(vma, next); > > if (ret) > > return ret; > > + vma_start_write(next); > > } > > > > init_multi_vma_prep(&vp, vma, NULL, remove_next ? next : NULL, NU= LL); > > @@ -708,6 +700,8 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_= area_struct *vma, > > if (vma_iter_prealloc(vmi)) > > return -ENOMEM; > > > > + vma_start_write(vma); > > + > > init_vma_prep(&vp, vma); > > vma_prepare(&vp); > > vma_adjust_trans_huge(vma, start, end, 0); > > @@ -946,10 +940,12 @@ struct vm_area_struct *vma_merge(struct vma_itera= tor *vmi, struct mm_struct *mm, > > /* Can we merge both the predecessor and the successor? */ > > if (merge_prev && merge_next && > > is_mergeable_anon_vma(prev->anon_vma, next->anon_vma, NULL)) = { > > + vma_start_write(next); > > remove =3D next; /* case 1 */ > > vma_end =3D next->vm_end; > > err =3D dup_anon_vma(prev, next); > > if (curr) { /* case 6 */ > > + vma_start_write(curr); > > remove =3D curr; > > remove2 =3D next; > > if (!next->anon_vma) > > @@ -958,6 +954,7 @@ struct vm_area_struct *vma_merge(struct vma_iterato= r *vmi, struct mm_struct *mm, > > } else if (merge_prev) { /* case 2 */ > > if (curr) { > > err =3D dup_anon_vma(prev, curr); > > + vma_start_write(curr); > > if (end =3D=3D curr->vm_end) { /* case 7 */ > > remove =3D curr; > > } else { /* case 5 */ > > @@ -969,6 +966,7 @@ struct vm_area_struct *vma_merge(struct vma_iterato= r *vmi, struct mm_struct *mm, > > res =3D next; > > if (prev && addr < prev->vm_end) { /* case 4 */ > > vma_end =3D addr; > > + vma_start_write(next); > > adjust =3D next; > > adj_start =3D -(prev->vm_end - addr); > > err =3D dup_anon_vma(next, prev); > > @@ -983,6 +981,7 @@ struct vm_area_struct *vma_merge(struct vma_iterato= r *vmi, struct mm_struct *mm, > > vma_pgoff =3D next->vm_pgoff - pglen; > > if (curr) { /* case 8 */ > > vma_pgoff =3D curr->vm_pgoff; > > + vma_start_write(curr); > > remove =3D curr; > > err =3D dup_anon_vma(next, curr); > > } > > @@ -996,6 +995,8 @@ struct vm_area_struct *vma_merge(struct vma_iterato= r *vmi, struct mm_struct *mm, > > if (vma_iter_prealloc(vmi)) > > return NULL; > > > > + vma_start_write(vma); > > + > > init_multi_vma_prep(&vp, vma, adjust, remove, remove2); > > VM_WARN_ON(vp.anon_vma && adjust && adjust->anon_vma && > > vp.anon_vma !=3D adjust->anon_vma); > > @@ -2373,6 +2374,9 @@ int __split_vma(struct vma_iterator *vmi, struct = vm_area_struct *vma, > > if (new->vm_ops && new->vm_ops->open) > > new->vm_ops->open(new); > > > > + vma_start_write(vma); > > + vma_start_write(new); > > + > > init_vma_prep(&vp, vma); > > vp.insert =3D new; > > vma_prepare(&vp); > > @@ -3078,6 +3082,8 @@ static int do_brk_flags(struct vma_iterator *vmi,= struct vm_area_struct *vma, > > if (vma_iter_prealloc(vmi)) > > goto unacct_fail; > > > > + vma_start_write(vma); > > + > > init_vma_prep(&vp, vma); > > vma_prepare(&vp); > > vma_adjust_trans_huge(vma, vma->vm_start, addr + len, 0); > > -- > > 2.41.0.487.g6d72f3e995-goog > >