Re: [PATCH 0/6] mm: fix pfn walker races with memory offline

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/27/26 14:49, Gregory Price (Meta) wrote:
> On 2026-10-27 13:51 +0100, David Hildenbrand (Arm) wrote:
>> On 10/27/26 12:31, Gregory Price (Meta) wrote:
>>>
>>> I actually have a reason for these fixes other than "bot said bad".
>>
>> So you found them on production kernels?
>>
>> We did not have a single real report of that for probably a decade or
>> even longer in production.
>>
>> Recently, we had the single first report about the compaction case from a
>> stress test (no production):
>>
>> https://lore.kernel.org/all/20260903095543.3752338-1-yuan1.liu@intel.com/
>>
>> "We are not sure whether this race is reachable under realistic
>> workloads or only under this synthetic stress test."
>>
>> Where I motivated that we need something clean.
>>
> 
> tl;dr:
> 
> while these are ai-generated, I'm not posting untested, unreviewed
> schlopp for the sake of getting patches upstream.

Appreciated.

For this long-standing considered low-priority thing in particular it is
reasonable to go back in time, see how often it was raised (quite often by me)
and how often it was actually reported (I found one case).

Then discuss the high level design in more than a single sentence :)

Discuss why we are not using it for other pfn_to_online_page() users, how it
interacts with zone->contig etc etc.

There really isn't a lot on the design in the cover letter.

> 
> I actually care about this issue.
> 
> ---
> 
> Some of these don't show up as crashes but instead as hung offline
> attempts (GFP_KERNEL landing on movable).  That shows up as our
> hypervisor just failing to overcommit / guests hanging.
> 
> The compaction/alloc_contig bug is one I've seen in production.

Right, but I suspect that's "[PATCH 4/6] mm/page_alloc: don't undo another
isolation in alloc_contig_range" which is *not at all* related to "mm: fix pfn
walker races with memory offline".

> 
> I've seen a handful of NULL derefs in production i suspect are the
> zonelist derefs, but it's hard to say (they're rare enough that we
> lose the dumps before someone has a thought to pull them).

That would be interesting to know.

> 
> I expect this will scale up as we increase the number of VMs and
> the amount of hotplug we're doing.
> 
> For clarity, we use ballooning + hotplug as a combo:
>  - ballooning to do cooperative overcommit
>  - hotplug to defend host stability when the overcommit ratio is extreme
>    (basically deflate_on_oom because a host stability issue)
> 
> We're pretty aggressive about ballooning guests to squeeze them and
> dropping the balloon to hot-unplug that memory when a guest is cold.

Yeah, that makes sense.

> 
> 
> skippable side note on virtio-balloon/mem:
> 
> I have some improvements to virtio-balloon and virtio-mem that allow for
> handoff from balloon directly to mem for hot-unplug without having to
> drop the balloon, but I haven't posted those (and we don't run those in
> production, so this is not the nexus of crashes).

Do you mean free-page reporting or actual balloon inflation/deflation paired
with virtio-mem. I hope not the latter, because that is just disgusting.

> 
> Similarly I have another extension that lets the host set a "NO_ALLOC"
> bit on the zone so a balloon-only system can provide the host a way to
> defend against mass deflate_on_oom events (we prefer guests to OOM and
> have the host to kill them due to PSI, rather than threaten host
> stability). Also not in production and not posted, but there have been
> a lot of experiments on overcommit here.
> 
>> The thing is that the race between offlining and actual memory removal
>> is extremely long. Poisoning is just a mechanism to find these theoretical
>> issues that could cause issues faster.
>>
>> And with all the other things going on, this has lower priority, and I
>> wanted to get back to the AI patches I have once the other thing is sorted.
>>
> 
> Sure, I realize I'm pushing this out during the merge window, but I
> felt it reasonable to get some thoughts on whether these fixes are
> worth considering for the window.

Just a matter of priority. I think this should be tackled in clean way indeed.

> 
> If the answer is no just put em on the back burner. We can carry
> fixes as-needed.
> 
>>>
>>> I have a reproducer for every single one of these (and the other
>>> series) - but a handful are unreachable due to a 2-statement read
>>> being emitted as a single read by the compiler (which can be undone
>>> by a targeted compiler flag). Those were included for completeness.
>>
>>
>> In general, I think we want something that will protect all
>> pfn_to_online_page() + zone->contig thingies, not just selected ones.
>> We discussed in the past using some sort of rcu for that.
>>
>> get_online_mems is usually the wrong approach.
>>
> 
> Agreed, although the PFN walks seem genuinely harder to wrangle into an
> good rcu shape.

And that's where the overall design needs to discuss this and how to resolve it.

:)

> 
> get_online_mems seems like the backportable solution, but i can see not
> wanting to have to go clean it back up later.
> 
>> protecting individual pfn_to_online_page is just whack a mole. Likely we want a
>> way to check whether pfn_to_online_page() is called from the wrong context, and
>> maybe we need a variant that can be called from "wrong context" for a good reason.
>>
> 
> Agreed.  Although I fear the structural fix for this will have uglier
> performance issue than we think.  Then again, maybe it doesn't matter
> (if you're doing a PFN walk, then you're likely already doing something
> stupidly expensive).

Which areas are you mostly concerned about and from which piece do you expect
the overhead?

> 
> Where i'm largely concerned is the page allocator and the
> compaction/contig code not taking a regression.
> 
> For zonelists i've ended up with something like this:
> 
> @@ -1461,7 +1466,7 @@ typedef struct pglist_data {
>          * Generally the first zones will be references to this node's
>          * node_zones.
>          */
> -       struct zonelist node_zonelists[MAX_ZONELISTS];
> +       struct zonelist __rcu *node_zonelists[MAX_ZONELISTS];
> 
> ...
> 
> static inline struct srcu_ctr __percpu *zonelist_read_lock(void)
> {
>        return srcu_read_lock_fast(&zonelist_srcu);
> }
> 
> static inline void zonelist_read_unlock(struct srcu_ctr __percpu *scp)
> {
>        srcu_read_unlock_fast(&zonelist_srcu, scp);
> }
> 
> DEFINE_LOCK_GUARD_0(zonelist_read, _T->scp = zonelist_read_lock(),
>                    zonelist_read_unlock(_T->scp),
>                    struct srcu_ctr __percpu *scp)
> 
> static inline struct zonelist *pgdat_zonelist(pg_data_t *pgdat, int type)
> {
>        return zonelist_dereference(pgdat->node_zonelists[type]);
> }
> 
> With this the zonelist cookie checks evaporate, but you still end up
> with guard() at every walk location - not great, but better i suppose.


For "broader" things that are not just a simple pfn_to_online_page() user, we
could also have memory online/offline sync in more efficient ways using the
memory notifier.

It's also tricky, but maybe for zone->contig + compaction the better solution?

> 
> I was hoping this could be sunken into the zonelist walk macros, but
> I haven't seen that pan out.  I've been spending a lot of time auditing
> that code for private nodes as well.
> 
> 
> On performance:
> 
> I've been hammering this with will-it-scale and a bunch of workload
> tests on a variety of hosts and have not seen any obvious regressions
> outside of powerpc (which i can't test directly, but SRCU-fast requires
> atomics for powerpc from what i can tell - so it will eat overhead
> without a fallback... if there is actually any hotplug ppc hardware out
> there to care about?)

So the big problem is the overhead on the read side for srcu?

If we can find something else to suspend compaction, we could maybe switch to
rcu for the remaining cases.

Now that I think about it, I recall why I didn't fix this earlier :)

-- 
Cheers,

David