Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Tue, 10 Oct 2023 21:51:40 -0400
From:   "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To:     Lorenzo Stoakes <lstoakes@gmail.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>,
        Vlastimil Babka <vbabka@suse.cz>, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2 4/5] mm: abstract merge for new VMAs into
 vma_merge_new_vma()
Message-ID: <20231011015140.arngzv47bdyyzfie@revolver>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
        Lorenzo Stoakes <lstoakes@gmail.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>,
        Vlastimil Babka <vbabka@suse.cz>, linux-fsdevel@vger.kernel.org
References: <cover.1696884493.git.lstoakes@gmail.com>
 <8525290591267805ffabf8a31b53f0290a6a4276.1696884493.git.lstoakes@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <8525290591267805ffabf8a31b53f0290a6a4276.1696884493.git.lstoakes@gmail.com>
User-Agent: NeoMutt/20220429
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bGphTS94ZnN0aVZ0bFFSSGo5OEczQ2NabDBRTDZxQ3JWK0pEaDBucjhMeU9w?=
 =?utf-8?B?TnNTQW9zVThPNjdQTHAweERYNnltaUFPVktCTjhwMzF1VFhLM1BRN3RUUlV1?=
 =?utf-8?B?Yys5MHVHZHgwWXowYkQyU0RLdkVaWXViWkVycGRDdjBsemFqS21tYzVpQTVJ?=
 =?utf-8?B?ZjZhNC9qQTdmT0hOZllXY3VrbE1hMG4xSEY5L3VkTXNvWEFBNkMxS0krRDlR?=
 =?utf-8?B?ZUp0dk9TN3JZVmlva0xYZGdMZTdzMUhGMzhDZjNWWWZaREZhR2R0UlV2K1Mw?=
 =?utf-8?B?SmF4bnZxaWNMRmF1ZElXL3FrZktrbnIvaGNSRzNpS2hGOTlrUFZ0a1FOTkdo?=
 =?utf-8?B?TWlkMlR0d2dKdENzSmNYcmRGMXFHdk4yWHhISGhDeXp2OEFKOW4wZnlsZ3R3?=
 =?utf-8?B?SlJiZFRtVEVRdDExRURDQnBWa2pGRnhXY2RxVllOd0VQaUZGNnRHaU1tRGw1?=
 =?utf-8?B?TkdIRUloSXNsYm9GakxOcitrL3dISXJUZjNyUGxVYlQzM2VEc2Q3WE9odGZo?=
 =?utf-8?B?WkVrMzhscDZlR2JGejRIMkFKWWxCcjljZW1teG9Rc0dmMFYwWUZjdEpjSDMx?=
 =?utf-8?B?a2ZRdStMSHh1ZVFyaVN1S1hBNVRDbWFQZENPWWgwbWFBSVVmZnNuZ0VOY0tk?=
 =?utf-8?B?dXkxWDNlZnV3d0VyMmk5TzVNQmZEZmh0NVREVytNOEFEYTZFek9LN1NwSGtM?=
 =?utf-8?B?MVBRYTN1RDUyOVkrSjhqdUhKb3EycjZYTzdYY3NsR0RjUG9RKzQ5bFJnTUZv?=
 =?utf-8?B?WDhMM3pMaHNFTlpTbDJhUDM5WkV4NU1tQU43UUF0QWNNazNIKzV1Z0J3c2s2?=
 =?utf-8?B?Y282Wm5GWU9YWFc3SW56MTkyWjdlT284WDRFUC9KSjR6UVBrdm9IcUlrUkp0?=
 =?utf-8?B?QUg0MEFmdWlsMFVCVDZiOUZTNkJod2JKVW1VNXZhdHlKYVhTaGhubXNaWmhH?=
 =?utf-8?B?d2szWnQwbHl4MFd2ZGV6WENtYnRpY0ozZy9mZU5yblBaL1B4OElvOWdFMkxp?=
 =?utf-8?B?OUd3VXdDU2RJcGNsTzBYNTFneUMxUWNUWlphMkNCZjRZWUQvZnp6RnVUb1JY?=
 =?utf-8?B?QlZoa094NDZHUUgzcXRBalFWL0swMmZTV3M4NnpBd2doVm5YZnNzNUFiaTUx?=
 =?utf-8?B?bUxBUHBqOUQ3bW5sVW05YXh1UlBlWFo4d3lTT3p3cFp0NFZMdWlCYTBtdnM4?=
 =?utf-8?B?aUhDby9YMTZ5NmRQalIwN2ptbW5FU1JadlNzbFptSEp3WVgvR3lvaGtwUURE?=
 =?utf-8?B?TFQxVlU4eVhmTTB5dE1iSDRJdjU1Qnc0dllIMnFNd0xXTXJlMjlzWFowaS92?=
 =?utf-8?B?QzNsT0NQQWZiQVRYOEliOGRnemE3cmpNWElndnpnVzNTeGxUWlNtemx5UnpS?=
 =?utf-8?B?RU1EdExETEg2UXViZytsTVArT2JuQU1zVVVYOHdRWWd3akdrZFdMeENHckRz?=
 =?utf-8?B?OGRBbnZZMTNBYm1lY2ZBU2Z1QkUxU3hQNWk2RS9aQ3hEL1Y2WmcydENSSUtu?=
 =?utf-8?B?N3oreGZDN0I5aXllR3h3NU9mVlJXSVZraEtjeEtwS3AxVDBSb0w2U09RZHE0?=
 =?utf-8?B?K0gzNXJxNW9CeFkrUjV2YlozNjNFQVpEVXRTK3hFNThQVTlydnRzV3Uvajd5?=
 =?utf-8?B?OXhuaHpSTEx1MUF6K1JDUFQ2UnJOVlJKeUJiOVYzQVZtZEFVU2U4VFVaSVc1?=
 =?utf-8?B?em1DM2k4NE1ZYm1CUVhkbzN5TWk3MW5GbmVKVzVBdE82dWd5c2JwU1dyYng5?=
 =?utf-8?B?dWk5MlkxeE4yZGM3Wkoza0JhNkU3Z0s1amdneXhpNFJmY1dmNXBocHY0MFlP?=
 =?utf-8?B?cmQ0clprSjBnblVndG5ZVC9RNlowMTFwNFdqR05lN3pBcFJrVGQyNTJzcFhk?=
 =?utf-8?B?dXBJTVRZNDNtN3QrUU9lcDBQaUY1bm54d2lJcDF6MTRFVTJpemZ6Yzhmc1NY?=
 =?utf-8?B?S291ajZRQmh4Ukp0YjdHMXJjVE13OFZRVHhFUTJWRGhZOWViOHY1a2FoRTRi?=
 =?utf-8?B?RnlSMDlxT3lUZmxEOHNOaTRPeWY3M2t5S2xGZTJPRFNmMy9SL0lWTnVNbTBB?=
 =?utf-8?B?QTZhYUtLY0FiR3VoME5qaXJqSm5qL3gxNkFaZjlVWmZBaUVUcDZXWW5uTFpM?=
 =?utf-8?B?VlpieTh2bjNzdWt4bFdjcEMwZnplVWZ1UVRaV2NGTCtmaSs5OGdidHdwbXR6?=
 =?utf-8?B?Ync9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?QUFHRE5rQldKNHBQNC84L1RJZDd2NERPaldyTmFPYTFkNVpqRnJGSURKZTJz?=
 =?utf-8?B?TUdqeUd6WlJXQXpDZW9SekJURkNsTmdxWDRESjVSdDJmclJ3VDNSNjVQS3JN?=
 =?utf-8?B?OVlvTWJxRDB0UExJaVkveG1hR0FrSS9rVThaK0crWjJXRk9YaG5rdVE4ckFh?=
 =?utf-8?B?b1VTT3BNYzlNUVowRDN0NWJIckRhMThnTmowOUlTYnFaUTN5MUNlaE4zYzVw?=
 =?utf-8?B?Sk9WR2F6eHBIQnZMbER1LzEvWlE1RExXcElYQW5ncS9BQ1N2dExJd2FudDQ0?=
 =?utf-8?B?dWFXUkt6ZExtc1JDdng4YXJZUmM4bWhTa2cxMlRaS2loQmxQVVVkeGJvSUdV?=
 =?utf-8?B?aTJtOTRBRTlZZS9RL3NFQ2pXSFBETnpOV3hFUzZVd28vU2Q4TWVlTUtQSitr?=
 =?utf-8?B?bFpZSXFoVlljSUJQSTNrRWhrSGlQNTMyU3FCWk9OZ3pLak0rY2V3YjgweThn?=
 =?utf-8?B?NWFWSi9LaklSSGozb3lMTU1ySnlxbUhRY21kOThHem5FOG1NTmR5K2R3SDR2?=
 =?utf-8?B?RXFzcUFiVkZWY0htSitlOEZPeW9UcGZJSS9UYTNOVW9iYjY2ZzNvcUIxb1Jm?=
 =?utf-8?B?bWpUZ3NVTDBGMFJ3STVqdnlGR0pTK3AxSm1sejl0Rkt5UjdORmxLRldBQWNo?=
 =?utf-8?B?YU54K3p0VlhkOFgzT1hRL2hEOFpSa0VDNGU1b1JtZFhiOVNveDdVaG11Lzd2?=
 =?utf-8?B?bWM4d3dqZGsxOGlGWHQ0STFxbDRmbHd6c01BN3pHRXFycVkzZE1ZSE16bkZq?=
 =?utf-8?B?VVRzSEpSUVFQdjU3UllKUnhiYVd1RlMxNDlpR2llMkhnK2EwRHVlc1BWWjBD?=
 =?utf-8?B?Mk1JZmJ3Mk9mRWlkZXU3TzIxZEE2VThDLzlRZzJhTytjT1RncktvZzhHSm4z?=
 =?utf-8?B?cXFiMnhkYVllanM3eVk4dXpDWktRb3NpWXJ3UjFPWGhTaGtDcXZBUzdHS2VE?=
 =?utf-8?B?WHdsOUwvRkhJT3N0MmxHcVI0ZTJkM1hVYUFtQUxVVjcrMnA4bEJNRk5hMmE3?=
 =?utf-8?B?N1U2Nkk4ZHE4bGNjdjlmRkRUeDhURUNYUmtqWEdTUnIrYVN0dC9VYWFiS0VU?=
 =?utf-8?B?dHpvTzgvbHRSNktWV2wzdHgrb2dTaFhRbkpTMisrNUREZ0xTcnBjZEtxclRC?=
 =?utf-8?B?VEx2UG5GQW55U0tJdWN5MWRHWlN4QVJwdVFPU0xHQnpseTlCWjNXaWlrSVdT?=
 =?utf-8?B?R1ZSVC8zWEE2WVNoL0pyUVN6cStGV29DMFZiaFoxQnJrbnRsaVFra2o3K2dw?=
 =?utf-8?B?NVQ5WUNHZjhDRGJpdUJhOGJMaUVBUzBUeEFVQVhxTER3OFAyUjQ2RGREZFJK?=
 =?utf-8?Q?UY3s8b5UDcw1U=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 008f7d50-e9c8-452d-10cf-08dbc9fc9ec0
