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 10697E7717F for ; Tue, 17 Dec 2024 16:28:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 69A516B0082; Tue, 17 Dec 2024 11:28:01 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 622346B0083; Tue, 17 Dec 2024 11:28:01 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 49C906B0085; Tue, 17 Dec 2024 11:28:01 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 2CE996B0082 for ; Tue, 17 Dec 2024 11:28:01 -0500 (EST) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id BD4D8A07E0 for ; Tue, 17 Dec 2024 16:28:00 +0000 (UTC) X-FDA: 82904981502.06.C2B3D83 Received: from mail-qt1-f173.google.com (mail-qt1-f173.google.com [209.85.160.173]) by imf30.hostedemail.com (Postfix) with ESMTP id 37CD380017 for ; Tue, 17 Dec 2024 16:27:00 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=dD4TSsTk; spf=pass (imf30.hostedemail.com: domain of surenb@google.com designates 209.85.160.173 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=1734452865; 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=NT/vkq1/V1NDu2ah/NePQKYf8DEcNklX6cXh0MMNAZU=; b=1DOsnRBSQpPzqILRpk+QgXqwckNgNJjrU5o1m/HjRPDGcijUHCurlzyITx76zyP1OZqB5w Pcv4F19op15gUIRXQpS+p/UFzalyedlAgbKIWcFo9MVu8AlillpeDPG1KkxwD8+q7y2UXD 31VvSeFFiGZ4cWmSLwcoOOQ7fnExmRI= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=dD4TSsTk; spf=pass (imf30.hostedemail.com: domain of surenb@google.com designates 209.85.160.173 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=1734452865; a=rsa-sha256; cv=none; b=nSLnk64Wx41Qvs+YhIbGPWXEVG66ovi3Aa4t0y0cVkeG8ek7a4PVEJIrRHFwaHEo/pKKg0 LCdvW/TGe+JRMUTF8P8nvJFyYwVH5sbywYDXD/GPS/AKMGqfHsuGOO+nWVdH1xYTxPyehF Gh3K05PNxc0ZcXH8ZaXii6ZlO3NWfio= Received: by mail-qt1-f173.google.com with SMTP id d75a77b69052e-467896541e1so303991cf.0 for ; Tue, 17 Dec 2024 08:27:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1734452878; x=1735057678; 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=NT/vkq1/V1NDu2ah/NePQKYf8DEcNklX6cXh0MMNAZU=; b=dD4TSsTkGAYNaz4UN5Vd+AfBHfZz8/iJ/H0nwwZhhKe35Q8DWsfIAQP4ugD5hDonFq lLl3W+gcKc9p/BdEU1Hwr4BpVYpuQ+AFubD716OdKypaSx56OfBXKO0++OGtv8MxIEpZ tW+o0aa6s3U4Da9qF6xIW+nuT6vD7gY2zZhotN5OqOZGFazlyDPu7qDAFbp/GrxA/gEF MVr5zWkaK+ngahv2tZco33S9PiCzEl4TIehsf0qvx3b1iaJH5omYCQoTs9gxfsEmXnYQ EONCRklkaeFLGcCItRMRzzAV/7XIvCPnN4YURWbSfgResTGs4cmvApaGsrxm1e3oebIN p1dg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734452878; x=1735057678; 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=NT/vkq1/V1NDu2ah/NePQKYf8DEcNklX6cXh0MMNAZU=; b=Mbd7mFjphw0hTXkC/F+jYQPPsMR62nABDyMbdcuA9EV9qoQ8sVYcuK/8pa3blIeOkt zFpB8egm0PDProHZBSqqKv90yDtiu4YWVoQ2Nd/9BrardQpSz5f6NMi8CB4xZep0DpKX FCeWvWHzCqCpIxooCAEwa1fVlyG7weKhNSagGV60UVhfeGJxEin6pjYcRIt1ma1Y+OXm /5PrwMIRXUiq8zqLTvetyDOuDpCPMs+MAqmskcqZLGGUqbeDJVUdxl+aq+D2ZAgKAr9S T+zeKmMo0Teae41UfqYJLy2G+tYYTx+3qGHdC0JurwBBnk/+1Gfi3FLdnZo2IBvradoH QSbQ== X-Forwarded-Encrypted: i=1; AJvYcCUlOF7Uiaofp5ePEixY+1tporW0RV2JkDTKpgNzX/NF+z0ZKMa+c/V3MQ2Rvll1nD36hX5dnu7Oxg==@kvack.org X-Gm-Message-State: AOJu0YyuSSmNQlMgIiVwwa/SZWzmCroVeOth+fIrvA3J1x+sWDUwv9OL VwBkKJlb9qOAKxeowhN/iFNIjaysW9VD2eOOnfgjsLe+krXBDMBjcZVM6iGXoDXd8CDaEoHv1R3 DUCBGodNLQmOKYpm8VHv5CWLOjqIZGAlFgB55 X-Gm-Gg: ASbGnctqS+ssZW+i0kZI/6+53WP09X3fBzpyAyo006dzRhBUEi7erAjgFTpHpqNRIoR xG9q4Kb365U85b1b95cIRwhK0QANiwdrNbBCxWerwq22gQEIwtyYTdqD1LYmqIUP8BxbF X-Google-Smtp-Source: AGHT+IFdrYrT/+JYttr7HyZfnxo0ZjpiJ1vNiTEa49+t7DbFwavsX/sY8uK5R0p1ZeEElNuT/XiNgIfB2zJmTxK8THw= X-Received: by 2002:ac8:5816:0:b0:466:8e4d:e981 with SMTP id d75a77b69052e-468fb0a6adfmr3691841cf.3.1734452877654; Tue, 17 Dec 2024 08:27:57 -0800 (PST) MIME-Version: 1.0 References: <20241216192419.2970941-1-surenb@google.com> <20241216192419.2970941-11-surenb@google.com> <20241216213753.GD9803@noisy.programming.kicks-ass.net> <20241217103035.GD11133@noisy.programming.kicks-ass.net> In-Reply-To: <20241217103035.GD11133@noisy.programming.kicks-ass.net> From: Suren Baghdasaryan Date: Tue, 17 Dec 2024 08:27:46 -0800 Message-ID: Subject: Re: [PATCH v6 10/16] mm: replace vm_lock and detached flag with a reference count To: Peter Zijlstra Cc: akpm@linux-foundation.org, willy@infradead.org, liam.howlett@oracle.com, lorenzo.stoakes@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, lokeshgidra@google.com, minchan@google.com, jannh@google.com, shakeel.butt@linux.dev, souravpanda@google.com, pasha.tatashin@soleen.com, klarasmodin@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: 37CD380017 X-Stat-Signature: b4tmrk5tq955pmt7sdszuz8ko9da9exg X-Rspam-User: X-Rspamd-Server: rspam11 X-HE-Tag: 1734452820-499157 X-HE-Meta: U2FsdGVkX19SmDU3Md7Tp3l19uJXHqDSp7CUHuvx9vqtl27tXovt7KanEaV/5bhXVXo75i3QSCvKjXzO+a7UiGd+IEwNsufpnEB/6bptTqQk8cgUGuSM7TPJGm0DqADzigbsmTw55DKXqeSi1BYnIuvDlT/NWCvHMbiSuIx7gup5fASwGbN6yQUBcaA07HW4KtriUn+C0UVIPmYi60iLnvD4DtDeR3TohFpb++iMaBRk/eOHahEV49lnQowQiYyV4WeoU76PZmmcQMSE+eBcZ/NHzJoyUVk3ldnX9/GOs5MHoHy/TXG60P9QYPnsdjWSwFCu3im0JvNS7gaN2HBTstybgwNu4xyGhmtBr7aVNTZ/M6sVhgo7abGjp3bqRC8F16KqeTQoE88CbRhE124ZsNCMIwXEml1nrpaPEiNxXS4TtIMHWv6tMXv6INi2RicV7KHCOI0g6Nc9lej+2VPU7DT14oaKK6dclxJGj0qSh0kgMrXD7zxyalZ+FX/OaAOZJ2O0O0RB8rCMc+hRyyDiIgWSwu1FFRUKXKCYn8qvRxNlLd3HsGm3ToZM3n6GcyqHPQ5aZSvgqYERx/MwXNQCkxxQBgiqmH7qov13uNIWm7vubztLu2MU7BpK3l52pRxpc36xPYzh8LCjuxfQ2T6fLqZd6+vskMKN7wOqokyQGYdeSb/PwAjUoepe5uyCI8V23ASlJOg7i3ZMD3vkXUd+6UrkSE5V+7Myw0juOpIHSw0mATWnTzpc3U6SDV6J8iaaLMw5QzaZ7wiivFU1KOgH+tNUKSIHShGPK7wZY9dxuMInDWi+t3IbLMcGcYS/apj9R536pr5GDSfPADRgfwUKfs/8ao/7wfgyAU4gp01jlQYFIcoAuPCQnB3iBLDGG3fwRiBTnZCtqahP8bO2yrvAi7+GLXe804stvXKaok33fD+9nhfmfzymDxpR+akedDmrVse5tWdEUr56oGczaSj E3dx+Xo5 y8huVcPgMFfv1BbStvGiS8cDQdfocSrGxVFB7adkXIAI2OW2/rYJmLp1mhFyhZtKJttNLAaSz0QnzzchPrQDLhDzmZiQCfHYkZZlgxFOr94sg/PGUdTQDPZc65M14Z7KLVxi6Ui9EjuW9jJbV8jqlr3HN1HKVFUJkBmABGRXE4zidaXx/3WwzXeJ+Mmuw+Y5nHqgBgd+M9CmYrLc9h6agJNmKIgpLIktVRfog5xQHXMUsXOLAysDWv9nUWs3UhVsw3Xqhvki2O6piuUwJdiJrkDcHXqnbjVW6HiFXF2K4Wjs/0u2G+JFYHiBrecJ7gCixHRNHukASXHZNGRoP79yN5LvNCeV70Ne/4Z/8vsLvzzBJ0N27wLMWSEFmWROOzVh8ColKwPMys0cN8OHd7NZwBgYviQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000032, 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, Dec 17, 2024 at 2:30=E2=80=AFAM Peter Zijlstra wrote: > > On Mon, Dec 16, 2024 at 01:44:45PM -0800, Suren Baghdasaryan wrote: > > On Mon, Dec 16, 2024 at 1:38=E2=80=AFPM Peter Zijlstra wrote: > > > > > > On Mon, Dec 16, 2024 at 11:24:13AM -0800, Suren Baghdasaryan wrote: > > > > +static inline void vma_refcount_put(struct vm_area_struct *vma) > > > > +{ > > > > + int refcnt; > > > > + > > > > + if (!__refcount_dec_and_test(&vma->vm_refcnt, &refcnt)) { > > > > + rwsem_release(&vma->vmlock_dep_map, _RET_IP_); > > > > + > > > > + if (refcnt & VMA_STATE_LOCKED) > > > > + rcuwait_wake_up(&vma->vm_mm->vma_writer_wait)= ; > > > > + } > > > > +} > > > > + > > > > /* > > > > * Try to read-lock a vma. The function is allowed to occasionally= yield false > > > > * locked result to avoid performance overhead, in which case we f= all back to > > > > @@ -710,6 +728,8 @@ static inline void vma_lock_init(struct vm_area= _struct *vma) > > > > */ > > > > static inline bool vma_start_read(struct vm_area_struct *vma) > > > > { > > > > + int oldcnt; > > > > + > > > > /* > > > > * Check before locking. A race might cause false locked resu= lt. > > > > * We can use READ_ONCE() for the mm_lock_seq here, and don't= need > > > > @@ -720,13 +740,20 @@ static inline bool vma_start_read(struct vm_a= rea_struct *vma) > > > > if (READ_ONCE(vma->vm_lock_seq) =3D=3D READ_ONCE(vma->vm_mm->= mm_lock_seq.sequence)) > > > > return false; > > > > > > > > + > > > > + rwsem_acquire_read(&vma->vmlock_dep_map, 0, 0, _RET_IP_); > > > > + /* Limit at VMA_STATE_LOCKED - 2 to leave one count for a wri= ter */ > > > > + if (unlikely(!__refcount_inc_not_zero_limited(&vma->vm_refcnt= , &oldcnt, > > > > + VMA_STATE_LOCKE= D - 2))) { > > > > + rwsem_release(&vma->vmlock_dep_map, _RET_IP_); > > > > return false; > > > > + } > > > > + lock_acquired(&vma->vmlock_dep_map, _RET_IP_); > > > > > > > > /* > > > > + * Overflow of vm_lock_seq/mm_lock_seq might produce false lo= cked result. > > > > * False unlocked result is impossible because we modify and = check > > > > + * vma->vm_lock_seq under vma->vm_refcnt protection and mm->m= m_lock_seq > > > > * modification invalidates all existing locks. > > > > * > > > > * We must use ACQUIRE semantics for the mm_lock_seq so that = if we are > > > > @@ -734,10 +761,12 @@ static inline bool vma_start_read(struct vm_a= rea_struct *vma) > > > > * after it has been unlocked. > > > > * This pairs with RELEASE semantics in vma_end_write_all(). > > > > */ > > > > + if (oldcnt & VMA_STATE_LOCKED || > > > > + unlikely(vma->vm_lock_seq =3D=3D raw_read_seqcount(&vma->= vm_mm->mm_lock_seq))) { > > > > + vma_refcount_put(vma); > > > > > > Suppose we have detach race with a concurrent RCU lookup like: > > > > > > vma =3D mas_lookup(); > > > > > > vma_start_write(); > > > mas_detach(); > > > vma_start_read() > > > rwsem_acquire_read() > > > inc // success > > > vma_mark_detach(); > > > dec_and_test // assumes 1->0 > > > // is actually 2->1 > > > > > > if (vm_lock_seq =3D=3D vma->v= m_mm_mm_lock_seq) // true > > > vma_refcount_put > > > dec_and_test() // 1->0 > > > *NO* rwsem_release() > > > > > > > Yes, this is possible. I think that's not a problem until we start > > reusing the vmas and I deal with this race later in this patchset. > > I think what you described here is the same race I mention in the > > description of this patch: > > https://lore.kernel.org/all/20241216192419.2970941-14-surenb@google.com= / > > I introduce vma_ensure_detached() in that patch to handle this case > > and ensure that vmas are detached before they are returned into the > > slab cache for reuse. Does that make sense? > > So I just replied there, and no, I don't think it makes sense. Just put > the kmem_cache_free() in vma_refcount_put(), to be done on 0. That's very appealing indeed and makes things much simpler. The problem I see with that is the case when we detach a vma from the tree to isolate it, then do some cleanup and only then free it. That's done in vms_gather_munmap_vmas() here: https://elixir.bootlin.com/linux/v6.12.5/source/mm/vma.c#L1240 and we even might reattach detached vmas back: https://elixir.bootlin.com/linux/v6.12.5/source/mm/vma.c#L1312. IOW, detached state is not final and we can't destroy the object that reached this state. We could change states to: 0=3Dunused (we can free the object), 1=3Ddetached, 2=3Dattached, etc. but then vma_start_read() should do something like refcount_inc_more_than_one() instead of refcount_inc_not_zero(). Would you be ok with such an approach? > > Anyway, my point was more about the weird entanglement of lockdep and > the refcount. Just pull the lockdep annotation out of _put() and put it > explicitly in the vma_start_read() error paths and vma_end_read(). Ok, I think that's easy. > > Additionally, having vma_end_write() would allow you to put a lockdep > annotation in vma_{start,end}_write() -- which was I think the original > reason I proposed it a while back, that and having improved clarity when > reading the code, since explicitly marking the end of a section is > helpful. The vma->vmlock_dep_map is tracking vma->vm_refcnt, not the vma->vm_lock_seq (similar to how today vma->vm_lock has its lockdep tracking that rw_semaphore). If I implement vma_end_write() then it will simply be something like: void vma_end_write(vma) { vma_assert_write_locked(vma); vma->vm_lock_seq =3D UINT_MAX; } so, vmlock_dep_map would not be involved. If you want to track vma->vm_lock_seq with a separate lockdep, that would be more complicated. Specifically for vma_end_write_all() that would require us to call rwsem_release() on all locked vmas, however we currently do not track individual locked vmas. vma_end_write_all() allows us not to worry about tracking them, knowing that once we do mmap_write_unlock() they all will get unlocked with one increment of mm->mm_lock_seq. If your suggestion is to replace vma_end_write_all() with vma_end_write() and unlock vmas individually across the mm code, that would be a sizable effort. If that is indeed your ultimate goal, I can do that as a separate project: introduce vma_end_write(), gradually add them in required places (not yet sure how complex that would be), then retire vma_end_write_all() and add a lockdep for vma->vm_lock_seq.