Received-SPF: pass (google.com: domain of linux-kernel+bounces-73490-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 20 Feb 2024 13:10:16 -0500
From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To: Yajun Deng <yajun.deng@linux.dev>
Cc: Lorenzo Stoakes <lstoakes@gmail.com>, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org, vbabka@suse.cz
Subject: Re: [PATCH] mm/mmap: Add case 9 in vma_merge()
Message-ID: <20240220181016.2gq7v7cmollbntgg@revolver>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
	Yajun Deng <yajun.deng@linux.dev>,
	Lorenzo Stoakes <lstoakes@gmail.com>, akpm@linux-foundation.org,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org, vbabka@suse.cz
References: <20240218085028.3294332-1-yajun.deng@linux.dev>
 <a5cd692e-34e3-4bc1-a8fa-f6bb56f04e8a@lucifer.local>
 <f88de15e-508f-f651-0164-346845d23e85@linux.dev>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f88de15e-508f-f651-0164-346845d23e85@linux.dev>
User-Agent: NeoMutt/20220429
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?f9hZTFSIfxsNZ8nNvRR7CY4fk6TGo1ZBQYJOH47RWqEMpZE+LEv8eJGpxq/1?=
 =?us-ascii?Q?pVKXHiZOQ4ni59fvOuXuvNAzln5N+C6maTdWhTUvAPWCgNpguZaJpN8i4hum?=
 =?us-ascii?Q?dF7shGRLiSYVI1jP6UmlMqrYUeHW7yErrvHAevh9i0S6co8OLrHf3uzcCz+p?=
 =?us-ascii?Q?8nW9u5xguTXrLnH/ArdJBPayOZIjWNg7rrkT+AkCjO6qmYAW0XLXQS5YAyPs?=
 =?us-ascii?Q?b8qP9ff/4s1XDqk9LUdtKvg4mTRCrgrOTSHqTtNLDepp4UYN/QhDlpJGMXTZ?=
 =?us-ascii?Q?QTymcwlgX38veMKLcbZ7c3FaFJDiXbCxKWIBbN5yd8M0h27+adXPtXg5daEu?=
 =?us-ascii?Q?jirTWzG4/Dju8Czca1DVgvsLW2+hOxaTkIa1OYndtsQ1+QuO+XzLqHLdei2W?=
 =?us-ascii?Q?FfBoBy5UQf4lPfQktQ2yG3+cy3QLFwxXUFS8S1mK4JrBudMsjv5P6De5wMvu?=
 =?us-ascii?Q?Nwla7HsRjbN9Bar3z/FddPm/fNAUp0oy5asNNJHtVZwWJbRutXP4lTUz/Zi5?=
 =?us-ascii?Q?5bo+uxQGSsgFpTYZGWI+0QzYP4sr1JfWBLCY+anhO1McSP5nG+03Xoe1KozR?=
 =?us-ascii?Q?48lm6vgDPa/zrVXCiGmHwAa6uldehU8jpAxSoF+RCM+PoJDFzJRm8Hz7K/Tw?=
 =?us-ascii?Q?30gg77TV5sjIgh9ovT3L/WGntJXw1eD/Fs1somrAdSmqElCAOvPcFjynbmJs?=
 =?us-ascii?Q?ZVEiBw2wruYdcL+XCKobzVkpZdk1g1XFRDpu4pxOa1mCowS2P+LP1rnneBWA?=
 =?us-ascii?Q?vzAbIDi6uXZ/dvqK9lryI2HI3G+gYH+dUx1fuGOO+Tay+vMAea/hKOx4MJ+Y?=
 =?us-ascii?Q?eJVfeah0It9yeOlWb+MN8B2pM0VsH/Da4cfhkCMzVkGOwG6njXPTFvK0g5aB?=
 =?us-ascii?Q?aC/3c0EbhngshF/0c3852gE+kbat2jt96y17Ic09Sy4uX1b604safSHnS2Sh?=
 =?us-ascii?Q?/uut0LPW8sldutNnzy/N6zWCiKAb7RKrTpe/xfmVt3Hw68yTQbkteRnApydb?=
 =?us-ascii?Q?gZEsN4UIpUfBz4ISA2Df3b1+NPFgtizQlhFsHGCkFnaUWbfjLL4fE+uGCYiy?=
 =?us-ascii?Q?ZdAiiZF+ML1HStCUaiUIZiWOhViI0GdbVpSPC++PvtoUjtejhUQFEFjjBzl3?=
 =?us-ascii?Q?anchfrwTGwBbnMXKDOr/3dAQ4b35y7Nz/+25Y8e7mvt5u87hyVOMW5PZIofm?=
 =?us-ascii?Q?iPFTETXULRqYHoxpxJOwBkCa7Brukk4/J1ChOs0mrW5V/+EQGhyW4NIK6Ehd?=
 =?us-ascii?Q?YrPU49YP9vp7+Cp0ChK5w/EVp6no3AGr4DJkZmd1eJDW+1OP3JnGPh/Rc7ao?=
 =?us-ascii?Q?U7CAHiDiaIwz8jPHMOEYz/6Yws8fMavcyxoQe7JHn4P0ID4Novlrk0AQWFfK?=
 =?us-ascii?Q?yipUb7HdNs3bPBJSlmihsvGTTBEm6gJ4SY/cH3ePjupsRE1/pk21fC9RuHpY?=
 =?us-ascii?Q?+qCC3iW0cSHAQdju3Md8AhGFbvUoJjuduq4uzC0OVWNJDcvxtSerVQSUeiMu?=
 =?us-ascii?Q?EOtC8fL9XCOGpUL6hfZ4oEgSrFMFZqm2DLuThUuWcRlya8UqaLBBrAerQRCL?=
 =?us-ascii?Q?oG1dpH4HFPVgL5wCV5sCWGU5Fp8+JA7e1uWD0y5Jrc4K5da4caPoxX0iI+tL?=
 =?us-ascii?Q?dw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	VHQTN5fftvcq2un6B4+Qw4I44LDOl34YZ0nd4ExMSDehU6vI6piDssf98CrZsyUbUXoMyeimzZMM8TcU4lAmlZjOzKZ1eZI/aYDlKHMPbbON+32yufGP/8cejFKqmnbMc3pRH+I41L6VILq3pIBIxm9XtNgg4S0+6ERty/3UIcYxoyc3QQ10km4yRbsQI29UTaQILvYRnHeDxsjVy1kOF35ZQ9LcKUwQ06W8Ymwz4LKF+aMJvrPxscRbnte9BdmYDkH/BPiXi4HiXuc50vRXJWGcPJRG/MyUzgw6Y1CYqYefWh4wiIsHcm2GimwRUIdOTYN/WRvvoVhSicOGt1yqKop/9rMaeDVd4axtfniq33H4M3D8JUkSKOxjxtcRfJ1xImuTTnGLs/H8VnLicY35ZUCo1ihH9nYgFA6TuKgBWZTp0IncOEPHwCGCVsYlA0yxOAoUHwsvAGxl6VotXSrkPM9qQkNx4lmAYogVYFQrz0pkb9bsYJ80Xt39tAxeoYYWYcNHPkuAsBDvlPY1i8TLr8kIbPjxhZoZqJ8RSMiVy91ARhFPdYNp1hmJMYiRY0rbQ9mQB84pQFNpwa1B4R1UJrSki1NoC2eDant5/PuvVaI=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ec68275f-a711-452b-f285-08dc323f3198
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Feb 2024 18:10:17.7417
 (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: c4/Ygky+kSXpJ8FvsYTTDQS7TgsvxfGK/3YrIZJjcvgj9tILWu90qasdRtHAt6DJAe+McPWVP8/oHYU8qIVe0A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4798
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-20_06,2024-02-20_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 phishscore=0
 suspectscore=0 mlxscore=0 bulkscore=0 spamscore=0 mlxlogscore=652
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402200130
X-Proofpoint-GUID: dNONPGb_rgYpY1pjWhUnnlVN6XaORfBF
X-Proofpoint-ORIG-GUID: dNONPGb_rgYpY1pjWhUnnlVN6XaORfBF

* Yajun Deng <yajun.deng@linux.dev> [240219 22:00]:
> 
> On 2024/2/19 07:03, Lorenzo Stoakes wrote:
> > On Sun, Feb 18, 2024 at 04:50:28PM +0800, Yajun Deng wrote:
> > > If the prev vma exists and the end is less than the end of prev, we
> > > can return NULL immediately. This reduces unnecessary operations.
> > > 
> > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> > Adding Vlastimil, while get_maintainers.pl might not show it very clearly,
> > myself, Vlastimil and Liam often work with vma_merge() so it's handy to cc
> > us on these if you can!
> Okay.
> > > ---
> > >   mm/mmap.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index 8f176027583c..b738849321c0 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -827,7 +827,7 @@ can_vma_merge_after(struct vm_area_struct *vma, unsigned long vm_flags,
> > >    *
> > >    *     ****             ****                   ****
> > >    *    PPPPPPNNNNNN    PPPPPPNNNNNN       PPPPPPCCCCCC
> > > - *    cannot merge    might become       might become
> > > + *    cannot merge 9  might become       might become
> > While I welcome your interest here :) I am not a fan of the 'case' approach
> > to this function as-is and plan to heavily refactor this when I get a chance.
> > 
> > But at any rate, an early-exit situation is not a merge case, merge cases
> > describe cases where we _can_ merge, so we can drop this case 9 stuff (this
> > is not your fault, it's understandable why you would label this, this
> > function is just generally unclear).
> 
> Yes, it's not a merge case. I label this to make it easier to understand.

But it isn't.  It's not a case at all, it's a failure to merge.

> 
> > >    *                    PPNNNNNNNNNN       PPPPPPPPPPCC
> > >    *    mmap, brk or    case 4 below       case 5 below
> > >    *    mremap move:
> > > @@ -890,6 +890,9 @@ static struct vm_area_struct
> > >   	if (vm_flags & VM_SPECIAL)
> > >   		return NULL;
> > > 
> > > +	if (prev && end < prev->vm_end) /* case 9 */
> > > +		return NULL;
> > > +
> > I need to get back into vma_merge() head space, but I don't actually think
> > a caller that's behaving correctly should ever do this. I know the ASCII
> > diagram above lists it as a thing that can happen, but I think we
> > implicitly avoid this from the way we invoke callers. Either prev == vma as
> > per vma_merge_extend(), or the loops that invoke vma_merge_new_vma()
> > wouldn't permit this to occur.
> No, it will actually happen. That's why I submitted this patch.

Can you elaborate on where it happens?  I mean, you seem to have already
looked into it but haven't shared what you found of where it reduces the
unnecessary operations.

Such a detail should also be added to the commit log so that, when the
call sites change, this check could be dropped - or be seen as
necessary.