X-MS-Exchange-CrossTenant-AuthSource: SN6PR10MB3022.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Oct 2023 01:51:43.7764
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ZEsfbpM3ASkqBoSRiM56X3nlUkhl7DG26J2peVD5DtIVBjUGOS0UG/Lx3891uDJ8kwZWSVt1Vhna2hH0VQJLog==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR10MB6258
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.267,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-10-10_19,2023-10-10_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 spamscore=0 adultscore=0
 suspectscore=0 mlxlogscore=926 phishscore=0 bulkscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2309180000
 definitions=main-2310110015
X-Proofpoint-ORIG-GUID: knl4WPuuBihT2ORrjrlMZp_F59lQTc5i
X-Proofpoint-GUID: knl4WPuuBihT2ORrjrlMZp_F59lQTc5i
X-Spam-Status: No, score=2.7 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Tue, 10 Oct 2023 18:52:23 -0700 (PDT)
X-Spam-Level: **

* Lorenzo Stoakes <lstoakes@gmail.com> [231009 16:53]:
> Only in mmap_region() and copy_vma() do we attempt to merge VMAs which
> occupy entirely new regions of virtual memory.
>=20
> 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.
>=20
> This also paves the way for a simplification of the core vma_merge()
> implementation, as we seek to make it entirely an implementation detail.
>=20
> 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.

>=20
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> ---
>  mm/mmap.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
>=20
> 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_itera=
tor *vmi,
>  	return NULL;
>  }
> =20
> +/*
> + * Attempt to merge a newly mapped VMA with those adjacent to it. The ca=
ller
> + * 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.

> +{
> +	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, unsig=
ned long addr,
>  		 * vma again as we may succeed this time.
>  		 */
>  		if (unlikely(vm_flags !=3D vma->vm_flags && prev)) {
> -			merge =3D 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 =3D vma_merge_new_vma(&vmi, prev, vma,
> +						  vma->vm_start, vma->vm_end,
> +						  pgoff);
                                                   =E2=94=94 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_stru=
ct **vmap,
>  	if (new_vma && new_vma->vm_start < addr + len)
>  		return NULL;	/* should never get here */
> =20
> -	new_vma =3D 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 =3D vma_merge_new_vma(&vmi, prev, vma, addr, addr + len, pgoff)=
;
>  	if (new_vma) {
>  		/*
>  		 * Source vma may have been merged into new_vma
> --=20
> 2.42.0
>=20