Re: [PATCH] mm/page_alloc: fix deadlock on cpu_hotplug_lock in __accept_page()

Re: [PATCH] mm/page_alloc: fix deadlock on cpu_hotplug_lock in __accept_page()

[Dave Hansen]

On 3/29/25 10:10, Kirill A. Shutemov wrote:
> +		if (system_wq)
> +			schedule_work(&zone->unaccepted_cleanup);
> +		else
> +			unaccepted_cleanup_work(&zone->unaccepted_cleanup);
> +	}
>  }

The 'system_wq' check seems like an awfully big hack. No other
schedule_work() user does anything similar that I can find across the tree.

Instead of hacking in some internal state, could you use 'system_state',
like:

	if (system_state == SYSTEM_BOOTING)
		unaccepted_cleanup_work(&zone->unaccepted_cleanup);
	else
		schedule_work(&zone->unaccepted_cleanup);

The other method would be to make it more opportunistic? Basically,
detect when it might deadlock:

bool try_to_dec()
{
	if (!cpus_read_trylock())
		return false;

	static_branch_dec_cpuslocked(&zones_with_unaccepted_pages);
	cpus_read_unlock();

	return true;
}

That still requires a bit in the zone to say whether the
static_branch_dec() was deferred or not, though. It's kinda open-coding
schedule_work().

Re: [PATCH] mm/page_alloc: fix deadlock on cpu_hotplug_lock in __accept_page()

[Kirill A. Shutemov]

On Mon, Mar 31, 2025 at 12:07:07PM -0700, Dave Hansen wrote:
> On 3/29/25 10:10, Kirill A. Shutemov wrote:
> > +		if (system_wq)
> > +			schedule_work(&zone->unaccepted_cleanup);
> > +		else
> > +			unaccepted_cleanup_work(&zone->unaccepted_cleanup);
> > +	}
> >  }
> 
> The 'system_wq' check seems like an awfully big hack. No other
> schedule_work() user does anything similar that I can find across the tree.

I don't see how it is "an awfully big hack". It is "use system_wq if it is
ready".

Maybe it is going to be marginally cleaner if schedule_work() would be
open-coded:

		if (system_wq)
		        queue_work(system_wq, &zone->unaccepted_cleanup);
		else
			unaccepted_cleanup_work(&zone->unaccepted_cleanup);

?

> 
> Instead of hacking in some internal state, could you use 'system_state',
> like:
> 
> 	if (system_state == SYSTEM_BOOTING)
> 		unaccepted_cleanup_work(&zone->unaccepted_cleanup);
> 	else
> 		schedule_work(&zone->unaccepted_cleanup);

Really? The transition points between these states are arbitrary defined.
Who said that if we are out of SYSTEM_BOOTING we can use system_wq?
Tomorrow we can introduce additional state between BOOTING and SCHEDULING
and this code will be silently broken. The same for any new state before
BOOTING.

> The other method would be to make it more opportunistic? Basically,
> detect when it might deadlock:
> 
> bool try_to_dec()
> {
> 	if (!cpus_read_trylock())
> 		return false;
> 
> 	static_branch_dec_cpuslocked(&zones_with_unaccepted_pages);
> 	cpus_read_unlock();
> 
> 	return true;
> }
> 
> That still requires a bit in the zone to say whether the
> static_branch_dec() was deferred or not, though. It's kinda open-coding
> schedule_work().

It will also require special handling for soft CPU online/offline.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov