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

[Vlastimil Babka <vbabka@suse.cz>]

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.

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.

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.

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.

> 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?

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

> 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
>> 
> 
>