Date:   Mon, 20 Mar 2023 12:27:08 -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>,
        David Hildenbrand <david@redhat.com>,
        Matthew Wilcox <willy@infradead.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        maple-tree@lists.infradead.org
Subject: Re: [PATCH 2/4] mm/mmap/vma_merge: set next to NULL if not applicable
Message-ID: <20230320162708.xq64y4ig6f3sf747@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>,
        David Hildenbrand <david@redhat.com>,
        Matthew Wilcox <willy@infradead.org>,
        Vlastimil Babka <vbabka@suse.cz>, maple-tree@lists.infradead.org
References: <cover.1679137163.git.lstoakes@gmail.com>
 <4d717269303d8a6fe1d837968c252eeb6ff1d7e5.1679137163.git.lstoakes@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4d717269303d8a6fe1d837968c252eeb6ff1d7e5.1679137163.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: =?us-ascii?Q?HcCYqbGQIp3q40cAcNVZa6Hn3NfMCzxWt7/ZqYwiCp5dzU/E8QmboyqnU8pf?=
 =?us-ascii?Q?pv2YgyH/KrnlMLwkooHcMPYDdTjQX0EQSjPdokGQnldWRu/u7bplXEo0tW5o?=
 =?us-ascii?Q?ddRtw6i1xvYLPP53YMvLfLlqy/se+LeEyN9tqHzOo1doalk1z7AZE8GOERG+?=
 =?us-ascii?Q?aBckjNNB/laeYEgJ7Mu13f9PnCV8Hg3ScDi5HZIsOFhVpd0svY0OsQYMXNfD?=
 =?us-ascii?Q?ICFlnbEarlbdKVgSii3J7YXBV2IKEBHpiKGngdoPYNshFH/In0J3Zv39q4GZ?=
 =?us-ascii?Q?cMOjA0Bw/Qoq7W065qCIZgT6GT9JthJhE0Oap9mtdm8AtlOHOkRy16xe8G5z?=
 =?us-ascii?Q?3Zl5bQS6QndfTlV5KdzJ8qwy2SpYpvO9OXsTw6t2V6Gjn6oBAhLQiKs4I61M?=
 =?us-ascii?Q?WwTGO79EiW7ZnHb6UhpQ3dYpEHWvZ8GFzlU2vNbKhRgxNRN5RvHPaNCVv0XT?=
 =?us-ascii?Q?eK1oaa7pMW2/VVqXCgtrwL85oqDbhwyKs56kce07Q9jrCKkvehpP69Zd//W3?=
 =?us-ascii?Q?ZSlzqAc0Gzn1ip1tqXl32apL8n4VH28q0RGXF+qVDnhDUfJa+Nj91YZyDzME?=
 =?us-ascii?Q?c70wVuqx3taw0haglCRqfVnKergDuP9eD6eCrWWV9RbZsbObi1BPn3lFcJXa?=
 =?us-ascii?Q?lpmYfrbjDVWqhwWxHfyvCzxuvTeFNlCxytbhtGS2hAd/V6uw2CTzI3oqPI1i?=
 =?us-ascii?Q?M+IHZO8pcgHavUdmMz/Kao2Gyc/HQMmwmvSZCaBFrizVaM6bwKtsWHIxsWiw?=
 =?us-ascii?Q?OLpV2haxgsiT4BrLbsbD09THmzVtixzPetMTvPkubPb5LHEZH2HcGTYTncnD?=
 =?us-ascii?Q?y5hGxAb+oCXisagDJklUvqwxGAsYBsQPyzV6h9Ajg4vCxBN+WF6izEWy0+Ay?=
 =?us-ascii?Q?Vg80B80rXo/wGUksZMCTIjdbWaZRLkK8O4n14oRytICFtWnTtCPuPmw2G4W5?=
 =?us-ascii?Q?mVg/fibhc8cGeTzjGwzzrEzlQnhTp/URvubdLl8o0suKVWGJB85Wets3L1Nb?=
 =?us-ascii?Q?+dRiAbVY3xum7a1dnQLD3hgcgRF+VNlcXzpPTXSAX+D3ypqziGq4/dEBReOL?=
 =?us-ascii?Q?QbUXCOTzv+jczfREqC96eGoW1DV/OG4+C30iuqCeMKRPpgTbjOenggM14XaI?=
 =?us-ascii?Q?tsVOL0ykXkZ30468F8a8vZRiMFGWOj7KJgXJKevybeum5NpleOy679atflJO?=
 =?us-ascii?Q?PdQYT8udIPI2ETKyrOWgqFjYEudXlEAQJqikZ07xFBjZWdRvBB2Y3bm7O8+/?=
 =?us-ascii?Q?c2x+BmTpxZwEAs/xIT+iZKk6SpFEUV+C5I0khezHBMrqOWk7O70KRXN7fWUe?=
 =?us-ascii?Q?wF7CECZ98fNUy4k9rI7K3tY5jg8+3DSzm5uvwkHOaaaqQv/Aj/ubub2PQtKz?=
 =?us-ascii?Q?fumudVr67nzf382DXjRFE/lhfHp8fjoyssWkN/XdE8tKkjJfTJeShV7syQ1u?=
 =?us-ascii?Q?HA2WRNhkfriMSZLBaDEmhUntaxe1tuiv1KDUkj7Kknw34WnFttHLJyja1DN1?=
 =?us-ascii?Q?70VRfpaz4l4QGwetuJmOf0PR9mO3ye6Wt73vXTYyZMbxbGK4qOMe12C5hma7?=
 =?us-ascii?Q?gBWr2Ez+jGH+2pmqywSLk6zRKdM3dzLysKTVPODX3Rze5fhJGNLRLl4c3M/W?=
 =?us-ascii?Q?Yw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?uGQ8amvLHDpaDCyg1R2+6vZongzyi3xV4Y3QBQlXJMWS80BXQE1n7jPdDaEA?=
 =?us-ascii?Q?L7z2wL+lZnDTMBSrqnYeS2Y/vja8Kl6EmE2s6f33/bfye4ND6XRrcEbYbpLz?=
 =?us-ascii?Q?ezKzscnt1ZTzumphYAqFTwhvx7QhDgNDNh2pTIk2Pjcu80bCPiHJlBxEiVv2?=
 =?us-ascii?Q?ASqqr4dwwC+YEdEEYEYMDI0XiNufxPlewYP/cyh/InjrmKyO4C07RMpet2Jb?=
 =?us-ascii?Q?9z66hsG26SRvOHK/BI19N0AL9fzT0dpgyhy74OV+yTDUP0s7kjsvnglbOyDx?=
 =?us-ascii?Q?BjARW+Y9WNEcFBmMalI04taRpPjE5u3jtovfwhwIp6W5lLaPi89fNhL/fFsS?=
 =?us-ascii?Q?GbD2EjuTOl5M/yFVOargiMz8yO/rRm7HicUIvJbTQkWDEaDV9IfmueSB6t6N?=
 =?us-ascii?Q?5ROSmfOq9G4Z+N2yRriu3qBaah2+CYP7StrF9vgaSdM1qVgm86XIdaylenKR?=
 =?us-ascii?Q?CGgzkc4awLj3+t09kraNJZAaL1QqwXMNqJh/rYGSYHp6AnOHoA7sNQOMRC0g?=
 =?us-ascii?Q?sBM+w9YLQd6enRip/1uR4KaxD6wx63GuLCLTCfzWNkDqjcZCaQGSHp05p/Yd?=
 =?us-ascii?Q?Zd+y6JzamDB/4IdrC1FHY/h+pDh40tEKz2AacUTTvvhkykGZ7gLLnIW23Oel?=
 =?us-ascii?Q?icWZGTxiPJOXwdbpLw9O0jf2fC+i4yPw3zlcfkhVfFkQRcKWOv7tXk2DVlsm?=
 =?us-ascii?Q?oCMqXoJ/Nj2mkXRXUgzP06hvJsqUx+dpxcfBMLh/ELQf+wfaVk8q4ERtARoR?=
 =?us-ascii?Q?3tqjaPX7wEcOjmTqZCYiHHnX3+f4R6p2j488XyI3mDz/c5TwVa58c5Hu1lMo?=
 =?us-ascii?Q?V+38HyayJNSIgzGk9n5tf86FGVt0SXr+rV9+7i5TeQ3xfHGcx4JmKZ1A1/IK?=
 =?us-ascii?Q?xSkeF366TGquokNkKEXXKARl58nYZ9jox9HivFzqiUeBvXCN3f12uYgo0edE?=
 =?us-ascii?Q?Tcmw721Y6YryqiyJWMHlNKT6dUUJ5YxCpoi4ALb1aW1N65SENX2o3BpUDLUy?=
 =?us-ascii?Q?Vr5/?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2ac13b5f-8495-4f88-3f66-08db295ff4da
