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/26/25 at 01:01pm, Michal Hocko wrote:
> On Wed 26-02-25 19:51:12, Baoquan He wrote:
> > On 02/26/25 at 12:00pm, Michal Hocko wrote:
> > > On Wed 26-02-25 11:52:41, Michal Hocko wrote:
> > > > On Wed 26-02-25 18:00:26, Baoquan He wrote:
> > > > > On 02/26/25 at 07:54am, Michal Hocko wrote:
> > > > [...]
> > > > > > In any case 96a5c186efff seems incorrect because it assumes that the
> > > > > > protection has anything to do with how higher zone is populated while
> > > > > > the protection fundamentaly protects lower zone from higher zones
> > > > > > allocation. Those allocations are independent on the actual memory in
> > > > > > that zone.
> > > > > 
> > > > > The protection value was introduced in non-NUMA time, and later adapted
> > > > > to NUMA system. While it still only reflects each zone with other zones
> > > > > within one specific node. We may need take this opportunity to
> > > > > reconsider it, e.g in the FALLBACK zonelists case it needs take crossing
> > > > > nodes into account.
> > > > 
> > > > Are you suggesting zone fallback list to interleave nodes? I.e.
> > > > numa_zonelist_order we used to have in the past and that has been
> > > > removed by c9bff3eebc09 ("mm, page_alloc: rip out ZONELIST_ORDER_ZONE").
> > 
> > Hmm, if Gabriel can provide detailed node/zone information of the
> > system, we can check if there's anything we can do to adjust
> > zone->lowmem_reserve[] to reflect its real usage and semantics.
> 
> 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.

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.

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
>