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 739D6CD98C7 for ; Wed, 11 Oct 2023 06:48:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DCAB7940008; Wed, 11 Oct 2023 02:48:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D7AC18D0002; Wed, 11 Oct 2023 02:48:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C6A5C940008; Wed, 11 Oct 2023 02:48:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id B5F038D0002 for ; Wed, 11 Oct 2023 02:48:50 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 8A7C616018D for ; Wed, 11 Oct 2023 06:48:50 +0000 (UTC) X-FDA: 81332252820.14.99CFE44 Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) by imf14.hostedemail.com (Postfix) with ESMTP id AC47010000C for ; Wed, 11 Oct 2023 06:48:48 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Vu4S2vVR; spf=pass (imf14.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.41 as permitted sender) smtp.mailfrom=lstoakes@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=1697006928; 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=cqFmO2+2FrIdfi4i9ENoBkFxqNjBz5nx6bXommNjl7M=; b=1TYgaMaz/xjTKq2uy3dNDpsGSkMc1f1/ikNhr65iHqZ8ZRhhqUMAVxPKIc7n/v70NsDdGe w+nyaBVQ2NUbDW8XEcTJVEs5eu07l0OlClsBxH5qNlk7naFKoXZB82+4RKvA58U3ybSpq8 0KJCyYHTOufpsRThRLzVNaDdhdkVtqo= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1697006928; a=rsa-sha256; cv=none; b=NwiYovXBV8z+6/xaq2CvfdtehA1A4AhB6ON4klk1dE0gAbM6Cy1b6f5s7swUCCVnT+/V9Z nldAYSkqopD5yubzEvYYSxcooNKo+kn2SoH8ygkt+ECI61g5f2pQlrYRDRLY0+AMAaqrgS eKu9BwhKnOIrNijE5oq5JJFbvJ2JTpk= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Vu4S2vVR; spf=pass (imf14.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.41 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-wm1-f41.google.com with SMTP id 5b1f17b1804b1-4065f29e933so63738185e9.1 for ; Tue, 10 Oct 2023 23:48:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1697006927; x=1697611727; darn=kvack.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=cqFmO2+2FrIdfi4i9ENoBkFxqNjBz5nx6bXommNjl7M=; b=Vu4S2vVRkAlzZFuS6y96x50BpqAZrd1sYdtzVFhc7mzGdJpu7TF0xHWWmYbyugz1oi hz/alKyWx+wgtQnkNWz8QEE7cmSxmcRhHl+n62bpmK4oOyJ23YB4jdh93/HFK5ovOsz2 JdRFVM1jnjBL+qeennUqXnZPkBjVrTRqMsp9sCCBolTcwU3V6ksOOGT+6ElE1WrSiGP3 pSsraA3FrLmeMPLavrFHJf5xVGJ1zpaO/H30sWgby+hkgGdiNG85PlhYYVFNNUe1imYX CBoG5OK/nB7te0CRtYDZIDP7LbtUi9Q7pkAtpxMyBQhPmChLO+On/I5w/Tenq0BsGxVF 1uaA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697006927; x=1697611727; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=cqFmO2+2FrIdfi4i9ENoBkFxqNjBz5nx6bXommNjl7M=; b=AovW50/6PD9NKixzgBlXXnVq/xYf2PO+2dcwZz7YeYMG7EoBlLRJOWoJBM8mAda1gb zF1HM3TjYwYACbLM8RpvZbSFXLFfEii0ty43JVbRrVIUDqMOldYeI/s6SVYhQg9/Pfy4 NLH+/O9nxDSm9lv3GZhr+VL5y6hNqUKej5IV91vuvmsWHwPDbpBdH5XNzehDiOf841zo w86mkGDh+4/MRqT4OSDIboo5yNhFHU/RLqIAaCJtuVW03LBOWY+EYlbG2Wze01QVE29i UzYj91AbpaluKs3cDn5znz82sHPqvAUfXLFq3QVc4uM/ukdyubKOxAUTrqkTDhb9hFsL K9nA== X-Gm-Message-State: AOJu0Yw1OSA3M+odM/RSPz5QwvAh5yeCzNfPXCtNwSD+4As+wA9yeW0e jFdSeGcnjYSmW4fTe8lKVhA= X-Google-Smtp-Source: AGHT+IEG2ekD3+biT2AFwRrgndK5hGQ4Qsm39lkF0PjkgCFwVoSTvwH+6Oky4EqVlW4W9Ij9Zdc8YQ== X-Received: by 2002:a05:600c:2189:b0:405:7400:1e4c with SMTP id e9-20020a05600c218900b0040574001e4cmr17992312wme.35.1697006926909; Tue, 10 Oct 2023 23:48:46 -0700 (PDT) Received: from localhost ([2a00:23c5:dc8c:8701:1663:9a35:5a7b:1d76]) by smtp.gmail.com with ESMTPSA id f12-20020a7bcd0c000000b003fefb94ccc9sm15755217wmj.11.2023.10.10.23.48.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 10 Oct 2023 23:48:45 -0700 (PDT) Date: Wed, 11 Oct 2023 07:48:44 +0100 From: Lorenzo Stoakes To: "Liam R. Howlett" , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Alexander Viro , Christian Brauner , Vlastimil Babka , linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v2 4/5] mm: abstract merge for new VMAs into vma_merge_new_vma() Message-ID: <211daa6d-220a-4477-a357-bfe9e0678fc8@lucifer.local> References: <8525290591267805ffabf8a31b53f0290a6a4276.1696884493.git.lstoakes@gmail.com> <20231011015140.arngzv47bdyyzfie@revolver> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20231011015140.arngzv47bdyyzfie@revolver> X-Rspamd-Queue-Id: AC47010000C X-Rspam-User: X-Stat-Signature: mbxbd7eptrxe7a6mmkpb1qp4qc4u1i87 X-Rspamd-Server: rspam03 X-HE-Tag: 1697006928-117098 X-HE-Meta: U2FsdGVkX1+Jp6v8XNSCpJLPIZJnUumcYlj6ciym7DriN4V8fFjZ8iqSd52F131t/A23ktm+ssze/I6cZ0kzmhzHteZrMCELg+6mJjLGan5iKbnAwo8DgN+JQziU5SAwgO1pBr1fS13KUZanOmHUD/5MxTfIgkkAKNQmP+DrUC2URMPe5ul+MiezznyPBkR7zB2F0Iz4lsyd2Cy/LJ9en2smMTugcqP1iVQdBWGriKZpV8R0521dI8IqAV6m9R+hDe9fOKASK4OaXV5bGrgBHj9nc62rTd5W2Hcs/qhFWK0SuRxFRKTt8dhewg2pKWKE/U7EiOZJdAHj/dtandlvLM35/gBUvu50IMto3qVfU6T/EZ1CHAH8Pkz/25PzEt34yeltgdbW0OZsc19JaCwhjZdxTjhrdRGFPRf0PwYrwfDUECU28yqnoBpNn0B9HZMIpmS0q+N3y4/gB3D6SjFMfAvTwizOZMoyAYqE8FBF6z9UMXT3nshP4SaLeGa+yPGJmtXtSQ4KPQHIsIeZS9vLzpbczkL83Krh5KIs5jdnxZ2cH+o4BKr4YDeKbcD5tb6ynfGrbxvvNeiX229AiApfZ942sDp5h8vfJWQwhNvd1AwCxCGPCj7NxoLah21HX0UuB6W9dm14Ir0yMF5YwXjtuUo/zZmBM4tkwCDwrUcsJpczlyXyPCxmm146LO/9Z3MrUsXvD9r2ReUzpyJQm2QtaJmNgx5wCY1TePWcQFKxDGW3DtEeYMYcvd9txxukR2Kk1ZeGc0AMYlJ+S536eU1QgGKaCrH682J+LdBHkb+8IY5RZQ0GMA10J4Z/RAeCKSNO2LmX/scePK1+4sXvE+cxlZTB3oV4UOZTZORA0ymbYbP0T2cvCMEkk19MmdLv4iLZr4qmg62wQ37j/JzXfyedMe9I+qYrCILrXqhcJ92nLQfWMhPWmaG7AvFpB11G4CcIfi1ON32cwzt0bXlm1Z5 D6eKfXRg ZzeC5I3uQVTl9nhkEcw4XZMlGASSlYTUVY6UmN3uR1lbdPoPh0PAAN7cLcPaIYRkEW9UZxCZujHyq423asHd+KaBg1qumfOklPC2VWiugNZnAYWSCo5i99WpRvoTV00nGjbafiysjFHEZ1lAnQFyOcztYiUPuqur38lToITuPhZgGSEU9pHuX8MPPGqqtE/TI64Or7H3tprCaL7tCOT+N/p0/DTTgLRLTe7wMXYAEVTgrk9DllMAtvNgc2DAPcsQ0cEAi9XrLxDM5SqfRVTqjSTnWx3nGZPOZmgWjaZoDEZ5jXGuCCcepJMfUZ0EKAlH69eq293VrZuXZn9CUu3eMdmAnksiPRSvmMxQpmRIP4tSCySb0mYlKRvpjSnS7xChqxNFYki7LY6FtSlE2SKqn7rff9Ju3gfi7mQcHn1tR6Z8F9iKDkV5TPHcsXJWD6JHIp5ljn6D5Mz+j5aZx9tUTFL0u/A== 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: On Tue, Oct 10, 2023 at 09:51:40PM -0400, Liam R. Howlett wrote: > * Lorenzo Stoakes [231009 16:53]: > > Only in mmap_region() and copy_vma() do we attempt to merge VMAs which > > occupy entirely new regions of virtual memory. > > > > We can abstract this logic and make the intent of this invocations of it > > completely explicit, rather than invoking vma_merge() with an inscrutable > > wall of parameters. > > > > This also paves the way for a simplification of the core vma_merge() > > implementation, as we seek to make it entirely an implementation detail. > > > > Note that on mmap_region(), VMA fields are initialised to zero, so we can > > simply reference these rather than explicitly specifying NULL. > > I don't think that's accurate.. mmap_region() sets the start, end, > offset, flags. It also passes this vma into a driver, so I'm not sure > we can rely on them being anything after that? The whole reason > vma_merge() is attempted in this case is because the driver may have > changed vma->vm_flags on us. Your way may actually be better since the > driver may set something we assume is NULL today. Yeah I think I wasn't clear here - I meant to say that we memset -> 0 so all fields that are not specified (e.g. not start, end, offset, flags). However you make a very good point re: the driver, which I hadn't thought of, also it's worth saying here that we specifically only do this for a file-backed mapping just for complete clarity. I will add a note to this part of the v3 series asking Andrew to update the comment. > > > > > Reviewed-by: Vlastimil Babka > > Signed-off-by: Lorenzo Stoakes > > --- > > mm/mmap.c | 27 ++++++++++++++++++++------- > > 1 file changed, 20 insertions(+), 7 deletions(-) > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > index 17c0dcfb1527..33aafd23823b 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -2482,6 +2482,22 @@ struct vm_area_struct *vma_modify(struct vma_iterator *vmi, > > return NULL; > > } > > > > +/* > > + * Attempt to merge a newly mapped VMA with those adjacent to it. The caller > > + * must ensure that [start, end) does not overlap any existing VMA. > > + */ > > +static struct vm_area_struct *vma_merge_new_vma(struct vma_iterator *vmi, > > + struct vm_area_struct *prev, > > + struct vm_area_struct *vma, > > + unsigned long start, > > + unsigned long end, > > + pgoff_t pgoff) > > It's not a coding style, but if you used two tabs here, it may make this > more condensed. Checkpatch shouts at me about aligning to the paren, I obviously could just put "static struct vm_area_struct *" on the line before to make this a bit better though. If we go to a v4 will fix, otherwise I think probably ok to leave even if a bit squished for now? > > > +{ > > + return vma_merge(vmi, vma->vm_mm, prev, start, end, vma->vm_flags, > > + vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma), > > + vma->vm_userfaultfd_ctx, anon_vma_name(vma)); > > +} > > + > > /* > > * do_vmi_align_munmap() - munmap the aligned region from @start to @end. > > * @vmi: The vma iterator > > @@ -2837,10 +2853,9 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > > * vma again as we may succeed this time. > > */ > > if (unlikely(vm_flags != vma->vm_flags && prev)) { > > - merge = vma_merge(&vmi, mm, prev, vma->vm_start, > > - vma->vm_end, vma->vm_flags, NULL, > > - vma->vm_file, vma->vm_pgoff, NULL, > > - NULL_VM_UFFD_CTX, NULL); > > + merge = vma_merge_new_vma(&vmi, prev, vma, > > + vma->vm_start, vma->vm_end, > > + pgoff); > └ vma->vm_pgoff > > if (merge) { > > /* > > * ->mmap() can change vma->vm_file and fput > > @@ -3382,9 +3397,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap, > > if (new_vma && new_vma->vm_start < addr + len) > > return NULL; /* should never get here */ > > > > - new_vma = vma_merge(&vmi, mm, prev, addr, addr + len, vma->vm_flags, > > - vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma), > > - vma->vm_userfaultfd_ctx, anon_vma_name(vma)); > > + new_vma = vma_merge_new_vma(&vmi, prev, vma, addr, addr + len, pgoff); > > if (new_vma) { > > /* > > * Source vma may have been merged into new_vma > > -- > > 2.42.0 > >