* Re: [PATCH] mm/page_alloc: fix deadlock on cpu_hotplug_lock in __accept_page()
2025-03-31 19:07 ` [PATCH] mm/page_alloc: fix deadlock on cpu_hotplug_lock in __accept_page() Dave Hansen
@ 2025-04-01 7:25 ` Kirill A. Shutemov
0 siblings, 0 replies; 2+ messages in thread
From: Kirill A. Shutemov @ 2025-04-01 7:25 UTC (permalink / raw)
To: Dave Hansen
Cc: Andrew Morton, Mike Rapoport, David Hildenbrand, Vlastimil Babka,
Mel Gorman, Tom Lendacky, Kalra, Ashish, Rick Edgecombe,
linux-mm, linux-coco, linux-kernel, Srikanth Aithal
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
^ permalink raw reply [flat|nested] 2+ messages in thread