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 B025AC4345F for ; Fri, 12 Apr 2024 14:53:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1EFBC6B0092; Fri, 12 Apr 2024 10:53:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1A05E6B0093; Fri, 12 Apr 2024 10:53:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 068AD6B0099; Fri, 12 Apr 2024 10:53:45 -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 DB6EB6B0092 for ; Fri, 12 Apr 2024 10:53:44 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 9D83E160E27 for ; Fri, 12 Apr 2024 14:53:44 +0000 (UTC) X-FDA: 82001173968.28.E66970A Received: from mail-yb1-f174.google.com (mail-yb1-f174.google.com [209.85.219.174]) by imf20.hostedemail.com (Postfix) with ESMTP id E5A751C0023 for ; Fri, 12 Apr 2024 14:53:41 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=iSW1XJyd; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf20.hostedemail.com: domain of surenb@google.com designates 209.85.219.174 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=1712933621; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=5K/DJBnoyvERt3WtfT7CAOLfE3UxNuInZh7dWu2DHmg=; b=eCjvwfsS20bRp5xPhlm1FjzDm8+jRQ7+z0TOiagdM1kYFFnDSphauii15WRfexyMUp8YDF KMDk0GGQXH/CnNavuvfSFRd/xx2N2LotrvjtK1PCpJNZaKVmGdkmZc0NphrZFpQ2x+b2G0 Hkc4rKF9/Yvbsuc+esHV3m2gskQp2iU= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=iSW1XJyd; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf20.hostedemail.com: domain of surenb@google.com designates 209.85.219.174 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1712933621; a=rsa-sha256; cv=none; b=6SFQx22IQHyKce1DfY44aD9NnfPraq5O4tSNLGun02Zkx13LcPf6L6B+An38nK1txPo5jB BlVszp6hvd631vmioy1wVLjkcSir/msdvahiNYV8iLCG10LeyAmxyKQv4rK/O+Ff+hO3PZ rS7M9EjjyM50qxOyiELH/Cf0jg31+5E= Received: by mail-yb1-f174.google.com with SMTP id 3f1490d57ef6-d9b9adaf291so1041809276.1 for ; Fri, 12 Apr 2024 07:53:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1712933621; x=1713538421; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=5K/DJBnoyvERt3WtfT7CAOLfE3UxNuInZh7dWu2DHmg=; b=iSW1XJydpJ0kPaLelXPBR6SdOsE28SSMFvyT69ZnPMHXVH62QV84+ejnzvEw9qId95 4geg4f6i5FwmHp8cm6Yowqlz+d16sKCh/ZnbsPt16d26SUWHMUAIGk8j7TWJHChn2wj+ yP3+/OspJnHSCNGJx61yZyo5QspDvgaeVVSuTX9v9dyqBHN06oknPzjD0DZJlcKbyj2E XiRYgr0m0EaxTU7XzOCt52TEeeVSNwb78zTVjqh5V5cCMeEFxHnx1G8NBRbEzUCHEQh6 OzqZ6JgyK6z0LPrwX0qluMEVXGxPoTtWCvshbyWvDQXX83whutf01uyl2uqoiFrzzSpj 5GuQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712933621; x=1713538421; h=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=5K/DJBnoyvERt3WtfT7CAOLfE3UxNuInZh7dWu2DHmg=; b=mHXPJTjfZFVFGXAsGbmiwKHcc66SyvU7eiZofRRVJWoZQeZnJbsvix5cVFtQ+gGqJq 1K3Nzlwbtcs+CLs3mu6BVgEgcJiaw9N31NvHWJ/IlBDm2mtVq0XuQN/4NbQ3hJ4eUPvp D+o6UFK4ySQPZxwTKbYnJwvVEhXf0At8AXoIENZnCFlFPKLYKWDocT9y37qdZVByYeWz hWaZFyPLnP6ibIOBqCLCyxyNy6rPR4f8nDOjSabjylQFG9Dg+rRuSOTz/uqH28gmTdd/ C0h1UgSElGVV3Oc+BU4t4NT4SPLiJYcF+zZZ3n4oVHsWwZytkm6dYb+9diwG7mg00CAm tnhQ== X-Forwarded-Encrypted: i=1; AJvYcCWQxu6RnP5OuLzWiHC+IKZnqp4tpTrZASPKBSzjVqhsDsrh1pd+wI2G30Qg1N10XHH7Hz8uUNKzGoce0WtqgowR1tM= X-Gm-Message-State: AOJu0YxTerTAY/kknIvuhGipCETbVMcfMdD5f1bJDT+dhnvGjhohvcHT /ZgbqAbDRRiHhbWq7aydERVfHGJx3zaj8HV7J9nkh1G6hrmZUcO3NFJW52iCd3/BxsaWCpAOXZY urne6EJu2xIMzQWcy3CLZBALVc1NT5++MvsfA X-Google-Smtp-Source: AGHT+IFHKR/qDl+DlYY4jvRMdgwnLXgJjI4wKssW7TniEbLMNpIrl/h29MIOaSBAZZQde9NxtdrVqpHlqq7bUlOb7UI= X-Received: by 2002:a25:c54f:0:b0:dd9:20d6:fd2 with SMTP id v76-20020a25c54f000000b00dd920d60fd2mr2754289ybe.27.1712933620733; Fri, 12 Apr 2024 07:53:40 -0700 (PDT) MIME-Version: 1.0 References: <20240410170621.2011171-1-peterx@redhat.com> <20240411171319.almhz23xulg4f7op@revolver> In-Reply-To: From: Suren Baghdasaryan Date: Fri, 12 Apr 2024 09:53:29 -0500 Message-ID: Subject: Re: [PATCH] mm: Always sanity check anon_vma first for per-vma locks To: Matthew Wilcox Cc: Peter Xu , "Liam R. Howlett" , LKML , linux-mm@kvack.org, Andrew Morton , Lokesh Gidra , Alistair Popple Content-Type: multipart/alternative; boundary="000000000000fd54480615e76f93" X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: E5A751C0023 X-Stat-Signature: hex976h7f73ao451tjnmpyoeenudhzyc X-Rspam-User: X-HE-Tag: 1712933621-196490 X-HE-Meta: U2FsdGVkX1/gVOXlwZnrSiTgqGk0hSO6nuy1/RJYFGBbttb1tsRbZQgiKVfBcR0/WUPqqQ/AwxscyX96Vk1/rBDi47jUqC6hQu95F9aL+k5hrJmKiQJGMQCXRGPnjM0+6utqsvCmqcLwgZVdPHoQ0v/EsUnJ+Y83nUzcpHT6wvLwSZeB54q3wW/ZdaCnabUHnRA/iYoGGiumGOHMUoq20hLYk7DEXZK/GPH+j2HcpOOH1e0XQkUjWySTmle5+CIjAq9k1LnHFIKyZEKgB5sk8R7MLOJ/K+0c/SOwRusEFQnUyAkARLFbaC//bQFqlvj2zFioTb2Wst6pQcGzFEgVN8P4FDchJwiP2j6UnP3S6KoTAiHdazTDZgUHB4T8dMcLTL7PRsrISfiCdOggJr2aUOiVn89HVCPNUGUWF18WEPxDIf6orts0ZxzKZYdbHqtlKhSQjKWP9/KGhCnlxNvO/21opVpXToGmifthLmcJyn78GrtaK9j5Fl1/zWgF41tD+z1UkdFHxumE0bg3ALjnEPcs5yWFf0eKfSD9C2DzlXniT9v2iV29o7QW2/dgEQFNxCO2LJetE+WOdtmgBzSGUYn7G2C+OhcM8xC0t9iPifh5cDx/bym9bIuhBgHaqgqTC6/suY0tbjpePkdYb4GQpzGUYiwh2c2u6RMRITygdSXVqzLDaMcVYOiCjHAiEM930+7QvUks756vNlqpq3STa1zhXGhilg3/IxYOhIHNbIodtEkEynz36ARjb/gCUK2SU2rtFdQVEeUO3h0pQ6Ki/P2wWiIo8LMn2drL/KxXlEfBvIyOEqeZPFmKM9UgBG5LAg8Xo/NRyHKsVO9hiyYbeC3gacGA1fnlkpvQUUXVxiD9IvB7tDo0Rm20K7Nj4njeaum6/XB5b/dgWaB+aGbsMJRPM9WrzJoRfEvby3bv6limOr4u/mSluTyt14OBQa5URE9jOXuuj1F5LxGqB3L k1rznoXA z0tB0C8V2NOcwFmpqZG24hv7vbgi/XuHcJoHMWO1yUnoUVoKOozlhAo/8I64aOPBxcfe/eam+71vkV/OKliRwXGAjfHCLp0HDpLaVD9Cu+kv+JVPuQrqtbQVmwbfdCuvKfbmDoRrn6Xzk6IZye8eh7OjHX7HHSsHEuwRe7od52POniX31FceD439g/tdIFwgz6/v8yLiHrN6CG5fw9mslJ0tGDVOfeTtOdTO/x3XEpPXunTce8w3XLiAfMTeBaQduzvODDpaRAH+fZFY+MSD8iNDbPHNymaeHPbU7csRhE4wGMaMeiG7kLokXoJD8F34vmHU3PdyDym7uLkn9/PCEwwaxzGEkXlcx8qPZy+3jlXUR+6p9UWxjtspMpGCbNnTpdoMeXyqR2KSL2VeHzlPKQTSA0Cbi6b1rNDpUplA8vXCza1DqlxUD/FB5Ig== X-Bogosity: Ham, tests=bogofilter, spamicity=0.001569, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: --000000000000fd54480615e76f93 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Apr 12, 2024, 9:16=E2=80=AFAM Matthew Wilcox = wrote: > On Fri, Apr 12, 2024 at 06:06:46AM -0700, Suren Baghdasaryan wrote: > > > On Fri, Apr 12, 2024 at 04:14:16AM +0100, Matthew Wilcox wrote: > > > > - /* > > > > - * find_mergeable_anon_vma uses adjacent vmas which are not > locked. > > > > - * This check must happen after vma_start_read(); otherwise= , > a > > > > - * concurrent mremap() with MREMAP_DONTUNMAP could > dissociate the VMA > > > > - * from its anon_vma. > > > > - */ > > > > - if (unlikely(vma_is_anonymous(vma) && !vma->anon_vma)) > > > > - goto inval_end_read; > > > > - > > > > /* Check since vm_start/vm_end might change before we lock > the VMA */ > > > > if (unlikely(address < vma->vm_start || address >=3D > vma->vm_end)) > > > > goto inval_end_read; > > > > > > > > That takes a few insns out of the page fault path (good!) at the co= st > > > > of one extra trip around the fault handler for the first fault on a= n > > > > anon vma. It makes the file & anon paths more similar to each othe= r > > > > (good!) > > > > > > > > We'd need some data to be sure it's really a win, but less code is > > > > always good. > > > > I agree, if we make this change we should keep this comment and maybe > > move it into vmf_anon_prepare() > > Most of the comment makes no sense if you move it out of > lock_vma_under_rcu(). It's justifying where it needs to be in that > function. If it's no longer in that function, there's not much left of > the comment. What part do you think is valuable and needs to be retained= ? > Unless vmf_anon_prepare() already explains why vma->anon_vma poses a problem for per-vma locks, we should have an explanation there. This comment would serve that purpose IMO. > --000000000000fd54480615e76f93 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Fri, Apr 12, 2024, 9:16=E2=80=AFAM Matthew Wilcox &= lt;willy@infradead.org> wrote= :
On Fri, Apr 12, 2024 at 06:06:46A= M -0700, Suren Baghdasaryan wrote:
> > On Fri, Apr 12, 2024 at 04:14:16AM +0100, Matthew Wilcox wrote: > > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0/*
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 * find_mergeable_anon_vma uses = adjacent vmas which are not locked.
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 * This check must happen after = vma_start_read(); otherwise, a
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 * concurrent mremap() with MREM= AP_DONTUNMAP could dissociate the VMA
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 * from its anon_vma.
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0if (unlikely(vma_is_anonymous(vm= a) && !vma->anon_vma))
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto= inval_end_read;
> > > -
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Check since vm_start/vm_= end might change before we lock the VMA */
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (unlikely(address < v= ma->vm_start || address >=3D vma->vm_end))
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0goto inval_end_read;
> > >
> > > That takes a few insns out of the page fault path (good!) at= the cost
> > > of one extra trip around the fault handler for the first fau= lt on an
> > > anon vma.=C2=A0 It makes the file & anon paths more simi= lar to each other
> > > (good!)
> > >
> > > We'd need some data to be sure it's really a win, bu= t less code is
> > > always good.
>
> I agree, if we make this change we should keep this comment and maybe<= br> > move it into vmf_anon_prepare()

Most of the comment makes no sense if you move it out of
lock_vma_under_rcu().=C2=A0 It's justifying where it needs to be in tha= t
function.=C2=A0 If it's no longer in that function, there's not muc= h left of
the comment.=C2=A0 What part do you think is valuable and needs to be retai= ned?

Unless vmf_anon_prepare() already explains why vma->anon_vma poses a= problem for per-vma locks, we should have an explanation there. This comme= nt would serve that purpose IMO.




--000000000000fd54480615e76f93--