Received-SPF: pass (google.com: domain of linux-kernel+bounces-76747-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: Thu, 22 Feb 2024 10:07:03 -0500
From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To: Yajun Deng <yajun.deng@linux.dev>
Cc: akpm@linux-foundation.org, vbabka@suse.cz, lstoakes@gmail.com,
        surenb@google.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm/mmap: convert all mas except mas_detach to vma
 iterator
Message-ID: <20240222150703.u55kpmdlog5hrld3@revolver>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
	Yajun Deng <yajun.deng@linux.dev>, akpm@linux-foundation.org,
	vbabka@suse.cz, lstoakes@gmail.com, surenb@google.com,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <20240218023155.2684469-1-yajun.deng@linux.dev>
 <9f76ce23-67b1-ccbe-a722-1db0e8f0a408@linux.dev>
 <20240220180603.dhnthx2nk5455opm@revolver>
 <0ff93990-b8b6-89ce-0174-4023599059e7@linux.dev>
 <20240221143103.bkhewpq2wirsoyqw@revolver>
 <d9b4624a-214d-2f49-3883-4d62f60a9de4@linux.dev>
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <d9b4624a-214d-2f49-3883-4d62f60a9de4@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: 
	=?iso-8859-1?Q?KaEvAmFdWe7OET9zq47pVfh1Ca7mU2loTCv9ZK00xtvLXqbcEqCMPFQKmH?=
 =?iso-8859-1?Q?7slvj1jmeMo39VWGM5YkUyyPGJDJTZu3S3YbbT7v5dBgxz/dLIXAcSdE8F?=
 =?iso-8859-1?Q?bRniKEIIRE1Af8Uvh0/qNRneq03wVvPMSANEwR5P7BnFpLlf+G90mWwbdT?=
 =?iso-8859-1?Q?O1N/zdi6eOtoGfm2+ZAk+arH+IYyFZ43btLziNy21vdpcymjrPMzuACEuE?=
 =?iso-8859-1?Q?BqMA/p5LJ1jf6QBfjZM7ubTPuGUwERc60gOlYE5XuOk7imKjukfABVQe7u?=
 =?iso-8859-1?Q?ZRY4lhfQGSMb1Cr2zoTAao/I4wHSl1o4v3Ae8PBOYkII1cmh+5plOvtwGr?=
 =?iso-8859-1?Q?mIbsNuIRyDhznPbDlRVnHmrDaZ76MuJbZRG8BSpjXE+g/HOs499cmApcAy?=
 =?iso-8859-1?Q?o0h7LD+u9nreu1v9H3nHwF0+21Dutpun9gTSDyhM+jEU4g3NAX6n8kz6PY?=
 =?iso-8859-1?Q?rBthWXCoopR+/qwx8aLjePBZi6bULu8x3lz6VIau+lzckw/YPHPXq9hW5e?=
 =?iso-8859-1?Q?Ol2O9ArK1tg/SMY8qjjZugLXbf54ohvP9UK/eaMd8MVPjT/Rp8vkFmUOze?=
 =?iso-8859-1?Q?GKw0uM+BMDDqsJDEgT8ZKUFT5JCEt1YfzA1/YAMB7KYcShm/UXg4PTvZpv?=
 =?iso-8859-1?Q?HJPHSouI5WYYKqww+cmcW+/FKqfVmhDvgsffJZfNaeOIN+6Dhrwd/lFfFO?=
 =?iso-8859-1?Q?wIKV9vbBsYmLi1PbGyOzC8uLDgH+0ZxcTAmLxPuzbLeoHsHRn8APYIQVrP?=
 =?iso-8859-1?Q?QQeu97HD551Rw6Y21IW9u5U1pdOgg4hCPGiImP04qaFDC7GcbnOzJLvv4w?=
 =?iso-8859-1?Q?bOyw+t57OuIwkA9J39NrlW3ay84bIUdmlQ/AsgutGTIeeeKQUXvVvyHa+8?=
 =?iso-8859-1?Q?ddutmg05WROvU3KEXS7btYAWx1dGf29BiwjbkG/WVZF4sFABpEcZeZ7hRw?=
 =?iso-8859-1?Q?OzXOJuaTCoGIeZmo2C4m/YK483rqx0vGr+yL1/CzLZeFqCJGKN97Mp5pFP?=
 =?iso-8859-1?Q?zBs4/BxyfTWVgTgoRiy0SzjuMewa/PRlt9BRnDNAUJbJwhwGfjmJ4EpZiX?=
 =?iso-8859-1?Q?EPXRwB9xe469SzwI9Z7GN4fbrO5FnT5mcgvO93VrpB2iyql3ZWTTeqlSqu?=
 =?iso-8859-1?Q?kMvAv6f0FwsIqpCEzhP9lJSBTlG0uv7Tksp7v1EhO0nA7+2P5UpHDMdRHJ?=
 =?iso-8859-1?Q?8XrnhehsMQscPNIh8wOAcp03AVQfx297+dtop2UABD3U127x45HYXR/Jx9?=
 =?iso-8859-1?Q?twhA6UFXAZ1sQb9VYVBVm7SDx+mtMqONc8HFjilrdOuibVYwZBm0nWerFV?=
 =?iso-8859-1?Q?yxTX3Q+idHyxEDjUBNtriucddPzXFYRn2/TEWZ1Miw9CFkYZwdxtUB3q2/?=
 =?iso-8859-1?Q?Rypo/ywgnaZ3CKZw+92uPEc8aCMAPlNY4OjfkonYkZMPtMPVGH0EHbnDMg?=
 =?iso-8859-1?Q?QbAC8qa+evtK5Ekek9ejZWABOUMdquOcrLKzm3zXkjFK3Kinr6aQA8XQQb?=
 =?iso-8859-1?Q?Jk0xJ9QzALUcj8TtDAAn3Rk+kqzSvIyhf10oXJfN7kMELUCrLJg/eMo8eZ?=
 =?iso-8859-1?Q?FyJmUMw9NS1bbgFkiEurslu+jcAJe5dceYb+Ego9FajW7Lcj7zxRXJ1wOo?=
 =?iso-8859-1?Q?aOV8R69XdAcIoto4HqVjFMll/Klc0tl9hJ?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	GtnrhgxiXE4GaE0TU+IZyFk5d2pMmgJmWzgcq6g56VZ73ZSkQLw5csrCT+71gtQoAKxEYm8dfDcaeZ252YDES/tlYPeXuanUdoMm0QW0qVPLkKiytR3NP3WTOJ21PFgvKsdoZX6W9s0PJUZzwBTCWfBOevzbeMikP48d/B303U+NF81OMaa9QzWY0aVTjdCNu19Df00PzSbRX6uzDQOYvekoINrIbdqN6zHjkyRNWp5HJbyLKVQj8Lu1Mh+ZeJI7O30jtwkh7zIQbVcCKodZ0P2/OzQqZ7XYUXFAapPGeZZAil/B2uStCF1X0i7XN2DhrLb1Ka+AvXKy1Wrwyi2ptjqXbBZ/fYh9F50B19Q2cZoOvpYtN6JjrP/mBWg65TRCduP5avHh2HBoUqGkbw25BSXpXJZpbOJ27DmOE4WEPYa6+d52N6UmCPW9A5pRSqg/6mS+u8BQrjlBzi2tRkQhBTJ5FxYH0MoOS2aiLqNHt1kSeWsNjHl4uV+L2IIhCDMKb9BneXiyqoE2iMSOVDa6T9Edyz+/x8W8mE8B6kthmosSG+zS+IMBh5oGscBPcvplaOfUEdyUeg3X7S2ovX6XXXlL0KUhp77k5V3TwPsaezE=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: be4017f8-be58-43d4-c2be-08dc33b7ee38
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2024 15:07:04.9938
 (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: LiPzRaAYyjxaAJFpjvSPvjhEhb00X7HpTPgo4rBkiGCt2zAtLlEEXGfPMK7NY1A4zJwFtZ/swkkQsuc4sqFmPw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB7277
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-22_11,2024-02-22_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=999
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402220120
X-Proofpoint-GUID: veZ0LG3zZLTkT18JZWyvnvEXUNMx3zNp
X-Proofpoint-ORIG-GUID: veZ0LG3zZLTkT18JZWyvnvEXUNMx3zNp

* Yajun Deng <yajun.deng@linux.dev> [240222 03:56]:
..

> > > > > > @@ -1959,11 +1958,12 @@ static int expand_upwards(struct vm_are=
a_struct *vma, unsigned long address)
> > > > > >     	struct vm_area_struct *next;
> > > > > >     	unsigned long gap_addr;
> > > > > >     	int error =3D 0;
> > > > > > -	MA_STATE(mas, &mm->mm_mt, vma->vm_start, address);
> > > > > > +	VMA_ITERATOR(vmi, mm, 0);
> > > > > >     	if (!(vma->vm_flags & VM_GROWSUP))
> > > > > >     		return -EFAULT;
> > > > > > +	vma_iter_config(&vmi, vma->vm_start, address);
> > > > This is confusing.  I think you are doing this so that the vma iter=
ator
> > > > is set up the same as the maple state, and not what is logically
> > > > necessary?
> > >=20
> > > Yes, VMA_ITERATOR can only pass one address.
> > >=20
> > > > > >     	/* Guard against exceeding limits of the address space. */
> > > > > >     	address &=3D PAGE_MASK;
> > > > > >     	if (address >=3D (TASK_SIZE & PAGE_MASK))
> > > > > > @@ -1985,15 +1985,15 @@ static int expand_upwards(struct vm_are=
a_struct *vma, unsigned long address)
> > > > > >     	}
> > > > > >     	if (next)
> > > > > > -		mas_prev_range(&mas, address);
> > > > > > +		mas_prev_range(&vmi.mas, address);
> > > > This isn't really hiding the maple state.
> > >=20
> > > Okay,=A0 I will create a new helper function for this in the mm/inter=
nal.h.
> > >=20
> > > >=20
> > > > > > -	__mas_set_range(&mas, vma->vm_start, address - 1);
> > > > > > -	if (mas_preallocate(&mas, vma, GFP_KERNEL))
> > > > > > +	vma_iter_config(&vmi, vma->vm_start, address);
> > > > The above maple state changes is to get the maple state to point to=
 the
> > > > correct area for the preallocation call below.  This seems unnecess=
ary
> > > > to me.
> > > >=20
> > > > We really should just set it up correctly.  Unfortunately, with the=
 VMA
> > > > iterator, that's not really possible on initialization.
> > > >=20
> > > > What we can do is use the vma->vm_start for the initialization, the=
n use
> > > > vma_iter_config() here.  That will not reset any state - but that's=
 fine
> > > > because the preallocation is the first call that actually uses it
> > > > anyways.
> > > >=20
> > > > So we can initialize with vma->vm_start, don't call vma_iter_config
> > > > until here, and also drop the if (next) part.
> > > >=20
> > > > This is possible here because it's not optimised like the
> > > > expand_upwards() case, which uses the state to check prev and avoid=
s an
> > > > extra walk.
> > > >=20
> > > > Please make sure to test with the ltp tests on the stack combining,=
 etc
> > > > on a platform that expands down.
>=20
>=20
> It seems something wrong about this description. This change is in
> expand_upwards(), but not in
>=20
> expand_downwards(). So we should test it on a platform that expands up.

Oh, yes.  Test on the platform that expands upwards would be best.
Sorry about the mix up.

>And
> drop the if (next) part
>=20
> is unnecessary. Did I get that right?

Yes, I think the if (next) part is unnecessary because the maple
state/vma iterator has not actually moved - we use
find_vma_intersection() to locate next and not the iterator.  This is
different than what we do in the expand_downwards.

Note that, in the even that we reach the limit and cannot return a
usable address, these functions will call the counterpart and search in
the opposite direction.

>=20
> > >=20
> > > Okay, I will test it.
> > Testing this can be tricky.  Thanks for looking at it.
> >=20
..


Thanks,
Liam