X-MS-Exchange-CrossTenant-AuthSource: SN6PR10MB3022.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Mar 2023 16:27:11.1820
 (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: aGcI0Kh7RrjyT6V4HCGayiu06s0XQuPr4lb7ihgQM5+p2tu7cvV/rnfx4IJBsm8QgPptXx5p8MuWg1Rsc/y4WQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4806
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-03-20_13,2023-03-20_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 phishscore=0 mlxscore=0
 mlxlogscore=679 malwarescore=0 suspectscore=0 adultscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303150002
 definitions=main-2303200139
X-Proofpoint-ORIG-GUID: K7m-N-zEeK0H6VhQweMcqi68uwuicmR3
X-Proofpoint-GUID: K7m-N-zEeK0H6VhQweMcqi68uwuicmR3
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

* Lorenzo Stoakes <lstoakes@gmail.com> [230318 07:15]:
> We are only interested in next if end == next->vm_start (in which case we
> check to see if we can set merge_next), so perform this check alongside
> checking whether curr should be set.
> 
> This groups all of the simple range checks together and establishes the
> invariant that, if prev, curr or next are non-NULL then their positions are
> as expected.
> 
> Additionally, use the abstract 'vma' object to look up the possible curr or
> next VMA in order to avoid any confusion as to what these variables
> represent - now curr and next are assigned once and only once.
> 
> This has no functional impact.
> 
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> ---
>  mm/mmap.c | 61 ++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 49 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index c9834364ac98..66893fc72e03 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -930,15 +930,53 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  	if (vm_flags & VM_SPECIAL)
>  		return NULL;
>  
> -	curr = find_vma(mm, prev ? prev->vm_end : 0);
> -	if (curr && curr->vm_end == end)		/* cases 6, 7, 8 */
> -		next = find_vma(mm, curr->vm_end);
> -	else
> -		next = curr;
> +	/*
> +	 * If there is a previous VMA specified, find the next, otherwise find
> +	 * the first.
> +	 */
> +	vma = find_vma(mm, prev ? prev->vm_end : 0);
> +
> +	/*
> +	 * Does the input range span an existing VMA? If so, we designate this
> +	 * VMA 'curr'. The caller will have ensured that curr->vm_start == addr.
> +	 *
> +	 * Cases 5 - 8.
> +	 */
> +	if (vma && end > vma->vm_start) {
> +		curr = vma;

It might be better to set:
curr = find_vma_intersection(mm, prev ? prev->vm_end : 0, end);

>  
> -	/* In cases 1 - 4 there's no CCCC vma */
> -	if (curr && end <= curr->vm_start)
> +		/*
> +		 * If the addr - end range spans this VMA entirely, then we
> +		 * check to see if another VMA follows it.
> +		 *
> +		 * If it is _immediately_ adjacent (checked below), then we
> +		 * designate it 'next' (cases 6 - 8).
> +		 */
> +		if (curr->vm_end == end)
> +			vma = find_vma(mm, curr->vm_end);

You can change this to:
next = vma_lookup(mm, curr->vm_end);
Then you don't need to validate below, in this case.

> +		else
> +			/* Case 5. */
> +			vma = NULL;


> +	} else {
> +		/*
> +		 * The addr - end range either spans the end of prev or spans no
> +		 * VMA at all - in either case we dispense with 'curr' and
> +		 * maintain only 'prev' and (possibly) 'next'.

Possibly next here would be:
next = vma_lookup(mm, end);
I think?

> +		 *
> +		 * Cases 1 - 4.
> +		 */
>  		curr = NULL;
> +	}
> +
> +	/*
> +	 * We only actually examine the next VMA if it is immediately adjacent
> +	 * to end which sits either at the end of a hole (cases 1 - 3), PPPP
> +	 * (case 4) or CCCC (cases 6 - 8).
> +	 */
> +	if (vma && end == vma->vm_start)
> +		next = vma;
> +	else
> +		next = NULL;

If I'm correct above, then we can drop this next checking.

>  
>  	/* verify some invariant that must be enforced by the caller */
>  	VM_WARN_ON(prev && addr <= prev->vm_start);
> @@ -959,11 +997,10 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  		}
>  	}
>  	/* Can we merge the successor? */
> -	if (next && end == next->vm_start &&
> -			mpol_equal(policy, vma_policy(next)) &&
> -			can_vma_merge_before(next, vm_flags,
> -					     anon_vma, file, pgoff+pglen,
> -					     vm_userfaultfd_ctx, anon_name)) {
> +	if (next && mpol_equal(policy, vma_policy(next)) &&
> +	    can_vma_merge_before(next, vm_flags,
> +				 anon_vma, file, pgoff+pglen,
> +				 vm_userfaultfd_ctx, anon_name)) {

I think we can keep this chunk with the next = vma_lookup() changes as
well.

>  		merge_next = true;
>  	}
>  
> -- 
> 2.39.2
>