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 3798FC3DA41 for ; Wed, 10 Jul 2024 17:14:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 92EC56B0089; Wed, 10 Jul 2024 13:14:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8DDCF6B008A; Wed, 10 Jul 2024 13:14:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7A5A56B0096; Wed, 10 Jul 2024 13:14:50 -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 5CA466B0089 for ; Wed, 10 Jul 2024 13:14:50 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 2283E401B4 for ; Wed, 10 Jul 2024 17:14:50 +0000 (UTC) X-FDA: 82324492740.28.BAA1268 Received: from mail-yw1-f169.google.com (mail-yw1-f169.google.com [209.85.128.169]) by imf13.hostedemail.com (Postfix) with ESMTP id 4D23720009 for ; Wed, 10 Jul 2024 17:14:48 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=UFv10UwC; spf=pass (imf13.hostedemail.com: domain of surenb@google.com designates 209.85.128.169 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=1720631656; 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=Zxl2kkis+kjcyTxFXjBubE9OIoPPn2QmhQmRL9LDMpA=; b=M6xA4uBKhY+X6WHSlvNABsLN7pmnnwYXh7W1jU8kqUtNvDw8+p/GBh33TdZbHAbvH+LR7p oPBwzwdzpdnsgT9gt+lDCvhUghTz95fHNNz9S+VEtjp2DptM+Id7YrpoA60yi02Mwa4Gxa /W5Txu4nG+FZFpuJDb9m8fiYYdHMAkk= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1720631656; a=rsa-sha256; cv=none; b=L5jKNLy0j+L0UqFIw4gB97u5RQgfw9Ny+oWpX0pPMyAaIc6n8FOd07S+2FZXhb8aotr5mF bEAjBflgstA31T3W/k05+rAKe52WPRkfPLCdXpz3S6jjPLvZNfCRBmWUDAbSRiPYjmW62Q n20geDGYwePUODxsMAVOhu/jSSMi4l8= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=UFv10UwC; spf=pass (imf13.hostedemail.com: domain of surenb@google.com designates 209.85.128.169 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-yw1-f169.google.com with SMTP id 00721157ae682-65bceed88a2so7883297b3.1 for ; Wed, 10 Jul 2024 10:14:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1720631687; x=1721236487; 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=Zxl2kkis+kjcyTxFXjBubE9OIoPPn2QmhQmRL9LDMpA=; b=UFv10UwCxltvKGAiX0W602a02kWqhLjShW9A1O2k5fX+SSyKePifeXKka69I5Xirg3 wXdRM7LxftMCCF/W43Nj3lpyS/Qdn3y5ucrlaz6tj5onMFPZ+xlEQnSdfyTWoOqN6KWg YcH2yycMglMsoIJSdle4ObBgrqa49sN7XgRWSL+hOKVCFYuA+jFaKqFHQ9daqhdTP68u 6V6w0Ff8FjdavhIs1yi/GT32NIObBNXGD8v0TA4IjDgjMESBy99+TVMyksVzQYK1/U0a WsGAn4yHxmPqaknhv444svp9N+vzbHufjcLi/bWBdboU8rzt3tSn8RPX6PTOa4R/9CdR 4BtQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1720631687; x=1721236487; 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=Zxl2kkis+kjcyTxFXjBubE9OIoPPn2QmhQmRL9LDMpA=; b=e3JLcHrEdqZTkWn+MN6I01Y6BO1/CWz3mgrhHtCGojx64SwHuYpCszz1c8xGQNPZ29 TdApSvtKdjyaOVJhXgzXfUoYmqYwlPlcyTO4CrJo/oSaOJrIbsh//XQmFRkAvTDeat/E YLRc6QMrx3wbLg8utLC7eEKgh7a4uSpTCK4NZWi7PtGE0naH3CIT39VHBC4lMRjOYejw UkJY+iutAP1i62uXPQbr+jPRl0jxwUrwBSVHPFbB6qsCinF26Pt26cPKEM5FERcBxif8 6y65QbPht1oNp4CT2Jh0+ySeM7U0aLrXIi7GvmIfk9YWtXv4l+iEUcZvyOU53hxKMMdF taTA== X-Forwarded-Encrypted: i=1; AJvYcCUZYFJbzFrxVUbNh67PPN1EHyVQkm4lo4SB3vsfHXvN3zSKThcgQ9TnCFVWXMOLQPAT6fmfmrunhEdYLiRAqYIyEDo= X-Gm-Message-State: AOJu0YwHs9tTvGRELlTn5RScJ7atqAoEMmwTYBe3cqtWsQqM5zX3AoYX ZJT8I+2jIiZJYzyMQFjDrWbCDztP2xmIY16C86zr11jNDB+GD+3Wfu6IGXANi85FcBakrK3itJ4 H2zUX3AMiuVutLsZDwvpmTZuxgI6jGG0hF20y X-Google-Smtp-Source: AGHT+IGApC8Pv8BfeUz/PxcB9I5vb5voVQrPZLkDp6gNRGlMNWMZwnIL5x2ugu2cR6eLBuhRfE6vaJmn5NZuZGQgGQo= X-Received: by 2002:a81:8450:0:b0:627:dc03:575a with SMTP id 00721157ae682-658f0fb31cemr64099237b3.49.1720631686957; Wed, 10 Jul 2024 10:14:46 -0700 (PDT) MIME-Version: 1.0 References: <20240704182718.2653918-1-Liam.Howlett@oracle.com> <20240704182718.2653918-12-Liam.Howlett@oracle.com> <37ea5831-2163-4086-8b2c-baff3be2e5ad@lucifer.local> In-Reply-To: <37ea5831-2163-4086-8b2c-baff3be2e5ad@lucifer.local> From: Suren Baghdasaryan Date: Wed, 10 Jul 2024 10:14:32 -0700 Message-ID: Subject: Re: [PATCH v3 11/16] mm/mmap: Track start and end of munmap in vma_munmap_struct To: Lorenzo Stoakes Cc: "Liam R. Howlett" , linux-mm@kvack.org, Andrew Morton , Vlastimil Babka , Lorenzo Stoakes , Matthew Wilcox , sidhartha.kumar@oracle.com, "Paul E . McKenney" , Bert Karwatzki , Jiri Olsa , linux-kernel@vger.kernel.org, Kees Cook Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 4D23720009 X-Stat-Signature: fztmqgxxaokoaiuomz1ofbyznw7qzcje X-HE-Tag: 1720631688-634404 X-HE-Meta: U2FsdGVkX18SYSLsqcxR8phkY/BIQ/7cRDvMM43rydcq26jAbBOxwt+Op+NRgI/KM6/UC3bMjwy1JbhSJSf2AD9g8/HrXa76OTO7FDfpZUhLio6L/NR6MnYXNS9P5K9MIv+B+SRm+TM1z0REZzVVoByESfJLaFY0IiJ/s4+dwG4ecnt7TrOpzvG6omRcAN487ledhv+LChdzp8VCdiLe/boRGfHZ/Ffh/z04MOIjsfZtWlyKiNRyfv8fapY2rGa1vdOWA+/6laB93MFjh7kI1k/eUmgFv39/WsAZdRYUIpxXmw50G8mF8igzjCcQPK3vzxXpx7C4ntkA6Nd+/kQPjTklIdQLwbywvfavEEL/yEcg2wjfy67lobiNa/hVjV191I91AO5Z8AlCNoXL0Rwn/Dy5dKQTELVAAILIM+mXkwKUy0Lwj9nei+IhcqUf2LTUsmF9TmezKH59+v5rFhiXtiKagufucE+qMteGBgHHBPHoEg1A2nztCLjAmC9Azqp3m1bbB5XnLNo9nFEmg59sDelYe7KkgSc2LphU5PErw/AfgD1KdGstyV5f/bM40MOvgF5Qlh89UaBfaXCf5PEQ6bxIxq/4sgz6c3rfd19HBt2f1rEUUEug0TmqikpFEbzcFgepBeSke+4PaIrOwPlfmGEX3mrTEHE7MHkGB9kTVsRkvX4TpSbdLaRGDZf5WzunDRqMxXqA8U9D6hMrJnIkEtxUtDoBb/s3/U69PZ9Be3pC1xIsoUBHpPtmS3BXfFKIdIXQ10z9eLcXHWpvCl2f3Y4hqOKl320WjZP11WSA+X2rDga8tQuXwUNN0v7+XTqkRsEtcI9sK/FSyRGKQ1eHYCIGQHapyfTf3SM+T0R9n7qsDv9jHfskqZA+KBQiRK3XOr+g/4lzK71Im4U7gXvD4MMYmEDWf3z+T8FM2vNpz2y8bvvR6dutosaIDcVjAPYZrSRXe/iFGo9JdHa9t+D EnkJCrYN s+EOHQsz6AtoL5E8uSYRtuBWqAgYXWu8JpZWu9Cp6c/LAeAQHAmO/AgsfH+f4mj7SVgL2q+Uengk1dvcHuQOtk/8OVDsHGW8Jnw5t63aCRZauh9CBFqanXHJlV+f6FaoBSWRsR5zqdya1STJbckx/iFC5GV6ZJIAzVBy+uK3YdMenhsnrQJtFuAU4+ilX1rCY1A7jUHpOIwthDrn6sSAPzrCoyJ1K+sG7YLQAKubHmV6h7J1EdEaXDaQwitWcvOYiy5ZswFdHr6eEwK0= 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, Jul 5, 2024 at 1:27=E2=80=AFPM Lorenzo Stoakes wrote: > > On Thu, Jul 04, 2024 at 02:27:13PM GMT, Liam R. Howlett wrote: > > From: "Liam R. Howlett" > > > > Set the start and end address for munmap when the prev and next are > > gathered. This is needed to avoid incorrect addresses being used durin= g > > the vms_complete_munmap_vmas() function if the prev/next vma are > > expanded. > > When we spoke about this separately you mentioned that specific arches ma= y > be more likely to encounter this issue, perhaps worth mentioning somethin= g > about that in the commit msg? Unless I misunderstood you. > > > > > Add a new helper vms_complete_pte_clear(), which is needed later and > > will avoid growing the argument list to unmap_region() beyond the 9 it > > already has. > > My word. > > > > > Signed-off-by: Liam R. Howlett > > --- > > mm/internal.h | 2 ++ > > mm/mmap.c | 34 +++++++++++++++++++++++++++------- > > 2 files changed, 29 insertions(+), 7 deletions(-) > > > > diff --git a/mm/internal.h b/mm/internal.h > > index 8cbbbe7d40f3..4c9f06669cc4 100644 > > --- a/mm/internal.h > > +++ b/mm/internal.h > > @@ -1493,6 +1493,8 @@ struct vma_munmap_struct { > > struct list_head *uf; /* Userfaultfd list_head */ > > unsigned long start; /* Aligned start addr */ > > unsigned long end; /* Aligned end addr */ > > + unsigned long unmap_start; > > + unsigned long unmap_end; > > int vma_count; /* Number of vmas that will be re= moved */ > > unsigned long nr_pages; /* Number of pages being removed = */ > > unsigned long locked_vm; /* Number of locked pages */ > > diff --git a/mm/mmap.c b/mm/mmap.c > > index ecf55d32e804..45443a53be76 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -525,6 +525,8 @@ static inline void init_vma_munmap(struct vma_munma= p_struct *vms, > > vms->vma_count =3D 0; > > vms->nr_pages =3D vms->locked_vm =3D vms->nr_accounted =3D 0; > > vms->exec_vm =3D vms->stack_vm =3D vms->data_vm =3D 0; > > + vms->unmap_start =3D FIRST_USER_ADDRESS; > > + vms->unmap_end =3D USER_PGTABLES_CEILING; > > } > > > > /* > > @@ -2610,6 +2612,26 @@ static inline void abort_munmap_vmas(struct ma_s= tate *mas_detach) > > __mt_destroy(mas_detach->tree); > > } > > > > + > > +static void vms_complete_pte_clear(struct vma_munmap_struct *vms, > > + struct ma_state *mas_detach, bool mm_wr_locked) > > +{ > > + struct mmu_gather tlb; > > + > > + /* > > + * We can free page tables without write-locking mmap_lock becaus= e VMAs > > + * were isolated before we downgraded mmap_lock. > > + */ > > + mas_set(mas_detach, 1); > > + lru_add_drain(); > > + tlb_gather_mmu(&tlb, vms->mm); > > + update_hiwater_rss(vms->mm); > > + unmap_vmas(&tlb, mas_detach, vms->vma, vms->start, vms->end, vms-= >vma_count, mm_wr_locked); > > + mas_set(mas_detach, 1); > > I know it's necessary as unmap_vmas() will adjust mas_detach, but it kind > of aesthetically sucks to set it to 1, do some stuff, then set it to 1 > again. But this is not a big deal :>) > > > + free_pgtables(&tlb, mas_detach, vms->vma, vms->unmap_start, vms->= unmap_end, mm_wr_locked); > > Yeah this bit definitely needs a comment I think, this is very confusing > indeed. Under what circumstances will these differ from [vms->start, > vms->end), etc.? > > I'm guessing it's to do with !vms->prev and !vms->next needing to be set = to > [FIRST_USER_ADDRESS, USER_PGTABLES_CEILING)? > > > + tlb_finish_mmu(&tlb); > > +} > > + > > /* > > * vms_complete_munmap_vmas() - Finish the munmap() operation > > * @vms: The vma munmap struct > > @@ -2631,13 +2653,7 @@ static void vms_complete_munmap_vmas(struct vma_= munmap_struct *vms, > > if (vms->unlock) > > mmap_write_downgrade(mm); > > > > - /* > > - * We can free page tables without write-locking mmap_lock becaus= e VMAs > > - * were isolated before we downgraded mmap_lock. > > - */ > > - mas_set(mas_detach, 1); > > - unmap_region(mm, mas_detach, vms->vma, vms->prev, vms->next, > > - vms->start, vms->end, vms->vma_count, !vms->unlock); > > + vms_complete_pte_clear(vms, mas_detach, !vms->unlock); > > /* Update high watermark before we lower total_vm */ > > update_hiwater_vm(mm); > > /* Stat accounting */ > > @@ -2699,6 +2715,8 @@ static int vms_gather_munmap_vmas(struct vma_munm= ap_struct *vms, > > goto start_split_failed; > > } > > vms->prev =3D vma_prev(vms->vmi); > > + if (vms->prev) > > + vms->unmap_start =3D vms->prev->vm_end; > > > > /* > > * Detach a range of VMAs from the mm. Using next as a temp varia= ble as > > @@ -2757,6 +2775,8 @@ static int vms_gather_munmap_vmas(struct vma_munm= ap_struct *vms, > > } > > > > vms->next =3D vma_next(vms->vmi); > > + if (vms->next) > > + vms->unmap_end =3D vms->next->vm_start; > > > > #if defined(CONFIG_DEBUG_VM_MAPLE_TREE) > > /* Make sure no VMAs are about to be lost. */ > > -- > > 2.43.0 > > > > Other than wanting some extra comments, this looks fine and I know how > hard-won the unmap range bit of this change was so: > > Reviewed-by: Lorenzo Stoakes Ok, another case when code duplication will be removed in the next patch. L= GTM. Reviewed-by: Suren Baghdasaryan