Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Zi Yan <ziy@nvidia.com>
To:     Oscar Salvador <osalvador@suse.de>
Cc:     David Hildenbrand <david@redhat.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Michael Ellerman <mpe@ellerman.id.au>,
        Christoph Hellwig <hch@lst.de>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Robin Murphy <robin.murphy@arm.com>,
        linuxppc-dev@lists.ozlabs.org,
        virtualization@lists.linux-foundation.org,
        iommu@lists.linux-foundation.org, Vlastimil Babka <vbabka@suse.cz>,
        Mel Gorman <mgorman@techsingularity.net>,
        Eric Ren <renzhengeek@gmail.com>
Subject: Re: [PATCH v4 4/7] mm: make alloc_contig_range work at pageblock granularity
Date:   Fri, 04 Feb 2022 10:19:55 -0500
Message-ID: <E188FF7F-F52E-4DB6-B258-B9CCD7058E56@nvidia.com>
In-Reply-To: <Yf0wpFmtckRRzLFg@localhost.localdomain>
References: <20220119190623.1029355-1-zi.yan@sent.com>
 <20220119190623.1029355-5-zi.yan@sent.com>
 <Yf0wpFmtckRRzLFg@localhost.localdomain>
Content-Type: multipart/signed;
 boundary="=_MailMate_CF6EF116-A822-4563-B0A1-701D7A4A3D2E_=";
 micalg=pgp-sha512; protocol="application/pgp-signature"
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?MYlY9vtc4LkesFFmrBpKU3ppZcwYFoj0OjO6/a6TQJc/jqg0v6Aihys5YQN2?=
 =?us-ascii?Q?N5EEe5s/RI0bUy/7lFLlchrgw1RWlJaXI+Rm9NpaQJyAtrGo5EbOgA5nzQVC?=
 =?us-ascii?Q?VVWe/ZuOSxT9QZtabXfyQUclHxF1Z0fkripE4r4o0gd5JAsWJwdG2kKmJZSY?=
 =?us-ascii?Q?kTouZHhcQlWP2n60NhGEPY73Qn9d8l6durpCLCmiQx7tnPlH5+6hqh7ogu1H?=
 =?us-ascii?Q?5+HgzcB56lVZTdjbu6XAJJslLvnq2oRN4pjvndPJqar/uxlOPDlTTVU3tipU?=
 =?us-ascii?Q?Y/C1AiFzNlD1DEBB09oIbupZesQJAVeSJeGaXdstnlN7AYVk4wF+w2LtqkE2?=
 =?us-ascii?Q?yNEryFZmVbCnnUMsAGJ3GdqeDvUXXdbgIewapXGC9u9B7sTLbjZGbyNQ+i7j?=
 =?us-ascii?Q?nWiYyKQzzjiljeJLVmhmJeftrlLK8K4VuoeAJ++1WyXbTT8o705cYLeNd3lZ?=
 =?us-ascii?Q?mMC+5HT0M5hNQeOA4mTLi+Dson0zxBXVoYTA+ES9+cptaXM8qP+T54qQ3FIu?=
 =?us-ascii?Q?U+jg4Ja7XQE9DPoNPsoCgBtaFx79fhv/bp+TOXc2b14COF4tZoK5g55evUX7?=
 =?us-ascii?Q?GQXW9/gaxOTxzei1POG+SLzNO8urJ4WTvav91N2edm/plTskHnDP2kK7nFaB?=
 =?us-ascii?Q?AehiqZGJKJ5esbWAguHc8wzFZHWDyMVdnN9iFs3JG7ByULwXVEQFda1d5IKf?=
 =?us-ascii?Q?S8E6c4gkDOtMyg+zac1utySHgjd8KrJEVT0aq1e/w+ck2htO/MCQ7ORCOPfj?=
 =?us-ascii?Q?QgM57BfhPwvp3eTSmpRc3GPnFdWIS/skxcIAsFUnn9kQQ3bPRiexZP4+xx8P?=
 =?us-ascii?Q?VmIlaHUzR1ix0G0TK5ObQrS8LywHwmryLvO0Uai8iK4CgndqzButguYPcfE9?=
 =?us-ascii?Q?NA6KSa/qr/IBbViGBWPZqMkjSlOF1I3R/rTD8a3MpNzuLPqrVJ7uOMQVLdgC?=
 =?us-ascii?Q?/fxxLIaTlbeubAG+/szU126fz7dAmUU+OWvDA258Sawy0bAtqlpi+eXbMPj8?=
 =?us-ascii?Q?pzl7NZ3jXCHrhINDAYnk8VYWfSfCS6LyotKOiR8FMmAgJGzZ6QutNZv5Du9+?=
 =?us-ascii?Q?w0RYQcsDwlea8ZB87snMKqYHD3htn5u3bNbUwab6MSkWhjWraaL7Y/bYR6rs?=
 =?us-ascii?Q?sH8iQzb6mJAPlDBDmB3i6u+6iVwsPlaDzUOFFdc+TUuOx/zYTwqvXqgVksAx?=
 =?us-ascii?Q?EupdCnOlfaPm0SSHFvKkJPLOlxd8fHiT8t0bu0tzD/Nxr73Tf2jEm/4sxqMh?=
 =?us-ascii?Q?5aPu8p2/29C4sG5kCElgX9Ds6rTcsoLR51U9Zcb45ZlyyN3uREOlzVda8BZR?=
 =?us-ascii?Q?4Q7UFdbt0U1lCFmldWkyLaSoBsntT4Be6l1Vz7uQLdNJPCYFE4M1K6LVYxbk?=
 =?us-ascii?Q?UDkAEzlTEPrHk+njqOhiZPWmKGuw9EJvYuuR71jmM+eQ9BTE98Uu+C7/uw8c?=
 =?us-ascii?Q?+nf6Qsauc0FM6cJh5mXawh6Vfk3VdbH5gVeeXYhRYrKk63fgTIpETIegBLn1?=
 =?us-ascii?Q?d4UCwPIx8VZsg824T3rGLqrF5cpkftL3P2GNvhqwwysLUglqttC0l8PRcsK+?=
 =?us-ascii?Q?xOF2RSc/cstIYJAJeRQ=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5a956559-b64f-4979-77b1-08d9e7f1ce66
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3819.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Feb 2022 15:19:59.2154
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ppPayiSDGijsFFjIbEGTWSVzWfJCsvtDoDoI1Xeid+1AyAHAIEWnH1hIxD3hl+B8
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4371
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

