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 04DD4C25B75 for ; Fri, 31 May 2024 16:37:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 91DB56B00A6; Fri, 31 May 2024 12:37:58 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8CF966B00A7; Fri, 31 May 2024 12:37:58 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7479F6B00A8; Fri, 31 May 2024 12:37:58 -0400 (EDT) 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 5446C6B00A6 for ; Fri, 31 May 2024 12:37:58 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 0ACC8C0EF9 for ; Fri, 31 May 2024 16:37:58 +0000 (UTC) X-FDA: 82179247836.12.4688A75 Received: from mail-pg1-f172.google.com (mail-pg1-f172.google.com [209.85.215.172]) by imf07.hostedemail.com (Postfix) with ESMTP id 430104000F for ; Fri, 31 May 2024 16:37:56 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=eu7jMgGM; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf07.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.215.172 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1717173476; 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=I9OsVhc7MRu17ZxqE7Uec7qOOSBjzF0OYY1R4Nallcs=; b=IpwxTjVnhxys9MaqOmEknHIsuqXPC7qnsfYaj7P+zjJi9EcG3aOU7R2ex0cNn6lz65iy6p JGSvogDvt/dmkldC1TLNoRAOwClgVJPzEOQ11UA+plP3UNZ5pG9NEbUZZApz7qj8lk1H3M Wfs9Rfyq3Tc7enOtnSQTQXsvf4M8i8Y= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=eu7jMgGM; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf07.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.215.172 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1717173476; a=rsa-sha256; cv=none; b=wmPU9VN66cuZB16ylXHv5Leoi8tnQhDv+a1CjV6BZM9F024wkowu6a7NokjD+wnjHIoqkQ dlA8GjCPXFW3WafcSzKbNWf8auWwPK1TIsUF8fsmu/xsPtRpx6eo8ybrGZJCZg+YvM0V9Q PSxhTFYJQts8ZQ0ZRsTuVp5R8VecMzw= Received: by mail-pg1-f172.google.com with SMTP id 41be03b00d2f7-6c55e3d2740so438621a12.0 for ; Fri, 31 May 2024 09:37:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1717173475; x=1717778275; 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=I9OsVhc7MRu17ZxqE7Uec7qOOSBjzF0OYY1R4Nallcs=; b=eu7jMgGMZsdu4gciEZsgjrvNjSY5ii0s5IcsnTQPXTLE5ScxWWH/gEGKwnhYEeTnio 6iL6UK1EHs+/U1/1U9JJTSci71/a6rsEEjiMvMHMktM+KQBtSY6/tyNOmR8J011ACVM0 V7bAxdLbCBHs8kW25Baw31o5dPXDZzJwuey+xb78V8raceIi24CoZakywsbjPVa1a4px yUUxq1nZaUxybfNWet/8SQsuKmq/0WVQG9Cv+3y9iv4z03MTqlNqiMB6hygAtw0kYJp3 kaTW1S8cm/eGjhbzhcyETCN9pipg/vjIaBeGoicijjJ2YT4qYufD/DHe290EuZHLlcvz Vr6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717173475; x=1717778275; 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=I9OsVhc7MRu17ZxqE7Uec7qOOSBjzF0OYY1R4Nallcs=; b=hVD+pnOTJxJlIroI74Sdi5wBVWMh9ZRt1n0HRV48waekI+lDZnK2azNOp2+PpAGe/n DkAriyy533gpxB4Ih2oK4Sm4PpnDi2MmrN30wUWXYDtqJhjlW85Sw/skHIaDWLuLyAOd Hv8KMXMz57vBN0kU+rgR1KnRf4FS7uDVVD/Uxz9HCjK5sSDt6cqUsugjWyVhFzd8v5bk xnxhON+RqLlq37W1KSHysi5HmWx+QLA8GTXlZz9Q3kEABTuqLG3EHGI/w3aepcqtT36S R+PDIDNb1B77PGV8mq7DuOGsvTWV1uhxA7K0j+xXq1Fk5WyORTOOqEQBL1dTwZd80GKO BZww== X-Forwarded-Encrypted: i=1; AJvYcCUQ1rs0LtCRjyglp4S9gyKTH4iW+20BaiRcRMMeKThr2zfC4Ms7fmCqhbMxBePSebE7M/IHBMfBBO6PbZhLiItpjyU= X-Gm-Message-State: AOJu0YyFCXk294itSpOHps4x18P02Tgn91SXrSYluJQwT8xUXKCvcgo9 CnVKrGEYytdwjLaofR1Un1CkG4krJEfvILB6VnnQcni6Pip7xPQArAjxye8WX64OHWZS+GnRv2i atyLMMxRwHYwrCxi+5L9Z5odA+sI= X-Google-Smtp-Source: AGHT+IF7FL1AYCjP1BwUnG05McUJ4KkwiviRElNE88xHJCWG+NZK2inRtrMj/0pKr5HBygGg0ozDqiOuo4p2k7eRfYU= X-Received: by 2002:a17:90a:3482:b0:2c0:19bd:543a with SMTP id 98e67ed59e1d1-2c1c43ee447mr3657114a91.9.1717173474799; Fri, 31 May 2024 09:37:54 -0700 (PDT) MIME-Version: 1.0 References: <20240524041032.1048094-1-andrii@kernel.org> <20240524041032.1048094-5-andrii@kernel.org> In-Reply-To: From: Andrii Nakryiko Date: Fri, 31 May 2024 09:37:42 -0700 Message-ID: Subject: Re: [PATCH v2 4/9] fs/procfs: use per-VMA RCU-protected locking in PROCMAP_QUERY API To: "Liam R. Howlett" , Andrii Nakryiko , 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, surenb@google.com, rppt@kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 430104000F X-Stat-Signature: 7xithyn1cu3e13gtdu8pq77s5ndjupc4 X-Rspam-User: X-Rspamd-Server: rspam11 X-HE-Tag: 1717173476-284215 X-HE-Meta: U2FsdGVkX1/wVVC1fRrC/ny2QqHUhfC1G0s+YXraCjW8eGUonWwE25X3BV6NHZxJ22iT8UNoZJQY/ypazkbDBVmsEaNQOD01knQf8PkCCzaJy3lJU7gsIqNe0kAnfJ/C+70d8LVZIQMoMiz5PKixUjJLR+2p6p4xWqSyBkG9QAExzE+qLhwa/BtiBAlHFhwyoem45d0jVi8l1Hyc96NvwKeMboefDRGdICx97yUs7/+gbwZp48eflMclLd9Pub8ycZ5bZt65c1c+00O6FzvMtoVT3QwUjJ6A8bOhGybO3dJVXxGzPJ0ejhqj/Y0AsTggS+dRbBljmJhEGPNWCfOxg0bo+X8q52MUmoJLwC69bsucAbRR4cyUtm5gH6gl+w5pndMsB87ifru3KBUwZ4zIqubVMMwvXscPZMSYMajJQ04g/eR31SH/9VqT1UkMpEjlik1faxWije2+5Ou8C18c05ZBNl5Ie4k02sqZxWEuh9yhsdyG7rTsXLjoVlhBc6sNGH+ptrGwq5YGhEZyPgca4CUqJkYo1/JOonqR4GpH1/5OzX/EUUnVvU15aelBHTzPgak7A2jx7L6+W9VWHiMDDSUJzpZvZxCiS1rDYXrdGreixmZUo6RWvig/9SzmUXxFbHi4BetF0gIHKnhNYQ8WKZzIQNDmBj6QmK49rCXRRBetXTTeWc1n/BXeAW803N3n3+HvloAxryQpbW55K5isfD8ui9DvPpHDsJ1UeZ5aV9DAKEqKGiDv29zGlfAWC87ERn0bb++A6ea7F7+8CSOb3DUaM1FBliB7pHpl6DIBOLY1mvIMBOt1FOqucqfzgDztaqIh3QLrjOH/RbQ6uKhXhXjGroQYZ5obqfDRwnOyVtfFvi1uotAmc8b6CFsjyD7M50YqZWdoU4NLoCsxlApeCmtC5VsnCV8Ei+1joDy63wUIRmcUNdiz/yRhTrOtXtxodyG+yVZfNoBcwPRBYF/ dIwZvIFW wM0uIaYpEHGlfP2355FypgHpN3WLNNYaN0Iz6fAtBJfbLle3Drod8txh279WbNn+PdEk2+JQpVWjoTNxRnnjKKJynjtilChN5tCMzC+jKmtLhoO1P+XNBGDRCqjUjZO6SLArVC5YOXU+zV+Txz5ofzXyBSylNShEjP5/h8laxPCzm9PmM9AmsjquT69/ZhvVat7upmuvQV6VU6GZuqwr8eBAVFJ/b5nSWu5jXCT9OMwPldMrTPy+t7il9+Zit0+1ixacBIuJ25Ugh4wV3pjgcNm3w/gGGkjFeNjz1kqTywN04w7TfJxyVNKdXpfgtgXjeW4Xb4F5gPvCqSg4j/Z1xTbiu2MQgPNthGSWkAW3XrAG/HhkyRixTV+xgWRCe7dh5mt8dWiInCVGwkA5KtCp3ro0Toe69cbnWTOX9Qrqarn7KIwRT5bwp38DNqAa/LlxYyQy1YfIlGmv8hh4AFpwKtiCZhDRV5Xr1O0RIsxNv0mLCsmOMnbdsBp6XadslCOYXTAmv06K85nQodN+3PpwQsbmK3gdN/s3DXpBhy0DSOn/gCjh4+JI5ATQrx8M0ZqzQKDP2ICoxklvUIHvMHr6MQ0FWa7HVJOBfmZUpO6NWn5sUiGk2m1mVZr7IUHugrSFbtTcQsSkKJ8PiNU59WidHs7Ju3VnciMk25ZOr 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 Fri, May 31, 2024 at 6:38=E2=80=AFAM Liam R. Howlett wrote: > > * Andrii Nakryiko [240528 16:37]: > > On Fri, May 24, 2024 at 12:48=E2=80=AFPM Liam R. Howlett > > wrote: > > > > > > * Andrii Nakryiko [240524 00:10]: > > > > Attempt to use RCU-protected per-VAM 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. > > > > > > Thanks for doing this. > > > > > > > > > > > Signed-off-by: Andrii Nakryiko > > > > --- > > > > fs/proc/task_mmu.c | 42 ++++++++++++++++++++++++++++++++++-------- > > > > 1 file changed, 34 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > > > > index 8ad547efd38d..2b14d06d1def 100644 > > > > --- a/fs/proc/task_mmu.c > > > > +++ b/fs/proc/task_mmu.c > > > > @@ -389,12 +389,30 @@ static int pid_maps_open(struct inode *inode,= struct file *file) > > > > ) > > > > > > > > static struct vm_area_struct *query_matching_vma(struct mm_struct = *mm, > > > > - unsigned long addr, = u32 flags) > > > > + unsigned long addr, = u32 flags, > > > > + bool *mm_locked) > > > > { > > > > struct vm_area_struct *vma; > > > > + bool mmap_locked; > > > > + > > > > + *mm_locked =3D mmap_locked =3D false; > > > > > > > > next_vma: > > > > - vma =3D find_vma(mm, addr); > > > > + if (!mmap_locked) { > > > > + /* if we haven't yet acquired mmap_lock, try to use l= ess disruptive per-VMA */ > > > > + vma =3D find_and_lock_vma_rcu(mm, addr); > > > > + if (IS_ERR(vma)) { > > > > > > There is a chance that find_and_lock_vma_rcu() will return NULL when > > > there should never be a NULL. > > > > > > If you follow the MAP_FIXED call to mmap(), you'll land in map_region= () > > > which does two operations: munmap(), then the mmap(). Since this was > > > behind a lock, it was fine. Now that we're transitioning to rcu > > > readers, it's less ideal. We have a race where we will see that gap. > > > In this implementation we may return NULL if the MAP_FIXED is at the = end > > > of the address space. > > > > > > It might also cause issues if we are searching for a specific address > > > and we will skip a VMA that is currently being inserted by MAP_FIXED. > > > > > > The page fault handler doesn't have this issue as it looks for a > > > specific address then falls back to the lock if one is not found. > > > > > > This problem needs to be fixed prior to shifting the existing proc ma= ps > > > file to using rcu read locks as well. We have a solution that isn't > > > upstream or on the ML, but is being tested and will go upstream. > > > > Ok, any ETA for that? Can it be retrofitted into > > find_and_lock_vma_rcu() once the fix lands? It's not ideal, but I > > think it's acceptable (for now) for this new API to have this race, > > given it seems quite unlikely to be hit in practice. > > > > Worst case, we can leave the per-VMA RCU-protected bits out until we > > have this solution in place, and then add it back when ready. > > I've sent the patches to Suren for testing on the /proc//maps he is > doing as he could recreate this issue, but I think he is busy with other > things. They are isolated to the mm changes so I can send you the same > patches to include in this patch set. This does increase the risk of > issues with the patch set, so you can have a look and decide how you > want to proceed. Please do send them, without seeing them it's hard to make a judgement call if it's ok to proceed without them. If the changes are transparent to the rest of my changes in this patch set, of course I'd prefer to keep it simpler and land them separately. > > > > > > > > > > + /* failed to take per-VMA lock, fallback to m= map_lock */ > > > > + if (mmap_read_lock_killable(mm)) > > > > + return ERR_PTR(-EINTR); > > > > + > > > > + *mm_locked =3D mmap_locked =3D true; > > > > + vma =3D find_vma(mm, addr); > > > > > > If you lock the vma here then drop the mmap lock, then you should be > > > able to simplify the code by avoiding the passing of the mmap_locked > > > variable around. > > > > > > It also means we don't need to do an unlokc_vma() call, which indicat= es > > > we are going to end the vma read but actually may be unlocking the mm= . > > > > > > This is exactly why I think we need a common pattern and infrastructu= re > > > to do this sort of walking. > > > > > > Please have a look at userfaultfd patches here [1]. Note that > > > vma_start_read() cannot be used in the mmap_read_lock() critical > > > section. > > > > Ok, so you'd like me to do something like below, right? > > > > vma =3D find_vma(mm, addr); > > if (vma) > > down_read(&vma->vm_lock->lock) > > mmap_read_unlock(mm); > > > > ... and for the rest of logic always assume having per-VMA lock. ... > > > > > > The problem here is that I think we can't assume per-VMA lock, because > > it's gated by CONFIG_PER_VMA_LOCK, so I think we'll have to deal with > > this mmap_locked flag either way. Or am I missing anything? > > The per-vma lock being used depends on the CONFIG_PER_VMA_LOCK, so that > flag tells us which lock has been taken. > > > > > I don't think the flag makes things that much worse, tbh, but I'm > > happy to accommodate any better solution that would work regardless of > > CONFIG_PER_VMA_LOCK. > > > > > > > > > + } > > > > + } else { > > > > + /* if we have mmap_lock, get through the search as fa= st as possible */ > > > > + vma =3D find_vma(mm, addr); > > > > > > I think the only way we get here is if we are contending on the mmap > > > lock. This is actually where we should try to avoid holding the lock= ? > > > > > > > + } > > > > > > > > /* no VMA found */ > > > > if (!vma) > > > > @@ -428,18 +446,25 @@ static struct vm_area_struct *query_matching_= vma(struct mm_struct *mm, > > > > skip_vma: > > > > /* > > > > * If the user needs closest matching VMA, keep iterating. > > > > + * But before we proceed we might need to unlock current VMA. > > > > */ > > > > addr =3D vma->vm_end; > > > > + if (!mmap_locked) > > > > + vma_end_read(vma); > > > > if (flags & PROCMAP_QUERY_COVERING_OR_NEXT_VMA) > > > > goto next_vma; > > > > no_vma: > > > > - mmap_read_unlock(mm); > > > > + if (mmap_locked) > > > > + mmap_read_unlock(mm); > > > > return ERR_PTR(-ENOENT); > > > > } > > > > > > > > -static void unlock_vma(struct vm_area_struct *vma) > > > > +static void unlock_vma(struct vm_area_struct *vma, bool mm_locked) > > > > > > Confusing function name, since it may not be doing anything with the > > > vma lock. > > > > Would "unlock_vma_or_mm()" be ok? > > The way that seemed most clear in the userfaultfd code > (/mm/userfaultfd.c), seemed to focus on what we were undoing instead of > the lock we were unlocking. Instead of saying "unlock one or the other" > we have "uffd_mfill_unlock()", and have two versions of that function > that take the same argument. This way we can have the same blocks of > code calling the same thing, with a different lock/unlock happening > based on the CONFIG_PER_VMA_LOCK compile time option. If that makes > sense to you, then I'd prefer it over the other options - none are > ideal. ok, two implementations of the same function sounds fine to me, I'll do that in the next revision, thanks! > > Note that people didn't like the "unlock_" name, even on static > functions as it implies it can be used everywhere and may conflict with > a global function in the future [1]. yep, ack > > [1] https://lore.kernel.org/linux-mm/20240426144506.1290619-4-willy@infra= dead.org/ > > Thanks, > Liam