Re: [PATCH 09/10] mm: page_isolation: prepare for hygienic freelists

[Zi Yan]

On 21 Mar 2024, at 10:24, Johannes Weiner wrote:

> On Thu, Mar 21, 2024 at 09:13:57PM +0800, kernel test robot wrote:
>> Hi Johannes,
>>
>> kernel test robot noticed the following build warnings:
>>
>> [auto build test WARNING on akpm-mm/mm-everything]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Johannes-Weiner/mm-page_alloc-remove-pcppage-migratetype-caching/20240321-020814
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
>> patch link:    https://lore.kernel.org/r/20240320180429.678181-10-hannes%40cmpxchg.org
>> patch subject: [PATCH 09/10] mm: page_isolation: prepare for hygienic freelists
>> config: i386-randconfig-003-20240321 (https://download.01.org/0day-ci/archive/20240321/202403212118.ye7lcKjD-lkp@xxxxxxxxx/config)
>> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240321/202403212118.ye7lcKjD-lkp@xxxxxxxxx/reproduce)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <lkp@xxxxxxxxx>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/202403212118.ye7lcKjD-lkp@xxxxxxxxx/
>>
>> All warnings (new ones prefixed by >>):
>>
>>    mm/page_alloc.c: In function 'move_freepages_block_isolate':
>>>> mm/page_alloc.c:688:17: warning: array subscript 11 is above array bounds of 'struct free_area[11]' [-Warray-bounds]
>>      688 |  zone->free_area[order].nr_free--;
>>          |  ~~~~~~~~~~~~~~~^~~~~~~
>>>> mm/page_alloc.c:688:17: warning: array subscript 11 is above array bounds of 'struct free_area[11]' [-Warray-bounds]
>
> I think this is a bug in the old gcc.
>
> We have this in move_freepages_block_isolate():
>
> 	/* We're the starting block of a larger buddy */
> 	if (PageBuddy(page) && buddy_order(page) > pageblock_order) {
> 		int mt = get_pfnblock_migratetype(page, pfn);
> 		int order = buddy_order(page);
>
> 		if (!is_migrate_isolate(mt))
> 			__mod_zone_freepage_state(zone, -(1UL << order), mt);
> 		del_page_from_free_list(page, zone, order);
>
> And this config doesn't have hugetlb enabled, so:
>
> /* If huge pages are not used, group by MAX_ORDER_NR_PAGES */
> #define pageblock_order         MAX_PAGE_ORDER
>
> If buddies were indeed >MAX_PAGE_ORDER, this would be an out-of-bounds
> access when delete updates the freelist count. Of course, buddies per
> definition cannot be larger than MAX_PAGE_ORDER. But the older gcc
> doesn't seem to realize this branch in this configuration is dead.
>
> Maybe we can help it out and make the impossible scenario a bit more
> explicit? Does this fixlet silence the warning?
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index efb2581ac142..4cdc356e73f6 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1698,6 +1698,10 @@ bool move_freepages_block_isolate(struct zone *zone, struct page *page,
>  				       NULL, NULL))
>  		return false;
>
> +	/* No splits needed if buddies can't span multiple blocks */
> +	if (pageblock_order == MAX_PAGE_ORDER)
> +		goto move;
> +
>  	/* We're a tail block in a larger buddy */
>  	pfn = find_large_buddy(start_pfn);
>  	if (pfn != start_pfn) {
> @@ -1725,7 +1729,7 @@ bool move_freepages_block_isolate(struct zone *zone, struct page *page,
>  		split_large_buddy(zone, page, pfn, order);
>  		return true;
>  	}
> -
> +move:
>  	mt = get_pfnblock_migratetype(page, start_pfn);
>  	nr_moved = move_freepages(zone, start_pfn, end_pfn, migratetype);
>  	if (!is_migrate_isolate(mt))
>
> Zi Yan, does this look sane to you as well?
Yes. This and patch 9 look good to me. Thanks.

For both, Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>

I also tested them locally and confirm it is a gcc bug and this fix
works for gcc-9.3:
1. gcc-13.2 does not give any warning for the original patch 9
2. gcc-9.3 gives the warning for the origin patch, but the warning goes
away with this patch applied.

--
Best Regards,
Yan, Zi

Attachment: signature.asc
Description: OpenPGP digital signature