--=_MailMate_CF6EF116-A822-4563-B0A1-701D7A4A3D2E_=
Content-Type: text/plain

On 4 Feb 2022, at 8:56, Oscar Salvador wrote:

> On Wed, Jan 19, 2022 at 02:06:20PM -0500, Zi Yan wrote:
>> From: Zi Yan <ziy@nvidia.com>
>>
>> alloc_contig_range() worked at MAX_ORDER-1 granularity to avoid merging
>> pageblocks with different migratetypes. It might unnecessarily convert
>> extra pageblocks at the beginning and at the end of the range. Change
>> alloc_contig_range() to work at pageblock granularity.
>>
>> It is done by restoring pageblock types and split >pageblock_order free
>> pages after isolating at MAX_ORDER-1 granularity and migrating pages
>> away at pageblock granularity. The reason for this process is that
>> during isolation, some pages, either free or in-use, might have >pageblock
>> sizes and isolating part of them can cause free accounting issues.
>> Restoring the migratetypes of the pageblocks not in the interesting
>> range later is much easier.
>
> Hi Zi Yan,
>
> Due to time constraints I only glanced over, so some comments below
> about stuff that caught my eye:

Thanks for looking at the patches!

>
>> +static inline void split_free_page_into_pageblocks(struct page *free_page,
>> +				int order, struct zone *zone)
>> +{
>> +	unsigned long pfn;
>> +
>> +	spin_lock(&zone->lock);
>> +	del_page_from_free_list(free_page, zone, order);
>> +	for (pfn = page_to_pfn(free_page);
>> +	     pfn < page_to_pfn(free_page) + (1UL << order);
>
> It migt make sense to have a end_pfn variable so that does not have to
> be constantly evaluated. Or maybe the compiler is clever enough to only
> evualuate it once.

Sure. Will add end_pfn.

>
>> +	     pfn += pageblock_nr_pages) {
>> +		int mt = get_pfnblock_migratetype(pfn_to_page(pfn), pfn);
>> +
>> +		__free_one_page(pfn_to_page(pfn), pfn, zone, pageblock_order,
>> +				mt, FPI_NONE);
>> +	}
>> +	spin_unlock(&zone->lock);
>
> It is possible that free_page's order is already pageblock_order, so I
> would add a one-liner upfront to catch that case and return, otherwise
> we do the delete_from_freelist-and-free_it_again dance.

Make sense. Will do.

>
>> +	/* Save the migratepages of the pageblocks before start and after end */
>> +	num_pageblock_to_save = (alloc_start - isolate_start) / pageblock_nr_pages
>> +				+ (isolate_end - alloc_end) / pageblock_nr_pages;
>> +	saved_mt =
>> +		kmalloc_array(num_pageblock_to_save,
>> +			      sizeof(unsigned char), GFP_KERNEL);
>> +	if (!saved_mt)
>> +		return -ENOMEM;
>> +
>> +	num = save_migratetypes(saved_mt, isolate_start, alloc_start);
>> +
>> +	num = save_migratetypes(&saved_mt[num], alloc_end, isolate_end);
>
> I really hope we can put all this magic within start_isolate_page_range,
> and the counterparts in undo_isolate_page_range.
>

That is my plan too.

> Also, I kinda dislike the &saved_mt thing. I thought about some other
> approaches but nothing that wasn't too specific for this case, and I
> guess we want that function to be as generic as possible.
>
I do not like it either. This whole save and restore thing should go away
once I make MIGRATE_ISOLATE a standalone bit, so that the original
migrateypes will not be overwritten during isolation. Hopefully, I can
work on it soon to get rid of this hunk completely.

>> +	/*
>> +	 * Split free page spanning [alloc_end, isolate_end) and put the
>> +	 * pageblocks in the right migratetype list
>> +	 */
>> +	for (outer_end = alloc_end; outer_end < isolate_end;) {
>> +		unsigned long begin_pfn = outer_end;
>> +
>> +		order = 0;
>> +		while (!PageBuddy(pfn_to_page(outer_end))) {
>> +			if (++order >= MAX_ORDER) {
>> +				outer_end = begin_pfn;
>> +				break;
>> +			}
>> +			outer_end &= ~0UL << order;
>> +		}
>> +
>> +		if (outer_end != begin_pfn) {
>> +			order = buddy_order(pfn_to_page(outer_end));
>> +
>> +			/*
>> +			 * split the free page has start page and put the pageblocks
>> +			 * in the right migratetype list
>> +			 */
>> +			VM_BUG_ON(outer_end + (1UL << order) <= begin_pfn);
>
> How could this possibily happen?

Right. It is not possible. Will remove it.

>
>> +			{
>> +				struct page *free_page = pfn_to_page(outer_end);
>> +
>> +				split_free_page_into_pageblocks(free_page, order, cc.zone);
>> +			}
>> +			outer_end += 1UL << order;
>> +		} else
>> +			outer_end = begin_pfn + 1;
>>  	}
>
> I think there are cases could optimize for. If the page has already been
> split in pageblock by the outer_start loop, we could skip this outer_end
> logic altogether.
>
> E.g: An order-10 page is split in two pageblocks. There's nothing else
> to be done, right? We could skip this.

Yes. I will think about it more and do some optimization.


--
Best Regards,
Yan, Zi

--=_MailMate_CF6EF116-A822-4563-B0A1-701D7A4A3D2E_=
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQJDBAEBCgAtFiEEh7yFAW3gwjwQ4C9anbJR82th+ooFAmH9RBwPHHppeUBudmlk
aWEuY29tAAoJEJ2yUfNrYfqKnOsQAIU3r16Lp8RxLENu2TSxCPl2+fMmwEqS20CT
MDPYfQUBORoFxakM+qGSMT/0LRZlTVq7JjTH2DDKdX+bMeRGDYlKZa84Gx5HTNEf
/QUmSywcK/tkrnBG1L+HpG0URjo2P76E7yosayOqqpzekEK+8D8oD22/APg2uLl8
s5jI5IZ0cKHj2HCRwYl17CxkK0SP3WrvG/opxslk4VM0zhM9ROoEA2RmaUCeQops
5VWEY5njlLjvTxRbYfe8LgP59Scr1eqz11DHloUE4WvjJVCkwcJwUaBOrtMPHwIn
0WDK4hpODePaMoDt/RLZDZOu+jj2cTVbmwz0cPvmxN9lClifvgWi0AkzbNuZiExF
WNbArON/QOT2E7DSFIi7rrmikRC1VuVlU+mh6BWel9cnZ4hIuVmNdDZDrzQ7Obn8
FUNYDrsx9moo/hDML0KpBtswUCD65aMEiX/CpNOyihFOE1EY967sl5OXps2QdZHP
s3WDb/vpwupvLqHJ7qi8Pw8PN7T3gQWPX2bBcqMnK/H8LVbqs4K0tkVQBQsZUhgR
n82NTVw5E5gw1wGwLA2YbK6Teq9DKYShU+HZ0tUwUbXSQkRzm/6h5dISWG2FtBfu
O/fUi8vzKe9kNrjzhca/q+L5V4iBDzp9F+4x7KUdwyo8XZ90VM9HngIrHGVhK4Hx
CDifByzk
=9+3R
-----END PGP SIGNATURE-----

--=_MailMate_CF6EF116-A822-4563-B0A1-701D7A4A3D2E_=--