On 9/12/23 17:03, Johannes Weiner wrote:
> On Tue, Sep 12, 2023 at 03:47:45PM +0200, Vlastimil Babka wrote:
>> I think after this change we should [...]
>
> Speaking of follow-ups, AFAICS we no longer need those either:
Seems so, but the comments do talk about races, so once those are sorted out :)
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 9cad31de1bf5..bea499fbca58 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1751,13 +1751,6 @@ static void steal_suitable_fallback(struct zone *zone, struct page *page,
>
> old_block_type = get_pageblock_migratetype(page);
>
> - /*
> - * This can happen due to races and we want to prevent broken
> - * highatomic accounting.
> - */
> - if (is_migrate_highatomic(old_block_type))
> - goto single_page;
> -
> /* Take ownership for orders >= pageblock_order */
> if (current_order >= pageblock_order) {
> change_pageblock_range(page, current_order, start_type);
> @@ -1926,24 +1919,15 @@ static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
> continue;
>
> /*
> - * In page freeing path, migratetype change is racy so
> - * we can counter several free pages in a pageblock
> - * in this loop although we changed the pageblock type
> - * from highatomic to ac->migratetype. So we should
> - * adjust the count once.
> + * It should never happen but changes to
> + * locking could inadvertently allow a per-cpu
> + * drain to add pages to MIGRATE_HIGHATOMIC
> + * while unreserving so be safe and watch for
> + * underflows.
> */
> - if (is_migrate_highatomic_page(page)) {
> - /*
> - * It should never happen but changes to
> - * locking could inadvertently allow a per-cpu
> - * drain to add pages to MIGRATE_HIGHATOMIC
> - * while unreserving so be safe and watch for
> - * underflows.
> - */
> - zone->nr_reserved_highatomic -= min(
> - pageblock_nr_pages,
> - zone->nr_reserved_highatomic);
> - }
> + zone->nr_reserved_highatomic -= min(
> + pageblock_nr_pages,
> + zone->nr_reserved_highatomic);
>
> /*
> * Convert to ac->migratetype and avoid the normal
>
> I think they were only in place because we could change the highatomic
> status of pages on the pcplist, and those pages would then end up on
> some other freelist due to the stale pcppage cache.
>
> I replaced them locally with WARNs and ran an hour or so of kernel
> builds under pressure. It didn't trigger. So I would send a follow up
> to remove them.
>
> Unless you point me to a good reason why they're definitely still
> needed - in which case this is a moot proposal - but then we should
> make the comments more specific.
