Re: [PATCH] Revert "mm/page_alloc.c: don't show protection in zone's ->lowmem_reserve[] for empty zone"

[Baoquan He <bhe@redhat.com>]

On 02/27/25 at 10:16am, Vlastimil Babka wrote:
> On 2/26/25 16:57, Baoquan He wrote:
> > On 02/26/25 at 01:01pm, Michal Hocko wrote:
> >> 
> >> Why do you think anything needs to be adjusted?
> > 
> > No, I don't think like that. But I am wondering what makes you get
> > the conclusion.
> > 
> >> 
> >> > I haven't thought of the whole zone fallback list to interleave nodes
> >> > which invovles a lot of change.
> >> > 
> >> > > 
> >> > > Btw. has 96a5c186efff tried to fix any actual runtime problem? The
> >> > > changelog doesn't say much about that. 
> >> > 
> >> > No, no actual problem was observed on tht.
> >> 
> >> OK
> >> 
> >> > I was just trying to make
> >> > clear the semantics because I was confused by its obscure value printing
> >> > of zone->lowmem_reserve[] in /proc/zoneinfo.
> >> > 
> >> > I think we can merge this reverting firstly, then to investigate how to
> >> > better clarify it.
> >> 
> >> What do you think needs to be clarify? How exactly is the original
> >> output confusing?
> > 
> > When I did the change, I wrote the reason in commit log. I don't think
> > you care to read it from your talking. Let me explain again, in
> > setup_per_zone_lowmem_reserve(), each zone's protection value is
> > calculated based on its own node's zones. E.g below on node 0, its
> > Movable zone and Device zone are empty but still show influence on
> > Normal/DMA32/DMA zone, this is unreasonable from the protection value
> > calculating code and its showing.

Ah, I saw your mail when I finished my replying to Michal. Thanks for
your sharing with deliberate details, I almost agree with them all.

> 
> It's not unreasonable. A GFP_HIGHUSER_MOVABLE can use up to the Movable
> zone, so e.g. the dma32 zone should be protected from such an allocation, so
> it has space for GFP_DMA32 restricted allocations.

Yes, I didn't realize that when I did the change in commit 96a5c186efff,
sorry about that.

> 
> If no Movable zone exists, but Normal zone does, the result is the
> protection will be the same for GFP_KERNEL allocations (that can use up to
> the Normal zone) and GFP_HIGHUSER_MOVABLE allocations. (i.e. the number of
> 22134 in your listing is the same for both indexes). That's fine. But
> setting the protection from Movable allocations to 0 as commit 96a5c186efff
> did was simply a bug, as that can directly lead to GFP_HIGHUSER_MOVABLE
> depleting ZONE_DMA32.

Yes, agree. I think that's the reason Gabriel observed the regression.

> 
> The only "unreasonable" part here is that we define and show protections
> from ZONE_DEVICE allocations. The usage of this zone is AFAIK completely
> separate from normal page allocation through zonelists, so we could exclude
> it, if anyone cared enough.

I think this is not the only unreasonable part.
sysctl_lowmem_reserve_ratio is a knob provided for user to tune the
memory management. While the underlying code relative to the set ratio
can't meet the expections. Even though we revert my patch, it seems to
work well, while the protection value is not under good management. It
just happens to work. Because the protection value is calculated
relative to __GFP_THISNODE allocation, while ignoring the FALLBACK
allocation. I think you have pointed that out greatly in below comment.

> 
> > If really as your colleague Gabriel said, the protection value of DMA zone
> > on node 0 will impact allocation when targeted zone is Movable zone, we
> > may need consider the protection value calcuation acorss nodes. Because
> > the impact happens among different nodes. I only said we can do
> > investigation, I didn't said we need change or have to change.
> 
> There might be a theoretical issue if e.g. Node 0 only contained DMA and
> DMA32 zones and nothing else, while the Normal zone is on Node 1, there
> would be no protection for DMA/DMA32 zones from Normal allocations, as
> setup_per_zone_lowmem_reserve() considers each node separately and thus
> would not take Normal zone size from Node 1 into account.
> 
> Should we sum zone sizes accross all nodes then? But then __GFP_THISNODE
> Normal allocations for node 0 would never succeed? Or we'd need a separate
> lowmem_reserve array for those?

Yeah, I have the same thought as you here. We may need adapation here.

> 
> I guess the issue doesn't happen in practice. In any case it's out of scope
> of the reverted commit and the revert.

It could happen on arm64 because arm64 only has ZONE_DMA by default and
its boundary is not fixed. I saw all zones are ZONE_DMA on arm64, I
guess it could be easier to see a arm64 system which only has ZONE_DMA
on node 0 and ZONE_NORMAL/MOVABLE on other nodes.

> 
> > Node 0, zone      DMA
> >   ......
> >   pages free     2816
> >         ......
> >         protection: (0, 1582, 23716, 23716, 23716)
> > Node 0, zone    DMA32
> >   pages free     403269
> >         ......
> >         protection: (0, 0, 22134, 22134, 22134)
> > Node 0, zone   Normal
> >   pages free     5423879
> >         ......
> >         protection: (0, 0, 0, 0, 0)
> > Node 0, zone  Movable
> >   pages free     0
> >         ......
> >         protection: (0, 0, 0, 0, 0)
> > Node 0, zone   Device
> >   pages free     0
> >         ......
> >         protection: (0, 0, 0, 0, 0)
> > 
> >> 
> >> -- 
> >> Michal Hocko
> >> SUSE Labs
> >> 
> > 
> > 
>