On 02/26/25 at 06:46pm, Michal Hocko wrote:
> On Wed 26-02-25 23:57:48, Baoquan He wrote:
> > 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.
>
> You said that in the commit message without explanation why. Also I
> claim this is just wrong because the zone's protection is independent on
> the size of the zone that it is protected from. I have explained why I
> believe but let me reiterate. ZONE_DMA32 should be protected from
> GFP_MOVABLE even if the zone movable is empty (same as if it had a
> single or many pages). Why? Because, the lowmem reserve protects low
> memory allocation requests.
>
> See my point? Is that reasoning clear?
Very clear. But now the protection is calculated node by node. Please
think about one case, Node 0 only has ZONE_DMA and ZONE_DMA32, Node 1
and 2, 3 ...N have NORMAL_ZONE and MOVABLE_ZONE. How could ZONE_DMA32
be protected from GFP_MOVABLE? Linux kernel has restriction on the node
layout where Node 0 can't do this? Especailly on arm64, there's only
ZONE_DMA and its boundary is not fixed some time, what if system vendor
arranges the Node 0 only having ZONE_DMA?
Secondly, the existing protection ratio was created based on the old x86
system. It may not be fit for the current ARCH, e.g arm64, it only has
ZONE_DMA which is under 4G by default, the default ratio obviously not
suitable any more. And we can clearly feel that the current protection
value is for __GFP_THISNODE allocation.
======
/*
* results with 256, 32 in the lowmem_reserve sysctl:
* 1G machine -> (16M dma, 800M-16M normal, 1G-800M high)
* 1G machine -> (16M dma, 784M normal, 224M high)
* NORMAL allocation will leave 784M/256 of ram reserved in the ZONE_DMA
* HIGHMEM allocation will leave 224M/32 of ram reserved in ZONE_NORMAL
* HIGHMEM allocation will leave (224M+784M)/256 of ram reserved in ZONE_DMA
*
* TBD: should special case ZONE_DMA32 machines here - in those we normally
* don't need any ZONE_NORMAL reservation
*/
static int sysctl_lowmem_reserve_ratio[MAX_NR_ZONES] = {
======
So my thought is we either have two-dimention protection value, one is for
__GFP_THISNODE allocation, 2nd dimention is for FALLBACK allocation.
struct zone {
......
long lowmem_reserve[MAX_ZONELISTS][MAX_NR_ZONES];
}
Or we count in one higher zone's amount in all nodes when calculating
the proction value for lower zone, while the formula need be adjusted
because the one zone's calculated page number could be huge and the the
protections value could be bigger than the lower zone's page number.
Or leave it until one true problem occur, we can consider to fix it
accordingly.
Any one is fine to me.
>
> P.S.
> I think we can have a more productive discussion without accusations.
Yes, we can, and I have no doubt about it always, no matter when and
with whom.
