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 2FA9FC27C54 for ; Thu, 6 Jun 2024 18:09:47 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 959E06B00A1; Thu, 6 Jun 2024 14:09:46 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8E2C46B00AA; Thu, 6 Jun 2024 14:09:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 75C6B6B00B9; Thu, 6 Jun 2024 14:09:46 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 53E716B00A1 for ; Thu, 6 Jun 2024 14:09:46 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id EEE931A16E0 for ; Thu, 6 Jun 2024 18:09:45 +0000 (UTC) X-FDA: 82201251930.01.B38BE81 Received: from mail-pj1-f41.google.com (mail-pj1-f41.google.com [209.85.216.41]) by imf23.hostedemail.com (Postfix) with ESMTP id 25D3A140010 for ; Thu, 6 Jun 2024 18:09:43 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=m5jt+WWu; spf=pass (imf23.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.216.41 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1717697384; 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=r5OklrM6JwTm3HO8tSX1yK0hEi0xNRql5wG9ok44Rvg=; b=7hCGhb+QE9BAVDJ8bsXcqlXRC9d1XmT56UgTNBrJdozN6uOwJYtG7xqH3KgFw1dhfZA5ld iq9gwYVOtN3N9jzlqO23xSsX9aJpPr1tFek3BajzrhBxPrwfoT2R5Udif2n26cNb/0brfd 5Ftn4m7d9DFRloyJyfgYoKp5oIRFskU= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=m5jt+WWu; spf=pass (imf23.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.216.41 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1717697384; a=rsa-sha256; cv=none; b=bo/z2n6R3xFnCMp3m3Loi0AniXFY84X25qQpWSVrAT1ZI3LWnhy9n0AVIuSzAlOPPD3U/n NmOFnnUNyN1bqjMz4rIaGyoYLkbB9uEQpZdxVrROtsSt21HXsiMPPGF2eLAy6ORw5dT8FL wozMiAJRPrhBHGxhlt5nxtQrUt1dPdc= Received: by mail-pj1-f41.google.com with SMTP id 98e67ed59e1d1-2c2283707b3so1086969a91.0 for ; Thu, 06 Jun 2024 11:09:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1717697383; x=1718302183; darn=kvack.org; 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=r5OklrM6JwTm3HO8tSX1yK0hEi0xNRql5wG9ok44Rvg=; b=m5jt+WWuBwFuzSa6wIbi0HzcAF8IN31XJ6YlUNMo4SgAdhf+7V6VBYiB8jHshFkFfe TD6FsucocG9TuMAT0atY9Ql7gMQaila4+uthxcId+k+ODcF2STCBOWDPCrjP+XSScUF9 ydW+Guy/5jhhvQVzLEAiDSzKgdLoxx5ogqDlTlbotxdRr4Gwi1VWV88jAhEf41SXhrgl KvV+0VW6/DHZyV70b8+ewqRFAUF2nrkxl9U4+G41L+PPugkyfYoKud0ZDRirYtVvudtA TT0kr0pBnda8A1HVkLHqEO44HvZ2SiC1jHO5+Ljot4Xn3RAXFXiKas8jyAXaQF957D/4 8IGg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717697383; x=1718302183; 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=r5OklrM6JwTm3HO8tSX1yK0hEi0xNRql5wG9ok44Rvg=; b=A6pN/WB6e/3N6xYqfn+TRKgjNeJE/SBWia3fKANiXUXDTuffBmb1famhtOoqzJFjiu gDMbxoQcr21UUG8TFao7BvhidSEEihdjX99VRyascDnGp62igQfkod+59TvM7J9R2T26 JIUOsGQZI171S6l4HxEVBiJ/sXBE5ZGqLSXKQL4cil9+1sFld2EBhgxLpTKsgegymEX1 7K12FdAPSxaRyDmtOPY8hD0P6HFd8Xln5lJFFyKT9DCYLkm+CTGnQm0OkbBayd63MsXl +7PNmbr6Wt2K1B2cevokhqFUG2oBQj5cL+uTamJW8FqoT7gVv8n8SqOPpP2fNGu/gUBS EIAg== X-Forwarded-Encrypted: i=1; AJvYcCWd4Je5fo6jO42b26DjXTEy11hU4oyYqNvPXLvAb2raadQnOtUFNy2MLwnidyIy3DTLdRiSDBVxZqk4VqCquIHIkN8= X-Gm-Message-State: AOJu0YzrUyAoUyyA5EfdKIqpYg6TC38sYkJuPnFUd2p7YSkYBeXLcIq6 9t7aVj7UhP+kAeiPl34dUEg3zYzjz8T1yzR5WsA/GNPrxIxtJTcvu2OTqhrv/H8KjwgeTX7pV6N hYPfh5lcwI/NHMUcNUbjZeKtJZ8w= X-Google-Smtp-Source: AGHT+IE2vgamaVfO416HSacKOgFDRZp6Of3eAzhvkx6J5QoVRXuCcj8K++0BpcHhbM0bVu33tOq4JFowQg2f8/r9/fQ= X-Received: by 2002:a17:90b:1013:b0:2bf:9ed7:a79b with SMTP id 98e67ed59e1d1-2c2bcada703mr290487a91.19.1717697382797; Thu, 06 Jun 2024 11:09:42 -0700 (PDT) MIME-Version: 1.0 References: <20240605002459.4091285-1-andrii@kernel.org> <20240605002459.4091285-5-andrii@kernel.org> In-Reply-To: From: Andrii Nakryiko Date: Thu, 6 Jun 2024 11:09:30 -0700 Message-ID: Subject: Re: [PATCH v3 4/9] fs/procfs: use per-VMA RCU-protected locking in PROCMAP_QUERY API To: "Liam R. Howlett" , Andrii Nakryiko , Suren Baghdasaryan , Andrii Nakryiko , linux-fsdevel@vger.kernel.org, brauner@kernel.org, viro@zeniv.linux.org.uk, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, bpf@vger.kernel.org, gregkh@linuxfoundation.org, linux-mm@kvack.org, rppt@kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 25D3A140010 X-Stat-Signature: rggmpeqt6fraxcbawyi4p1twf9cbh37s X-Rspam-User: X-HE-Tag: 1717697383-783968 X-HE-Meta: U2FsdGVkX19jwZrLlg8+Gnj8Aus2Z37md3moIfa8AHiGnjrjP/HytjRXw4ygzDaWikFRzi+bGCQ+d16QyOD42mJDjkTuSQSepGdena/rFfEV0aYayIseHEbEZLqgqQ4LH0ccU2PDGQ64vrM4ztIG7o4ouKMsF5WYbGCMcFO2c6QCVZW6L+LmhQdI4jUk3QSfDLH1SwyR+n/RLrMV8QrW2QWS+QInNWCKjqJuMRH/fUSiP14REgqUbtaR2oXMK8P24O+YHyz34ZkzRoYl8LX4Uk2pJY4wZgJEbTeZZYYzCRzUluVsUpisnADRkOSHqLztwSIOFYwZ+B77SlaKQxQHyFyTYB0olNu5QgyZlcYRRDjwnHAhCEvl0UUX8Qj587+zoSoH7vAkESxHOqPkGjIohbcjNkEbXsgzqJtP3L2TRi6kl+CNkmBYToPrNIKuvPSUxyrKbg01aewTSmwZCsdQg7ZJ5uLkrNFaj1fGOIBql8MxQZ0HTBAJUr1fcGDSLW/lllI2CGf+lRwe1j9v4ytWsjXi2/wWYFMz+fJQFJ0YOh7qIMK2b3LC68FGwc/nDWfU/5OCmxAYDuuo/SBACxGdr6RwlgB9YIzH9g7cNHb7zrMLC+CJubp87ClDdhYPYhZss8/NnwQwnEKnM40KtuJIVNzBvy4N4E/5+WoCGcJRUefwruRx8fP/SXF4koD+PvMKzWFeCXqFDT3hXdznuoiL6SLP5bflCJhW+cQ+yEvvwXnSDUmhbAgyvl2gYA6S3sdWlZDXBr3IBVuY94XeWqhaDgVkWHb4RoTkgaWDEkKCkwZ/bcZxgniq7BrHuMf2LT5Ok40ccHlMvxdJT4AQLWRzwEquKjM2oo87Qq4vrD5Kgv9nZBp0IRr2YtGgAigKHybRhILfkLwJMoiRKTArU0fyVTvjHgYNBS0MNp1uMtISEG53h2VN7zrZ8uwIOkoB3iiaMgakiWrxExiADhcM07x Kpzpjjff FE0vIaRKSh58cFu6SBxlqkHc9jTErgykY9F0M3Qi3Ek9tmdDuQftSTc1umbExMPvBjApWMinI7QrZDoWhYVOnCCxN8xAnzh+hK8YNj2l4T6DwLSV/lgLf7PNXOXYj5v2py2EWH+nM2y4DnE6JtNIHJAs+vZgAyfAfpU6NXEoMoGUYD8iMfe3GaWjBEg2Tgx85zAElPXc6u1xWFpaHt3AZGWn43qubsYIhDRbFct4Rya6mmr6wG4CN6/dBqEJt21sU/ncUzYW4SmGGVi02gwh9E3BZTdaxmHomlqr/81+hqRQrR5RNF9Ywa76LoqBY1cm3t2fRyNOyftpLh5Ykj37MAndlkCDu1XRKZ6wGxITmz/3SpLpCehqLGix+2xPIHUtczlh58uZLCUTaDr4M0oTrnTfDQQhWMLbkGyR93gUFTPL3vSJoyi1PfbUjGrPcx+mcPAsBFfiA0bsZOAPc0uXRDCYzjkygBVgFJ6PuNHSvX5zVYhAyooJ1yp0f7xrVkTXez79GXdsMICgSKmEniD9nw1Jk0uDX0/hmAWJlqE1RLp4NtKdy2ZbJ4XUJ9JXe16y2MBIU4eFwchlBWQ0IYECkHZMcWqweBpwfTNpFIWhRKMrEYNrrHlxI6S5FRT32DKulUbM9A1ruAb3hZFf7/1Sbdd/GAht8FWEsas7g0gR943kIWVtjo0RaHuVRJGYFkc1fItX3R7d4ezNmJIml2M2BJDKutaK7xwK0HaJX3BiomalLal9MqoACKKhFHg== 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 Thu, Jun 6, 2024 at 10:15=E2=80=AFAM Liam R. Howlett wrote: > > * Andrii Nakryiko [240606 12:52]: > > On Wed, Jun 5, 2024 at 4:16=E2=80=AFPM Suren Baghdasaryan wrote: > > > > > > On Tue, Jun 4, 2024 at 5:25=E2=80=AFPM Andrii Nakryiko wrote: > > > > > > > > Attempt to use RCU-protected per-VMA lock when looking up requested= VMA > > > > as much as possible, only falling back to mmap_lock if per-VMA lock > > > > failed. This is done so that querying of VMAs doesn't interfere wit= h > > > > other critical tasks, like page fault handling. > > > > > > > > This has been suggested by mm folks, and we make use of a newly add= ed > > > > internal API that works like find_vma(), but tries to use per-VMA l= ock. > > > > > > > > We have two sets of setup/query/teardown helper functions with diff= erent > > > > implementations depending on availability of per-VMA lock (conditio= ned > > > > on CONFIG_PER_VMA_LOCK) to abstract per-VMA lock subtleties. > > > > > > > > When per-VMA lock is available, lookup is done under RCU, attemptin= g to > > > > take a per-VMA lock. If that fails, we fallback to mmap_lock, but t= hen > > > > proceed to unconditionally grab per-VMA lock again, dropping mmap_l= ock > > > > immediately. In this configuration mmap_lock is never helf for long= , > > > > minimizing disruptions while querying. > > > > > > > > When per-VMA lock is compiled out, we take mmap_lock once, query VM= As > > > > using find_vma() API, and then unlock mmap_lock at the very end onc= e as > > > > well. In this setup we avoid locking/unlocking mmap_lock on every l= ooked > > > > up VMA (depending on query parameters we might need to iterate a fe= w of > > > > them). > > > > > > > > Signed-off-by: Andrii Nakryiko > > > > --- > > > > fs/proc/task_mmu.c | 46 ++++++++++++++++++++++++++++++++++++++++++= ++++ > > > > 1 file changed, 46 insertions(+) > > > > > > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > > > > index 614fbe5d0667..140032ffc551 100644 > > > > --- a/fs/proc/task_mmu.c > > > > +++ b/fs/proc/task_mmu.c > > > > @@ -388,6 +388,49 @@ static int pid_maps_open(struct inode *inode, = struct file *file) > > > > PROCMAP_QUERY_VMA_FLAGS \ > > > > ) > > > > > > > > +#ifdef CONFIG_PER_VMA_LOCK > > > > +static int query_vma_setup(struct mm_struct *mm) > > > > +{ > > > > + /* in the presence of per-VMA lock we don't need any setup/= teardown */ > > > > + return 0; > > > > +} > > > > + > > > > +static void query_vma_teardown(struct mm_struct *mm, struct vm_are= a_struct *vma) > > > > +{ > > > > + /* in the presence of per-VMA lock we need to unlock vma, i= f present */ > > > > + if (vma) > > > > + vma_end_read(vma); > > > > +} > > > > + > > > > +static struct vm_area_struct *query_vma_find_by_addr(struct mm_str= uct *mm, unsigned long addr) > > > > +{ > > > > + struct vm_area_struct *vma; > > > > + > > > > + /* try to use less disruptive per-VMA lock */ > > > > + vma =3D find_and_lock_vma_rcu(mm, addr); > > > > + if (IS_ERR(vma)) { > > > > + /* failed to take per-VMA lock, fallback to mmap_lo= ck */ > > > > + if (mmap_read_lock_killable(mm)) > > > > + return ERR_PTR(-EINTR); > > > > + > > > > + vma =3D find_vma(mm, addr); > > > > + if (vma) { > > > > + /* > > > > + * We cannot use vma_start_read() as it may= fail due to > > > > + * false locked (see comment in vma_start_r= ead()). We > > > > + * can avoid that by directly locking vm_lo= ck under > > > > + * mmap_lock, which guarantees that nobody = can lock the > > > > + * vma for write (vma_start_write()) under = us. > > > > + */ > > > > + down_read(&vma->vm_lock->lock); > > > > > > Hi Andrii, > > > The above pattern of locking VMA under mmap_lock and then dropping > > > mmap_lock is becoming more common. Matthew had an RFC proposal for an > > > API to do this here: > > > https://lore.kernel.org/all/ZivhG0yrbpFqORDw@casper.infradead.org/. I= t > > > might be worth reviving that discussion. > > > > Sure, it would be nice to have generic and blessed primitives to use > > here. But the good news is that once this is all figured out by you mm > > folks, it should be easy to make use of those primitives here, right? > > > > > > > > > + } > > > > + > > > > + mmap_read_unlock(mm); > > > > > > Later on in your code you are calling get_vma_name() which might call > > > anon_vma_name() to retrieve user-defined VMA name. After this patch > > > this operation will be done without holding mmap_lock, however per > > > https://elixir.bootlin.com/linux/latest/source/include/linux/mm_types= .h#L582 > > > this function has to be called with mmap_lock held for read. Indeed > > > with debug flags enabled you should hit this assertion: > > > https://elixir.bootlin.com/linux/latest/source/mm/madvise.c#L96. > > The documentation on the first link says to hold the lock or take a > reference, but then we assert the lock. If you take a reference to the > anon vma name, then we will trigger the assert. Either the > documentation needs changing or the assert is incorrect - or I'm missing > something? > > > > > Sigh... Ok, what's the suggestion then? Should it be some variant of > > mmap_assert_locked() || vma_assert_locked() logic, or it's not so > > simple? > > > > Maybe I should just drop the CONFIG_PER_VMA_LOCK changes for now until > > all these gotchas are figured out for /proc//maps anyway, and > > then we can adapt both text-based and ioctl-based /proc//maps > > APIs on top of whatever the final approach will end up being the right > > one? > > > > Liam, any objections to this? The whole point of this patch set is to > > add a new API, not all the CONFIG_PER_VMA_LOCK gotchas. My > > implementation is structured in a way that should be easily amenable > > to CONFIG_PER_VMA_LOCK changes, but if there are a few more subtle > > things that need to be figured for existing text-based > > /proc//maps anyways, I think it would be best to use mmap_lock > > for now for this new API, and then adopt the same final > > CONFIG_PER_VMA_LOCK-aware solution. > > The reason I was hoping to have the new interface use the per-vma > locking from the start is to ensure the guarantees that we provide to > the users would not change. We'd also avoid shifting to yet another > mmap_lock users. > Yep, it's completely understandable. And you see that I changed the structure quite a lot to abstract away mmap_lock vs vm_lock details. I'm afraid anon_vma_name() is quite an obstacle, unfortunately, and seems like it should be addressed first, but I'm just not qualified enough to do this. > I also didn't think it would complicate your series too much, so I > understand why you want to revert to the old locking semantics. I'm > fine with you continuing with the series on the old lock. Thanks for > trying to make this work. > I'm happy to keep the existing structure of the code, and (intentionally) all the CONFIG_PER_VMA_LOCK logic is in separate patches, so it's easy to do. I'd love to help adopt a per-VMA lock once all the pieces are figured out. Hopefully anon_vma_name() is the last one remaining :) So please keep me cc'ed on relevant patches. As I mentioned, I just don't feel like I would be able to solve the anon_vma_name() problem, but of course I wouldn't want to be completely blocked by it as well. > Regards, > Liam